* [PATCH v1 0/1] Not to Update Bitwidth Variable in Static Paging
@ 2021-04-14 2:59 Kun Qin
2021-04-14 2:59 ` [PATCH v1 1/1] UefiCpuPkg: PiSmmCpuDxeSmm: Not to Change Bitwidth During " Kun Qin
0 siblings, 1 reply; 6+ messages in thread
From: Kun Qin @ 2021-04-14 2:59 UTC (permalink / raw)
To: devel; +Cc: Eric Dong, Ray Ni, Laszlo Ersek, Rahul Kumar
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.
However, 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. As a
result, access rights to pages residing higher than 39 bits in the
physical address space will not be correctly configured (i.e. restricted)
in the SMM page table entries.
This change attempts to fix this issue by switching to caching the global
bitwidth variable in local variable for SetStaticPageTable routine.
Patch v1 branch: https://github.com/kuqin12/edk2/tree/bitwidth_paging
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>
Kun Qin (1):
UefiCpuPkg: PiSmmCpuDxeSmm: Not to Change Bitwidth During Static
Paging
UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 25 +++++++++++---------
1 file changed, 14 insertions(+), 11 deletions(-)
--
2.31.0.windows.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v1 1/1] UefiCpuPkg: PiSmmCpuDxeSmm: Not to Change Bitwidth During Static Paging
2021-04-14 2:59 [PATCH v1 0/1] Not to Update Bitwidth Variable in Static Paging Kun Qin
@ 2021-04-14 2:59 ` Kun Qin
2021-04-14 9:15 ` Laszlo Ersek
2021-04-14 9:49 ` Ni, Ray
0 siblings, 2 replies; 6+ messages in thread
From: Kun Qin @ 2021-04-14 2:59 UTC (permalink / raw)
To: devel; +Cc: Eric Dong, Ray Ni, Laszlo Ersek, Rahul Kumar
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.
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(-)
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;
@@ -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.
--
2.31.0.windows.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v1 1/1] UefiCpuPkg: PiSmmCpuDxeSmm: Not to Change Bitwidth During Static Paging
2021-04-14 2:59 ` [PATCH v1 1/1] UefiCpuPkg: PiSmmCpuDxeSmm: Not to Change Bitwidth During " Kun Qin
@ 2021-04-14 9:15 ` Laszlo Ersek
2021-04-14 17:33 ` Kun Qin
2021-04-14 9:49 ` Ni, Ray
1 sibling, 1 reply; 6+ messages in thread
From: Laszlo Ersek @ 2021-04-14 9:15 UTC (permalink / raw)
To: Kun Qin, devel; +Cc: Eric Dong, Ray Ni, Rahul Kumar
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.
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1 1/1] UefiCpuPkg: PiSmmCpuDxeSmm: Not to Change Bitwidth During Static Paging
2021-04-14 2:59 ` [PATCH v1 1/1] UefiCpuPkg: PiSmmCpuDxeSmm: Not to Change Bitwidth During " Kun Qin
2021-04-14 9:15 ` Laszlo Ersek
@ 2021-04-14 9:49 ` Ni, Ray
2021-04-14 17:26 ` Kun Qin
1 sibling, 1 reply; 6+ messages in thread
From: Ni, Ray @ 2021-04-14 9:49 UTC (permalink / raw)
To: Kun Qin, devel@edk2.groups.io; +Cc: Dong, Eric, Laszlo Ersek, Kumar, Rahul1
Is it possible to let SetStaticPageTable() accept another parameter "PhysicalAddressBits" so it doesn't update the global one?
Using this way, SetStaticPageTable() can avoid reference the global variable completely.
> -----Original Message-----
> From: Kun Qin <kuqin12@gmail.com>
> Sent: Wednesday, April 14, 2021 10:59 AM
> To: devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Laszlo
> Ersek <lersek@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>
> Subject: [PATCH v1 1/1] UefiCpuPkg: PiSmmCpuDxeSmm: Not to Change
> Bitwidth During Static Paging
>
> 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.
>
> 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(-)
>
> 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;
> @@ -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.
> --
> 2.31.0.windows.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1 1/1] UefiCpuPkg: PiSmmCpuDxeSmm: Not to Change Bitwidth During Static Paging
2021-04-14 9:49 ` Ni, Ray
@ 2021-04-14 17:26 ` Kun Qin
0 siblings, 0 replies; 6+ messages in thread
From: Kun Qin @ 2021-04-14 17:26 UTC (permalink / raw)
To: Ni, Ray, devel@edk2.groups.io; +Cc: Dong, Eric, Laszlo Ersek, Kumar, Rahul1
Hi Ray,
I do not have a preference in using local variable or input parameter. I
can change the SetStaticPageTable interface to accept bitwidth in v2.
Regards,
Kun
On 04/14/2021 02:49, Ni, Ray wrote:
> Is it possible to let SetStaticPageTable() accept another parameter "PhysicalAddressBits" so it doesn't update the global one?
> Using this way, SetStaticPageTable() can avoid reference the global variable completely.
>
>> -----Original Message-----
>> From: Kun Qin <kuqin12@gmail.com>
>> Sent: Wednesday, April 14, 2021 10:59 AM
>> To: devel@edk2.groups.io
>> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Laszlo
>> Ersek <lersek@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>
>> Subject: [PATCH v1 1/1] UefiCpuPkg: PiSmmCpuDxeSmm: Not to Change
>> Bitwidth During Static Paging
>>
>> 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.
>>
>> 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(-)
>>
>> 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;
>> @@ -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.
>> --
>> 2.31.0.windows.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1 1/1] UefiCpuPkg: PiSmmCpuDxeSmm: Not to Change Bitwidth During Static Paging
2021-04-14 9:15 ` Laszlo Ersek
@ 2021-04-14 17:33 ` Kun Qin
0 siblings, 0 replies; 6+ messages in thread
From: Kun Qin @ 2021-04-14 17:33 UTC (permalink / raw)
To: Laszlo Ersek, devel; +Cc: Eric Dong, Ray Ni, Rahul Kumar
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.
>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-04-14 17:33 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-04-14 2:59 [PATCH v1 0/1] Not to Update Bitwidth Variable in Static Paging Kun Qin
2021-04-14 2:59 ` [PATCH v1 1/1] UefiCpuPkg: PiSmmCpuDxeSmm: Not to Change Bitwidth During " Kun Qin
2021-04-14 9:15 ` Laszlo Ersek
2021-04-14 17:33 ` Kun Qin
2021-04-14 9:49 ` Ni, Ray
2021-04-14 17:26 ` Kun Qin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox