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...@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> > --- > 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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-