linux-kernel - Re: [PATCH v2 3/6] KVM: x86/mmu: only zap EPT when guest MTRR changes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <99c982ed53f0dd7e0723f566b5527091b7e4e54c.camel@intel.com>
Date:   Wed, 10 May 2023 10:54:51 +0000
From:   "Huang, Kai" <kai.huang@...el.com>
To:     "Zhao, Yan Y" <yan.y.zhao@...el.com>,
        "Gao, Chao" <chao.gao@...el.com>
CC:     "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "pbonzini@...hat.com" <pbonzini@...hat.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Christopherson,, Sean" <seanjc@...gle.com>
Subject: Re: [PATCH v2 3/6] KVM: x86/mmu: only zap EPT when guest MTRR changes

On Wed, 2023-05-10 at 16:00 +0800, Yan Zhao wrote:
> On Wed, May 10, 2023 at 01:39:25PM +0800, Chao Gao wrote:
> > On Tue, May 09, 2023 at 09:51:43PM +0800, Yan Zhao wrote:
> > > Call new helper kvm_zap_gfn_for_memtype() to skip zap mmu if EPT is not
> > > enabled.
> > > 
> > > When guest MTRR changes and it's desired to zap TDP entries to remove
> > > stale mappings, only do it when EPT is enabled, because only memory type
> > > of EPT leaf is affected by guest MTRR with noncoherent DMA present.
> > > 
> > > Signed-off-by: Yan Zhao <yan.y.zhao@...el.com>
> > > ---
> > > arch/x86/kvm/mtrr.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/x86/kvm/mtrr.c b/arch/x86/kvm/mtrr.c
> > > index 9fac1ec03463..62ebb9978156 100644
> > > --- a/arch/x86/kvm/mtrr.c
> > > +++ b/arch/x86/kvm/mtrr.c
> > > @@ -330,7 +330,7 @@ static void update_mtrr(struct kvm_vcpu *vcpu, u32 msr)
> > > 		var_mtrr_range(&mtrr_state->var_ranges[index], &start, &end);
> > > 	}
> > > 
> > > -	kvm_zap_gfn_range(vcpu->kvm, gpa_to_gfn(start), gpa_to_gfn(end));
> > > +	kvm_zap_gfn_for_memtype(vcpu->kvm, gpa_to_gfn(start), gpa_to_gfn(end));
> > 
> > I am wondering if the check of shadow_memtype_mask (now inside the
> > kvm_zap_gfn_for_memtype()) should be moved to the beginning of update_mtrr().
> > Because if EPT isn't enabled, computing @start/@end is useless and can be
> > skipped.
> 
> No, shadow_memtype_mask is not accessible in mtrr.c.
> It's better to confine it in mmu related files, e.g. mmu/mmu.c,
> mmu/spte.c
> 

No, I think it should be shadow_memtype_mask.

Conceptually, after commit 38bf9d7bf277 ("KVM: x86/mmu: Add shadow mask for
effective host MTRR memtype"), I believe in update_mtrr() the check:

	if (msr == MSR_IA32_CR_PAT || !tdp_enabled ||
              !kvm_arch_has_noncoherent_dma(vcpu->kvm))
                return;

should be:

	if (msr == MSR_IA32_CR_PAT || !shadow_memtype_mask ||
              !kvm_arch_has_noncoherent_dma(vcpu->kvm))
                return;

Because the intention of 'shadow_memtype_mask' is to use it as a dedicated
variable to represent hardware has capability to override the host MTRRs (which
is the case for EPT), instead of relying on TDP being enabled.

That being said, to me it's more reasonable to have a separate patch to replace
the '!tdp_enabled' check with '!shadow_memtype_mask' in update_mtrr(), perhaps
with a Fixes tag for commit 38bf9d7bf277.

The kvm_zap_gfn_range() should be remain unchanged.

For the problem that shadow_memtype_mask isn't accessible in mtrr.c I think you
can include "mmu/spte.h"?

>