[PATCH v2 0/6] Clean up the supported xfeatures

[PATCH v2 0/6] Clean up the supported xfeatures

[Aaron Lewis]

Make sure the supported xfeatures, i.e. EDX:EAX of CPUID.(EAX=0DH,ECX=0),
for MPX, AVX-512, and AMX are in a valid state and follow the rules
outlined in the SDM vol 1, section 13.3 ENABLING THE XSAVE FEATURE SET
AND XSAVE-ENABLED FEATURES.  While those rules apply to the enabled
xfeatures, i.e. XCR0, use them to set the supported xfeatures.  That way
if they are used by userspace or a guest to set the enabled xfeatures,
they won't cause a #GP.  

A test is then added to verify the supported xfeatures are in this
sanitied state.

Aaron Lewis (6):
  KVM: x86: Clear all MPX xfeatures if they are not all set
  KVM: x86: Clear all AVX-512 xfeatures if they are not all set
  KVM: x86: Clear all AMX xfeatures if they are not all set
  KVM: selftests: Hoist XGETBV and XSETBV to make them more accessible
  KVM: selftests: Add XFEATURE flags to common code
  KVM: selftests: Add XCR0 Test

 arch/x86/kvm/cpuid.c                          |  19 +++
 tools/testing/selftests/kvm/Makefile          |   1 +
 .../selftests/kvm/include/x86_64/processor.h  |  35 +++++
 tools/testing/selftests/kvm/x86_64/amx_test.c |  46 ++-----
 .../selftests/kvm/x86_64/xcr0_cpuid_test.c    | 121 ++++++++++++++++++
 5 files changed, 187 insertions(+), 35 deletions(-)
 create mode 100644 tools/testing/selftests/kvm/x86_64/xcr0_cpuid_test.c

-- 
2.39.0.314.g84b9a713c41-goog

[PATCH v2 1/6] KVM: x86: Clear all supported MPX xfeatures if they are not all set

[Aaron Lewis]

Be a good citizen and don't allow any of the supported MPX xfeatures[1]
to be set if they can't all be set.  That way userspace or a guest
doesn't fail if it attempts to set them in XCR0.

[1] CPUID.(EAX=0DH,ECX=0):EAX.BNDREGS[bit-3]
    CPUID.(EAX=0DH,ECX=0):EAX.BNDCSR[bit-4]

Suggested-by: Jim Mattson <jmattson@google.com>
Signed-off-by: Aaron Lewis <aaronlewis@google.com>
---
 arch/x86/kvm/cpuid.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index c4e8257629165..2431c46d456b4 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -855,6 +855,16 @@ static int __do_cpuid_func_emulated(struct kvm_cpuid_array *array, u32 func)
 	return 0;
 }
 
+static u64 sanitize_xcr0(u64 xcr0)
+{
+	u64 mask;
+
+	mask = XFEATURE_MASK_BNDREGS | XFEATURE_MASK_BNDCSR;
+	if ((xcr0 & mask) != mask)
+		xcr0 &= ~mask;
+
+	return xcr0;
+}
+
 static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
 {
 	struct kvm_cpuid_entry2 *entry;
@@ -982,6 +992,8 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
 		u64 permitted_xcr0 = kvm_caps.supported_xcr0 & xstate_get_guest_group_perm();
 		u64 permitted_xss = kvm_caps.supported_xss;
 
+		permitted_xcr0 = sanitize_xcr0(permitted_xcr0);
+
 		entry->eax &= permitted_xcr0;
 		entry->ebx = xstate_required_size(permitted_xcr0, false);
 		entry->ecx = entry->ebx;
-- 
2.39.0.314.g84b9a713c41-goog

Re: [PATCH v2 1/6] KVM: x86: Clear all supported MPX xfeatures if they are not all set

[Xiaoyao Li]

On 12/31/2022 12:24 AM, Aaron Lewis wrote:
> Be a good citizen and don't allow any of the supported MPX xfeatures[1]
> to be set if they can't all be set.  That way userspace or a guest
> doesn't fail if it attempts to set them in XCR0.
> 
> [1] CPUID.(EAX=0DH,ECX=0):EAX.BNDREGS[bit-3]
>      CPUID.(EAX=0DH,ECX=0):EAX.BNDCSR[bit-4]
> 
> Suggested-by: Jim Mattson <jmattson@google.com>
> Signed-off-by: Aaron Lewis <aaronlewis@google.com>
> ---
>   arch/x86/kvm/cpuid.c | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index c4e8257629165..2431c46d456b4 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -855,6 +855,16 @@ static int __do_cpuid_func_emulated(struct kvm_cpuid_array *array, u32 func)
>   	return 0;
>   }
>   
> +static u64 sanitize_xcr0(u64 xcr0)
> +{
> +	u64 mask;
> +
> +	mask = XFEATURE_MASK_BNDREGS | XFEATURE_MASK_BNDCSR;
> +	if ((xcr0 & mask) != mask)
> +		xcr0 &= ~mask;

Maybe it can WARN_ON_ONCE() here.

It implies either a kernel bug that permitted_xcr0 is invalid or a 
broken HW.

> +	return xcr0;
> +}
> +
>   static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
>   {
>   	struct kvm_cpuid_entry2 *entry;
> @@ -982,6 +992,8 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
>   		u64 permitted_xcr0 = kvm_caps.supported_xcr0 & xstate_get_guest_group_perm();
>   		u64 permitted_xss = kvm_caps.supported_xss;
>   
> +		permitted_xcr0 = sanitize_xcr0(permitted_xcr0);
> +
>   		entry->eax &= permitted_xcr0;
>   		entry->ebx = xstate_required_size(permitted_xcr0, false);
>   		entry->ecx = entry->ebx;

Re: [PATCH v2 1/6] KVM: x86: Clear all supported MPX xfeatures if they are not all set

[Sean Christopherson]

On Mon, Jan 02, 2023, Xiaoyao Li wrote:
> On 12/31/2022 12:24 AM, Aaron Lewis wrote:
> > Be a good citizen and don't allow any of the supported MPX xfeatures[1]
> > to be set if they can't all be set.  That way userspace or a guest
> > doesn't fail if it attempts to set them in XCR0.
> > 
> > [1] CPUID.(EAX=0DH,ECX=0):EAX.BNDREGS[bit-3]
> >      CPUID.(EAX=0DH,ECX=0):EAX.BNDCSR[bit-4]
> > 
> > Suggested-by: Jim Mattson <jmattson@google.com>
> > Signed-off-by: Aaron Lewis <aaronlewis@google.com>
> > ---
> >   arch/x86/kvm/cpuid.c | 12 ++++++++++++
> >   1 file changed, 12 insertions(+)
> > 
> > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > index c4e8257629165..2431c46d456b4 100644
> > --- a/arch/x86/kvm/cpuid.c
> > +++ b/arch/x86/kvm/cpuid.c
> > @@ -855,6 +855,16 @@ static int __do_cpuid_func_emulated(struct kvm_cpuid_array *array, u32 func)
> >   	return 0;
> >   }
> > +static u64 sanitize_xcr0(u64 xcr0)
> > +{
> > +	u64 mask;
> > +
> > +	mask = XFEATURE_MASK_BNDREGS | XFEATURE_MASK_BNDCSR;
> > +	if ((xcr0 & mask) != mask)
> > +		xcr0 &= ~mask;
> 
> Maybe it can WARN_ON_ONCE() here.
> 
> It implies either a kernel bug that permitted_xcr0 is invalid or a broken
> HW.

I'm pretty sure KVM can't WARN, as this falls into the category of "it's technically
architecturally legal to report only one of the features, but real hardware will
always report both".

Re: [PATCH v2 1/6] KVM: x86: Clear all supported MPX xfeatures if they are not all set

[Sean Christopherson]

On Fri, Dec 30, 2022, Aaron Lewis wrote:
> Be a good citizen and don't allow any of the supported MPX xfeatures[1]
> to be set if they can't all be set.  That way userspace or a guest
> doesn't fail if it attempts to set them in XCR0.
> 
> [1] CPUID.(EAX=0DH,ECX=0):EAX.BNDREGS[bit-3]
>     CPUID.(EAX=0DH,ECX=0):EAX.BNDCSR[bit-4]
> 
> Suggested-by: Jim Mattson <jmattson@google.com>
> Signed-off-by: Aaron Lewis <aaronlewis@google.com>
> ---
>  arch/x86/kvm/cpuid.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index c4e8257629165..2431c46d456b4 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -855,6 +855,16 @@ static int __do_cpuid_func_emulated(struct kvm_cpuid_array *array, u32 func)
>  	return 0;
>  }
>  
> +static u64 sanitize_xcr0(u64 xcr0)
> +{
> +	u64 mask;
> +
> +	mask = XFEATURE_MASK_BNDREGS | XFEATURE_MASK_BNDCSR;
> +	if ((xcr0 & mask) != mask)
> +		xcr0 &= ~mask;
> +
> +	return xcr0;
> +}
> +
>  static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
>  {
>  	struct kvm_cpuid_entry2 *entry;
> @@ -982,6 +992,8 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
>  		u64 permitted_xcr0 = kvm_caps.supported_xcr0 & xstate_get_guest_group_perm();
>  		u64 permitted_xss = kvm_caps.supported_xss;
>  
> +		permitted_xcr0 = sanitize_xcr0(permitted_xcr0);


This isn't 100% correct, all usage needs to be sanitized so that KVM provides a
consistent view.  E.g. KVM_CAP_XSAVE2 would report the wrong size.

	case KVM_CAP_XSAVE2: {
		u64 guest_perm = xstate_get_guest_group_perm();

		r = xstate_required_size(kvm_caps.supported_xcr0 & guest_perm, false);
		if (r < sizeof(struct kvm_xsave))
			r = sizeof(struct kvm_xsave);
		break;
	}

Barring a kernel bug, xstate_get_guest_group_perm() will never report partial
support, so I think the easy solution is to sanitize kvm_caps.suport_xcr0.

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 2480b8027a45..7ea06c58eaf6 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -9344,6 +9344,10 @@ int kvm_arch_init(void *opaque)
        if (boot_cpu_has(X86_FEATURE_XSAVE)) {
                host_xcr0 = xgetbv(XCR_XFEATURE_ENABLED_MASK);
                kvm_caps.supported_xcr0 = host_xcr0 & KVM_SUPPORTED_XCR0;
+               if (!(kvm_caps.supported_xcr0 & XFEATURE_MASK_BNDREGS) ||
+                   !(kvm_caps.supported_xcr0 & XFEATURE_MASK_BNDCSR))
+                       kvm_caps.supported_xcr0 &= ~(XFEATURE_MASK_BNDREGS |
+                                                    XFEATURE_MASK_BNDCSR);
        }
 
        if (pi_inject_timer == -1)


> +
>  		entry->eax &= permitted_xcr0;
>  		entry->ebx = xstate_required_size(permitted_xcr0, false);
>  		entry->ecx = entry->ebx;
> -- 
> 2.39.0.314.g84b9a713c41-goog
> 

Re: [PATCH v2 1/6] KVM: x86: Clear all supported MPX xfeatures if they are not all set

[Aaron Lewis]

> >  static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
> >  {
> >       struct kvm_cpuid_entry2 *entry;
> > @@ -982,6 +992,8 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
> >               u64 permitted_xcr0 = kvm_caps.supported_xcr0 & xstate_get_guest_group_perm();
> >               u64 permitted_xss = kvm_caps.supported_xss;
> >
> > +             permitted_xcr0 = sanitize_xcr0(permitted_xcr0);
>
>
> This isn't 100% correct, all usage needs to be sanitized so that KVM provides a
> consistent view.  E.g. KVM_CAP_XSAVE2 would report the wrong size.
>
>         case KVM_CAP_XSAVE2: {
>                 u64 guest_perm = xstate_get_guest_group_perm();
>
>                 r = xstate_required_size(kvm_caps.supported_xcr0 & guest_perm, false);
>                 if (r < sizeof(struct kvm_xsave))
>                         r = sizeof(struct kvm_xsave);
>                 break;
>         }
>
> Barring a kernel bug, xstate_get_guest_group_perm() will never report partial
> support, so I think the easy solution is to sanitize kvm_caps.suport_xcr0.
>

When I run xcr0_cpuid_test it fails because
xstate_get_guest_group_perm() reports partial support on SPR.  It's
reporting 0x206e7 rather than the 0x6e7 I was hoping for.  That's why
I went down the road of sanitizing xcr0.  Though, if it's expected for
that to report something valid then sanitizing seems like the wrong
approach.  If xcr0 is invalid it should stay invalid, and it should
cause a test to fail.

Looking at how xstate_get_guest_group_perm() comes through with
invalid bits I came across this commit:

2308ee57d93d ("x86/fpu/amx: Enable the AMX feature in 64-bit mode")

-       /* [XFEATURE_XTILE_DATA] = XFEATURE_MASK_XTILE, */
+       [XFEATURE_XTILE_DATA] = XFEATURE_MASK_XTILE_DATA,

Seems like it should really be:

+       [XFEATURE_XTILE_DATA] = XFEATURE_MASK_XTILE,

With that change xstate_get_guest_group_perm() should no longer report
partial support.

That means this entire series can be simplified to a 'fixes patch' for
commit 2308ee57d93d and xcr0_cpuid_test to demonstrate the fix.

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 2480b8027a45..7ea06c58eaf6 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -9344,6 +9344,10 @@ int kvm_arch_init(void *opaque)
>         if (boot_cpu_has(X86_FEATURE_XSAVE)) {
>                 host_xcr0 = xgetbv(XCR_XFEATURE_ENABLED_MASK);
>                 kvm_caps.supported_xcr0 = host_xcr0 & KVM_SUPPORTED_XCR0;
> +               if (!(kvm_caps.supported_xcr0 & XFEATURE_MASK_BNDREGS) ||
> +                   !(kvm_caps.supported_xcr0 & XFEATURE_MASK_BNDCSR))
> +                       kvm_caps.supported_xcr0 &= ~(XFEATURE_MASK_BNDREGS |
> +                                                    XFEATURE_MASK_BNDCSR);
>         }
>
>         if (pi_inject_timer == -1)
>
>
> > +
> >               entry->eax &= permitted_xcr0;
> >               entry->ebx = xstate_required_size(permitted_xcr0, false);
> >               entry->ecx = entry->ebx;
> > --
> > 2.39.0.314.g84b9a713c41-goog
> >

Re: [PATCH v2 1/6] KVM: x86: Clear all supported MPX xfeatures if they are not all set

[Chang S. Bae]

On 1/10/2023 6:49 AM, Aaron Lewis wrote:
> 
> When I run xcr0_cpuid_test it fails because
> xstate_get_guest_group_perm() reports partial support on SPR.  It's
> reporting 0x206e7 rather than the 0x6e7 I was hoping for.  That's why
> I went down the road of sanitizing xcr0.  Though, if it's expected for
> that to report something valid then sanitizing seems like the wrong
> approach.  If xcr0 is invalid it should stay invalid, and it should
> cause a test to fail.

FWIW, we have this [1]:

/* Features which are dynamically enabled for a process on request */
#define XFEATURE_MASK_USER_DYNAMIC	XFEATURE_MASK_XTILE_DATA

IOW, TILE_CFG is not part of the dynamic state. Because this state is 
not XFD-supported, we can't enforce the state use. SDM has relevant text 
here [2]:

"LDTILECFG and TILERELEASE initialize the TILEDATA state component. An 
execution of either of these instructions does not generate #NM when 
XCR0[18] = IA32_XFD[18] = 1; instead, it initializes TILEDATA normally.
(Note that STTILECFG does not use the TILEDATA state component. Thus, an 
execution of this instruction does
not generate #NM when XCR0[18] = IA32_XFD[18] = 1.)"

> Looking at how xstate_get_guest_group_perm() comes through with
> invalid bits I came across this commit:
> 
> 2308ee57d93d ("x86/fpu/amx: Enable the AMX feature in 64-bit mode")
> 
> -       /* [XFEATURE_XTILE_DATA] = XFEATURE_MASK_XTILE, */
> +       [XFEATURE_XTILE_DATA] = XFEATURE_MASK_XTILE_DATA,
> 
> Seems like it should really be:
> 
> +       [XFEATURE_XTILE_DATA] = XFEATURE_MASK_XTILE,

Thus, the change was intentional as far as I can remember.

Thank,
Chang

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/include/asm/fpu/xstate.h#n50
[2] SDM Vol 1. 13.14 Extended Feature Disable (XFD), 
https://www.intel.com/content/www/us/en/developer/articles/technical/intel-sdm.html

Re: [PATCH v2 1/6] KVM: x86: Clear all supported MPX xfeatures if they are not all set

[Mingwei Zhang]

On Tue, Jan 10, 2023, Chang S. Bae wrote:
> On 1/10/2023 6:49 AM, Aaron Lewis wrote:
> > 
> > When I run xcr0_cpuid_test it fails because
> > xstate_get_guest_group_perm() reports partial support on SPR.  It's
> > reporting 0x206e7 rather than the 0x6e7 I was hoping for.  That's why
> > I went down the road of sanitizing xcr0.  Though, if it's expected for
> > that to report something valid then sanitizing seems like the wrong
> > approach.  If xcr0 is invalid it should stay invalid, and it should
> > cause a test to fail.
> 
> FWIW, we have this [1]:
> 
> /* Features which are dynamically enabled for a process on request */
> #define XFEATURE_MASK_USER_DYNAMIC	XFEATURE_MASK_XTILE_DATA
> 
> IOW, TILE_CFG is not part of the dynamic state. Because this state is not
> XFD-supported, we can't enforce the state use. SDM has relevant text here
> [2]:
> 
> "LDTILECFG and TILERELEASE initialize the TILEDATA state component. An
> execution of either of these instructions does not generate #NM when
> XCR0[18] = IA32_XFD[18] = 1; instead, it initializes TILEDATA normally.
> (Note that STTILECFG does not use the TILEDATA state component. Thus, an
> execution of this instruction does
> not generate #NM when XCR0[18] = IA32_XFD[18] = 1.)"
> 
> > Looking at how xstate_get_guest_group_perm() comes through with
> > invalid bits I came across this commit:
> > 
> > 2308ee57d93d ("x86/fpu/amx: Enable the AMX feature in 64-bit mode")
> > 
> > -       /* [XFEATURE_XTILE_DATA] = XFEATURE_MASK_XTILE, */
> > +       [XFEATURE_XTILE_DATA] = XFEATURE_MASK_XTILE_DATA,
> > 
> > Seems like it should really be:
> > 
> > +       [XFEATURE_XTILE_DATA] = XFEATURE_MASK_XTILE,
> 
> Thus, the change was intentional as far as I can remember.
> 
> Thank,
> Chang
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/include/asm/fpu/xstate.h#n50
> [2] SDM Vol 1. 13.14 Extended Feature Disable (XFD), https://www.intel.com/content/www/us/en/developer/articles/technical/intel-sdm.html
> 

Hmm. Given that XFEATURE_XTILE_DATA is a dynamic feature, that means
XFEATURE_XTILE_CFG could be set without XFEATURE_XTILE_DATA. Shall we
also consider loose the constraints at __kvm_set_xcr() as well?

https://elixir.bootlin.com/linux/v6.1.4/source/arch/x86/kvm/x86.c#L1079

Re: [PATCH v2 1/6] KVM: x86: Clear all supported MPX xfeatures if they are not all set

[Chang S. Bae]

On 1/12/2023 10:21 AM, Mingwei Zhang wrote:
> 
> Hmm. Given that XFEATURE_XTILE_DATA is a dynamic feature, that means
> XFEATURE_XTILE_CFG could be set without XFEATURE_XTILE_DATA.

No, XCR0 is different from the IA32_XFD MSR. Check this in the SDM -- 
Vol.1 13.3 "Enabling the XSAVE Feature Set and XSAVE-Enabled Features":

"In addition, executing the XSETBV instruction causes a general-
  protection fault (#GP) if ECX = 0 and EAX[17] ≠ EAX[18] (TILECFG and
  TILEDATA must be enabled together). This implies that the value of
  XCR0[18:17] is always either 00b or 11b."

Thanks,
Chang

Re: [PATCH v2 1/6] KVM: x86: Clear all supported MPX xfeatures if they are not all set

[Mingwei Zhang]

On Thu, Jan 12, 2023, Chang S. Bae wrote:
> On 1/12/2023 10:21 AM, Mingwei Zhang wrote:
> > 
> > Hmm. Given that XFEATURE_XTILE_DATA is a dynamic feature, that means
> > XFEATURE_XTILE_CFG could be set without XFEATURE_XTILE_DATA.
> 
> No, XCR0 is different from the IA32_XFD MSR. Check this in the SDM -- Vol.1
> 13.3 "Enabling the XSAVE Feature Set and XSAVE-Enabled Features":
> 
> "In addition, executing the XSETBV instruction causes a general-
>  protection fault (#GP) if ECX = 0 and EAX[17] ≠ EAX[18] (TILECFG and
>  TILEDATA must be enabled together). This implies that the value of
>  XCR0[18:17] is always either 00b or 11b."
> 
> Thanks,
> Chang

Hmm. Make sense. The proper execution of XSETBV was the key point of the
patch. But the permitted_xcr0 and supported_xcr0 seems never used
directly as the parameter of XSETBV, but only for size calculation.

One more question: I am very confused by the implementation of
xstate_get_guest_group_perm(). It seems fetching the xfeatures from the
host process (&current->group_leader->thread.fpu). Is this intentional?
Does that mean in order to enable AMX in the guest we have to enable it
on the host process first?

Re: [PATCH v2 1/6] KVM: x86: Clear all supported MPX xfeatures if they are not all set

[Chang S. Bae]

On 1/12/2023 11:17 AM, Mingwei Zhang wrote:
> 
> But the permitted_xcr0 and supported_xcr0 seems never used
> directly as the parameter of XSETBV, but only for size calculation.

Yeah, I saw that too, and tried to improve it [1]. Maybe this is not a 
big deal in KVM.

> One more question: I am very confused by the implementation of
> xstate_get_guest_group_perm(). It seems fetching the xfeatures from the
> host process (&current->group_leader->thread.fpu). Is this intentional?
> Does that mean in order to enable AMX in the guest we have to enable it
> on the host process first?

Yes, it was designed that QEMU requests permissions via arch_prctl() 
before creating vCPU threads [2][3]. Granted, this feature capability 
will be advertised to the guest. Then, it will *enable* the feature, right?

Thanks,
Chang

[1] 
https://lore.kernel.org/kvm/20220823231402.7839-2-chang.seok.bae@intel.com/
[2] https://lore.kernel.org/lkml/87wnmf66m5.ffs@tglx/
[3] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=980fe2fddcff

Re: [PATCH v2 1/6] KVM: x86: Clear all supported MPX xfeatures if they are not all set

[Mingwei Zhang]

On Thu, Jan 12, 2023, Chang S. Bae wrote:
> On 1/12/2023 11:17 AM, Mingwei Zhang wrote:
> > 
> > But the permitted_xcr0 and supported_xcr0 seems never used
> > directly as the parameter of XSETBV, but only for size calculation.
> 
> Yeah, I saw that too, and tried to improve it [1]. Maybe this is not a big
> deal in KVM.
> 
> > One more question: I am very confused by the implementation of
> > xstate_get_guest_group_perm(). It seems fetching the xfeatures from the
> > host process (&current->group_leader->thread.fpu). Is this intentional?
> > Does that mean in order to enable AMX in the guest we have to enable it
> > on the host process first?
> 
> Yes, it was designed that QEMU requests permissions via arch_prctl() before
> creating vCPU threads [2][3]. Granted, this feature capability will be
> advertised to the guest. Then, it will *enable* the feature, right?
> 
> Thanks,
> Chang
> 
> [1]
> https://lore.kernel.org/kvm/20220823231402.7839-2-chang.seok.bae@intel.com/
> [2] https://lore.kernel.org/lkml/87wnmf66m5.ffs@tglx/
> [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=980fe2fddcff
> 

Thanks for the clarification. Yeah, that was out of my expectation since
I assumed AMX enabling in the guest should be orthogonal to the enabling
in the host. But since AMX requires dynamic size of fp_state, host
awareness of larger fp_state is highly intended.

The only comment I would have is that it seems not following the least
privilege principle as host process (QEMU) may not have the motivation
to do any matrix multiplication. But this is a minor one.

Since this enabling once per-process, I am wondering when after
invocation of arch_prctl(2), all of the host threads will have a larger
fp_state? If so, that might be a sizeable overhead since host userspace
may have lots of threads doing various of other things, i.e., they may
not be vCPU threads.

Re: [PATCH v2 1/6] KVM: x86: Clear all supported MPX xfeatures if they are not all set

[Chang S. Bae]

On 1/12/2023 1:21 PM, Mingwei Zhang wrote:
> 
> The only comment I would have is that it seems not following the least
> privilege principle as host process (QEMU) may not have the motivation
> to do any matrix multiplication. But this is a minor one.
> 
> Since this enabling once per-process, I am wondering when after
> invocation of arch_prctl(2), all of the host threads will have a larger
> fp_state? If so, that might be a sizeable overhead since host userspace
> may have lots of threads doing various of other things, i.e., they may
> not be vCPU threads.

No, the permission request does not immediately result in the kernel's 
XSAVE buffer expansion, but only when the state is about used. As 
XFD-armed, the state use will raise #NM. Then, it will reallocate the 
task's fpstate via this call chain:

#NM --> handle_xfd_event() --> xfd_enable_feature() --> fpstate_realloc()

Thanks,
Chang

Re: [PATCH v2 1/6] KVM: x86: Clear all supported MPX xfeatures if they are not all set

[Mingwei Zhang]

On Thu, Jan 12, 2023 at 1:33 PM Chang S. Bae <chang.seok.bae@intel.com> wrote:
>
> On 1/12/2023 1:21 PM, Mingwei Zhang wrote:
> >
> > The only comment I would have is that it seems not following the least
> > privilege principle as host process (QEMU) may not have the motivation
> > to do any matrix multiplication. But this is a minor one.
> >
> > Since this enabling once per-process, I am wondering when after
> > invocation of arch_prctl(2), all of the host threads will have a larger
> > fp_state? If so, that might be a sizeable overhead since host userspace
> > may have lots of threads doing various of other things, i.e., they may
> > not be vCPU threads.
>
> No, the permission request does not immediately result in the kernel's
> XSAVE buffer expansion, but only when the state is about used. As
> XFD-armed, the state use will raise #NM. Then, it will reallocate the
> task's fpstate via this call chain:
>
> #NM --> handle_xfd_event() --> xfd_enable_feature() --> fpstate_realloc()
>
> Thanks,
> Chang

Thanks for the info. But I think you are talking about host level AMX
enabling. This is known to me. I am asking about how AMX was enabled
by QEMU and used by vCPU threads in the guest. After digging a little
bit, I think I understand it now.

So, it should be the following: (in fact, the guest fp_state is not
allocated lazily but at the very beginning at KVM_SET_CPUID2 time).

  kvm_set_cpuid() / kvm_set_cpuid2() ->
    kvm_check_cpuid() ->
      fpu_enable_guest_xfd_features() ->
        __xfd_enable_feature() ->
          fpstate_realloc()

Note that KVM does intercept #NM for the guest, but only for the
handling of XFD_ERR.

Prior to the kvm_set_cpuid() or kvm_set_cpuid2() call, the QEMU thread
should ask for permission via arch_prctl(REQ_XCOMP_GUEST_PERM) in
order to become a vCPU thread. Otherwise, the above call sequence will
fail. Fortunately, asking-for-guest-permission is only needed once per
process (per-VM).

Because of the above, the non-vCPU threads do not need to create a
larger fp_state unless/until they invoke kvm_set_cpuid() or
kvm_set_cpuid2().

Now, I think that closes the loop for me.

Thanks.

-Mingwei

Re: [PATCH v2 1/6] KVM: x86: Clear all supported MPX xfeatures if they are not all set

[Aaron Lewis]

On Fri, Jan 13, 2023 at 12:26 AM Mingwei Zhang <mizhang@google.com> wrote:
>
> On Thu, Jan 12, 2023 at 1:33 PM Chang S. Bae <chang.seok.bae@intel.com> wrote:
> >
> > On 1/12/2023 1:21 PM, Mingwei Zhang wrote:
> > >
> > > The only comment I would have is that it seems not following the least
> > > privilege principle as host process (QEMU) may not have the motivation
> > > to do any matrix multiplication. But this is a minor one.
> > >
> > > Since this enabling once per-process, I am wondering when after
> > > invocation of arch_prctl(2), all of the host threads will have a larger
> > > fp_state? If so, that might be a sizeable overhead since host userspace
> > > may have lots of threads doing various of other things, i.e., they may
> > > not be vCPU threads.
> >
> > No, the permission request does not immediately result in the kernel's
> > XSAVE buffer expansion, but only when the state is about used. As
> > XFD-armed, the state use will raise #NM. Then, it will reallocate the
> > task's fpstate via this call chain:
> >
> > #NM --> handle_xfd_event() --> xfd_enable_feature() --> fpstate_realloc()
> >
> > Thanks,
> > Chang
>
> Thanks for the info. But I think you are talking about host level AMX
> enabling. This is known to me. I am asking about how AMX was enabled
> by QEMU and used by vCPU threads in the guest. After digging a little
> bit, I think I understand it now.
>
> So, it should be the following: (in fact, the guest fp_state is not
> allocated lazily but at the very beginning at KVM_SET_CPUID2 time).
>
>   kvm_set_cpuid() / kvm_set_cpuid2() ->
>     kvm_check_cpuid() ->
>       fpu_enable_guest_xfd_features() ->
>         __xfd_enable_feature() ->
>           fpstate_realloc()
>
> Note that KVM does intercept #NM for the guest, but only for the
> handling of XFD_ERR.
>
> Prior to the kvm_set_cpuid() or kvm_set_cpuid2() call, the QEMU thread
> should ask for permission via arch_prctl(REQ_XCOMP_GUEST_PERM) in
> order to become a vCPU thread. Otherwise, the above call sequence will
> fail. Fortunately, asking-for-guest-permission is only needed once per
> process (per-VM).
>
> Because of the above, the non-vCPU threads do not need to create a
> larger fp_state unless/until they invoke kvm_set_cpuid() or
> kvm_set_cpuid2().
>
> Now, I think that closes the loop for me.
>
> Thanks.
>
> -Mingwei

I'd still like to clean up CPUID.(EAX=0DH,ECX=0):EAX.XTILECFG[17] by
keeping it consistent with CPUID.(EAX=0DH,ECX=0):EAX.XTILEDATA[18] in
the guest, but it's not clear to me what the best way to do that is.
The crux of the issue is that xstate_get_guest_group_perm() returns
partial support for AMX when userspace doesn't call
prctl(ARCH_REQ_XCOMP_GUEST_PERM), I.e. the guest CPUID will report
XTILECFG=1 and XTILEDATA=0 in that case.  In that situation, XTILECFG
should be cleared for it to be consistent.  I can see two ways of
potentially doing that:

1. We can ensure that perm->__state_perm never stores partial support.

2. We can sanitize the bits in xstate_get_guest_group_perm() before
they are returned, to ensure KVM never sees partial support.

I like the idea of #1, but if that has negative effects on the host or
XFD I'm open to #2.  Though, XFD has its own field, so I thought that
wouldn't be an issue.  Would it work to set __state_perm and/or
default_features (what originally sets __state_perm) to a consistent
view, so partial support is never returned from
xstate_get_guest_group_perm()?

Re: [PATCH v2 1/6] KVM: x86: Clear all supported MPX xfeatures if they are not all set

[Chang S. Bae]

On 1/13/2023 6:43 AM, Aaron Lewis wrote:
> 
> I'd still like to clean up CPUID.(EAX=0DH,ECX=0):EAX.XTILECFG[17] by
> keeping it consistent with CPUID.(EAX=0DH,ECX=0):EAX.XTILEDATA[18] in
> the guest, but it's not clear to me what the best way to do that is.
> The crux of the issue is that xstate_get_guest_group_perm() returns
> partial support for AMX when userspace doesn't call
> prctl(ARCH_REQ_XCOMP_GUEST_PERM), I.e. the guest CPUID will report
> XTILECFG=1 and XTILEDATA=0 in that case.  In that situation, XTILECFG
> should be cleared for it to be consistent.  I can see two ways of
> potentially doing that:
> 
> 1. We can ensure that perm->__state_perm never stores partial support.
> 
> 2. We can sanitize the bits in xstate_get_guest_group_perm() before
> they are returned, to ensure KVM never sees partial support.
> 
> I like the idea of #1, but if that has negative effects on the host or
> XFD I'm open to #2.  Though, XFD has its own field, so I thought that
> wouldn't be an issue.  Would it work to set __state_perm and/or
> default_features (what originally sets __state_perm) to a consistent
> view, so partial support is never returned from
> xstate_get_guest_group_perm()?

FWIW, I was trying to clarify that ARCH_GET_XCOMP_GUEST_PERM is a 
variant of ARCH_GET_XCOMP_PERM in the documentation [1]. So, I guess #1 
will have some side-effect (at least confusion) for this semantics.

There may be some ways to transform the permission bits to the 
XCR0-capable bits. For instance, when considering this permission 
support in host, the highest feature number was considered to denote the 
rest feature bits [2].

Thanks,
Chang

[1] 
https://lore.kernel.org/lkml/20220922195810.23248-5-chang.seok.bae@intel.com/
[2] https://lore.kernel.org/lkml/878rz7fyhe.ffs@tglx/

Re: [PATCH v2 1/6] KVM: x86: Clear all supported MPX xfeatures if they are not all set

[Mingwei Zhang]

On Tue, Jan 17, 2023 at 12:34 PM Chang S. Bae <chang.seok.bae@intel.com> wrote:
>
> On 1/13/2023 6:43 AM, Aaron Lewis wrote:
> >
> > I'd still like to clean up CPUID.(EAX=0DH,ECX=0):EAX.XTILECFG[17] by
> > keeping it consistent with CPUID.(EAX=0DH,ECX=0):EAX.XTILEDATA[18] in
> > the guest, but it's not clear to me what the best way to do that is.
> > The crux of the issue is that xstate_get_guest_group_perm() returns
> > partial support for AMX when userspace doesn't call
> > prctl(ARCH_REQ_XCOMP_GUEST_PERM), I.e. the guest CPUID will report
> > XTILECFG=1 and XTILEDATA=0 in that case.  In that situation, XTILECFG
> > should be cleared for it to be consistent.  I can see two ways of
> > potentially doing that:
> >
> > 1. We can ensure that perm->__state_perm never stores partial support.
> >
> > 2. We can sanitize the bits in xstate_get_guest_group_perm() before
> > they are returned, to ensure KVM never sees partial support.
> >
> > I like the idea of #1, but if that has negative effects on the host or
> > XFD I'm open to #2.  Though, XFD has its own field, so I thought that
> > wouldn't be an issue.  Would it work to set __state_perm and/or
> > default_features (what originally sets __state_perm) to a consistent
> > view, so partial support is never returned from
> > xstate_get_guest_group_perm()?
>
> FWIW, I was trying to clarify that ARCH_GET_XCOMP_GUEST_PERM is a
> variant of ARCH_GET_XCOMP_PERM in the documentation [1]. So, I guess #1
> will have some side-effect (at least confusion) for this semantics.

Right, before talking in this thread, I was not aware of the other
arch_prctl(2) for AMX. But when I look into it, it looks reasonable to
me from an engineering point of view since it seems reusing almost all
of the code in the host.

ARCH_GET_XCOMP_GUEST_PERM is to ask for "guest permission", but it is
not just about the "permission" (the fp_state size increase for AMX).
It is also about the CPUID feature bits exposure. So for the host side
of AMX usage, this is fine, since there is no partial CPUID exposure
problem. But the guest side does have it.

So, can we just grant two bits instead of 1 bit? For that, I find 1)
seems more reasonable than 2). Of course, if there are many side
effects, option #2 could be considered as well. But before that, it
will be good to understand where the side effects are.

>
> There may be some ways to transform the permission bits to the
> XCR0-capable bits. For instance, when considering this permission
> support in host, the highest feature number was considered to denote the
> rest feature bits [2].

Hmm, this is out of my concern since it is about the host-level
enabling. I have no problem with the existing host side permission
control for AMX.

However, for me, [2] does not seem a little hacky but I get the point.
The concern is that how do we know in the future, the highest bit
always indicates lower bits? Will Intel CPU features always follow
this style?  Even so, there is no such guidance/guarantee that other
CPU vendors (e.g., AMD) will do the same. Also what if there are more
than 1 dynamic features for a feature? Please kindly correct me.

Thanks.
-Mingwei


>
> Thanks,
> Chang
>
> [1]
> https://lore.kernel.org/lkml/20220922195810.23248-5-chang.seok.bae@intel.com/
> [2] https://lore.kernel.org/lkml/878rz7fyhe.ffs@tglx/

Re: [PATCH v2 1/6] KVM: x86: Clear all supported MPX xfeatures if they are not all set

[Chang S. Bae]

On 1/17/2023 2:39 PM, Mingwei Zhang wrote:
> On Tue, Jan 17, 2023 at 12:34 PM Chang S. Bae <chang.seok.bae@intel.com> wrote:
>>
>> On 1/13/2023 6:43 AM, Aaron Lewis wrote:
>>>
>>> I'd still like to clean up CPUID.(EAX=0DH,ECX=0):EAX.XTILECFG[17] by
>>> keeping it consistent with CPUID.(EAX=0DH,ECX=0):EAX.XTILEDATA[18] in
>>> the guest, but it's not clear to me what the best way to do that is.
>>> The crux of the issue is that xstate_get_guest_group_perm() returns
>>> partial support for AMX when userspace doesn't call
>>> prctl(ARCH_REQ_XCOMP_GUEST_PERM), I.e. the guest CPUID will report
>>> XTILECFG=1 and XTILEDATA=0 in that case.  In that situation, XTILECFG
>>> should be cleared for it to be consistent.  I can see two ways of
>>> potentially doing that:
>>>
>>> 1. We can ensure that perm->__state_perm never stores partial support.
>>>
>>> 2. We can sanitize the bits in xstate_get_guest_group_perm() before
>>> they are returned, to ensure KVM never sees partial support.
>>>
>>> I like the idea of #1, but if that has negative effects on the host or
>>> XFD I'm open to #2.  Though, XFD has its own field, so I thought that
>>> wouldn't be an issue.  Would it work to set __state_perm and/or
>>> default_features (what originally sets __state_perm) to a consistent
>>> view, so partial support is never returned from
>>> xstate_get_guest_group_perm()?
>>
>> FWIW, I was trying to clarify that ARCH_GET_XCOMP_GUEST_PERM is a
>> variant of ARCH_GET_XCOMP_PERM in the documentation [1]. So, I guess #1
>> will have some side-effect (at least confusion) for this semantics.
> 
> Right, before talking in this thread, I was not aware of the other
> arch_prctl(2) for AMX. But when I look into it, it looks reasonable to
> me from an engineering point of view since it seems reusing almost all
> of the code in the host.
> 
> ARCH_GET_XCOMP_GUEST_PERM is to ask for "guest permission", but it is
> not just about the "permission" (the fp_state size increase for AMX).
> It is also about the CPUID feature bits exposure. So for the host side
> of AMX usage, this is fine, since there is no partial CPUID exposure
> problem. But the guest side does have it.

I think this is still about permission. While lazy allocation is 
possible, this pre-allocation makes it simple. So this is how that 
allocation part is implemented, right?

> So, can we just grant two bits instead of 1 bit? For that, I find 1)
> seems more reasonable than 2). Of course, if there are many side
> effects, option #2 could be considered as well. But before that, it
> will be good to understand where the side effects are.
Sorry, I don't get it. But I guess maintainers will dictate it.

Also, I would feel the code can be more easily adjusted for the KVM/Qemu 
use if it is part of KVM API [3]. Then, the generic arch_prctl options 
can remain as it is.

>> There may be some ways to transform the permission bits to the
>> XCR0-capable bits. For instance, when considering this permission
>> support in host, the highest feature number was considered to denote the
>> rest feature bits [2].
> 
> Hmm, this is out of my concern since it is about the host-level
> enabling. I have no problem with the existing host side permission
> control for AMX.
> 
> However, for me, [2] does not seem a little hacky but I get the point.
> The concern is that how do we know in the future, the highest bit
> always indicates lower bits? Will Intel CPU features always follow
> this style?  Even so, there is no such guidance/guarantee that other
> CPU vendors (e.g., AMD) will do the same. Also what if there are more
> than 1 dynamic features for a feature? Please kindly correct me.

At least, that works for AMX I thought. I was not saying anything for 
the future feature. The code can be feature-specific here, no?

Thanks,
Chang

[3] 
https://lore.kernel.org/kvm/20220823231402.7839-1-chang.seok.bae@intel.com/

[PATCH v2 2/6] KVM: x86: Clear all supported AVX-512 xfeatures if they are not all set

[Aaron Lewis]

Be a good citizen and don't allow any of the supported AVX-512
xfeatures[1] to be set if they can't all be set.  That way userspace or
a guest doesn't fail if it attempts to set them in XCR0.

It's important to note that in order to set any of the AVX-512
xfeatures, the SSE[bit-1] and AVX[bit-2] must also be set.

[1] CPUID.(EAX=0DH,ECX=0):EAX.OPMASK[bit-5]
    CPUID.(EAX=0DH,ECX=0):EAX.ZMM_Hi256[bit-6]
    CPUID.(EAX=0DH,ECX=0):EAX.ZMM_Hi16_ZMM[bit-7]

Suggested-by: Jim Mattson <jmattson@google.com>
Signed-off-by: Aaron Lewis <aaronlewis@google.com>
---
 arch/x86/kvm/cpuid.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 2431c46d456b4..89ad8cd865173 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -862,6 +862,10 @@ static u64 sanitize_xcr0(u64 xcr0) {
 	if ((xcr0 & mask) != mask)
 		xcr0 &= ~mask;
 
+	mask = XFEATURE_MASK_SSE | XFEATURE_MASK_YMM | XFEATURE_MASK_AVX512;
+	if ((xcr0 & mask) != mask)
+		xcr0 &= ~XFEATURE_MASK_AVX512;
+
 	return xcr0;
 }
 
-- 
2.39.0.314.g84b9a713c41-goog

Re: [PATCH v2 2/6] KVM: x86: Clear all supported AVX-512 xfeatures if they are not all set

[Sean Christopherson]

On Fri, Dec 30, 2022, Aaron Lewis wrote:
> Be a good citizen and don't allow any of the supported AVX-512
> xfeatures[1] to be set if they can't all be set.  That way userspace or
> a guest doesn't fail if it attempts to set them in XCR0.

The form letter shortlog+changelo+code doesn't fit AVX-512.  There's only one
AVX512 flag, SSE and AVX and are pure prerequisites and exist independently of
AVX512.

> It's important to note that in order to set any of the AVX-512
> xfeatures, the SSE[bit-1] and AVX[bit-2] must also be set.
> 
> [1] CPUID.(EAX=0DH,ECX=0):EAX.OPMASK[bit-5]
>     CPUID.(EAX=0DH,ECX=0):EAX.ZMM_Hi256[bit-6]
>     CPUID.(EAX=0DH,ECX=0):EAX.ZMM_Hi16_ZMM[bit-7]
> 
> Suggested-by: Jim Mattson <jmattson@google.com>
> Signed-off-by: Aaron Lewis <aaronlewis@google.com>
> ---
>  arch/x86/kvm/cpuid.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 2431c46d456b4..89ad8cd865173 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -862,6 +862,10 @@ static u64 sanitize_xcr0(u64 xcr0) {
>  	if ((xcr0 & mask) != mask)
>  		xcr0 &= ~mask;
>  
> +	mask = XFEATURE_MASK_SSE | XFEATURE_MASK_YMM | XFEATURE_MASK_AVX512;

Checking AVX512 is unnecessary.  If it's not set, the AND-NOT is a nop.

> +	if ((xcr0 & mask) != mask)
> +		xcr0 &= ~XFEATURE_MASK_AVX512;

This can be:

	if (!(xcr0 & XFEATURE_MASK_SSE) || !(xcr0 & XFEATURE_MASK_YMM))
		xcr0 &= ~XFEATURE_MASK_AVX512

to better capture the dependency.

Re: [PATCH v2 2/6] KVM: x86: Clear all supported AVX-512 xfeatures if they are not all set

[Sean Christopherson]

On Wed, Jan 04, 2023, Sean Christopherson wrote:
> On Fri, Dec 30, 2022, Aaron Lewis wrote:
> > Be a good citizen and don't allow any of the supported AVX-512
> > xfeatures[1] to be set if they can't all be set.  That way userspace or
> > a guest doesn't fail if it attempts to set them in XCR0.
> 
> The form letter shortlog+changelo+code doesn't fit AVX-512.  There's only one
> AVX512 flag, SSE and AVX and are pure prerequisites and exist independently of
> AVX512.

Ugh, literacy issues.  AVX512 isn't a singular flag.  Argh.

Can you split this up into two separate patches?  One to require all AVX512 features,
and one to clear AVX512 if SSE or AVX isn't supported?

[PATCH v2 3/6] KVM: x86: Clear all supported AMX xfeatures if they are not all set

[Aaron Lewis]

Be a good citizen and don't allow any of the supported AMX xfeatures[1]
to be set if they can't all be set.  That way userspace or a guest
doesn't fail if it attempts to set them in XCR0.

[1] CPUID.(EAX=0DH,ECX=0):EAX.XTILE_CFG[bit-17]
    CPUID.(EAX=0DH,ECX=0):EAX.XTILE_DATA[bit-18]

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Aaron Lewis <aaronlewis@google.com>
---
 arch/x86/kvm/cpuid.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 89ad8cd865173..bdccc4ddb45b1 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -866,6 +866,9 @@ static u64 sanitize_xcr0(u64 xcr0) {
 	if ((xcr0 & mask) != mask)
 		xcr0 &= ~XFEATURE_MASK_AVX512;
 
+	if ((xcr0 & XFEATURE_MASK_XTILE) != XFEATURE_MASK_XTILE)
+		xcr0 &= ~XFEATURE_MASK_XTILE;
+
 	return xcr0;
 }
 
-- 
2.39.0.314.g84b9a713c41-goog

[PATCH v2 4/6] KVM: selftests: Hoist XGETBV and XSETBV to make them more accessible

[Aaron Lewis]

The instructions XGETBV and XSETBV are useful to other tests.  Move
them to processor.h to make them available to be used more broadly.

No functional change intended.

Signed-off-by: Aaron Lewis <aaronlewis@google.com>
Reviewed-by: Jim Mattson <jmattson@google.com>
---
 .../selftests/kvm/include/x86_64/processor.h  | 19 +++++++++++++++
 tools/testing/selftests/kvm/x86_64/amx_test.c | 24 +++----------------
 2 files changed, 22 insertions(+), 21 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
index 2a5f47d513884..5f06d6f27edf7 100644
--- a/tools/testing/selftests/kvm/include/x86_64/processor.h
+++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
@@ -493,6 +493,25 @@ static inline void set_cr4(uint64_t val)
 	__asm__ __volatile__("mov %0, %%cr4" : : "r" (val) : "memory");
 }
 
+static inline u64 xgetbv(u32 index)
+{
+	u32 eax, edx;
+
+	__asm__ __volatile__("xgetbv;"
+		     : "=a" (eax), "=d" (edx)
+		     : "c" (index));
+	return eax | ((u64)edx << 32);
+}
+
+static inline void xsetbv(u32 index, u64 value)
+{
+	u32 eax = value;
+	u32 edx = value >> 32;
+
+	__asm__ __volatile__("xsetbv" :: "a" (eax), "d" (edx), "c" (index));
+}
+
+
 static inline struct desc_ptr get_gdt(void)
 {
 	struct desc_ptr gdt;
diff --git a/tools/testing/selftests/kvm/x86_64/amx_test.c b/tools/testing/selftests/kvm/x86_64/amx_test.c
index bd72c6eb3b670..4b733ad218313 100644
--- a/tools/testing/selftests/kvm/x86_64/amx_test.c
+++ b/tools/testing/selftests/kvm/x86_64/amx_test.c
@@ -68,24 +68,6 @@ struct xtile_info {
 
 static struct xtile_info xtile;
 
-static inline u64 __xgetbv(u32 index)
-{
-	u32 eax, edx;
-
-	asm volatile("xgetbv;"
-		     : "=a" (eax), "=d" (edx)
-		     : "c" (index));
-	return eax + ((u64)edx << 32);
-}
-
-static inline void __xsetbv(u32 index, u64 value)
-{
-	u32 eax = value;
-	u32 edx = value >> 32;
-
-	asm volatile("xsetbv" :: "a" (eax), "d" (edx), "c" (index));
-}
-
 static inline void __ldtilecfg(void *cfg)
 {
 	asm volatile(".byte 0xc4,0xe2,0x78,0x49,0x00"
@@ -121,7 +103,7 @@ static inline void check_cpuid_xsave(void)
 
 static bool check_xsave_supports_xtile(void)
 {
-	return __xgetbv(0) & XFEATURE_MASK_XTILE;
+	return xgetbv(0) & XFEATURE_MASK_XTILE;
 }
 
 static void check_xtile_info(void)
@@ -177,9 +159,9 @@ static void init_regs(void)
 	cr4 |= X86_CR4_OSXSAVE;
 	set_cr4(cr4);
 
-	xcr0 = __xgetbv(0);
+	xcr0 = xgetbv(0);
 	xcr0 |= XFEATURE_MASK_XTILE;
-	__xsetbv(0x0, xcr0);
+	xsetbv(0x0, xcr0);
 }
 
 static void __attribute__((__flatten__)) guest_code(struct tile_config *amx_cfg,
-- 
2.39.0.314.g84b9a713c41-goog

[PATCH v2 5/6] KVM: selftests: Add XFEATURE masks to common code

[Aaron Lewis]

Add XFEATURE masks to processor.h to make them more broadly available
in KVM selftests.

They were taken from fpu/types.h, which included a difference in
spacing between the ones in amx_test from XTILECFG and XTILEDATA, to
XTILE_CFG and XTILE_DATA.  This has been reflected in amx_test.

Signed-off-by: Aaron Lewis <aaronlewis@google.com>
---
 .../selftests/kvm/include/x86_64/processor.h  | 16 ++++++++++++++
 tools/testing/selftests/kvm/x86_64/amx_test.c | 22 +++++++------------
 2 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
index 5f06d6f27edf7..c1132ac277227 100644
--- a/tools/testing/selftests/kvm/include/x86_64/processor.h
+++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
@@ -45,6 +45,22 @@
 #define X86_CR4_SMAP		(1ul << 21)
 #define X86_CR4_PKE		(1ul << 22)
 
+#define XFEATURE_MASK_SSE		BIT_ULL(1)
+#define XFEATURE_MASK_YMM		BIT_ULL(2)
+#define XFEATURE_MASK_BNDREGS		BIT_ULL(3)
+#define XFEATURE_MASK_BNDCSR		BIT_ULL(4)
+#define XFEATURE_MASK_OPMASK		BIT_ULL(5)
+#define XFEATURE_MASK_ZMM_Hi256		BIT_ULL(6)
+#define XFEATURE_MASK_Hi16_ZMM		BIT_ULL(7)
+#define XFEATURE_MASK_XTILE_CFG		BIT_ULL(17)
+#define XFEATURE_MASK_XTILE_DATA	BIT_ULL(18)
+
+#define XFEATURE_MASK_AVX512		(XFEATURE_MASK_OPMASK \
+					 | XFEATURE_MASK_ZMM_Hi256 \
+					 | XFEATURE_MASK_Hi16_ZMM)
+#define XFEATURE_MASK_XTILE		(XFEATURE_MASK_XTILE_DATA \
+					 | XFEATURE_MASK_XTILE_CFG)
+
 /* Note, these are ordered alphabetically to match kvm_cpuid_entry2.  Eww. */
 enum cpuid_output_regs {
 	KVM_CPUID_EAX,
diff --git a/tools/testing/selftests/kvm/x86_64/amx_test.c b/tools/testing/selftests/kvm/x86_64/amx_test.c
index 4b733ad218313..14a7656620d5f 100644
--- a/tools/testing/selftests/kvm/x86_64/amx_test.c
+++ b/tools/testing/selftests/kvm/x86_64/amx_test.c
@@ -33,12 +33,6 @@
 #define MAX_TILES			16
 #define RESERVED_BYTES			14
 
-#define XFEATURE_XTILECFG		17
-#define XFEATURE_XTILEDATA		18
-#define XFEATURE_MASK_XTILECFG		(1 << XFEATURE_XTILECFG)
-#define XFEATURE_MASK_XTILEDATA		(1 << XFEATURE_XTILEDATA)
-#define XFEATURE_MASK_XTILE		(XFEATURE_MASK_XTILECFG | XFEATURE_MASK_XTILEDATA)
-
 #define XSAVE_HDR_OFFSET		512
 
 struct xsave_data {
@@ -187,14 +181,14 @@ static void __attribute__((__flatten__)) guest_code(struct tile_config *amx_cfg,
 	__tilerelease();
 	GUEST_SYNC(5);
 	/* bit 18 not in the XCOMP_BV after xsavec() */
-	set_xstatebv(xsave_data, XFEATURE_MASK_XTILEDATA);
-	__xsavec(xsave_data, XFEATURE_MASK_XTILEDATA);
-	GUEST_ASSERT((get_xstatebv(xsave_data) & XFEATURE_MASK_XTILEDATA) == 0);
+	set_xstatebv(xsave_data, XFEATURE_MASK_XTILE_DATA);
+	__xsavec(xsave_data, XFEATURE_MASK_XTILE_DATA);
+	GUEST_ASSERT((get_xstatebv(xsave_data) & XFEATURE_MASK_XTILE_DATA) == 0);
 
 	/* xfd=0x40000, disable amx tiledata */
-	wrmsr(MSR_IA32_XFD, XFEATURE_MASK_XTILEDATA);
+	wrmsr(MSR_IA32_XFD, XFEATURE_MASK_XTILE_DATA);
 	GUEST_SYNC(6);
-	GUEST_ASSERT(rdmsr(MSR_IA32_XFD) == XFEATURE_MASK_XTILEDATA);
+	GUEST_ASSERT(rdmsr(MSR_IA32_XFD) == XFEATURE_MASK_XTILE_DATA);
 	set_tilecfg(amx_cfg);
 	__ldtilecfg(amx_cfg);
 	/* Trigger #NM exception */
@@ -206,11 +200,11 @@ static void __attribute__((__flatten__)) guest_code(struct tile_config *amx_cfg,
 
 void guest_nm_handler(struct ex_regs *regs)
 {
-	/* Check if #NM is triggered by XFEATURE_MASK_XTILEDATA */
+	/* Check if #NM is triggered by XFEATURE_MASK_XTILE_DATA */
 	GUEST_SYNC(7);
-	GUEST_ASSERT(rdmsr(MSR_IA32_XFD_ERR) == XFEATURE_MASK_XTILEDATA);
+	GUEST_ASSERT(rdmsr(MSR_IA32_XFD_ERR) == XFEATURE_MASK_XTILE_DATA);
 	GUEST_SYNC(8);
-	GUEST_ASSERT(rdmsr(MSR_IA32_XFD_ERR) == XFEATURE_MASK_XTILEDATA);
+	GUEST_ASSERT(rdmsr(MSR_IA32_XFD_ERR) == XFEATURE_MASK_XTILE_DATA);
 	/* Clear xfd_err */
 	wrmsr(MSR_IA32_XFD_ERR, 0);
 	/* xfd=0, enable amx */
-- 
2.39.0.314.g84b9a713c41-goog

Re: [PATCH v2 5/6] KVM: selftests: Add XFEATURE masks to common code

[Sean Christopherson]

On Fri, Dec 30, 2022, Aaron Lewis wrote:
> Add XFEATURE masks to processor.h to make them more broadly available
> in KVM selftests.
> 
> They were taken from fpu/types.h, which included a difference in

Nit, state the rename as a command, e.g.

  Use the names from the kernel's fpu/types.h for consistency, i.e. rename
  XTILECFG and XTILEDATA to XTILE_CFG and XTILE_DATA respectively.

> spacing between the ones in amx_test from XTILECFG and XTILEDATA, to
> XTILE_CFG and XTILE_DATA.  This has been reflected in amx_test.
> 
> Signed-off-by: Aaron Lewis <aaronlewis@google.com>
> ---
>  .../selftests/kvm/include/x86_64/processor.h  | 16 ++++++++++++++
>  tools/testing/selftests/kvm/x86_64/amx_test.c | 22 +++++++------------
>  2 files changed, 24 insertions(+), 14 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
> index 5f06d6f27edf7..c1132ac277227 100644
> --- a/tools/testing/selftests/kvm/include/x86_64/processor.h
> +++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
> @@ -45,6 +45,22 @@
>  #define X86_CR4_SMAP		(1ul << 21)
>  #define X86_CR4_PKE		(1ul << 22)
>  

Add 

 #define XFEATURE_MASK_FP		BIT_ULL(0)

so that you don't have to open code the literal in the next patch.

> +#define XFEATURE_MASK_SSE		BIT_ULL(1)
> +#define XFEATURE_MASK_YMM		BIT_ULL(2)
> +#define XFEATURE_MASK_BNDREGS		BIT_ULL(3)
> +#define XFEATURE_MASK_BNDCSR		BIT_ULL(4)
> +#define XFEATURE_MASK_OPMASK		BIT_ULL(5)
> +#define XFEATURE_MASK_ZMM_Hi256		BIT_ULL(6)
> +#define XFEATURE_MASK_Hi16_ZMM		BIT_ULL(7)
> +#define XFEATURE_MASK_XTILE_CFG		BIT_ULL(17)
> +#define XFEATURE_MASK_XTILE_DATA	BIT_ULL(18)
> +
> +#define XFEATURE_MASK_AVX512		(XFEATURE_MASK_OPMASK \
> +					 | XFEATURE_MASK_ZMM_Hi256 \
> +					 | XFEATURE_MASK_Hi16_ZMM)

'|' on the previous line please, i.e.

#define XFEATURE_MASK_AVX512		(XFEATURE_MASK_OPMASK | \
					 XFEATURE_MASK_ZMM_Hi256 | \
					 XFEATURE_MASK_Hi16_ZMM)

> +#define XFEATURE_MASK_XTILE		(XFEATURE_MASK_XTILE_DATA \
> +					 | XFEATURE_MASK_XTILE_CFG)

Same comment here.

[PATCH v2 6/6] KVM: selftests: Add XCR0 Test

[Aaron Lewis]

Test that the supported xfeatures, i.e. EDX:EAX of CPUID.(EAX=0DH,ECX=0),
don't set userspace up for failure.

Though it isn't architectural, test that the supported xfeatures
aren't set in a half baked state that will cause a #GP if used to
execute XSETBV.

Check that the rules for XCR0 described in the SDM vol 1, section
13.3 ENABLING THE XSAVE FEATURE SET AND XSAVE-ENABLED FEATURES, are
followed for the supported xfeatures too.

Signed-off-by: Aaron Lewis <aaronlewis@google.com>
---
 tools/testing/selftests/kvm/Makefile          |   1 +
 .../selftests/kvm/x86_64/xcr0_cpuid_test.c    | 121 ++++++++++++++++++
 2 files changed, 122 insertions(+)
 create mode 100644 tools/testing/selftests/kvm/x86_64/xcr0_cpuid_test.c

diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index 1750f91dd9362..e2e56c82b8a90 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -104,6 +104,7 @@ TEST_GEN_PROGS_x86_64 += x86_64/vmx_tsc_adjust_test
 TEST_GEN_PROGS_x86_64 += x86_64/vmx_nested_tsc_scaling_test
 TEST_GEN_PROGS_x86_64 += x86_64/xapic_ipi_test
 TEST_GEN_PROGS_x86_64 += x86_64/xapic_state_test
+TEST_GEN_PROGS_x86_64 += x86_64/xcr0_cpuid_test
 TEST_GEN_PROGS_x86_64 += x86_64/xss_msr_test
 TEST_GEN_PROGS_x86_64 += x86_64/debug_regs
 TEST_GEN_PROGS_x86_64 += x86_64/tsc_msrs_test
diff --git a/tools/testing/selftests/kvm/x86_64/xcr0_cpuid_test.c b/tools/testing/selftests/kvm/x86_64/xcr0_cpuid_test.c
new file mode 100644
index 0000000000000..6bef362872154
--- /dev/null
+++ b/tools/testing/selftests/kvm/x86_64/xcr0_cpuid_test.c
@@ -0,0 +1,121 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * XCR0 cpuid test
+ *
+ * Copyright (C) 2022, Google LLC.
+ */
+
+#include <fcntl.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/ioctl.h>
+
+#include "test_util.h"
+
+#include "kvm_util.h"
+#include "processor.h"
+
+static uint64_t get_supported_user_xfeatures(void)
+{
+	uint32_t a, b, c, d;
+
+	cpuid(0xd, &a, &b, &c, &d);
+
+	return a | ((uint64_t)d << 32);
+}
+
+static void guest_code(void)
+{
+	uint64_t xcr0_rest;
+	uint64_t supported_xcr0;
+	uint64_t xfeature_mask;
+	uint64_t supported_state;
+
+	set_cr4(get_cr4() | X86_CR4_OSXSAVE);
+
+	xcr0_rest = xgetbv(0);
+	supported_xcr0 = get_supported_user_xfeatures();
+
+	GUEST_ASSERT(xcr0_rest == 1ul);
+
+	/* Check AVX */
+	xfeature_mask = XFEATURE_MASK_SSE | XFEATURE_MASK_YMM;
+	supported_state = supported_xcr0 & xfeature_mask;
+	GUEST_ASSERT(supported_state != XFEATURE_MASK_YMM);
+
+	/* Check MPX */
+	xfeature_mask = XFEATURE_MASK_BNDREGS | XFEATURE_MASK_BNDCSR;
+	supported_state = supported_xcr0 & xfeature_mask;
+	GUEST_ASSERT((supported_state == xfeature_mask) ||
+		     (supported_state == 0ul));
+
+	/* Check AVX-512 */
+	xfeature_mask = XFEATURE_MASK_SSE | XFEATURE_MASK_YMM |
+			XFEATURE_MASK_AVX512;
+	supported_state = supported_xcr0 & xfeature_mask;
+	GUEST_ASSERT((supported_state == xfeature_mask) ||
+		     ((supported_state & XFEATURE_MASK_AVX512) == 0ul));
+
+	/* Check AMX */
+	xfeature_mask = XFEATURE_MASK_XTILE;
+	supported_state = supported_xcr0 & xfeature_mask;
+	GUEST_ASSERT((supported_state == xfeature_mask) ||
+		     (supported_state == 0ul));
+
+	GUEST_SYNC(0);
+
+	xsetbv(0, supported_xcr0);
+
+	GUEST_DONE();
+}
+
+static void guest_gp_handler(struct ex_regs *regs)
+{
+	GUEST_ASSERT(!"Failed to set the supported xfeature bits in XCR0.");
+}
+
+int main(int argc, char *argv[])
+{
+	struct kvm_vcpu *vcpu;
+	struct kvm_run *run;
+	struct kvm_vm *vm;
+	struct ucall uc;
+
+	TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_XSAVE));
+
+	/* Tell stdout not to buffer its content */
+	setbuf(stdout, NULL);
+
+	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
+	run = vcpu->run;
+
+	vm_init_descriptor_tables(vm);
+	vcpu_init_descriptor_tables(vcpu);
+
+	while (1) {
+		vcpu_run(vcpu);
+
+		TEST_ASSERT(run->exit_reason == KVM_EXIT_IO,
+			    "Unexpected exit reason: %u (%s),\n",
+			    run->exit_reason,
+			    exit_reason_str(run->exit_reason));
+
+		switch (get_ucall(vcpu, &uc)) {
+		case UCALL_SYNC:
+			vm_install_exception_handler(vm, GP_VECTOR,
+						     guest_gp_handler);
+			break;
+		case UCALL_ABORT:
+			REPORT_GUEST_ASSERT(uc);
+			break;
+		case UCALL_DONE:
+			goto done;
+		default:
+			TEST_FAIL("Unknown ucall %lu", uc.cmd);
+		}
+	}
+
+done:
+	kvm_vm_free(vm);
+	return 0;
+}
-- 
2.39.0.314.g84b9a713c41-goog

Re: [PATCH v2 6/6] KVM: selftests: Add XCR0 Test

[Sean Christopherson]

On Fri, Dec 30, 2022, Aaron Lewis wrote:
> +static uint64_t get_supported_user_xfeatures(void)

I would rather put this in processor.h too, with a "this_cpu" prefix.  Maybe
this_cpu_supported_xcr0() or this_cpu_supported_user_xfeatures()?

> +{
> +	uint32_t a, b, c, d;
> +
> +	cpuid(0xd, &a, &b, &c, &d);
> +
> +	return a | ((uint64_t)d << 32);
> +}
> +
> +static void guest_code(void)
> +{
> +	uint64_t xcr0_rest;

s/rest/reset ?

> +	uint64_t supported_xcr0;
> +	uint64_t xfeature_mask;
> +	uint64_t supported_state;
> +
> +	set_cr4(get_cr4() | X86_CR4_OSXSAVE);
> +
> +	xcr0_rest = xgetbv(0);
> +	supported_xcr0 = get_supported_user_xfeatures();
> +
> +	GUEST_ASSERT(xcr0_rest == 1ul);

XFEATURE_MASK_FP instead of 1ul.

> +
> +	/* Check AVX */
> +	xfeature_mask = XFEATURE_MASK_SSE | XFEATURE_MASK_YMM;
> +	supported_state = supported_xcr0 & xfeature_mask;
> +	GUEST_ASSERT(supported_state != XFEATURE_MASK_YMM);

Oof, this took me far too long to read correctly.  What about?

	/* AVX can be supported if and only if SSE is supported. */
	GUEST_ASSERT((supported_xcr0 & XFEATURE_MASK_SSE) ||
		     !(supported_xcr0 & XFEATURE_MASK_YMM));

Hmm or maybe add helpers?  Printing the info on failure would also make it easier
to debug.  E.g.

static void check_all_or_none_xfeature(uint64_t supported_xcr0, uint64_t mask)
{
	supported_xcr0 &= mask;

	GUEST_ASSERT_2(!supported_xcr0 || supported_xcr0 == mask,
		       supported_xcr0, mask);
}

static void check_xfeature_dependencies(uint64_t supported_xcr0, uint64_t mask,
					uint64_t dependencies)
{
	supported_xcr0 &= (mask | dependencies);

	GUEST_ASSERT_3(!(supported_xcr0 & mask) ||
		       supported_xcr0 == (mask | dependencies),
		       supported_xcr0, mask, dependencies);
}

would yield

	check_xfeature_dependencies(supported_xcr0, XFEATURE_MASK_YMM,
				    XFEATURE_MASK_SSE);

and then for AVX512:

	check_xfeature_dependencies(supported_xcr0, XFEATURE_MASK_AVX512,
				    XFEATURE_MASK_SSE | XFEATURE_MASK_YMM);
	check_all_or_none_xfeature(supported_xcr0, XFEATURE_MASK_AVX512);

That would more or less eliminate the need for comments, and IMO makes it more
obvious what is being checked.

> +	xsetbv(0, supported_xcr0);
> +
> +	GUEST_DONE();
> +}
> +
> +static void guest_gp_handler(struct ex_regs *regs)
> +{
> +	GUEST_ASSERT(!"Failed to set the supported xfeature bits in XCR0.");

I'd rather add an xsetbv_safe() variant than install a #GP handler.  That would
also make it super easy to add negative testing.  E.g. (completely untested)

static inline uint8_t xsetbv_safe(uint32_t index, uint64_t value)
{
	u32 eax = value;
	u32 edx = value >> 32;

	return kvm_asm_safe("xsetbv", "a" (eax), "d" (edx), "c" (index));
}

and
	vector = xsetbv_safe(0, supported_xcr0);
	GUEST_ASSERT_2(!vector, supported_xcr0, vector);

and rudimentary negative testing

	for (i = 0; i < 64; i++) {
		if (supported_xcr0 & BIT_ULL(i))
			continue;

		vector = xsetbv_safe(0, supported_xcr0 | BIT_ULL(i));
		GUEST_ASSERT_2(vector == GP_VECTOR, supported_xcr0, vector);
	}

Re: [PATCH v2 6/6] KVM: selftests: Add XCR0 Test

[Aaron Lewis]

On Wed, Jan 4, 2023 at 5:13 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Fri, Dec 30, 2022, Aaron Lewis wrote:
> > +static uint64_t get_supported_user_xfeatures(void)
>
> I would rather put this in processor.h too, with a "this_cpu" prefix.  Maybe
> this_cpu_supported_xcr0() or this_cpu_supported_user_xfeatures()?

this_cpu_supported_xcr0 works for me.

>
> > +{
> > +     uint32_t a, b, c, d;
> > +
> > +     cpuid(0xd, &a, &b, &c, &d);
> > +
> > +     return a | ((uint64_t)d << 32);
> > +}
> > +
> > +static void guest_code(void)
> > +{
> > +     uint64_t xcr0_rest;
>
> s/rest/reset ?
>
> > +     uint64_t supported_xcr0;
> > +     uint64_t xfeature_mask;
> > +     uint64_t supported_state;
> > +
> > +     set_cr4(get_cr4() | X86_CR4_OSXSAVE);
> > +
> > +     xcr0_rest = xgetbv(0);
> > +     supported_xcr0 = get_supported_user_xfeatures();
> > +
> > +     GUEST_ASSERT(xcr0_rest == 1ul);
>
> XFEATURE_MASK_FP instead of 1ul.
>
> > +
> > +     /* Check AVX */
> > +     xfeature_mask = XFEATURE_MASK_SSE | XFEATURE_MASK_YMM;
> > +     supported_state = supported_xcr0 & xfeature_mask;
> > +     GUEST_ASSERT(supported_state != XFEATURE_MASK_YMM);
>
> Oof, this took me far too long to read correctly.  What about?
>
>         /* AVX can be supported if and only if SSE is supported. */
>         GUEST_ASSERT((supported_xcr0 & XFEATURE_MASK_SSE) ||
>                      !(supported_xcr0 & XFEATURE_MASK_YMM));
>
> Hmm or maybe add helpers?  Printing the info on failure would also make it easier
> to debug.  E.g.
>
> static void check_all_or_none_xfeature(uint64_t supported_xcr0, uint64_t mask)
> {
>         supported_xcr0 &= mask;
>
>         GUEST_ASSERT_2(!supported_xcr0 || supported_xcr0 == mask,
>                        supported_xcr0, mask);
> }
>
> static void check_xfeature_dependencies(uint64_t supported_xcr0, uint64_t mask,
>                                         uint64_t dependencies)
> {
>         supported_xcr0 &= (mask | dependencies);
>
>         GUEST_ASSERT_3(!(supported_xcr0 & mask) ||
>                        supported_xcr0 == (mask | dependencies),
>                        supported_xcr0, mask, dependencies);
> }
>
> would yield
>
>         check_xfeature_dependencies(supported_xcr0, XFEATURE_MASK_YMM,
>                                     XFEATURE_MASK_SSE);
>
> and then for AVX512:
>
>         check_xfeature_dependencies(supported_xcr0, XFEATURE_MASK_AVX512,
>                                     XFEATURE_MASK_SSE | XFEATURE_MASK_YMM);
>         check_all_or_none_xfeature(supported_xcr0, XFEATURE_MASK_AVX512);
>
> That would more or less eliminate the need for comments, and IMO makes it more
> obvious what is being checked.

The reason I chose not to use helpers here was it hides the line
number the assert occured on.  With helpers you have multiple places
the problem came from and one place it's asserting.  The way I wrote
it the line number in the assert tells you exactly where the problem
occured.

I get you can deduce the line number with the values passed back in
the assert, but the assert information printed out on the host has to
be purposefully vague because you get one fmt for the entire test.  If
I was able to do a printf style asserts from the guest, that would
allow me to provide more, clear context to tie it back.  Having the
line number where the assert fired I felt was useful to keep.

I guess one way to get the best of both worlds would be to have the
helpers return a bool rather than assert in the helper.  I could also
include the additional info you suggested in the asserts.

That said, if we do end up going with helpers I don't think we have to
call them both like in the AVX512 example.  They both check the same
thing, except check_xfeature_dependencies() additionally allows for
dependencies to be used.  E.g.

if you call:
check_xfeature_dependencies(supported_xcr0, XFEATURE_MASK_AVX512, 0)

it's the same as calling:
check_all_or_none_xfeature(supported_xcr0, XFEATURE_MASK_AVX512)

Maybe we should call them: check_xfeature() and
check_xfeature_dependencies(), or with_dependencies... something to
show they are related to each other.

>
> > +     xsetbv(0, supported_xcr0);
> > +
> > +     GUEST_DONE();
> > +}
> > +
> > +static void guest_gp_handler(struct ex_regs *regs)
> > +{
> > +     GUEST_ASSERT(!"Failed to set the supported xfeature bits in XCR0.");
>
> I'd rather add an xsetbv_safe() variant than install a #GP handler.  That would
> also make it super easy to add negative testing.  E.g. (completely untested)
>
> static inline uint8_t xsetbv_safe(uint32_t index, uint64_t value)
> {
>         u32 eax = value;
>         u32 edx = value >> 32;
>
>         return kvm_asm_safe("xsetbv", "a" (eax), "d" (edx), "c" (index));
> }
>
> and
>         vector = xsetbv_safe(0, supported_xcr0);
>         GUEST_ASSERT_2(!vector, supported_xcr0, vector);
>
> and rudimentary negative testing
>
>         for (i = 0; i < 64; i++) {
>                 if (supported_xcr0 & BIT_ULL(i))
>                         continue;
>
>                 vector = xsetbv_safe(0, supported_xcr0 | BIT_ULL(i));
>                 GUEST_ASSERT_2(vector == GP_VECTOR, supported_xcr0, vector);
>         }

I like the idea of this additional test.  I'll add it.

Re: [PATCH v2 6/6] KVM: selftests: Add XCR0 Test

[Sean Christopherson]

On Thu, Jan 05, 2023, Aaron Lewis wrote:
> On Wed, Jan 4, 2023 at 5:13 PM Sean Christopherson <seanjc@google.com> wrote:
> > Hmm or maybe add helpers?  Printing the info on failure would also make it easier
> > to debug.  E.g.
> >
> > static void check_all_or_none_xfeature(uint64_t supported_xcr0, uint64_t mask)
> > {
> >         supported_xcr0 &= mask;
> >
> >         GUEST_ASSERT_2(!supported_xcr0 || supported_xcr0 == mask,
> >                        supported_xcr0, mask);
> > }
> >
> > static void check_xfeature_dependencies(uint64_t supported_xcr0, uint64_t mask,
> >                                         uint64_t dependencies)
> > {
> >         supported_xcr0 &= (mask | dependencies);
> >
> >         GUEST_ASSERT_3(!(supported_xcr0 & mask) ||
> >                        supported_xcr0 == (mask | dependencies),
> >                        supported_xcr0, mask, dependencies);
> > }
> >
> > would yield
> >
> >         check_xfeature_dependencies(supported_xcr0, XFEATURE_MASK_YMM,
> >                                     XFEATURE_MASK_SSE);
> >
> > and then for AVX512:
> >
> >         check_xfeature_dependencies(supported_xcr0, XFEATURE_MASK_AVX512,
> >                                     XFEATURE_MASK_SSE | XFEATURE_MASK_YMM);
> >         check_all_or_none_xfeature(supported_xcr0, XFEATURE_MASK_AVX512);
> >
> > That would more or less eliminate the need for comments, and IMO makes it more
> > obvious what is being checked.
> 
> The reason I chose not to use helpers here was it hides the line
> number the assert occured on.  With helpers you have multiple places
> the problem came from and one place it's asserting.  The way I wrote
> it the line number in the assert tells you exactly where the problem
> occured.
> 
> I get you can deduce the line number with the values passed back in
> the assert,

But the line number ultimately doesn't matter, no?  In the original code, the
line number lets you know what feature bits failed, but in the proposed helpers
above, that's explicitly provided.

> but the assert information printed out on the host has to
> be purposefully vague because you get one fmt for the entire test

Right, but the line number of the helper disambiguates the data.  E.g. knowing
that the assert in check_xfeature_dependencies() fired lets the reader know what
the args mean.

> If I was able to do a printf style asserts from the guest, that would allow
> me to provide more, clear context to tie it back.

Yeah, we need to figure out a solution for that sooner than later.

> Having the line number where the assert fired I felt was useful to keep.
> 
> I guess one way to get the best of both worlds would be to have the
> helpers return a bool rather than assert in the helper.  I could also
> include the additional info you suggested in the asserts.

That seems like it will end up with hard to read code, and a lot of copy+paste.
E.g.

	GUEST_ASSERT_3(is_valid_xfeature(supported_xcr0, XFEATURE_MASK_AVX512,
					 XFEATURE_MASK_SSE | XFEATURE_MASK_YMM),
		       supported_xcr0, XFEATURE_MASK_AVX512,
		       XFEATURE_MASK_SSE | XFEATURE_MASK_YMM);

isn't the friendliest.

What about using macros?  It's a little gory, but I don't think it'll be a
maintenance issue, and the code is quite small.  And on the plus side, it's more
obvious that the "caller" is making an assertion.

#define ASSERT_ALL_OR_NONE_XFEATURE(supported_xcr0, mask)	\
do {								\
	uint64_t __supported = supported_xcr0 & (mask);		\
								\
	GUEST_ASSERT_2(!supported || supported == (mask),	\
		       supported, (mask));			\
while (0)

> That said, if we do end up going with helpers I don't think we have to
> call them both like in the AVX512 example.  They both check the same
> thing, except check_xfeature_dependencies() additionally allows for
> dependencies to be used.

My thought was to intentionally separate the checks by their semantics, even though
it means more checks.  Asserting that the dependencies are in place is backed by
architectural rules, whereas asserting the "all or nothing" stuff is KVM's own
software-defined rules.