On Wed, May 03, 2023, Kai Huang wrote: > On Wed, 2023-05-03 at 16:25 -0700, Sean Christopherson wrote: > > On Wed, May 03, 2023, Kai Huang wrote: > > > On Wed, 2023-05-03 at 11:28 -0700, Sean Christopherson wrote: > > > > From: Wenyao Hai <haiwenyao@xxxxxxxxxxxxx> > > > > > > > > Open code setting "vcpu->arch.pat" in vmx_set_msr() instead of bouncing > > > > through kvm_set_msr_common() to get to the same code in kvm_mtrr_set_msr(). > > > > > > What's the value of doing so, besides saving a function of kvm_set_msr_common()? > > > > To avoid complicating a very simple operation (writing vcpu->arch.pat), and to > > align with SVM. > > > > > PAT change shouldn't be something frequent so shouldn't in a performance > > > critical path. Given the PAT logic on Intel and AMD are basically the same , > > > isn't it better to do in kvm_set_msr_common()? > > > > I could go either way on calling into kvm_set_msr_common(). I agree that > > performance isn't a concern. Hmm, and kvm_set_msr_common() still has a case > > statement for MSR_IA32_CR_PAT, so handling the write fully in vendor code won't > > impact the code generation for other MSRs. > > > > Though I am leaning towards saying we should either handle loads and stores to > > vcpu->arch.pat in common code _or_ vendor code, i.e. either teach VMX and SVM to > > handle reads of PAT, or have their write paths call kvm_set_msr_common(). A mix > > of both is definitely odd. > > Agreed. Alternatively we can move SVM's setting vcpu->arch.pat to common code. > > > > > I don't have strong preference on which of those two we choose. I dislike duplicating > > logic across VMX and SVM, but on the other hands it's so little code. I think > > I'd vote for handling everything in vendor code, mostly because this gives the > > appearance that the write can fail, which is silly and misleading. > > > > ret = kvm_set_msr_common(vcpu, msr_info); > > No opinion either. First glance is having > > case MSR_IA32_CR_PAT: > vcpu->arch.pat = data; > > in kvm_set_msr_common() is clearer because it is symmetrical to the read path. > > Anyway your decision :) Duh, the obvious answer is to do ret = kvm_set_msr_common(vcpu, msr_info); if (ret) break; <vendor code here> That's an established pattern for other MSRs, and addresses my main concern of not unwinding the VMCS updates in the should-be-impossible scenario of kvm_set_msr_common() failing after the kvm_pat_valid() check. Thanks Kai!
