Received-SPF: pass (google.com: domain of linux-kernel+bounces-47028-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Wed, 31 Jan 2024 19:43:00 +0000
From: Mingwei Zhang <mizhang@google.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: Sean Christopherson <seanjc@google.com>,
	Aaron Lewis <aaronlewis@google.com>,
	"H. Peter Anvin" <hpa@zytor.com>, kvm@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/2] KVM: x86/pmu: Reset perf_capabilities in vcpu to 0
 if PDCM is disabled
Message-ID: <ZbqixOTlp61Lp-JV@google.com>
References: <20240124003858.3954822-1-mizhang@google.com>
 <20240124003858.3954822-2-mizhang@google.com>
 <ZbExcMMl-IAzJrfx@google.com>
 <CAAAPnDFAvJBuETUsBScX6WqSbf_j=5h_CpWwrPHwXdBxDg_LFQ@mail.gmail.com>
 <ZbGAXpFUso9JzIjo@google.com>
 <ZbGOK9m6UKkQ38bK@google.com>
 <ZbGUfmn-ZAe4lkiN@google.com>
 <b0b5ba26-505e-4247-b30d-9ba2bb0301c1@redhat.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <b0b5ba26-505e-4247-b30d-9ba2bb0301c1@redhat.com>

On Mon, Jan 29, 2024, Paolo Bonzini wrote:
> On 1/24/24 23:51, Sean Christopherson wrote:
> > > If we follow the suggestion by removing the initial value at vCPU
> > > creation time, then I think it breaks the existing VMM code, since that
> > > requires VMM to explicitly set the MSR, which I am not sure we do today.
> > Yeah, I'm hoping we can squeak by without breaking existing setups.
> > 
> > I'm 99% certain QEMU is ok, as QEMU has explicitly set MSR_IA32_PERF_CAPABILITIES
> > since support for PDCM/PERF_CAPABILITIES was added by commit ea39f9b643
> > ("target/i386: define a new MSR based feature word - FEAT_PERF_CAPABILITIES").
> > 
> > Frankly, if our VMM doesn't do the same, then it's wildly busted.  Relying on
> > KVM to define the vCPU is irresponsible, to put it nicely.
> 
> Yes, I tend to agree.

Discussed with Sean offline. Yes, I also agree that this should be
handled at VMM level. MSR_IA32_PERF_CAPABILITIES should be regarded as
part of the CPUID, or sort of. The diff is that its own
"KVM_GET_SUPPORTED_CPUID" (ie., the default value) should come from
KVM_GET_MSRS of the device ioctl.

Providing the default value for MSR_IA32_PERF_CAPABILITIES is really
making things messed. KVM has to always guard access to the cached guest
value with the checking of X86_FEATURE_PDCM. I believe
guest_cpuid_has(vcpu, X86_FEATURE_PDCM) will take runtime cost.

> 
> What QEMU does goes from the squeaky clean to the very debatable depending
> on the parameters you give it.
> 
> With "-cpu Haswell" and similar, it will provide values for all CPUID and
> MSR bits that match as much as possible values from an actual CPU model.  It
> will complain if there are some values that do not match[1].
> 
> With "-cpu host", it will copy values from KVM_GET_SUPPORTED_CPUID and from
> the feature MSRs, but only for features that it knows about.
> 
> With "-cpu host,migratable=no", it will copy values from
> KVM_GET_SUPPORTED_CPUID and from the feature MSRs, but only for *feature
> words* (CPUID registers, or MSRs) that it knows about.  This is where it
> becomes debatable, because a CPUID bit could be added without QEMU knowing
> the corresponding MSR.  In this case, the user probably expects the MSR to
> have a nonzero.  On one hand I agree that it would be irresponsible, on the
> other hand that's the point of "-cpu host,migratable=no".
> 
> If you want to proceed with the change, I don't have any problem with
> considering it a QEMU bug that it doesn't copy over to the guest any unknown
> leaves or MSRs.
> 
reply from another thread: CrosVM issue is not related to this one. It
might have something to do with KVM_GET_MSR_INDEX_LIST. I will come up
details later.
> Paolo
> 
> [1] Unfortunately it's not fatal because there are way way too many models,
> and also because until recently TCG lacked AVX---and therefore could only
> emulate completely some very old CPU models.  But with "-cpu
> Haswell,enforce" then everything's clean.
>