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

["Zhiguang Liu" <zhiguang.liu@intel.com>]

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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-