Received-SPF: pass (google.com: domain of linux-kernel+bounces-101632-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Wed, 13 Mar 2024 07:42:18 -0700
In-Reply-To: <DS0PR11MB63731F54EA26D14CF7D6A3FDDC2A2@DS0PR11MB6373.namprd11.prod.outlook.com>
Precedence: bulk
Mime-Version: 1.0
References: <20240313003739.3365845-1-mizhang@google.com> <DS0PR11MB63731F54EA26D14CF7D6A3FDDC2A2@DS0PR11MB6373.namprd11.prod.outlook.com>
Message-ID: <ZfG7SgyqTTtqF3cw@google.com>
Subject: Re: [PATCH] KVM: x86/pmu: Return correct value of IA32_PERF_CAPABILITIES
 for userspace after vCPU has run
From: Sean Christopherson <seanjc@google.com>
To: Wei W Wang <wei.w.wang@intel.com>
Cc: Mingwei Zhang <mizhang@google.com>, Paolo Bonzini <pbonzini@redhat.com>, 
	"H. Peter Anvin" <hpa@zytor.com>, "kvm@vger.kernel.org" <kvm@vger.kernel.org>, 
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, Aaron Lewis <aaronlewis@google.com>, 
	Jim Mattson <jmattson@google.com>
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Mar 13, 2024, Wei W Wang wrote:
> On Wednesday, March 13, 2024 8:38 AM, Mingwei Zhang wrote:
> > Return correct value of IA32_PERF_CAPABILITIES when userspace tries to =
read
> > it after vCPU has already run. Previously, KVM will always return the g=
uest
> > cached value on get_msr() even if guest CPUID lacks X86_FEATURE_PDCM. T=
he
> > guest cached value on default is kvm_caps.supported_perf_cap. However,
> > when userspace sets the value during live migration, the call fails bec=
ause of
> > the check on X86_FEATURE_PDCM.
>=20
> Could you point where in the set_msr path that could fail?
> (I don=E2=80=99t find there is a check of X86_FEATURE_PDCM in vmx_set_msr=
 and
> kvm_set_msr_common)

The changelog is misleading, it's not the PDCM feature bit, it's the PMU ve=
rsion
check in vmx_set_msr():

	case MSR_IA32_PERF_CAPABILITIES:
		if (data && !vcpu_to_pmu(vcpu)->version)
			return 1;

> > Initially, it sounds like a pure userspace issue. It is not. After vCPU=
 has run,
> > KVM should faithfully return correct value to satisify legitimate reque=
sts from
> > userspace such as VM suspend/resume and live migrartion. In this case, =
KVM
> > should return 0 when guest cpuid lacks X86_FEATURE_PDCM.=20
> Some typos above (satisfy, migration, CPUID)
>=20
> Seems the description here isn=E2=80=99t aligned to your code below?
> The code below prevents userspace from reading the MSR value (not return =
0 as the
> read value) in that case.

Ya.

> >So fix the
> > problem by adding an additional check in vmx_set_msr().
> >=20
> > Note that IA32_PERF_CAPABILITIES is emulated on AMD side, which is fine
> > because it set_msr() is guarded by kvm_caps.supported_perf_cap which is
> > always 0.
> >=20
> > Cc: Aaron Lewis <aaronlewis@google.com>
> > Cc: Jim Mattson <jmattson@google.com>
> > Signed-off-by: Mingwei Zhang <mizhang@google.com>
> > ---
> >  arch/x86/kvm/vmx/vmx.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >=20
> > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index
> > 40e3780d73ae..6d8667b56091 100644
> > --- a/arch/x86/kvm/vmx/vmx.c
> > +++ b/arch/x86/kvm/vmx/vmx.c
> > @@ -2049,6 +2049,17 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu,
> > struct msr_data *msr_info)
> >  		msr_info->data =3D to_vmx(vcpu)->msr_ia32_sgxlepubkeyhash
> >  			[msr_info->index - MSR_IA32_SGXLEPUBKEYHASH0];
> >  		break;
> > +	case MSR_IA32_PERF_CAPABILITIES:
> > +		/*
> > +		 * Host VMM should not get potentially invalid MSR value if
> > vCPU
> > +		 * has already run but guest cpuid lacks the support for the
> > +		 * MSR.
> > +		 */
> > +		if (msr_info->host_initiated &&
> > +		    kvm_vcpu_has_run(vcpu) &&
> > +		    !guest_cpuid_has(vcpu, X86_FEATURE_PDCM))
> > +			return 1;

As Wei pointed out, this doesn't match the changelog.  And I don't think th=
is is
what we want, at least not in isolation.  Making KVM more restrictive on us=
erspace
reads doesn't solve the live migration save/restore issue, and the kvm_vcpu=
_has_run()
adds yet another flavor of MSR handling.

We discussed this whole MSRs mess at PUCK this morning.  I forgot to hit RE=
CORD,
but Paolo took notes and will post them soon.

Going from memory, the plan is to:

  1. Commit to, and document, that userspace must do KVM_SET_CPUID{,2} prio=
r to
     KVM_SET_MSRS.

  2. Go with roughly what I proposed in the CET thread (for unsupported MSR=
S,
     read 0 and drop writes of '0')[*].

  3. Add a quire for PERF_CAPABILITIES, ARCH_CAPABILITIES, and PLATFORM_INF=
O
     (if quirk=3D=3Denabled, keep KVM's current behavior; if quirk=3D=3Ddis=
abled, zero-
      initialize the MSRs).

With those pieces in place, KVM can simply check X86_FEATURE_PDCM for both =
reads
and writes to PERF_CAPABILITIES, and the common userspace MSR handling will
convert "unsupported" to "success" as appropriate.

[*] https://lore.kernel.org/all/ZfDdS8rtVtyEr0UR@google.com