Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
Date:   Wed, 1 Nov 2023 07:41:55 -0700
In-Reply-To: <96c30a78fa95071e87045b7293c2cf796d4182a0.camel@redhat.com>
Mime-Version: 1.0
References: <20230914063325.85503-1-weijiang.yang@intel.com>
 <20230914063325.85503-10-weijiang.yang@intel.com> <96c30a78fa95071e87045b7293c2cf796d4182a0.camel@redhat.com>
Message-ID: <ZUJjs2F-vD1-cZS4@google.com>
Subject: Re: [PATCH v6 09/25] KVM: x86: Rework cpuid_get_supported_xcr0() to
 operate on vCPU data
From:   Sean Christopherson <seanjc@google.com>
To:     Maxim Levitsky <mlevitsk@redhat.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
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

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.
--

> 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.

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.