Re: [PATCH v3 02/12] x86/shadow: call sh_update_cr3() directly from sh_page_fault()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 16/05/2023 8:38 am, Jan Beulich wrote:
> There's no need for an indirect call here, as the mode is invariant
> throughout the entire paging-locked region. All it takes to avoid it is
> to have a forward declaration of sh_update_cr3() in place.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

> ---
> I find this and the respective Win7 related comment suspicious: If we
> really need to "fix up" L3 entries "on demand", wouldn't we better retry
> the shadow_get_and_create_l1e() rather than exit? The spurious page
> fault that the guest observes can, after all, not be known to be non-
> fatal inside the guest. That's purely an OS policy.
>
> Furthermore the sh_update_cr3() will also invalidate L3 entries which
> were loaded successfully before, but invalidated by the guest
> afterwards. I strongly suspect that the described hardware behavior is
> _only_ to load previously not-present entries from the PDPT, but not
> purge ones already marked present. IOW I think sh_update_cr3() would
> need calling in an "incremental" mode here. (The alternative of doing
> this in shadow_get_and_create_l3e() instead would likely be more
> cumbersome.)
>
> Beyond the "on demand" L3 entry creation I also can't see what guest
> actions could lead to the ASSERT() being inapplicable in the PAE case.
> The 3-level code in shadow_get_and_create_l2e() doesn't consult guest
> PDPTEs, and all other logic is similar to that for other modes.
>
> (See 89329d832aed ["x86 shadow: Update cr3 in PAE mode when guest walk
> succeed but shadow walk fails"].)

I recall this being an issue in flight when I joined Citrix, and relates
to PDPTRs.

Architecturally, PDPTRs are are loaded on MOV CR3, the TLB flushing
subset of MOV CR0/4, and Task Switch, and are otherwise non-coherent
with RAM.  PDPTRs are not shot down by INVLPG/#PF/etc (they're not part
of the TLB state, they're "roots" of the paging structures like CR3 is).

(If you recall, this was one of my major concerns with your PTE caching
series, and I still need to figure out how to make this case work.)

Intel maintains the behaviour with 4x PDPTR fields in the VMCS, but AMD
punted on the problem by declaring that 32bit PAE paging under NPT would
behave as "normal" PTEs and get loaded on demand.

For a while, there was a threat in the APM saying that this behaviour
might start applying to native scenarios too, but I recall AMD saying
that this hadn't happened and was unlikely to.


Either way.  All OSes need to cope with both behaviour.  It's probably
easiest for Shadow to stick to strictly architectural behaviour, rather
than trying to account for a behaviour that only manifests in another
context.

I vaguely recall discussion of Windows 7 expecting one behaviour and
shadow implementing the other, but it was 15 years ago and probably at
least partial speculation over an unfixed bug.

~Andrew