Re: [RFC PATCH v5 042/104] KVM: x86/mmu: Track shadow MMIO value/mask on a per-VM basis

[Sean Christopherson <seanjc@google.com>]

Sorry, missed my name...

On Fri, Apr 08, 2022, Isaku Yamahata wrote:
> On Tue, Apr 05, 2022 at 05:25:34PM +0200,
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> > On 3/4/22 20:48, isaku.yamahata@intel.com wrote:
> > > +	if (enable_ept) {
> > > +		const u64 init_value = enable_tdx ? VMX_EPT_SUPPRESS_VE_BIT : 0ull;
> > >   		kvm_mmu_set_ept_masks(enable_ept_ad_bits,
> > > -				      cpu_has_vmx_ept_execute_only());
> > > +				      cpu_has_vmx_ept_execute_only(), init_value);
> > > +		kvm_mmu_set_spte_init_value(init_value);
> > > +	}
> > 
> > I think kvm-intel.ko should use VMX_EPT_SUPPRESS_VE_BIT unconditionally as
> > the init value.  The bit is ignored anyway if the "EPT-violation #VE"
> > execution control is 0.
> > Otherwise looks good, but I have a couple more crazy ideas:
> > 
> > 1) there could even be a test mode where KVM enables the execution control,
> > traps #VE in the exception bitmap, and shouts loudly if it gets a #VE.  That
> > might avoid hard-to-find bugs due to forgetting about
> > VMX_EPT_SUPPRESS_VE_BIT.
> > 
> > 2) or even, perhaps the init_value for the TDP MMU could set bit 63
> > _unconditionally_, because KVM always sets the NX bit on AMD hardware.

Heh, took me a minute to realize you mean EFER.NX.  To clarify:

KVM requires NX support in hardware

	if (!boot_cpu_has(X86_FEATURE_NX)) {
		pr_err_ratelimited("NX (Execute Disable) not supported\n");
		return -EOPNOTSUPP;
	}

and 64-bit or PAE paging to enable NPT

	if (!IS_ENABLED(CONFIG_X86_64) && !IS_ENABLED(CONFIG_X86_PAE))
		npt_enabled = false;

and the _kernel_ forces EFER.NX=1 for 64-bit and PAE kernels.

But whether or not EFER.NX is enabled is irrelevant, it's only the initial value,
i.e. the SPTE is guaranteed to be !PRESENT, so hardware will never generate a
reserved bit #PF.

> > That would remove the whole infrastructure to keep shadow_init_value,
> > because it would be constant 0 in mmu.c and constant BIT(63) in tdp_mmu.c.
> > 
> > Sean, what do you think?

I like #2, though I suspect we'll still want shadow_init_value so that the MMU
caches can be shared without creating a mess.   But I still like keeping that
detail in the MMUs and out of the vendor modules, even though there's obviously
a hard dependency on the MMU doing the right thing.

> Then, I'll start with 1) because it's a bit hard for me to test 2) with real AMD
> hardware.  If someone is willing to test 2), I'm quite fine to implement 2)
> on top of 1).  2) isn't exclusive with 1).

I can test #2.

Tangentially related, the kvm_gfn_stolen_mask() exception to MMIO SPTEs is unnecessarily
convoluted and gross.  That's partly my fault as I should have just updated
enable_mmio_caching when hardware can't support it instead of using shadow_mmio_value
to convey that information.  I'll submit a patch to fix that, then is_mmio_spte()
can be left alone in the TDX series.