linux-kernel - Re: [PATCH v3 08/11] KVM: x86: move vmx code to get EPT memtype when CR0.CD=1 to x86 common code

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: <ZJ3sxm6CngYC7pno@google.com>
Date:   Thu, 29 Jun 2023 13:42:46 -0700
From:   Sean Christopherson <seanjc@...gle.com>
To:     Yan Zhao <yan.y.zhao@...el.com>
Cc:     kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
        pbonzini@...hat.com, chao.gao@...el.com, kai.huang@...el.com,
        robert.hoo.linux@...il.com
Subject: Re: [PATCH v3 08/11] KVM: x86: move vmx code to get EPT memtype when
 CR0.CD=1 to x86 common code

On Thu, Jun 29, 2023, Yan Zhao wrote:
> On Wed, Jun 28, 2023 at 03:57:09PM -0700, Sean Christopherson wrote:
> > On Fri, Jun 16, 2023, Yan Zhao wrote:
> > > Move code in vmx.c to get cache disabled memtype when non-coherent DMA
> > > present to x86 common code.
> > > 
> > > This is the preparation patch for later implementation of fine-grained gfn
> > > zap for CR0.CD toggles when guest MTRRs are honored.
> > > 
> > > No functional change intended.
> > > 
> > > Signed-off-by: Yan Zhao <yan.y.zhao@...el.com>
> > > ---
> > >  arch/x86/kvm/mtrr.c    | 19 +++++++++++++++++++
> > >  arch/x86/kvm/vmx/vmx.c | 10 +++++-----
> > >  arch/x86/kvm/x86.h     |  1 +
> > >  3 files changed, 25 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/arch/x86/kvm/mtrr.c b/arch/x86/kvm/mtrr.c
> > > index 3ce58734ad22..b35dd0bc9cad 100644
> > > --- a/arch/x86/kvm/mtrr.c
> > > +++ b/arch/x86/kvm/mtrr.c
> > > @@ -721,3 +721,22 @@ bool kvm_mtrr_check_gfn_range_consistency(struct kvm_vcpu *vcpu, gfn_t gfn,
> > >  
> > >  	return type == mtrr_default_type(mtrr_state);
> > >  }
> > > +
> > > +void kvm_mtrr_get_cd_memory_type(struct kvm_vcpu *vcpu, u8 *type, bool *ipat)
> > 
> > Hmm, I'm not convinced that this logic is subtle enough to warrant a common
> I added this patch because the memtype to use under CR0.CD=1 is determined by
> vmx specific code (i.e. vmx.c), while mtrr.c is a common code for x86.
> 
> I don't know if it's good to assume what vmx.c will return as in below open code. 
> (e.g. if someone added IPAT bit for CR0.CD=1 under the quirk, and forgot
> to update the code here, we actually need to zap everything rather than
> zap only non-WB ranges).
> 
> That's why I want to introduce a helper and let vmx.c call into it.
> (sorry, I didn't write a good commit message to explain the real intent).

No need to apologize, I fully understood the intent.  I'm just not convinced that
the risk of us screwing up this particular case is worth the extra layers of crud
that are necessary to let VMX and MTRRs share the core logic.

Absent emulating CR0.CD=1 with UC, setting IPAT is complete nonsense when KVM is
honoring the guest memtype.

I 100% agree that splitting the logic is less than ideal, but providing a common
helper feels forced and IMO yields significantly less readable code.  And exporting
kvm_mtrr_get_cd_memory_type() only adds to the confusion because calling it on
SVM, which can't fully ignore gPAT, is also nonsensical.