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....@intel.com> > Cc: Ni, Ray <ray...@intel.com>; Kumar, Rahul R <rahul.r.ku...@intel.com>; > Gerd Hoffmann <kra...@redhat.com>; Lee, Crystal <crystal...@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...@intel.com> > > Cc: Laszlo Ersek <ler...@redhat.com> > > Cc: Rahul Kumar <rahul1.ku...@intel.com> > > Cc: Gerd Hoffmann <kra...@redhat.com> > > Cc: Crystal Lee <crystal...@ami.com.tw> > > Signed-off-by: Zhiguang Liu <zhiguang....@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
