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

[Yan Zhao <yan.y.zhao@intel.com>]

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
> 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)()?