[edk2-devel] [PATCH] UefiCpuPkg: Fix issue that IsModified is wrongly set in PageTableMap

[edk2-devel] [PATCH] UefiCpuPkg: Fix issue that IsModified is wrongly set in PageTableMap

[Zhiguang Liu]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4614

Fix issue that IsModified is wrongly set in PageTableMap.

Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Crystal Lee <CrystalLee@ami.com.tw>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
 UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
index 36b2c4e6a3..164187f151 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
@@ -567,7 +567,10 @@ PageTableLibMapInLevel (
         OriginalCurrentPagingEntry.Uint64 = CurrentPagingEntry->Uint64;
         PageTableLibSetPle (Level, CurrentPagingEntry, Offset, Attribute, &CurrentMask);
 
-        if (OriginalCurrentPagingEntry.Uint64 != CurrentPagingEntry->Uint64) {
+        if (Modify && (OriginalCurrentPagingEntry.Uint64 != CurrentPagingEntry->Uint64)) {
+          //
+          // The page table entry can be changed by this function only when Modify is true.
+          //
           *IsModified = TRUE;
         }
       }
@@ -609,7 +612,10 @@ PageTableLibMapInLevel (
   // Check if ParentPagingEntry entry is modified here is enough. Except the changes happen in leaf PagingEntry during
   // the while loop, if there is any other change happens in page table, the ParentPagingEntry must has been modified.
   //
-  if (OriginalParentPagingEntry.Uint64 != ParentPagingEntry->Uint64) {
+  if (Modify && (OriginalParentPagingEntry.Uint64 != ParentPagingEntry->Uint64)) {
+    //
+    // The page table entry can be changed by this function only when Modify is true.
+    //
     *IsModified = TRUE;
   }
 
-- 
2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113513): https://edk2.groups.io/g/devel/message/113513
Mute This Topic: https://groups.io/mt/103636407/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] UefiCpuPkg: Fix issue that IsModified is wrongly set in PageTableMap

[Ni, Ray]

Reviewed-by: Ray Ni <ray.ni@intel.com>

Thanks,
Ray
> -----Original Message-----
> From: Liu, Zhiguang <zhiguang.liu@intel.com>
> Sent: Wednesday, January 10, 2024 1:38 PM
> To: devel@edk2.groups.io
> Cc: Liu, Zhiguang <zhiguang.liu@intel.com>; Ni, Ray <ray.ni@intel.com>; Laszlo
> Ersek <lersek@redhat.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Gerd
> Hoffmann <kraxel@redhat.com>; Lee, Crystal <crystalLee@ami.com.tw>
> Subject: [PATCH] UefiCpuPkg: Fix issue that IsModified is wrongly set in
> PageTableMap
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4614
> 
> Fix issue that IsModified is wrongly set in PageTableMap.
> 
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Crystal Lee <CrystalLee@ami.com.tw>
> Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> ---
>  UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> index 36b2c4e6a3..164187f151 100644
> --- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> +++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> @@ -567,7 +567,10 @@ PageTableLibMapInLevel (
>          OriginalCurrentPagingEntry.Uint64 = CurrentPagingEntry->Uint64;
>          PageTableLibSetPle (Level, CurrentPagingEntry, Offset, Attribute,
> &CurrentMask);
> 
> -        if (OriginalCurrentPagingEntry.Uint64 != CurrentPagingEntry->Uint64) {
> +        if (Modify && (OriginalCurrentPagingEntry.Uint64 != CurrentPagingEntry-
> >Uint64)) {
> +          //
> +          // The page table entry can be changed by this function only when
> Modify is true.
> +          //
>            *IsModified = TRUE;
>          }
>        }
> @@ -609,7 +612,10 @@ PageTableLibMapInLevel (
>    // Check if ParentPagingEntry entry is modified here is enough. Except the
> changes happen in leaf PagingEntry during
>    // the while loop, if there is any other change happens in page table, the
> ParentPagingEntry must has been modified.
>    //
> -  if (OriginalParentPagingEntry.Uint64 != ParentPagingEntry->Uint64) {
> +  if (Modify && (OriginalParentPagingEntry.Uint64 != ParentPagingEntry-
> >Uint64)) {
> +    //
> +    // The page table entry can be changed by this function only when Modify
> is true.
> +    //
>      *IsModified = TRUE;
>    }
> 
> --
> 2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113521): https://edk2.groups.io/g/devel/message/113521
Mute This Topic: https://groups.io/mt/103636407/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] UefiCpuPkg: Fix issue that IsModified is wrongly set in PageTableMap

[Laszlo Ersek]

On 1/10/24 06:38, Zhiguang Liu wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4614
> 
> Fix issue that IsModified is wrongly set in PageTableMap.
> 
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Crystal Lee <CrystalLee@ami.com.tw>
> Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> ---
>  UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> index 36b2c4e6a3..164187f151 100644
> --- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> +++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> @@ -567,7 +567,10 @@ PageTableLibMapInLevel (
>          OriginalCurrentPagingEntry.Uint64 = CurrentPagingEntry->Uint64;
>          PageTableLibSetPle (Level, CurrentPagingEntry, Offset, Attribute, &CurrentMask);
>  
> -        if (OriginalCurrentPagingEntry.Uint64 != CurrentPagingEntry->Uint64) {
> +        if (Modify && (OriginalCurrentPagingEntry.Uint64 != CurrentPagingEntry->Uint64)) {
> +          //
> +          // The page table entry can be changed by this function only when Modify is true.
> +          //
>            *IsModified = TRUE;
>          }
>        }
> @@ -609,7 +612,10 @@ PageTableLibMapInLevel (
>    // Check if ParentPagingEntry entry is modified here is enough. Except the changes happen in leaf PagingEntry during
>    // the while loop, if there is any other change happens in page table, the ParentPagingEntry must has been modified.
>    //
> -  if (OriginalParentPagingEntry.Uint64 != ParentPagingEntry->Uint64) {
> +  if (Modify && (OriginalParentPagingEntry.Uint64 != ParentPagingEntry->Uint64)) {
> +    //
> +    // The page table entry can be changed by this function only when Modify is true.
> +    //
>      *IsModified = TRUE;
>    }
>  

This function is incredibly complicated, so reviewing this patch is
hard, even after reading the bugzilla ticket.

The commit message is useless. It should contain a brief description of
the problem, and how the fix resolves the problem.

The documentation of the PageTableLibMapInLevel() function is wrong,
even before this patch. It documents the "IsModified" output-only
parameter as follows:

"TRUE means page table is modified. FALSE means page table is not modified."

This states that "IsModified" is always set on output, to either FALSE
or TRUE. Which is an incorrect statement; in reality the caller is
expected to pre-set (*IsModified) to FALSE, and PageTableLibMapInLevel()
will (conditionally!) perform a FALSE->TRUE transition only.

Now, this patch may fix a bug, but it makes the above-described
documentation issue worse, by further restricting the condition for said
FALSE->TRUE transition.

The fix per se looks vaguely reasonable to me (really the function is so
complicated that verifying this change from scratch would take me ages),
but minimally, the documentation of "IsModified" should certainly be
updated too. To something like this:

  @param[out] IsModified  If "Modify" is TRUE on input and the function
                          has actually modified the page table, then set
                          to TRUE on output. Not overwritten otherwise.

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113528): https://edk2.groups.io/g/devel/message/113528
Mute This Topic: https://groups.io/mt/103636407/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] UefiCpuPkg: Fix issue that IsModified is wrongly set in PageTableMap

[Ni, Ray]

> This function is incredibly complicated, so reviewing this patch is
> hard, even after reading the bugzilla ticket.
> 
> The commit message is useless. It should contain a brief description of
> the problem, and how the fix resolves the problem.
> 
> The documentation of the PageTableLibMapInLevel() function is wrong,
> even before this patch. It documents the "IsModified" output-only
> parameter as follows:
> 
> "TRUE means page table is modified. FALSE means page table is not
> modified."
> 
> This states that "IsModified" is always set on output, to either FALSE
> or TRUE. Which is an incorrect statement; in reality the caller is
> expected to pre-set (*IsModified) to FALSE, and PageTableLibMapInLevel()
> will (conditionally!) perform a FALSE->TRUE transition only.
> 
> Now, this patch may fix a bug, but it makes the above-described
> documentation issue worse, by further restricting the condition for said
> FALSE->TRUE transition.

Laszlo, thanks for the comments!
Though the fixing looks simple, Zhiguang and I did have several rounds of offline discussions
regarding how to fix it.

When the lib accesses the page table content, CPU would set the "Access" bit in the page entry
that points to the page table memory being accessed by the lib.

So, even when the "Modify" is FALSE (indicating caller doesn't want the lib to modify the page table),
lib code should not modify the page table but CPU still sets the "Access" bit in some of the entries due to
the reasons above.
I agree it will be better that the commit message carries above details.

Zhiguang,
Can we update the code to always assign "IsModified"? I thought we did that but it seems not.

> 
> The fix per se looks vaguely reasonable to me (really the function is so
> complicated that verifying this change from scratch would take me ages),
> but minimally, the documentation of "IsModified" should certainly be
> updated too. To something like this:
> 
>   @param[out] IsModified  If "Modify" is TRUE on input and the function
>                           has actually modified the page table, then
> set
>                           to TRUE on output. Not overwritten
> otherwise.
> 
> Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113568): https://edk2.groups.io/g/devel/message/113568
Mute This Topic: https://groups.io/mt/103636407/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] UefiCpuPkg: Fix issue that IsModified is wrongly set in PageTableMap

[Laszlo Ersek]

On 1/11/24 03:03, Ni, Ray wrote:
>> This function is incredibly complicated, so reviewing this patch is
>> hard, even after reading the bugzilla ticket.
>>
>> The commit message is useless. It should contain a brief description of
>> the problem, and how the fix resolves the problem.
>>
>> The documentation of the PageTableLibMapInLevel() function is wrong,
>> even before this patch. It documents the "IsModified" output-only
>> parameter as follows:
>>
>> "TRUE means page table is modified. FALSE means page table is not
>> modified."
>>
>> This states that "IsModified" is always set on output, to either FALSE
>> or TRUE. Which is an incorrect statement; in reality the caller is
>> expected to pre-set (*IsModified) to FALSE, and PageTableLibMapInLevel()
>> will (conditionally!) perform a FALSE->TRUE transition only.
>>
>> Now, this patch may fix a bug, but it makes the above-described
>> documentation issue worse, by further restricting the condition for said
>> FALSE->TRUE transition.
> 
> Laszlo, thanks for the comments!
> Though the fixing looks simple, Zhiguang and I did have several rounds of offline discussions
> regarding how to fix it.
> 
> When the lib accesses the page table content, CPU would set the "Access" bit in the page entry
> that points to the page table memory being accessed by the lib.
> 
> So, even when the "Modify" is FALSE (indicating caller doesn't want the lib to modify the page table),
> lib code should not modify the page table but CPU still sets the "Access" bit in some of the entries due to
> the reasons above.

Huh, tricky!

Should the comparison explicitly mask out the Accessed bit from each of
the "before" page table entry and the "after" one, perhaps?

> I agree it will be better that the commit message carries above details.
> 
> Zhiguang,
> Can we update the code to always assign "IsModified"? I thought we did that but it seems not.

That seems doable by (e.g.) setting (*IsModified) to FALSE right at the
top of the function, and then the logic would match the existent
comments, I think. However, I've not checked whether callers actually
rely on this "summing" logic of the IsModified output parameter -- like
call the function a number of times in a row, using a common local
variable to receive IsModified, and then check the local variable to see
if *any one* of the calls in the loop has modified the page table.

Thanks
Laszlo

> 
>>
>> The fix per se looks vaguely reasonable to me (really the function is so
>> complicated that verifying this change from scratch would take me ages),
>> but minimally, the documentation of "IsModified" should certainly be
>> updated too. To something like this:
>>
>>   @param[out] IsModified  If "Modify" is TRUE on input and the function
>>                           has actually modified the page table, then
>> set
>>                           to TRUE on output. Not overwritten
>> otherwise.
>>
>> Laszlo
> 
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113598): https://edk2.groups.io/g/devel/message/113598
Mute This Topic: https://groups.io/mt/103636407/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] UefiCpuPkg: Fix issue that IsModified is wrongly set in PageTableMap

[Zhiguang Liu]

Hi Laszlo,

I don't think it is a good idea to explicitly mask out the Accessed/Dirty bit. We can't assume Accessed/Dirty bit are only changed by hardware, because the caller also can change the Accessed/Dirty bit.

For API PageTableMap, the IsModified is already set as False in the beginning of the function.
For internal function PageTableLibMapInLevel, we don't set IsModified as False in the beginning on purpose, because it keeps the global state of whether the PageTable is changed.

I plan to change the comment as below to explicitly explain the behavior:

For internal function PageTableLibMapInLevel, the description of IsModified should be:
@param[in, out]     IsModified     change IsModified to True if page table is modified and input parameter Modify is TRUE.

For API PageTableMap, the description of IsModified should be: 
@param[out]     IsModified     TRUE means page table is modified by software or hardware. FALSE means page table is not modified by software.
If the output IsModified is FALSE, there is possibility that the page table is changed by hardware. It is ok because page table can be changed by hardware anytime, and we don't need to Flush TLB.

With these comments changed, I don't need to change C code in my patch.

BTW, I assume IsModified can be used as an indicator whether the caller need to flush TLB. Do you prefer to change the parameter name to IsFlushTlbNeeded? I am both fine.

Thanks
Zhiguang

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
> Ersek
> Sent: Thursday, January 11, 2024 4:57 PM
> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Liu, Zhiguang
> <zhiguang.liu@intel.com>
> Cc: Kumar, Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffmann
> <kraxel@redhat.com>; Lee, Crystal <crystalLee@ami.com.tw>
> Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg: Fix issue that IsModified is
> wrongly set in PageTableMap
> 
> On 1/11/24 03:03, Ni, Ray wrote:
> >> This function is incredibly complicated, so reviewing this patch is
> >> hard, even after reading the bugzilla ticket.
> >>
> >> The commit message is useless. It should contain a brief description
> >> of the problem, and how the fix resolves the problem.
> >>
> >> The documentation of the PageTableLibMapInLevel() function is wrong,
> >> even before this patch. It documents the "IsModified" output-only
> >> parameter as follows:
> >>
> >> "TRUE means page table is modified. FALSE means page table is not
> >> modified."
> >>
> >> This states that "IsModified" is always set on output, to either
> >> FALSE or TRUE. Which is an incorrect statement; in reality the caller
> >> is expected to pre-set (*IsModified) to FALSE, and
> >> PageTableLibMapInLevel() will (conditionally!) perform a FALSE->TRUE
> transition only.
> >>
> >> Now, this patch may fix a bug, but it makes the above-described
> >> documentation issue worse, by further restricting the condition for
> >> said
> >> FALSE->TRUE transition.
> >
> > Laszlo, thanks for the comments!
> > Though the fixing looks simple, Zhiguang and I did have several rounds
> > of offline discussions regarding how to fix it.
> >
> > When the lib accesses the page table content, CPU would set the
> > "Access" bit in the page entry that points to the page table memory being
> accessed by the lib.
> >
> > So, even when the "Modify" is FALSE (indicating caller doesn't want
> > the lib to modify the page table), lib code should not modify the page
> > table but CPU still sets the "Access" bit in some of the entries due to the
> reasons above.
> 
> Huh, tricky!
> 
> Should the comparison explicitly mask out the Accessed bit from each of the
> "before" page table entry and the "after" one, perhaps?
> 
> > I agree it will be better that the commit message carries above details.
> >
> > Zhiguang,
> > Can we update the code to always assign "IsModified"? I thought we did
> that but it seems not.
> 
> That seems doable by (e.g.) setting (*IsModified) to FALSE right at the top of
> the function, and then the logic would match the existent comments, I think.
> However, I've not checked whether callers actually rely on this "summing" logic
> of the IsModified output parameter -- like call the function a number of times
> in a row, using a common local variable to receive IsModified, and then check
> the local variable to see if *any one* of the calls in the loop has modified the
> page table.
> 
> Thanks
> Laszlo
> 
> >
> >>
> >> The fix per se looks vaguely reasonable to me (really the function is
> >> so complicated that verifying this change from scratch would take me
> >> ages), but minimally, the documentation of "IsModified" should
> >> certainly be updated too. To something like this:
> >>
> >>   @param[out] IsModified  If "Modify" is TRUE on input and the function
> >>                           has actually modified the page table, then
> >> set
> >>                           to TRUE on output. Not overwritten
> >> otherwise.
> >>
> >> Laszlo
> >
> >
> >
> >
> >
> >
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113787): https://edk2.groups.io/g/devel/message/113787
Mute This Topic: https://groups.io/mt/103636407/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] UefiCpuPkg: Fix issue that IsModified is wrongly set in PageTableMap

[Laszlo Ersek]

On 1/15/24 03:59, Liu, Zhiguang wrote:
> Hi Laszlo,
> 
> I don't think it is a good idea to explicitly mask out the Accessed/Dirty bit. We can't assume Accessed/Dirty bit are only changed by hardware, because the caller also can change the Accessed/Dirty bit.
> 
> For API PageTableMap, the IsModified is already set as False in the beginning of the function.
> For internal function PageTableLibMapInLevel, we don't set IsModified as False in the beginning on purpose, because it keeps the global state of whether the PageTable is changed.
> 
> I plan to change the comment as below to explicitly explain the behavior:
> 
> For internal function PageTableLibMapInLevel, the description of IsModified should be:
> @param[in, out]     IsModified     change IsModified to True if page table is modified and input parameter Modify is TRUE.
> 
> For API PageTableMap, the description of IsModified should be: 
> @param[out]     IsModified     TRUE means page table is modified by software or hardware. FALSE means page table is not modified by software.
> If the output IsModified is FALSE, there is possibility that the page table is changed by hardware. It is ok because page table can be changed by hardware anytime, and we don't need to Flush TLB.
> 
> With these comments changed, I don't need to change C code in my patch.

OK, sounds reasonable to me. Thanks.

> 
> BTW, I assume IsModified can be used as an indicator whether the caller need to flush TLB. Do you prefer to change the parameter name to IsFlushTlbNeeded? I am both fine.

I think the new description of the parameter suffices (without renaming
the parameter).

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113838): https://edk2.groups.io/g/devel/message/113838
Mute This Topic: https://groups.io/mt/103636407/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] UefiCpuPkg: Fix issue that IsModified is wrongly set in PageTableMap

[Pedro Falcato]

On Thu, Jan 11, 2024 at 8:56 AM Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 1/11/24 03:03, Ni, Ray wrote:
> >> This function is incredibly complicated, so reviewing this patch is
> >> hard, even after reading the bugzilla ticket.
> >>
> >> The commit message is useless. It should contain a brief description of
> >> the problem, and how the fix resolves the problem.
> >>
> >> The documentation of the PageTableLibMapInLevel() function is wrong,
> >> even before this patch. It documents the "IsModified" output-only
> >> parameter as follows:
> >>
> >> "TRUE means page table is modified. FALSE means page table is not
> >> modified."
> >>
> >> This states that "IsModified" is always set on output, to either FALSE
> >> or TRUE. Which is an incorrect statement; in reality the caller is
> >> expected to pre-set (*IsModified) to FALSE, and PageTableLibMapInLevel()
> >> will (conditionally!) perform a FALSE->TRUE transition only.
> >>
> >> Now, this patch may fix a bug, but it makes the above-described
> >> documentation issue worse, by further restricting the condition for said
> >> FALSE->TRUE transition.
> >
> > Laszlo, thanks for the comments!
> > Though the fixing looks simple, Zhiguang and I did have several rounds of offline discussions
> > regarding how to fix it.
> >
> > When the lib accesses the page table content, CPU would set the "Access" bit in the page entry
> > that points to the page table memory being accessed by the lib.
> >
> > So, even when the "Modify" is FALSE (indicating caller doesn't want the lib to modify the page table),
> > lib code should not modify the page table but CPU still sets the "Access" bit in some of the entries due to
> > the reasons above.
>
> Huh, tricky!
>
> Should the comparison explicitly mask out the Accessed bit from each of
> the "before" page table entry and the "after" one, perhaps?

FWIW, clearing the A and D bits off of PTEs requires a TLB flush and,
as such, that change would break them.

In general:
 - You need a TLB flush when unmapping a page
 - You need a TLB flush when changing an already-mapped PTE (unless
you tolerate a stale TLB and want to eat a spurious page fault, which
is a valid technique)
 - You don't need a TLB flush when freshly mapping a page (unmapped ->
mapped) as x86 doesn't cache non-present PTEs

so you shouldn't need to inspect the PTE before and after; in fact,
that's erroneous as Intel CPUs can speculatively set the A and D bits
(they're slightly more careful since CET rolled around, but as far as
I've heard older Intel used to wildly set those bits speculatively)
and AMD ones can too (although they cannot speculatively set D).

I'd love to give out more feedback on this patch, but I *really* don't
understand what any of that function is doing :/

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113813): https://edk2.groups.io/g/devel/message/113813
Mute This Topic: https://groups.io/mt/103636407/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] UefiCpuPkg: Fix issue that IsModified is wrongly set in PageTableMap

[Laszlo Ersek]

On 1/15/24 10:43, Pedro Falcato wrote:
> On Thu, Jan 11, 2024 at 8:56 AM Laszlo Ersek <lersek@redhat.com> wrote:
>>
>> On 1/11/24 03:03, Ni, Ray wrote:
>>>> This function is incredibly complicated, so reviewing this patch is
>>>> hard, even after reading the bugzilla ticket.
>>>>
>>>> The commit message is useless. It should contain a brief description of
>>>> the problem, and how the fix resolves the problem.
>>>>
>>>> The documentation of the PageTableLibMapInLevel() function is wrong,
>>>> even before this patch. It documents the "IsModified" output-only
>>>> parameter as follows:
>>>>
>>>> "TRUE means page table is modified. FALSE means page table is not
>>>> modified."
>>>>
>>>> This states that "IsModified" is always set on output, to either FALSE
>>>> or TRUE. Which is an incorrect statement; in reality the caller is
>>>> expected to pre-set (*IsModified) to FALSE, and PageTableLibMapInLevel()
>>>> will (conditionally!) perform a FALSE->TRUE transition only.
>>>>
>>>> Now, this patch may fix a bug, but it makes the above-described
>>>> documentation issue worse, by further restricting the condition for said
>>>> FALSE->TRUE transition.
>>>
>>> Laszlo, thanks for the comments!
>>> Though the fixing looks simple, Zhiguang and I did have several rounds of offline discussions
>>> regarding how to fix it.
>>>
>>> When the lib accesses the page table content, CPU would set the "Access" bit in the page entry
>>> that points to the page table memory being accessed by the lib.
>>>
>>> So, even when the "Modify" is FALSE (indicating caller doesn't want the lib to modify the page table),
>>> lib code should not modify the page table but CPU still sets the "Access" bit in some of the entries due to
>>> the reasons above.
>>
>> Huh, tricky!
>>
>> Should the comparison explicitly mask out the Accessed bit from each of
>> the "before" page table entry and the "after" one, perhaps?
> 
> FWIW, clearing the A and D bits off of PTEs requires a TLB flush and,
> as such, that change would break them.

I didn't mean to clear the A/D bits inside the actual live PTEs, only in
those temporary / helper variables (or even just expressions) that we
use for comparing the before/after states.

> 
> In general:
>  - You need a TLB flush when unmapping a page
>  - You need a TLB flush when changing an already-mapped PTE (unless
> you tolerate a stale TLB and want to eat a spurious page fault, which
> is a valid technique)
>  - You don't need a TLB flush when freshly mapping a page (unmapped ->
> mapped) as x86 doesn't cache non-present PTEs
> 
> so you shouldn't need to inspect the PTE before and after;

this seems to invite further discussion wrt. what the function is
supposed to do at all...

> in fact,
> that's erroneous as Intel CPUs can speculatively set the A and D bits
> (they're slightly more careful since CET rolled around, but as far as
> I've heard older Intel used to wildly set those bits speculatively)
> and AMD ones can too (although they cannot speculatively set D).

What is the error behavior here? Assuming we consider a speculative A/D
setting by the hardware, the worst that can happen is that we spuriously
flush the TLB. Is that right? Doesn't seem extremely harmful.

> 
> I'd love to give out more feedback on this patch, but I *really* don't
> understand what any of that function is doing :/
> 

Yup.

(In general I'm just acknowledging that right now this is quite out of
my league...)

Thanks,
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113839): https://edk2.groups.io/g/devel/message/113839
Mute This Topic: https://groups.io/mt/103636407/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-