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