Re: [PATCH v1 1/1] UefiCpuPkg: PiSmmCpuDxeSmm: Not to Change Bitwidth During Static Paging

["Kun Qin" <kuqin12@gmail.com>]

Hi Laszlo,

Thanks for the feedback. I will update commit message for (2) and 
variable type for (3) in v2.

As per suggestion by Ray in another thread, we are moving 
"PhysicalAddressBits" from local variable to input parameter. So I will 
update the term "stack based variable" for (1) accordingly.

Regards,
Kun

On 04/14/2021 02:15, Laszlo Ersek wrote:
> On 04/14/21 04:59, Kun Qin wrote:
>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3300
>>
>> Current implementation of SetStaticPageTable routine in PiSmmCpuDxeSmm
>> driver will check a global variable mPhysicalAddressBits, and eventually
>> cap any value larger than 39 at 39.
>>
>> This global variable is used in ConvertMemoryPageAttributes, which backs
>> SmmSetMemoryAttributes and SmmClearMemoryAttributes. Thus for a processor
>> that supports more than 39 bits width, trying to mark page table regions
>> higher than 39-bit will always return EFI_UNSUPPROTED.
>>
>> This change replaced the changed bitwidth to a stack based variable.
> 
> (1) "local variable" is a more common expression.
> 
> If we wanted to be exact, we could call it "variable with automatic
> storage duration".
> 
> Either way, "stack" is irrelevant here, IMO.
> 
>>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Ray Ni <ray.ni@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Rahul Kumar <rahul1.kumar@intel.com>
>>
>> Signed-off-by: Kun Qin <kuqin12@gmail.com>
>> ---
>>   UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 25 +++++++++++---------
>>   1 file changed, 14 insertions(+), 11 deletions(-)
> 
> (2) I suggest adding the following line to the commit message, just
> above your Signed-off-by:
> 
> Fixes: 4eee0cc7cc0db74489b99c19eba056b53eda6358
> 
> Because the issue is a regression from that commit.
> 
>>
>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
>> index 6902584b1fbd..0caee8a27abe 100644
>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
>> @@ -226,6 +226,7 @@ SetStaticPageTable (
>>     UINTN                                         IndexOfPml4Entries;
>>     UINTN                                         IndexOfPdpEntries;
>>     UINTN                                         IndexOfPageDirectoryEntries;
>> +  UINT64                                        PhysicalAddressBits;
>>     UINT64                                        *PageMapLevel5Entry;
>>     UINT64                                        *PageMapLevel4Entry;
>>     UINT64                                        *PageMap;
> 
> (3) The "mPhysicalAddressBits" global variable has type UINT8. I don't
> see a reason for introducing the "PhysicalAddressBits" local variable
> with a different type.
> 
> The rest looks good to me.
> 
> Thanks
> Laszlo
> 
> 
>> @@ -237,26 +238,28 @@ SetStaticPageTable (
>>     // IA-32e paging translates 48-bit linear addresses to 52-bit physical addresses
>>     //  when 5-Level Paging is disabled.
>>     //
>> -  ASSERT (mPhysicalAddressBits <= 52);
>> -  if (!m5LevelPagingNeeded && mPhysicalAddressBits > 48) {
>> -    mPhysicalAddressBits = 48;
>> +  PhysicalAddressBits = mPhysicalAddressBits;
>> +
>> +  ASSERT (PhysicalAddressBits <= 52);
>> +  if (!m5LevelPagingNeeded && PhysicalAddressBits > 48) {
>> +    PhysicalAddressBits = 48;
>>     }
>>   
>>     NumberOfPml5EntriesNeeded = 1;
>> -  if (mPhysicalAddressBits > 48) {
>> -    NumberOfPml5EntriesNeeded = (UINTN) LShiftU64 (1, mPhysicalAddressBits - 48);
>> -    mPhysicalAddressBits = 48;
>> +  if (PhysicalAddressBits > 48) {
>> +    NumberOfPml5EntriesNeeded = (UINTN) LShiftU64 (1, PhysicalAddressBits - 48);
>> +    PhysicalAddressBits = 48;
>>     }
>>   
>>     NumberOfPml4EntriesNeeded = 1;
>> -  if (mPhysicalAddressBits > 39) {
>> -    NumberOfPml4EntriesNeeded = (UINTN) LShiftU64 (1, mPhysicalAddressBits - 39);
>> -    mPhysicalAddressBits = 39;
>> +  if (PhysicalAddressBits > 39) {
>> +    NumberOfPml4EntriesNeeded = (UINTN) LShiftU64 (1, PhysicalAddressBits - 39);
>> +    PhysicalAddressBits = 39;
>>     }
>>   
>>     NumberOfPdpEntriesNeeded = 1;
>> -  ASSERT (mPhysicalAddressBits > 30);
>> -  NumberOfPdpEntriesNeeded = (UINTN) LShiftU64 (1, mPhysicalAddressBits - 30);
>> +  ASSERT (PhysicalAddressBits > 30);
>> +  NumberOfPdpEntriesNeeded = (UINTN) LShiftU64 (1, PhysicalAddressBits - 30);
>>   
>>     //
>>     // By architecture only one PageMapLevel4 exists - so lets allocate storage for it.
>>
>