Re: [PATCH v2 12/25] KVM: x86/mmu: cleanup computation of MMU roles for two-dimensional paging

[Paolo Bonzini]

On 3/8/22 19:11, Sean Christopherson wrote:
> On Mon, Feb 21, 2022, Paolo Bonzini wrote:
> > Extended bits are unnecessary because page walking uses the CPU mode,
> > and EFER.NX/CR0.WP can be set to one unconditionally---matching the
> > format of shadow pages rather than the format of guest pages.
>
> But they don't match the format of shadow pages.  EPT has an equivalent to NX in
> that KVM can always clear X, but KVM explicitly supports running with EPT and
> EFER.NX=0 in the host (32-bit non-PAE kernels).

In which case bit 2 of EPTs doesn't change meaning, does it?

> CR0.WP equally confusing.  Yes, both EPT and NPT enforce write protection at all
> times, but EPT has no concept of user vs. supervisor in the EPT tables themselves,
> at least with respect to writes (thanks mode-based execution for the qualifier...).
> NPT is even worse as the APM explicitly states:
>
>    The host hCR0.WP bit is ignored under nested paging.
>
> Unless there's some hidden dependency I'm missing, I'd prefer we arbitrarily leave
> them zero.

Setting EFER.NX=0 might be okay for EPT/NPT, but I'd prefer to set it 
respectively to 1 (X bit always present) and host EFER.NX (NX bit 
present depending on host EFER).

For CR0.WP it should really be 1 in my opinion, because CR0.WP=0 implies 
having a concept of user vs. supervisor access: CR0.WP=1 is the 
"default", while CR0.WP=0 is "always allow *supervisor* writes".

> > even if only barely so, due to SMM and guest mode; for consistency,
> > pass it down to kvm_calc_tdp_mmu_root_page_role instead of querying
> > the vcpu with is_smm or is_guest_mode.
>
> The changelog should call out this is a _significant_ change in behavior for KVM,
> as it allows reusing shadow pages with different guest MMU "role bits".

Good point!  It's safe and arguably clea{n,r}er, but it's still a pretty 
large change.

> E.g. if this lands after the changes to not unload MMUs on cr0/cr4
> emulation, it will be quite the functional change.
I expect this to land first, so that the part where we don't really 
agree on the implementation comes last and benefits from a more 
understandable core.

Paolo