Re: [PATCH 07/16] x86/shadow: call sh_update_cr3() directly from sh_page_fault()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 22/03/2023 9:33 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>
> ---
> 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.)
>
> In any event emitting a TRC_SHADOW_DOMF_DYING trace record in this case
> looks wrong.
>
> 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 that there was a complicated bug, ultimately discovered because
Win7 changed behaviour vs older versions, and the shadow logic had been
written to AMD's PAE behaviour, not Intel's.

Remember that Intel and AMD differer in how PAE paging works between
root and non-root mode, and it is to do with whether all PDPTRs get
cached at once, or on demand.

Off the top of my head:
* 32bit PV guests get on-demand semantics (as they're really 4-level)
* VT-x strictly use architectural "PDPTRs get cached on mov to CR3"
semantics
* SVM with NPT have on-demand semantics
* SVM with shadow is model-specific as to which semantics is uses, IIRC
Fam15h and later are on-demand

These differences still manifest bugs in the common HVM code, the PTE
caching code, and the pagewalk code.

Looking at the comment specifically, I'm pretty sure it's wrong.  I
think that suggests we've got even more PDPTR bugs than I'd previously
identified.  In some copious free time, I do need to extend the
pagetable-emul XTF test to include PDPTR updates because it's the one
area of pagewalking that doesn't have any suitable testing right now.

~Andrew