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

[Yan Zhao]

On Fri, Jun 30, 2023 at 03:49:10PM +0800, Yan Zhao wrote:
> On Thu, Jun 29, 2023 at 01:42:46PM -0700, Sean Christopherson wrote:
> > 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:
> ...
> > > > > +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.
> Yes, I'm just paranoid :)
> 
> > 
> > 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
What about renaming it to kvm_honors_guest_mtrrs_get_cd_memtype()?
Then it's only needed to be called when guest mtrrs are honored and provides a
kind of enforcement. So that if there're other x86 participants (besides VMX/SVM)
who want to honor guest mtrr, the same memtype is used with CR0.CD=1.
(I know there might never be such kind of participants, or you may want to
update the code until they appear)

I tried in this way in v4 here
https://lore.kernel.org/all/20230714065356.20620-1-yan.y.zhao@xxxxxxxxx/.
Feel free to ask me to drop it if you still don't like it :)

> > 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.
> Ok. I get your concern now. You are right.
> Looks the easiest way now is to add some comments in VMM to caution that
> changes in memtype when noncoherent DMA present and CR0.CD=1 may lead to
> update of code for GFN zap.
> Or, do you think it's worth adding a new callback in kvm_x86_ops, e.g.
> static_call_cond(kvm_x86_get_cd_mt_honor_guest_mtrr)()?