Re: [PATCH v7 037/102] KVM: x86/mmu: Track shadow MMIO value/mask on a per-VM basis

[Isaku Yamahata]

On Wed, Jul 20, 2022 at 03:45:59PM +1200,
Kai Huang <kai.huang@xxxxxxxxx> wrote:

> > @@ -337,9 +335,8 @@ u64 mark_spte_for_access_track(u64 spte)
> >  	return spte;
> >  }
> >  
> > -void kvm_mmu_set_mmio_spte_mask(u64 mmio_value, u64 mmio_mask, u64 access_mask)
> > +void kvm_mmu_set_mmio_spte_mask(struct kvm *kvm, u64 mmio_value, u64 mmio_mask)
> >  {
> > -	BUG_ON((u64)(unsigned)access_mask != access_mask);
> >  	WARN_ON(mmio_value & shadow_nonpresent_or_rsvd_lower_gfn_mask);
> >  
> >  	if (!enable_mmio_caching)
> > @@ -366,12 +363,9 @@ void kvm_mmu_set_mmio_spte_mask(u64 mmio_value, u64 mmio_mask, u64 access_mask)
> >  	    WARN_ON(mmio_value && (__REMOVED_SPTE & mmio_mask) == mmio_value))
> >  		mmio_value = 0;
> >  
> > -	if (!mmio_value)
> > -		enable_mmio_caching = false;
> > -
> > -	shadow_mmio_value = mmio_value;
> > -	shadow_mmio_mask  = mmio_mask;
> > -	shadow_mmio_access_mask = access_mask;
> > +	kvm->arch.enable_mmio_caching = !!mmio_value;
> 
> KVM has a global enable_mmio_caching boolean, and I think we should honor it
> here (in this patch) by doing below first:
> 
> 	if (enabling_mmio_caching)
> 		mmio_value = 0;

This function already includes "if (!enable_mmio_caching) mmio_value = 0;" in
the beginning. (But not in this hunk, though).  So this patch honors the kernel
module parameter.


> > diff --git a/arch/x86/kvm/mmu/spte.h b/arch/x86/kvm/mmu/spte.h
> > index f5fd22f6bf5f..99bce92b596e 100644
> > --- a/arch/x86/kvm/mmu/spte.h
> > +++ b/arch/x86/kvm/mmu/spte.h
> > @@ -5,8 +5,6 @@
> >  
> >  #include "mmu_internal.h"
> >  
> > -extern bool __read_mostly enable_mmio_caching;
> > -
> 
> Here you removed the ability to control enable_mmio_caching globally.  It's not
> something you stated to do in the changelog.  Perhaps we should still keep it,
> and enforce it in kvm_mmu_set_mmio_spte_mask() as commented above.
> 
> And in upstream KVM, it is a module parameter.  What happens to it?

Ditto.  the upstredam kvm_mmu_set_mmio_spte_mask() has
"if (!enable_mmio_caching) mmio_value = 0;" and this patch keeps it.


> > diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> > index 36d2127cb7b7..52fb54880f9b 100644
> > --- a/arch/x86/kvm/vmx/tdx.c
> > +++ b/arch/x86/kvm/vmx/tdx.c
> > @@ -7,6 +7,7 @@
> >  #include "x86_ops.h"
> >  #include "tdx.h"
> >  #include "x86.h"
> > +#include "mmu.h"
> >  
> >  #undef pr_fmt
> >  #define pr_fmt(fmt) "tdx: " fmt
> > @@ -276,6 +277,9 @@ int tdx_vm_init(struct kvm *kvm)
> >  	int ret, i;
> >  	u64 err;
> >  
> > +	kvm_mmu_set_mmio_spte_mask(kvm, vmx_shadow_mmio_mask,
> > +				   vmx_shadow_mmio_mask);
> > +
> 
> I prefer to split this chunk out to another patch so this patch can be purely
> infrastructural.   In this way you can even move this patch around easily in
> this series.

Ok. I'll move it to a patch that touches TDX.
-- 
Isaku Yamahata <isaku.yamahata@xxxxxxxxx>