Messages in this thread | | | Date | Tue, 8 Mar 2022 19:24:57 +0100 | Subject | Re: [PATCH v2 12/25] KVM: x86/mmu: cleanup computation of MMU roles for two-dimensional paging | From | 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
| |