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

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

Hi Laszlo,

I don't think it is a good idea to explicitly mask out the Accessed/Dirty bit. We can't assume Accessed/Dirty bit are only changed by hardware, because the caller also can change the Accessed/Dirty bit.

For API PageTableMap, the IsModified is already set as False in the beginning of the function.
For internal function PageTableLibMapInLevel, we don't set IsModified as False in the beginning on purpose, because it keeps the global state of whether the PageTable is changed.

I plan to change the comment as below to explicitly explain the behavior:

For internal function PageTableLibMapInLevel, the description of IsModified should be:
@param[in, out]     IsModified     change IsModified to True if page table is modified and input parameter Modify is TRUE.

For API PageTableMap, the description of IsModified should be: 
@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 we don't need to Flush TLB.

With these comments changed, I don't need to change C code in my patch.

BTW, I assume IsModified can be used as an indicator whether the caller need to flush TLB. Do you prefer to change the parameter name to IsFlushTlbNeeded? I am both fine.

Thanks
Zhiguang

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
> Ersek
> Sent: Thursday, January 11, 2024 4:57 PM
> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Liu, Zhiguang
> <zhiguang.liu@intel.com>
> Cc: Kumar, Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffmann
> <kraxel@redhat.com>; Lee, Crystal <crystalLee@ami.com.tw>
> Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg: Fix issue that IsModified is
> wrongly set in PageTableMap
> 
> On 1/11/24 03:03, Ni, Ray wrote:
> >> 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.
> >
> > Laszlo, thanks for the comments!
> > Though the fixing looks simple, Zhiguang and I did have several rounds
> > of offline discussions regarding how to fix it.
> >
> > When the lib accesses the page table content, CPU would set the
> > "Access" bit in the page entry that points to the page table memory being
> accessed by the lib.
> >
> > So, even when the "Modify" is FALSE (indicating caller doesn't want
> > the lib to modify the page table), lib code should not modify the page
> > table but CPU still sets the "Access" bit in some of the entries due to the
> reasons above.
> 
> Huh, tricky!
> 
> Should the comparison explicitly mask out the Accessed bit from each of the
> "before" page table entry and the "after" one, perhaps?
> 
> > I agree it will be better that the commit message carries above details.
> >
> > Zhiguang,
> > Can we update the code to always assign "IsModified"? I thought we did
> that but it seems not.
> 
> That seems doable by (e.g.) setting (*IsModified) to FALSE right at the top of
> the function, and then the logic would match the existent comments, I think.
> However, I've not checked whether callers actually rely on this "summing" logic
> of the IsModified output parameter -- like call the function a number of times
> in a row, using a common local variable to receive IsModified, and then check
> the local variable to see if *any one* of the calls in the loop has modified the
> page table.
> 
> Thanks
> Laszlo
> 
> >
> >>
> >> 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 (#113787): https://edk2.groups.io/g/devel/message/113787
Mute This Topic: https://groups.io/mt/103636407/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-