On 1/15/24 10:43, Pedro Falcato wrote: > On Thu, Jan 11, 2024 at 8:56 AM Laszlo Ersek <ler...@redhat.com> wrote: >> >> 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? > > FWIW, clearing the A and D bits off of PTEs requires a TLB flush and, > as such, that change would break them.
I didn't mean to clear the A/D bits inside the actual live PTEs, only in those temporary / helper variables (or even just expressions) that we use for comparing the before/after states. > > In general: > - You need a TLB flush when unmapping a page > - You need a TLB flush when changing an already-mapped PTE (unless > you tolerate a stale TLB and want to eat a spurious page fault, which > is a valid technique) > - You don't need a TLB flush when freshly mapping a page (unmapped -> > mapped) as x86 doesn't cache non-present PTEs > > so you shouldn't need to inspect the PTE before and after; this seems to invite further discussion wrt. what the function is supposed to do at all... > in fact, > that's erroneous as Intel CPUs can speculatively set the A and D bits > (they're slightly more careful since CET rolled around, but as far as > I've heard older Intel used to wildly set those bits speculatively) > and AMD ones can too (although they cannot speculatively set D). What is the error behavior here? Assuming we consider a speculative A/D setting by the hardware, the worst that can happen is that we spuriously flush the TLB. Is that right? Doesn't seem extremely harmful. > > I'd love to give out more feedback on this patch, but I *really* don't > understand what any of that function is doing :/ > Yup. (In general I'm just acknowledging that right now this is quite out of my league...) Thanks, Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113839): https://edk2.groups.io/g/devel/message/113839 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] -=-=-=-=-=-=-=-=-=-=-=-
