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>
