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

["Pedro Falcato" <pedro.falcato@gmail.com>]

On Thu, Jan 11, 2024 at 8:56 AM Laszlo Ersek <lersek@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.

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; 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).

I'd love to give out more feedback on this patch, but I *really* don't
understand what any of that function is doing :/

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113813): https://edk2.groups.io/g/devel/message/113813
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]
-=-=-=-=-=-=-=-=-=-=-=-