Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
Message-ID: <d36ae1de672ce8eee122481448c5c5c4589cf8a6.camel@redhat.com>
Subject: Re: [PATCH v6 09/25] KVM: x86: Rework cpuid_get_supported_xcr0() to
 operate on vCPU data
From:   Maxim Levitsky <mlevitsk@redhat.com>
To:     Sean Christopherson <seanjc@google.com>
Cc:     Yang Weijiang <weijiang.yang@intel.com>, pbonzini@redhat.com,
        kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
        dave.hansen@intel.com, peterz@infradead.org, chao.gao@intel.com,
        rick.p.edgecombe@intel.com, john.allen@amd.com
Date:   Thu, 02 Nov 2023 20:25:49 +0200
In-Reply-To: <ZUJjs2F-vD1-cZS4@google.com>
References: <20230914063325.85503-1-weijiang.yang@intel.com>
         <20230914063325.85503-10-weijiang.yang@intel.com>
         <96c30a78fa95071e87045b7293c2cf796d4182a0.camel@redhat.com>
         <ZUJjs2F-vD1-cZS4@google.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.36.5 (3.36.5-2.fc32) 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Wed, 2023-11-01 at 07:41 -0700, Sean Christopherson wrote:
> On Tue, Oct 31, 2023, Maxim Levitsky wrote:
> > On Thu, 2023-09-14 at 02:33 -0400, Yang Weijiang wrote:
> > > From: Sean Christopherson <seanjc@google.com>
> > > 
> > > Rework and rename cpuid_get_supported_xcr0() to explicitly operate on vCPU
> > > state, i.e. on a vCPU's CPUID state.  Prior to commit 275a87244ec8 ("KVM:
> > > x86: Don't adjust guest's CPUID.0x12.1 (allowed SGX enclave XFRM)"), KVM
> > > incorrectly fudged guest CPUID at runtime,
> > Can you explain how commit 275a87244ec8 relates to this patch?
> > 
> > > which in turn necessitated massaging the incoming CPUID state for
> > > KVM_SET_CPUID{2} so as not to run afoul of kvm_cpuid_check_equal().
> > 
> > Can you link the commit that added this 'massaging' and explain on how this
> > relates to this patch?
> 
> It's commit 275a87244ec8, which is right above.  I think the missing part is an
> explicit call out that the massaging used cpuid_get_supported_xcr0() with the
> incoming "struct kvm_cpuid_entry2", i.e. without a "struct kvm_vcpu".
> 
> > Can you explain what is the problem that this patch is trying to solve?
> 
> Is this better?
> 
> --
> Rework and rename cpuid_get_supported_xcr0() to explicitly operate on vCPU
> state, i.e. on a vCPU's CPUID state, now that the only usage of the helper
> is to retrieve a vCPU's already-set CPUID.
> 
> Prior to commit 275a87244ec8 ("KVM: x86: Don't adjust guest's CPUID.0x12.1
> (allowed SGX enclave XFRM)"), KVM incorrectly fudged guest CPUID at
> runtime, which in turn necessitated massaging the incoming CPUID state for
> KVM_SET_CPUID{2} so as not to run afoul of kvm_cpuid_check_equal().  I.e.
> KVM also invoked cpuid_get_supported_xcr0() with the incoming CPUID state,
> and thus without an explicit vCPU object.

Ah, I understand you. I incorrectly assumed that KVM doesn't allow different CPUID
on different vCPUs, while the actual restriction that was recently placed was
to not allow changing a vCPU's CPUID, once a vCPU was in a guest mode once.

I also understand what you mean in regard to the commit 275a87244ec8 but IMHO
this part of the commit message is only adding to the confusion.

I think that this will be a better commit message:


"Rework and rename cpuid_get_supported_xcr0() to explicitly operate on vCPU state
i.e. on a vCPU's CPUID state.

This is needed because KVM permits different vCPUs to have different CPUIDs, 
and thus it is valid for each vCPU to have different set of supported bits in the XCR0"

> --
> 
> > Is it really allowed in x86 spec to have different supported mask of XCR0 bits
> > on different CPUs (assuming all CPUs of the same type)?
> 
> Yes, nothing in the SDM explicitly states that all cores in have identical feature
> sets.  And "assuming all CPUs of the same type" isn't really a valid constraint
> because it's very doable to put different SKUs into a multi-socket system.
> 
> Intel even (somewhat inadvertantly) kinda sorta shipped such CPUs, as Alder Lake
> P-cores support AVX512 but E-cores do not, and IIRC early (pre-production?) BIOS
> didn't disable AVX512 on the P-Cores, i.e. software could observe cores with and
> without AVX512.  That quickly got fixed because it confused software, but until
> Intel squashed AVX512 entirely with a microcode update, disabling E-Cores in BIOS
> would effectively enable AVX512 on the remaining P-Cores.

Yea, sure I know about this, that's why I said "same type". I just was under the impression
that KVM doesn't 'officially' support heterogeneous vCPUs and recently added a check
to ensure that all vCPUs have the same CPUID. Now I understand.

> 
> And it's not XCR0-related, but PMUs on Alder Lake (and all Intel hybrid CPUs) are
> truly heterogenous.  It's a mess for virtualization, but concrete proof that there
> are no architectural guarantees regarding homogeneity of feature sets.
> 
> > If true, does KVM supports it?
> 
> Yes.  Whether or not that's a good thing is definitely debatle, bug KVM's ABI for
> a very long time has allowed userspace to expose whatever it wants via KVM_SET_CPUID.

> 
> Getting (guest) software to play nice is an entirely different matter, but exposing
> heterogenous vCPUs isn't an architectural violation.
> 

Best regards,
	Maxim Levitsky