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

About the IsModified, current function doesn't consider that hardware
also may change the pagetable. The issue is that in the first call of
internal function PageTableLibMapInLevel, the function assume page
table is not changed, and add ASSERT to check. But hardware may change
the page table, which cause the ASSERT happens.
Fix the issue by considering the hardware may also change page table,
and document the detail in function header.

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>
---
 .../Library/CpuPageTableLib/CpuPageTableMap.c    | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
index 36b2c4e6a3..a3076ff2f6 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
@@ -274,7 +274,7 @@ IsAttributesAndMaskValidForNonPresentEntry (
                                     Page table entries that map the linear address range are reset to 0 before set to the new attribute
                                     when a new physical base address is set.
   @param[in]      Mask              The mask used for attribute. The corresponding field in Attribute is ignored if that in Mask is 0.
-  @param[out]     IsModified        TRUE means page table is modified. FALSE means page table is not modified.
+  @param[in, out] IsModified        Change IsModified to True if page table is modified and input parameter Modify is TRUE.
 
   @retval RETURN_INVALID_PARAMETER  For non-present range, Mask->Bits.Present is 0 but some other attributes are provided.
   @retval RETURN_INVALID_PARAMETER  For non-present range, Mask->Bits.Present is 1, Attribute->Bits.Present is 1 but some other attributes are not provided.
@@ -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;
   }
 
@@ -633,7 +639,9 @@ PageTableLibMapInLevel (
                                  Page table entries that map the linear address range are reset to 0 before set to the new attribute
                                  when a new physical base address is set.
   @param[in]      Mask           The mask used for attribute. The corresponding field in Attribute is ignored if that in Mask is 0.
-  @param[out]     IsModified     TRUE means page table is modified. FALSE means page table is not modified.
+  @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 caller don't need to Flush TLB.
 
   @retval RETURN_UNSUPPORTED        PagingMode is not supported.
   @retval RETURN_INVALID_PARAMETER  PageTable, BufferSize, Attribute or Mask is NULL.
-- 
2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113936): https://edk2.groups.io/g/devel/message/113936
Mute This Topic: https://groups.io/mt/103781942/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 1/17/24 09:09, Zhiguang Liu wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4614
> 
> About the IsModified, current function doesn't consider that hardware
> also may change the pagetable. The issue is that in the first call of
> internal function PageTableLibMapInLevel, the function assume page
> table is not changed, and add ASSERT to check. But hardware may change
> the page table, which cause the ASSERT happens.
> Fix the issue by considering the hardware may also change page table,
> and document the detail in function header.
> 
> 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>
> ---
>  .../Library/CpuPageTableLib/CpuPageTableMap.c    | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> index 36b2c4e6a3..a3076ff2f6 100644
> --- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> +++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> @@ -274,7 +274,7 @@ IsAttributesAndMaskValidForNonPresentEntry (
>                                      Page table entries that map the linear address range are reset to 0 before set to the new attribute
>                                      when a new physical base address is set.
>    @param[in]      Mask              The mask used for attribute. The corresponding field in Attribute is ignored if that in Mask is 0.
> -  @param[out]     IsModified        TRUE means page table is modified. FALSE means page table is not modified.
> +  @param[in, out] IsModified        Change IsModified to True if page table is modified and input parameter Modify is TRUE.
>  
>    @retval RETURN_INVALID_PARAMETER  For non-present range, Mask->Bits.Present is 0 but some other attributes are provided.
>    @retval RETURN_INVALID_PARAMETER  For non-present range, Mask->Bits.Present is 1, Attribute->Bits.Present is 1 but some other attributes are not provided.
> @@ -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;
>    }
>  
> @@ -633,7 +639,9 @@ PageTableLibMapInLevel (
>                                   Page table entries that map the linear address range are reset to 0 before set to the new attribute
>                                   when a new physical base address is set.
>    @param[in]      Mask           The mask used for attribute. The corresponding field in Attribute is ignored if that in Mask is 0.
> -  @param[out]     IsModified     TRUE means page table is modified. FALSE means page table is not modified.
> +  @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 caller don't need to Flush TLB.
>  
>    @retval RETURN_UNSUPPORTED        PagingMode is not supported.
>    @retval RETURN_INVALID_PARAMETER  PageTable, BufferSize, Attribute or Mask is NULL.

This patch looks good to me, thanks, except for one small wart: in the
documentation section of PageTableLibMapInLevel(), you change IsModified
from "@param[out]" to "@param[in, out]", which is correct, *but* a
similar change has been omitted for the actual parameter in the
parameter list:

  OUT    BOOLEAN             *IsModified

This should also become "IN OUT".

Thanks!
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113948): https://edk2.groups.io/g/devel/message/113948
Mute This Topic: https://groups.io/mt/103781942/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Thanks Laszlo for the comment, I will send a new version of patch to fix this.

Also include Pedro to see if Pedro have more comments.

Thanks
Zhiguang

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
> Ersek
> Sent: Wednesday, January 17, 2024 6:51 PM
> To: devel@edk2.groups.io; Liu, Zhiguang <zhiguang.liu@intel.com>
> Cc: Ni, Ray <ray.ni@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>;
> Gerd Hoffmann <kraxel@redhat.com>; Lee, Crystal <crystalLee@ami.com.tw>
> Subject: Re: [edk2-devel] [PATCH v2] UefiCpuPkg: Fix issue that IsModified is
> wrongly set in PageTableMap
> 
> On 1/17/24 09:09, Zhiguang Liu wrote:
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4614
> >
> > About the IsModified, current function doesn't consider that hardware
> > also may change the pagetable. The issue is that in the first call of
> > internal function PageTableLibMapInLevel, the function assume page
> > table is not changed, and add ASSERT to check. But hardware may change
> > the page table, which cause the ASSERT happens.
> > Fix the issue by considering the hardware may also change page table,
> > and document the detail in function header.
> >
> > 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>
> > ---
> >  .../Library/CpuPageTableLib/CpuPageTableMap.c    | 16 ++++++++++++----
> >  1 file changed, 12 insertions(+), 4 deletions(-)
> >
> > diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> > b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> > index 36b2c4e6a3..a3076ff2f6 100644
> > --- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> > +++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> > @@ -274,7 +274,7 @@ IsAttributesAndMaskValidForNonPresentEntry (
> >                                      Page table entries that map the linear address range are
> reset to 0 before set to the new attribute
> >                                      when a new physical base address is set.
> >    @param[in]      Mask              The mask used for attribute. The corresponding
> field in Attribute is ignored if that in Mask is 0.
> > -  @param[out]     IsModified        TRUE means page table is modified. FALSE
> means page table is not modified.
> > +  @param[in, out] IsModified        Change IsModified to True if page table is
> modified and input parameter Modify is TRUE.
> >
> >    @retval RETURN_INVALID_PARAMETER  For non-present range, Mask-
> >Bits.Present is 0 but some other attributes are provided.
> >    @retval RETURN_INVALID_PARAMETER  For non-present range, Mask-
> >Bits.Present is 1, Attribute->Bits.Present is 1 but some other attributes are
> not provided.
> > @@ -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;
> >    }
> >
> > @@ -633,7 +639,9 @@ PageTableLibMapInLevel (
> >                                   Page table entries that map the linear address range are
> reset to 0 before set to the new attribute
> >                                   when a new physical base address is set.
> >    @param[in]      Mask           The mask used for attribute. The corresponding
> field in Attribute is ignored if that in Mask is 0.
> > -  @param[out]     IsModified     TRUE means page table is modified. FALSE
> means page table is not modified.
> > +  @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 caller don't need to Flush TLB.
> >
> >    @retval RETURN_UNSUPPORTED        PagingMode is not supported.
> >    @retval RETURN_INVALID_PARAMETER  PageTable, BufferSize, Attribute or
> Mask is NULL.
> 
> This patch looks good to me, thanks, except for one small wart: in the
> documentation section of PageTableLibMapInLevel(), you change IsModified
> from "@param[out]" to "@param[in, out]", which is correct, *but* a similar
> change has been omitted for the actual parameter in the parameter list:
> 
>   OUT    BOOLEAN             *IsModified
> 
> This should also become "IN OUT".
> 
> Thanks!
> Laszlo
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113979): https://edk2.groups.io/g/devel/message/113979
Mute This Topic: https://groups.io/mt/103781942/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Thu, Jan 18, 2024 at 2:21 AM Liu, Zhiguang <zhiguang.liu@intel.com> wrote:
>
> Thanks Laszlo for the comment, I will send a new version of patch to fix this.
>
> Also include Pedro to see if Pedro have more comments.

The patch's subject really doesn't describe the fix (describe what you
did in the patch/commit, don't describe what you're fixing)

I also liked your IsFlushTlbNeeded suggestion (it would help clarify
what you actually want to keep track of).


--
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113998): https://edk2.groups.io/g/devel/message/113998
Mute This Topic: https://groups.io/mt/103781942/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

hi Pedro,
Thanks for the comments.
I will describe more about how to fix it in the commit message.
About the renaming, I hesitate about it, since there may be other usage about this param. However, I don't think it is a big concern. I will keep it as is for now.

Thanks
Zhiguang

> -----Original Message-----
> From: Pedro Falcato <pedro.falcato@gmail.com>
> Sent: Friday, January 19, 2024 12:19 AM
> To: Liu, Zhiguang <zhiguang.liu@intel.com>
> Cc: devel@edk2.groups.io; lersek@redhat.com; Ni, Ray <ray.ni@intel.com>;
> Kumar, Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffmann
> <kraxel@redhat.com>; Lee, Crystal <crystalLee@ami.com.tw>
> Subject: Re: [edk2-devel] [PATCH v2] UefiCpuPkg: Fix issue that IsModified is
> wrongly set in PageTableMap
> 
> On Thu, Jan 18, 2024 at 2:21 AM Liu, Zhiguang <zhiguang.liu@intel.com>
> wrote:
> >
> > Thanks Laszlo for the comment, I will send a new version of patch to fix this.
> >
> > Also include Pedro to see if Pedro have more comments.
> 
> The patch's subject really doesn't describe the fix (describe what you did in the
> patch/commit, don't describe what you're fixing)
> 
> I also liked your IsFlushTlbNeeded suggestion (it would help clarify what you
> actually want to keep track of).
> 
> 
> --
> Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114020): https://edk2.groups.io/g/devel/message/114020
Mute This Topic: https://groups.io/mt/103781942/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-