[PATCH v2 1/2] KVM: x86: Insert "AMD" in KVM_X86_FEATURE_PSFD

[PATCH v2 1/2] KVM: x86: Insert "AMD" in KVM_X86_FEATURE_PSFD

[Jim Mattson]

Intel and AMD have separate CPUID bits for each SPEC_CTRL bit. In the
case of every bit other than PFSD, the Intel CPUID bit has no vendor
name qualifier, but the AMD CPUID bit does. For consistency, rename
KVM_X86_FEATURE_PSFD to KVM_X86_FEATURE_AMD_PSFD.

No functional change intended.

Signed-off-by: Jim Mattson <jmattson@google.com>
Cc: Babu Moger <Babu.Moger@amd.com>
---
 v1 -> v2: Dropped patch 2/3.

 arch/x86/kvm/cpuid.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 75dcf7a72605..07be45c5bb93 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -62,7 +62,7 @@ u32 xstate_required_size(u64 xstate_bv, bool compacted)
  * This one is tied to SSB in the user API, and not
  * visible in /proc/cpuinfo.
  */
-#define KVM_X86_FEATURE_PSFD		(13*32+28) /* Predictive Store Forwarding Disable */
+#define KVM_X86_FEATURE_AMD_PSFD	(13*32+28) /* Predictive Store Forwarding Disable */
 
 #define F feature_bit
 #define SF(name) (boot_cpu_has(X86_FEATURE_##name) ? F(name) : 0)
@@ -673,7 +673,7 @@ void kvm_set_cpu_caps(void)
 		F(CLZERO) | F(XSAVEERPTR) |
 		F(WBNOINVD) | F(AMD_IBPB) | F(AMD_IBRS) | F(AMD_SSBD) | F(VIRT_SSBD) |
 		F(AMD_SSB_NO) | F(AMD_STIBP) | F(AMD_STIBP_ALWAYS_ON) |
-		__feature_bit(KVM_X86_FEATURE_PSFD)
+		__feature_bit(KVM_X86_FEATURE_AMD_PSFD)
 	);
 
 	/*
-- 
2.37.2.672.g94769d06f0-goog

[PATCH v2 2/2] KVM: x86: Expose Predictive Store Forwarding Disable on Intel parts

[Jim Mattson]

Intel enumerates support for PSFD in CPUID.(EAX=7,ECX=2):EDX.PSFD[bit
0]. Report support for this feature in KVM if it is available on the
host.

Presumably, this feature bit, like its AMD counterpart, is not welcome
in cpufeatures.h, so add a local definition of this feature in KVM.

Signed-off-by: Jim Mattson <jmattson@google.com>
---
 arch/x86/kvm/cpuid.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 07be45c5bb93..b5af9e451bef 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -62,6 +62,7 @@ u32 xstate_required_size(u64 xstate_bv, bool compacted)
  * This one is tied to SSB in the user API, and not
  * visible in /proc/cpuinfo.
  */
+#define KVM_X86_FEATURE_PSFD		0          /* Predictive Store Forwarding Disable */
 #define KVM_X86_FEATURE_AMD_PSFD	(13*32+28) /* Predictive Store Forwarding Disable */
 
 #define F feature_bit
@@ -677,9 +678,9 @@ void kvm_set_cpu_caps(void)
 	);
 
 	/*
-	 * AMD has separate bits for each SPEC_CTRL bit.
-	 * arch/x86/kernel/cpu/bugs.c is kind enough to
-	 * record that in cpufeatures so use them.
+	 * AMD has separate bits for each SPEC_CTRL bit.  Except for
+	 * PSFD, arch/x86/kernel/cpu/bugs.c is kind enough to record
+	 * that in cpufeatures so use them.
 	 */
 	if (boot_cpu_has(X86_FEATURE_IBPB))
 		kvm_cpu_cap_set(X86_FEATURE_AMD_IBPB);
@@ -880,13 +881,13 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
 		break;
 	/* function 7 has additional index. */
 	case 7:
-		entry->eax = min(entry->eax, 1u);
+		/* KVM only supports leaf 7 indices 0 through 2. */
+		max_idx = entry->eax = min(entry->eax, 2u);
 		cpuid_entry_override(entry, CPUID_7_0_EBX);
 		cpuid_entry_override(entry, CPUID_7_ECX);
 		cpuid_entry_override(entry, CPUID_7_EDX);
 
-		/* KVM only supports 0x7.0 and 0x7.1, capped above via min(). */
-		if (entry->eax == 1) {
+		if (max_idx >= 1) {
 			entry = do_host_cpuid(array, function, 1);
 			if (!entry)
 				goto out;
@@ -896,6 +897,16 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
 			entry->ecx = 0;
 			entry->edx = 0;
 		}
+		if (max_idx >= 2) {
+			entry = do_host_cpuid(array, function, 2);
+			if (!entry)
+				goto out;
+
+			entry->eax = 0;
+			entry->ebx = 0;
+			entry->ecx = 0;
+			entry->edx &= BIT(KVM_X86_FEATURE_PSFD);
+		}
 		break;
 	case 9:
 		break;
-- 
2.37.2.672.g94769d06f0-goog

Re: [PATCH v2 1/2] KVM: x86: Insert "AMD" in KVM_X86_FEATURE_PSFD

[Sean Christopherson]

On Tue, Aug 30, 2022, Jim Mattson wrote:
> Intel and AMD have separate CPUID bits for each SPEC_CTRL bit. In the
> case of every bit other than PFSD, the Intel CPUID bit has no vendor
> name qualifier, but the AMD CPUID bit does. For consistency, rename
> KVM_X86_FEATURE_PSFD to KVM_X86_FEATURE_AMD_PSFD.
> 
> No functional change intended.
> 
> Signed-off-by: Jim Mattson <jmattson@google.com>
> Cc: Babu Moger <Babu.Moger@amd.com>
> ---
>  v1 -> v2: Dropped patch 2/3.
> 
>  arch/x86/kvm/cpuid.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 75dcf7a72605..07be45c5bb93 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -62,7 +62,7 @@ u32 xstate_required_size(u64 xstate_bv, bool compacted)
>   * This one is tied to SSB in the user API, and not
>   * visible in /proc/cpuinfo.
>   */
> -#define KVM_X86_FEATURE_PSFD		(13*32+28) /* Predictive Store Forwarding Disable */
> +#define KVM_X86_FEATURE_AMD_PSFD	(13*32+28) /* Predictive Store Forwarding Disable */

This is asinine.  If KVM is forced to carry the feature bit then IMO we have every
right to the "real" name.  If we can't convince others that this belongs in
cpufeatures.h, then I vote to rename this to X86_FEATURE_AMD_PSFD so that we don't
have to special case this thing.

>  #define F feature_bit
>  #define SF(name) (boot_cpu_has(X86_FEATURE_##name) ? F(name) : 0)
> @@ -673,7 +673,7 @@ void kvm_set_cpu_caps(void)
>  		F(CLZERO) | F(XSAVEERPTR) |
>  		F(WBNOINVD) | F(AMD_IBPB) | F(AMD_IBRS) | F(AMD_SSBD) | F(VIRT_SSBD) |
>  		F(AMD_SSB_NO) | F(AMD_STIBP) | F(AMD_STIBP_ALWAYS_ON) |
> -		__feature_bit(KVM_X86_FEATURE_PSFD)
> +		__feature_bit(KVM_X86_FEATURE_AMD_PSFD)
>  	);
>  
>  	/*
> -- 
> 2.37.2.672.g94769d06f0-goog
> 

Re: [PATCH v2 1/2] KVM: x86: Insert "AMD" in KVM_X86_FEATURE_PSFD

[Jim Mattson]

On Tue, Aug 30, 2022 at 4:27 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Tue, Aug 30, 2022, Jim Mattson wrote:
> > Intel and AMD have separate CPUID bits for each SPEC_CTRL bit. In the
> > case of every bit other than PFSD, the Intel CPUID bit has no vendor
> > name qualifier, but the AMD CPUID bit does. For consistency, rename
> > KVM_X86_FEATURE_PSFD to KVM_X86_FEATURE_AMD_PSFD.
> >
> > No functional change intended.
> >
> > Signed-off-by: Jim Mattson <jmattson@google.com>
> > Cc: Babu Moger <Babu.Moger@amd.com>
> > ---
> >  v1 -> v2: Dropped patch 2/3.
> >
> >  arch/x86/kvm/cpuid.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > index 75dcf7a72605..07be45c5bb93 100644
> > --- a/arch/x86/kvm/cpuid.c
> > +++ b/arch/x86/kvm/cpuid.c
> > @@ -62,7 +62,7 @@ u32 xstate_required_size(u64 xstate_bv, bool compacted)
> >   * This one is tied to SSB in the user API, and not
> >   * visible in /proc/cpuinfo.
> >   */
> > -#define KVM_X86_FEATURE_PSFD         (13*32+28) /* Predictive Store Forwarding Disable */
> > +#define KVM_X86_FEATURE_AMD_PSFD     (13*32+28) /* Predictive Store Forwarding Disable */
>
> This is asinine.  If KVM is forced to carry the feature bit then IMO we have every
> right to the "real" name.  If we can't convince others that this belongs in
> cpufeatures.h, then I vote to rename this to X86_FEATURE_AMD_PSFD so that we don't
> have to special case this thing.

You won't get any argument from me!

If Borislav objects to seeing the feature in /proc/cpuinfo, can't we
just begin the cpufeatures.h descriptive comment with ""?

Re: [PATCH v2 2/2] KVM: x86: Expose Predictive Store Forwarding Disable on Intel parts

[Sean Christopherson]

On Tue, Aug 30, 2022, Jim Mattson wrote:
> Intel enumerates support for PSFD in CPUID.(EAX=7,ECX=2):EDX.PSFD[bit
> 0]. Report support for this feature in KVM if it is available on the
> host.
> 
> Presumably, this feature bit, like its AMD counterpart, is not welcome
> in cpufeatures.h, so add a local definition of this feature in KVM.
> 
> Signed-off-by: Jim Mattson <jmattson@google.com>
> ---
>  arch/x86/kvm/cpuid.c | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 07be45c5bb93..b5af9e451bef 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -62,6 +62,7 @@ u32 xstate_required_size(u64 xstate_bv, bool compacted)
>   * This one is tied to SSB in the user API, and not
>   * visible in /proc/cpuinfo.
>   */
> +#define KVM_X86_FEATURE_PSFD		0          /* Predictive Store Forwarding Disable */

I believe we can use "enum kvm_only_cpuid_leafs" to handle this.  E.g. 

	enum kvm_only_cpuid_leafs {
		CPUID_12_EAX	 = NCAPINTS,
		CPUID_7_2_EDX,
		NR_KVM_CPU_CAPS,

		NKVMCAPINTS = NR_KVM_CPU_CAPS - NCAPINTS,
	};

then the intended use of KVM_X86_FEATURE_*

	#define KVM_X86_FEATURE_PSFD	KVM_X86_FEATURE(CPUID_7_2_EDX, 0)

I _think_ we can then define an arbitrary word for X86_FEATURE_PSFD, e.g.

	#define X86_FEATURE_PSFD	(NKVMCAPINTS*32+0)

and then wire up the translation:

	static __always_inline u32 __feature_translate(int x86_feature)
	{
		if (x86_feature == X86_FEATURE_SGX1)
			return KVM_X86_FEATURE_SGX1;
		else if (x86_feature == X86_FEATURE_SGX2)
			return KVM_X86_FEATURE_SGX2;
		else if (x86_feature == X86_FEATURE_PSFD)
			return KVM_X86_FEATURE_PSFD;
	
		return x86_feature;
	}

I believe/hope that allows us to use at least cpuid_entry_override().  Open coding
masking of specific registers was a mess that I don't want to repeat.

Re: [PATCH v2 2/2] KVM: x86: Expose Predictive Store Forwarding Disable on Intel parts

[Jim Mattson]

On Tue, Aug 30, 2022 at 4:39 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Tue, Aug 30, 2022, Jim Mattson wrote:
> > Intel enumerates support for PSFD in CPUID.(EAX=7,ECX=2):EDX.PSFD[bit
> > 0]. Report support for this feature in KVM if it is available on the
> > host.
> >
> > Presumably, this feature bit, like its AMD counterpart, is not welcome
> > in cpufeatures.h, so add a local definition of this feature in KVM.
> >
> > Signed-off-by: Jim Mattson <jmattson@google.com>
> > ---
> >  arch/x86/kvm/cpuid.c | 23 +++++++++++++++++------
> >  1 file changed, 17 insertions(+), 6 deletions(-)
> >
> > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > index 07be45c5bb93..b5af9e451bef 100644
> > --- a/arch/x86/kvm/cpuid.c
> > +++ b/arch/x86/kvm/cpuid.c
> > @@ -62,6 +62,7 @@ u32 xstate_required_size(u64 xstate_bv, bool compacted)
> >   * This one is tied to SSB in the user API, and not
> >   * visible in /proc/cpuinfo.
> >   */
> > +#define KVM_X86_FEATURE_PSFD         0          /* Predictive Store Forwarding Disable */
>
> I believe we can use "enum kvm_only_cpuid_leafs" to handle this.  E.g.
>
>         enum kvm_only_cpuid_leafs {
>                 CPUID_12_EAX     = NCAPINTS,
>                 CPUID_7_2_EDX,
>                 NR_KVM_CPU_CAPS,
>
>                 NKVMCAPINTS = NR_KVM_CPU_CAPS - NCAPINTS,
>         };
>
> then the intended use of KVM_X86_FEATURE_*
>
>         #define KVM_X86_FEATURE_PSFD    KVM_X86_FEATURE(CPUID_7_2_EDX, 0)
>
> I _think_ we can then define an arbitrary word for X86_FEATURE_PSFD, e.g.
>
>         #define X86_FEATURE_PSFD        (NKVMCAPINTS*32+0)

We may run afoul of reverse_cpuid_check(), depending on usage.

> and then wire up the translation:
>
>         static __always_inline u32 __feature_translate(int x86_feature)
>         {
>                 if (x86_feature == X86_FEATURE_SGX1)
>                         return KVM_X86_FEATURE_SGX1;
>                 else if (x86_feature == X86_FEATURE_SGX2)
>                         return KVM_X86_FEATURE_SGX2;
>                 else if (x86_feature == X86_FEATURE_PSFD)
>                         return KVM_X86_FEATURE_PSFD;
>
>                 return x86_feature;
>         }
>
> I believe/hope that allows us to use at least cpuid_entry_override().  Open coding
> masking of specific registers was a mess that I don't want to repeat.

Maybe we can plead for a bit in word 11?

Re: [PATCH v2 2/2] KVM: x86: Expose Predictive Store Forwarding Disable on Intel parts

[Sean Christopherson]

On Tue, Aug 30, 2022, Jim Mattson wrote:
> On Tue, Aug 30, 2022 at 4:39 PM Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Tue, Aug 30, 2022, Jim Mattson wrote:
> > > Intel enumerates support for PSFD in CPUID.(EAX=7,ECX=2):EDX.PSFD[bit
> > > 0]. Report support for this feature in KVM if it is available on the
> > > host.
> > >
> > > Presumably, this feature bit, like its AMD counterpart, is not welcome
> > > in cpufeatures.h, so add a local definition of this feature in KVM.
> > >
> > > Signed-off-by: Jim Mattson <jmattson@google.com>
> > > ---
> > >  arch/x86/kvm/cpuid.c | 23 +++++++++++++++++------
> > >  1 file changed, 17 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > > index 07be45c5bb93..b5af9e451bef 100644
> > > --- a/arch/x86/kvm/cpuid.c
> > > +++ b/arch/x86/kvm/cpuid.c
> > > @@ -62,6 +62,7 @@ u32 xstate_required_size(u64 xstate_bv, bool compacted)
> > >   * This one is tied to SSB in the user API, and not
> > >   * visible in /proc/cpuinfo.
> > >   */
> > > +#define KVM_X86_FEATURE_PSFD         0          /* Predictive Store Forwarding Disable */
> >
> > I believe we can use "enum kvm_only_cpuid_leafs" to handle this.  E.g.
> >
> >         enum kvm_only_cpuid_leafs {
> >                 CPUID_12_EAX     = NCAPINTS,
> >                 CPUID_7_2_EDX,
> >                 NR_KVM_CPU_CAPS,
> >
> >                 NKVMCAPINTS = NR_KVM_CPU_CAPS - NCAPINTS,
> >         };
> >
> > then the intended use of KVM_X86_FEATURE_*
> >
> >         #define KVM_X86_FEATURE_PSFD    KVM_X86_FEATURE(CPUID_7_2_EDX, 0)
> >
> > I _think_ we can then define an arbitrary word for X86_FEATURE_PSFD, e.g.
> >
> >         #define X86_FEATURE_PSFD        (NKVMCAPINTS*32+0)
> 
> We may run afoul of reverse_cpuid_check(), depending on usage.

Oh, yeah, an entry in reverse_cpuid[] would also be needed.  For posterity since
PSFD doesn't need a KVM-only entry[*]...

[*] https://lore.kernel.org/all/Y0CrER%2FyiHheWiZw@google.com

Re: [PATCH v2 1/2] KVM: x86: Insert "AMD" in KVM_X86_FEATURE_PSFD

[Paolo Bonzini]

On 8/31/22 01:32, Jim Mattson wrote:
>> This is asinine.  If KVM is forced to carry the feature bit then IMO we have every
>> right to the "real" name.  If we can't convince others that this belongs in
>> cpufeatures.h, then I vote to rename this to X86_FEATURE_AMD_PSFD so that we don't
>> have to special case this thing.
> You won't get any argument from me!
> 
> If Borislav objects to seeing the feature in /proc/cpuinfo, can't we
> just begin the cpufeatures.h descriptive comment with ""?

Yes, since this is just part of word 13 it should just use "".

It won't help for the Intel one which uses CPUID[EAX=7,ECX=2] though.

Paolo