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

["Laszlo Ersek" <lersek@redhat.com>]

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 (#113598): https://edk2.groups.io/g/devel/message/113598
Mute This Topic: https://groups.io/mt/103636407/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-