Re: [PATCH 1/5] KVM: VMX: Open code writing vCPU's PAT in VMX's MSR handler

[Sean Christopherson <seanjc@google.com>]

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@uniontech.com>
> > > > 
> > > > 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!