Reviewed-by: Ray Ni <ray...@intel.com> Thanks, Ray > -----Original Message----- > From: Liu, Zhiguang <zhiguang....@intel.com> > Sent: Tuesday, January 23, 2024 3:16 PM > To: devel@edk2.groups.io > Cc: Liu, Zhiguang <zhiguang....@intel.com>; Ni, Ray <ray...@intel.com>; Laszlo > Ersek <ler...@redhat.com>; Kumar, Rahul R <rahul.r.ku...@intel.com>; Gerd > Hoffmann <kra...@redhat.com>; Lee, Crystal <crystal...@ami.com.tw>; > Pedro Falcato <pedro.falc...@gmail.com> > Subject: [PATCH v3] UefiCpuPkg: Fix issue that IsModified is wrongly set in > PageTableMap > > 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 adding addtional condition to only check if the page > table is changed when the software want to modify the page table. > Also, add more comment to explain this behavior. > > 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> > Cc: Pedro Falcato <pedro.falc...@gmail.com> > Signed-off-by: Zhiguang Liu <zhiguang....@intel.com> > --- > .../Library/CpuPageTableLib/CpuPageTableMap.c | 18 +++++++++++++----- > 1 file changed, 13 insertions(+), 5 deletions(-) > > diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c > b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c > index 36b2c4e6a3..ea6547970a 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. > @@ -294,7 +294,7 @@ PageTableLibMapInLevel ( > IN UINT64 Offset, > IN IA32_MAP_ATTRIBUTE *Attribute, > IN IA32_MAP_ATTRIBUTE *Mask, > - OUT BOOLEAN *IsModified > + IN OUT BOOLEAN *IsModified > ) > { > RETURN_STATUS Status; > @@ -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 (#114281): https://edk2.groups.io/g/devel/message/114281 Mute This Topic: https://groups.io/mt/103906298/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
