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