> 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. 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. > > 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 (#113568): https://edk2.groups.io/g/devel/message/113568 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] -=-=-=-=-=-=-=-=-=-=-=-
