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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZFPqLS08b0xT/PLa@google.com>
Date:   Thu, 4 May 2023 10:23:57 -0700
From:   Sean Christopherson <seanjc@...gle.com>
To:     Kai Huang <kai.huang@...el.com>
Cc:     "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "pbonzini@...hat.com" <pbonzini@...hat.com>,
        "guoke@...ontech.com" <guoke@...ontech.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "haiwenyao@...ontech.com" <haiwenyao@...ontech.com>
Subject: Re: [PATCH 1/5] KVM: VMX: Open code writing vCPU's PAT in VMX's MSR handler

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@...ontech.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!