[PATCH 0/3] KVM: x86: export supported_xcr0 via UAPI

[PATCH 0/3] KVM: x86: export supported_xcr0 via UAPI

[Paolo Bonzini]

While working on the QEMU support for AMX, I noticed that there is no
equivalent of ARCH_GET_XCOMP_SUPP in the KVM API.  This is important
because KVM_GET_SUPPORTED_CPUID is meant to be passed (by simple-minded
VMMs) to KVM_SET_CPUID2, and therefore it cannot include any dynamic
xsave states that have not been enabled.  Probing the availability of
dynamic xsave states therefore, requires a new ioctl or arch_prctl.

In order to avoid moving supported_xcr0 to the kernel from the KVM
module just for this use, and to ensure that the value can only be
probed if/after the KVM module has been loaded, this series goes
for the former option.

KVM_CHECK_EXTENSION cannot be used because it only has 32 bits of
output; in order to limit the growth of capabilities and ioctls, the
series adds a /dev/kvm variant of KVM_{GET,HAS}_DEVICE_ATTR that
can be used in the future and by other architectures.  It then
implements it in x86 with just one group (0) and attribute
(KVM_X86_XCOMP_GUEST_SUPP).

The corresponding changes to the tests, in patches 1 and 3, are
designed so that the code will be covered (to the possible extent)
even when running the tests on systems that do not support AMX.
However, the patches have not been tested with AMX.

Thanks,

Paolo


Paolo Bonzini (3):
  selftests: kvm: move vm_xsave_req_perm call to amx_test
  KVM: x86: add system attribute to retrieve full set of supported xsave
    states
  selftests: kvm: check dynamic bits against KVM_X86_XCOMP_GUEST_SUPP

 Documentation/virt/kvm/api.rst                |  4 +-
 arch/x86/include/uapi/asm/kvm.h               |  3 ++
 arch/x86/kvm/x86.c                            | 45 +++++++++++++++++++
 include/uapi/linux/kvm.h                      |  1 +
 tools/arch/x86/include/uapi/asm/kvm.h         |  3 ++
 tools/include/uapi/linux/kvm.h                |  1 +
 .../selftests/kvm/include/kvm_util_base.h     |  1 -
 .../selftests/kvm/include/x86_64/processor.h  |  1 +
 tools/testing/selftests/kvm/lib/kvm_util.c    |  7 ---
 .../selftests/kvm/lib/x86_64/processor.c      | 27 ++++++++---
 tools/testing/selftests/kvm/x86_64/amx_test.c |  2 +
 11 files changed, 80 insertions(+), 15 deletions(-)

-- 
2.31.1

[PATCH 1/3] selftests: kvm: move vm_xsave_req_perm call to amx_test

[Paolo Bonzini]

There is no need for tests other than amx_test to enable dynamic xsave
states.  Remove the call to vm_xsave_req_perm from generic code,
and move it inside the test.  While at it, allow customizing the bit
that is requested, so that future tests can use it differently.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 tools/testing/selftests/kvm/include/kvm_util_base.h  |  1 -
 .../testing/selftests/kvm/include/x86_64/processor.h |  1 +
 tools/testing/selftests/kvm/lib/kvm_util.c           |  7 -------
 tools/testing/selftests/kvm/lib/x86_64/processor.c   | 12 ++++++------
 tools/testing/selftests/kvm/x86_64/amx_test.c        |  2 ++
 5 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h
index 66775de26952..4ed6aa049a91 100644
--- a/tools/testing/selftests/kvm/include/kvm_util_base.h
+++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
@@ -345,7 +345,6 @@ struct kvm_vm *vm_create_with_vcpus(enum vm_guest_mode mode, uint32_t nr_vcpus,
  *   guest_code - The vCPU's entry point
  */
 void vm_vcpu_add_default(struct kvm_vm *vm, uint32_t vcpuid, void *guest_code);
-void vm_xsave_req_perm(void);
 
 bool vm_is_unrestricted_guest(struct kvm_vm *vm);
 
diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
index 423d8a61bd2e..8a470da7b71a 100644
--- a/tools/testing/selftests/kvm/include/x86_64/processor.h
+++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
@@ -458,6 +458,7 @@ uint64_t kvm_hypercall(uint64_t nr, uint64_t a0, uint64_t a1, uint64_t a2,
 struct kvm_cpuid2 *kvm_get_supported_hv_cpuid(void);
 void vcpu_set_hv_cpuid(struct kvm_vm *vm, uint32_t vcpuid);
 struct kvm_cpuid2 *vcpu_get_supported_hv_cpuid(struct kvm_vm *vm, uint32_t vcpuid);
+void vm_xsave_req_perm(int bit);
 
 enum x86_page_size {
 	X86_PAGE_SIZE_4K = 0,
diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index 8c53f96ab7fe..d8cf851ab119 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -393,13 +393,6 @@ struct kvm_vm *vm_create_with_vcpus(enum vm_guest_mode mode, uint32_t nr_vcpus,
 	struct kvm_vm *vm;
 	int i;
 
-#ifdef __x86_64__
-	/*
-	 * Permission needs to be requested before KVM_SET_CPUID2.
-	 */
-	vm_xsave_req_perm();
-#endif
-
 	/* Force slot0 memory size not small than DEFAULT_GUEST_PHY_PAGES */
 	if (slot0_mem_pages < DEFAULT_GUEST_PHY_PAGES)
 		slot0_mem_pages = DEFAULT_GUEST_PHY_PAGES;
diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
index 5f9d7e91dc69..c1d1c195a838 100644
--- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
+++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
@@ -665,16 +665,16 @@ static bool is_xfd_supported(void)
 	return !!(eax & CPUID_XFD_BIT);
 }
 
-void vm_xsave_req_perm(void)
+void vm_xsave_req_perm(int bit)
 {
-	unsigned long bitmask;
+	u64 bitmask;
 	long rc;
 
 	if (!is_xfd_supported())
-		return;
+		exit(KSFT_SKIP);
+
+	rc = syscall(SYS_arch_prctl, ARCH_REQ_XCOMP_GUEST_PERM, bit);
 
-	rc = syscall(SYS_arch_prctl, ARCH_REQ_XCOMP_GUEST_PERM,
-		     XSTATE_XTILE_DATA_BIT);
 	/*
 	 * The older kernel version(<5.15) can't support
 	 * ARCH_REQ_XCOMP_GUEST_PERM and directly return.
@@ -684,7 +684,7 @@ void vm_xsave_req_perm(void)
 
 	rc = syscall(SYS_arch_prctl, ARCH_GET_XCOMP_GUEST_PERM, &bitmask);
 	TEST_ASSERT(rc == 0, "prctl(ARCH_GET_XCOMP_GUEST_PERM) error: %ld", rc);
-	TEST_ASSERT(bitmask & XFEATURE_XTILE_MASK,
+	TEST_ASSERT(bitmask & (1ULL << bit),
 		    "prctl(ARCH_REQ_XCOMP_GUEST_PERM) failure bitmask=0x%lx",
 		    bitmask);
 }
diff --git a/tools/testing/selftests/kvm/x86_64/amx_test.c b/tools/testing/selftests/kvm/x86_64/amx_test.c
index 523c1e99ed64..52a3ef6629e8 100644
--- a/tools/testing/selftests/kvm/x86_64/amx_test.c
+++ b/tools/testing/selftests/kvm/x86_64/amx_test.c
@@ -329,6 +329,8 @@ int main(int argc, char *argv[])
 	u32 amx_offset;
 	int stage, ret;
 
+	vm_xsave_req_perm(XSTATE_XTILE_DATA_BIT);
+
 	/* Create VM */
 	vm = vm_create_default(VCPU_ID, 0, guest_code);
 
-- 
2.31.1

[PATCH 2/3] KVM: x86: add system attribute to retrieve full set of supported xsave states

[Paolo Bonzini]

Because KVM_GET_SUPPORTED_CPUID is meant to be passed (by simple-minded
VMMs) to KVM_SET_CPUID2, it cannot include any dynamic xsave states that
have not been enabled.  Probing those, for example so that they can be
passed to ARCH_REQ_XCOMP_GUEST_PERM, requires a new ioctl or arch_prctl.
The latter is in fact worse, even though that is what the rest of the
API uses, because it would require supported_xcr0 to be moved from the
KVM module to the kernel just for this use.  In addition, the value
would be nonsensical (or an error would have to be returned) until
the KVM module is loaded in.

KVM_CHECK_EXTENSION cannot be used because it only has 32 bits of
output; in order to limit the growth of capabilities and ioctls, the
series adds a /dev/kvm variant of KVM_{GET,HAS}_DEVICE_ATTR that
can be used in the future and by other architectures.  It then
implements it in x86 with just one group (0) and attribute
(KVM_X86_XCOMP_GUEST_SUPP).

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 Documentation/virt/kvm/api.rst  |  4 ++-
 arch/x86/include/uapi/asm/kvm.h |  3 +++
 arch/x86/kvm/x86.c              | 45 +++++++++++++++++++++++++++++++++
 include/uapi/linux/kvm.h        |  1 +
 4 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
index bb8cfddbb22d..a4267104db50 100644
--- a/Documentation/virt/kvm/api.rst
+++ b/Documentation/virt/kvm/api.rst
@@ -3268,6 +3268,7 @@ number.
 
 :Capability: KVM_CAP_DEVICE_CTRL, KVM_CAP_VM_ATTRIBUTES for vm device,
              KVM_CAP_VCPU_ATTRIBUTES for vcpu device
+             KVM_CAP_SYS_ATTRIBUTES for system (/dev/kvm) device (no set)
 :Type: device ioctl, vm ioctl, vcpu ioctl
 :Parameters: struct kvm_device_attr
 :Returns: 0 on success, -1 on error
@@ -3302,7 +3303,8 @@ transferred is defined by the particular attribute.
 ------------------------
 
 :Capability: KVM_CAP_DEVICE_CTRL, KVM_CAP_VM_ATTRIBUTES for vm device,
-	     KVM_CAP_VCPU_ATTRIBUTES for vcpu device
+             KVM_CAP_VCPU_ATTRIBUTES for vcpu device
+             KVM_CAP_SYS_ATTRIBUTES for system (/dev/kvm) device
 :Type: device ioctl, vm ioctl, vcpu ioctl
 :Parameters: struct kvm_device_attr
 :Returns: 0 on success, -1 on error
diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h
index 2da3316bb559..bf6e96011dfe 100644
--- a/arch/x86/include/uapi/asm/kvm.h
+++ b/arch/x86/include/uapi/asm/kvm.h
@@ -452,6 +452,9 @@ struct kvm_sync_regs {
 
 #define KVM_STATE_VMX_PREEMPTION_TIMER_DEADLINE	0x00000001
 
+/* attributes for system fd (group 0) */
+#define KVM_X86_XCOMP_GUEST_SUPP	0
+
 struct kvm_vmx_nested_state_data {
 	__u8 vmcs12[KVM_STATE_NESTED_VMX_VMCS_SIZE];
 	__u8 shadow_vmcs12[KVM_STATE_NESTED_VMX_VMCS_SIZE];
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 056e30f85424..b533301af95a 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4229,6 +4229,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 	case KVM_CAP_SREGS2:
 	case KVM_CAP_EXIT_ON_EMULATION_FAILURE:
 	case KVM_CAP_VCPU_ATTRIBUTES:
+	case KVM_CAP_SYS_ATTRIBUTES:
 		r = 1;
 		break;
 	case KVM_CAP_EXIT_HYPERCALL:
@@ -4331,7 +4332,35 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 		break;
 	}
 	return r;
+}
+
+static int kvm_x86_dev_get_attr(struct kvm_device_attr *attr)
+{
+	if (attr->group)
+		return -ENXIO;
+
+	switch (attr->attr) {
+	case KVM_X86_XCOMP_GUEST_SUPP:
+		if (put_user(supported_xcr0, (u64 __user *)attr->addr))
+			return -EFAULT;
+		return 0;
+	default:
+		return -ENXIO;
+		break;
+	}
+}
+
+static int kvm_x86_dev_has_attr(struct kvm_device_attr *attr)
+{
+	if (attr->group)
+		return -ENXIO;
 
+	switch (attr->attr) {
+	case KVM_X86_XCOMP_GUEST_SUPP:
+		return 0;
+	default:
+		return -ENXIO;
+	}
 }
 
 long kvm_arch_dev_ioctl(struct file *filp,
@@ -4422,6 +4451,22 @@ long kvm_arch_dev_ioctl(struct file *filp,
 	case KVM_GET_SUPPORTED_HV_CPUID:
 		r = kvm_ioctl_get_supported_hv_cpuid(NULL, argp);
 		break;
+	case KVM_GET_DEVICE_ATTR: {
+		struct kvm_device_attr attr;
+		r = -EFAULT;
+		if (copy_from_user(&attr, (void __user *)arg, sizeof(attr)))
+			break;
+		r = kvm_x86_dev_get_attr(&attr);
+		break;
+	}
+	case KVM_HAS_DEVICE_ATTR: {
+		struct kvm_device_attr attr;
+		r = -EFAULT;
+		if (copy_from_user(&attr, (void __user *)arg, sizeof(attr)))
+			break;
+		r = kvm_x86_dev_has_attr(&attr);
+		break;
+	}
 	default:
 		r = -EINVAL;
 		break;
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 9563d294f181..b46bcdb0cab1 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -1133,6 +1133,7 @@ struct kvm_ppc_resize_hpt {
 #define KVM_CAP_VM_MOVE_ENC_CONTEXT_FROM 206
 #define KVM_CAP_VM_GPA_BITS 207
 #define KVM_CAP_XSAVE2 208
+#define KVM_CAP_SYS_ATTRIBUTES 209
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
-- 
2.31.1

Re: [PATCH 2/3] KVM: x86: add system attribute to retrieve full set of supported xsave states

[Sean Christopherson]

On Wed, Jan 26, 2022, Paolo Bonzini wrote:
> +static int kvm_x86_dev_get_attr(struct kvm_device_attr *attr)
> +{
> +	if (attr->group)
> +		return -ENXIO;
> +
> +	switch (attr->attr) {
> +	case KVM_X86_XCOMP_GUEST_SUPP:
> +		if (put_user(supported_xcr0, (u64 __user *)attr->addr))

Deja vu[*].

  arch/x86/kvm/x86.c: In function ‘kvm_x86_dev_get_attr’:
  arch/x86/kvm/x86.c:4345:46: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
   4345 |                 if (put_user(supported_xcr0, (u64 __user *)attr->addr))
        |                                              ^
  arch/x86/include/asm/uaccess.h:221:31: note: in definition of macro ‘do_put_user_call’
    221 |         register __typeof__(*(ptr)) __val_pu asm("%"_ASM_AX);           \
        |                               ^~~
  arch/x86/kvm/x86.c:4345:21: note: in expansion of macro ‘put_user’
   4345 |                 if (put_user(supported_xcr0, (u64 __user *)attr->addr))
        |                     ^~~~~~~~
  arch/x86/kvm/x86.c:4345:46: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
   4345 |                 if (put_user(supported_xcr0, (u64 __user *)attr->addr))
        |                                              ^
  arch/x86/include/asm/uaccess.h:223:21: note: in definition of macro ‘do_put_user_call’
    223 |         __ptr_pu = (ptr);                                               \
        |                     ^~~
  arch/x86/kvm/x86.c:4345:21: note: in expansion of macro ‘put_user’
   4345 |                 if (put_user(supported_xcr0, (u64 __user *)attr->addr))
        |                     ^~~~~~~~
  arch/x86/kvm/x86.c:4345:46: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
   4345 |                 if (put_user(supported_xcr0, (u64 __user *)attr->addr))
        |                                              ^
  arch/x86/include/asm/uaccess.h:230:45: note: in definition of macro ‘do_put_user_call’
    230 |                        [size] "i" (sizeof(*(ptr)))                      \
        |                                             ^~~
  arch/x86/kvm/x86.c:4345:21: note: in expansion of macro ‘put_user’
   4345 |                 if (put_user(supported_xcr0, (u64 __user *)attr->addr))

Given that we're collectively 2 for 2 in mishandling {g,s}et_attr(), what about
a prep pacth like so?  Compile tested only...

From: Sean Christopherson <seanjc@google.com>
Date: Thu, 27 Jan 2022 07:31:53 -0800
Subject: [PATCH] KVM: x86: Add a helper to retrieve userspace address from
 kvm_device_attr

Add a helper to handle converting the u64 userspace address embedded in
struct kvm_device_attr into a userspace pointer, it's all too easy to
forget the intermediate "unsigned long" cast as well as the truncation
check.

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/x86.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 8033eca6f3a1..67836f7c71f5 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4335,14 +4335,28 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 	return r;
 }

+static inline void __user *kvm_get_attr_addr(struct kvm_device_attr *attr)
+{
+	void __user *uaddr = (void __user*)(unsigned long)attr->addr;
+
+	if ((u64)(unsigned long)uaddr != attr->addr)
+		return ERR_PTR(-EFAULT);
+	return uaddr;
+}
+
 static int kvm_x86_dev_get_attr(struct kvm_device_attr *attr)
 {
+	u64 __user *uaddr = kvm_get_attr_addr(attr);
+
 	if (attr->group)
 		return -ENXIO;

+	if (IS_ERR(uaddr))
+		return PTR_ERR(uaddr);
+
 	switch (attr->attr) {
 	case KVM_X86_XCOMP_GUEST_SUPP:
-		if (put_user(supported_xcr0, (u64 __user *)attr->addr))
+		if (put_user(supported_xcr0, uaddr))
 			return -EFAULT;
 		return 0;
 	default:
@@ -5070,11 +5084,11 @@ static int kvm_arch_tsc_has_attr(struct kvm_vcpu *vcpu,
 static int kvm_arch_tsc_get_attr(struct kvm_vcpu *vcpu,
 				 struct kvm_device_attr *attr)
 {
-	u64 __user *uaddr = (u64 __user *)(unsigned long)attr->addr;
+	u64 __user *uaddr = kvm_get_attr_addr(attr);
 	int r;

-	if ((u64)(unsigned long)uaddr != attr->addr)
-		return -EFAULT;
+	if (IS_ERR(uaddr))
+		return PTR_ERR(uaddr);

 	switch (attr->attr) {
 	case KVM_VCPU_TSC_OFFSET:
@@ -5093,12 +5107,12 @@ static int kvm_arch_tsc_get_attr(struct kvm_vcpu *vcpu,
 static int kvm_arch_tsc_set_attr(struct kvm_vcpu *vcpu,
 				 struct kvm_device_attr *attr)
 {
-	u64 __user *uaddr = (u64 __user *)(unsigned long)attr->addr;
+	u64 __user *uaddr = kvm_get_attr_addr(attr);
 	struct kvm *kvm = vcpu->kvm;
 	int r;

-	if ((u64)(unsigned long)uaddr != attr->addr)
-		return -EFAULT;
+	if (IS_ERR(uaddr))
+		return PTR_ERR(uaddr);

 	switch (attr->attr) {
 	case KVM_VCPU_TSC_OFFSET: {
--



[*] https://lore.kernel.org/all/20211007231647.3553604-1-seanjc@google.com


> +			return -EFAULT;
> +		return 0;
> +	default:
> +		return -ENXIO;
> +		break;
> +	}
> +}
> +

Re: [PATCH 2/3] KVM: x86: add system attribute to retrieve full set of supported xsave states

[Yang Zhong]

On Thu, Jan 27, 2022 at 03:35:50PM +0000, Sean Christopherson wrote:
> On Wed, Jan 26, 2022, Paolo Bonzini wrote:
> > +static int kvm_x86_dev_get_attr(struct kvm_device_attr *attr)
> > +{
> > +	if (attr->group)
> > +		return -ENXIO;
> > +
> > +	switch (attr->attr) {
> > +	case KVM_X86_XCOMP_GUEST_SUPP:
> > +		if (put_user(supported_xcr0, (u64 __user *)attr->addr))
> 
> Deja vu[*].
> 
>   arch/x86/kvm/x86.c: In function ‘kvm_x86_dev_get_attr’:
>   arch/x86/kvm/x86.c:4345:46: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
>    4345 |                 if (put_user(supported_xcr0, (u64 __user *)attr->addr))
>         |                                              ^
>   arch/x86/include/asm/uaccess.h:221:31: note: in definition of macro ‘do_put_user_call’
>     221 |         register __typeof__(*(ptr)) __val_pu asm("%"_ASM_AX);           \
>         |                               ^~~
>   arch/x86/kvm/x86.c:4345:21: note: in expansion of macro ‘put_user’
>    4345 |                 if (put_user(supported_xcr0, (u64 __user *)attr->addr))
>         |                     ^~~~~~~~
>   arch/x86/kvm/x86.c:4345:46: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
>    4345 |                 if (put_user(supported_xcr0, (u64 __user *)attr->addr))
>         |                                              ^
>   arch/x86/include/asm/uaccess.h:223:21: note: in definition of macro ‘do_put_user_call’
>     223 |         __ptr_pu = (ptr);                                               \
>         |                     ^~~
>   arch/x86/kvm/x86.c:4345:21: note: in expansion of macro ‘put_user’
>    4345 |                 if (put_user(supported_xcr0, (u64 __user *)attr->addr))
>         |                     ^~~~~~~~
>   arch/x86/kvm/x86.c:4345:46: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
>    4345 |                 if (put_user(supported_xcr0, (u64 __user *)attr->addr))
>         |                                              ^
>   arch/x86/include/asm/uaccess.h:230:45: note: in definition of macro ‘do_put_user_call’
>     230 |                        [size] "i" (sizeof(*(ptr)))                      \
>         |                                             ^~~
>   arch/x86/kvm/x86.c:4345:21: note: in expansion of macro ‘put_user’
>    4345 |                 if (put_user(supported_xcr0, (u64 __user *)attr->addr))
> 
> Given that we're collectively 2 for 2 in mishandling {g,s}et_attr(), what about
> a prep pacth like so?  Compile tested only...
>

  Sean, It's strange that I could not find those issues in my last day's build.
  
  My build environment:
  #make -v
  GNU Make 4.3
  
  # gcc -v
  gcc version 9.3.0 (Ubuntu 9.3.0-17ubuntu1~20.04)

  I will apply your extra patch to check again, thanks!
  

  Yang

 
> From: Sean Christopherson <seanjc@google.com>
> Date: Thu, 27 Jan 2022 07:31:53 -0800
> Subject: [PATCH] KVM: x86: Add a helper to retrieve userspace address from
>  kvm_device_attr
> 
> Add a helper to handle converting the u64 userspace address embedded in
> struct kvm_device_attr into a userspace pointer, it's all too easy to
> forget the intermediate "unsigned long" cast as well as the truncation
> check.
> 
> No functional change intended.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/x86.c | 28 +++++++++++++++++++++-------
>  1 file changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 8033eca6f3a1..67836f7c71f5 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4335,14 +4335,28 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>  	return r;
>  }
> 
> +static inline void __user *kvm_get_attr_addr(struct kvm_device_attr *attr)
> +{
> +	void __user *uaddr = (void __user*)(unsigned long)attr->addr;
> +
> +	if ((u64)(unsigned long)uaddr != attr->addr)
> +		return ERR_PTR(-EFAULT);
> +	return uaddr;
> +}
> +
>  static int kvm_x86_dev_get_attr(struct kvm_device_attr *attr)
>  {
> +	u64 __user *uaddr = kvm_get_attr_addr(attr);
> +
>  	if (attr->group)
>  		return -ENXIO;
> 
> +	if (IS_ERR(uaddr))
> +		return PTR_ERR(uaddr);
> +
>  	switch (attr->attr) {
>  	case KVM_X86_XCOMP_GUEST_SUPP:
> -		if (put_user(supported_xcr0, (u64 __user *)attr->addr))
> +		if (put_user(supported_xcr0, uaddr))
>  			return -EFAULT;
>  		return 0;
>  	default:
> @@ -5070,11 +5084,11 @@ static int kvm_arch_tsc_has_attr(struct kvm_vcpu *vcpu,
>  static int kvm_arch_tsc_get_attr(struct kvm_vcpu *vcpu,
>  				 struct kvm_device_attr *attr)
>  {
> -	u64 __user *uaddr = (u64 __user *)(unsigned long)attr->addr;
> +	u64 __user *uaddr = kvm_get_attr_addr(attr);
>  	int r;
> 
> -	if ((u64)(unsigned long)uaddr != attr->addr)
> -		return -EFAULT;
> +	if (IS_ERR(uaddr))
> +		return PTR_ERR(uaddr);
> 
>  	switch (attr->attr) {
>  	case KVM_VCPU_TSC_OFFSET:
> @@ -5093,12 +5107,12 @@ static int kvm_arch_tsc_get_attr(struct kvm_vcpu *vcpu,
>  static int kvm_arch_tsc_set_attr(struct kvm_vcpu *vcpu,
>  				 struct kvm_device_attr *attr)
>  {
> -	u64 __user *uaddr = (u64 __user *)(unsigned long)attr->addr;
> +	u64 __user *uaddr = kvm_get_attr_addr(attr);
>  	struct kvm *kvm = vcpu->kvm;
>  	int r;
> 
> -	if ((u64)(unsigned long)uaddr != attr->addr)
> -		return -EFAULT;
> +	if (IS_ERR(uaddr))
> +		return PTR_ERR(uaddr);
> 
>  	switch (attr->attr) {
>  	case KVM_VCPU_TSC_OFFSET: {
> --
> 
> 
> 
> [*] https://lore.kernel.org/all/20211007231647.3553604-1-seanjc@google.com
> 
> 
> > +			return -EFAULT;
> > +		return 0;
> > +	default:
> > +		return -ENXIO;
> > +		break;
> > +	}
> > +}
> > +

Re: [PATCH 2/3] KVM: x86: add system attribute to retrieve full set of supported xsave states

[Yang Zhong]

On Thu, Jan 27, 2022 at 03:35:50PM +0000, Sean Christopherson wrote:
> On Wed, Jan 26, 2022, Paolo Bonzini wrote:
> > +static int kvm_x86_dev_get_attr(struct kvm_device_attr *attr)
> > +{
> > +	if (attr->group)
> > +		return -ENXIO;
> > +
> > +	switch (attr->attr) {
> > +	case KVM_X86_XCOMP_GUEST_SUPP:
> > +		if (put_user(supported_xcr0, (u64 __user *)attr->addr))
> 
> Deja vu[*].
> 
>   arch/x86/kvm/x86.c: In function ‘kvm_x86_dev_get_attr’:
>   arch/x86/kvm/x86.c:4345:46: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
>    4345 |                 if (put_user(supported_xcr0, (u64 __user *)attr->addr))
>         |                                              ^
>   arch/x86/include/asm/uaccess.h:221:31: note: in definition of macro ‘do_put_user_call’
>     221 |         register __typeof__(*(ptr)) __val_pu asm("%"_ASM_AX);           \
>         |                               ^~~
>   arch/x86/kvm/x86.c:4345:21: note: in expansion of macro ‘put_user’
>    4345 |                 if (put_user(supported_xcr0, (u64 __user *)attr->addr))
>         |                     ^~~~~~~~
>   arch/x86/kvm/x86.c:4345:46: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
>    4345 |                 if (put_user(supported_xcr0, (u64 __user *)attr->addr))
>         |                                              ^
>   arch/x86/include/asm/uaccess.h:223:21: note: in definition of macro ‘do_put_user_call’
>     223 |         __ptr_pu = (ptr);                                               \
>         |                     ^~~
>   arch/x86/kvm/x86.c:4345:21: note: in expansion of macro ‘put_user’
>    4345 |                 if (put_user(supported_xcr0, (u64 __user *)attr->addr))
>         |                     ^~~~~~~~
>   arch/x86/kvm/x86.c:4345:46: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
>    4345 |                 if (put_user(supported_xcr0, (u64 __user *)attr->addr))
>         |                                              ^
>   arch/x86/include/asm/uaccess.h:230:45: note: in definition of macro ‘do_put_user_call’
>     230 |                        [size] "i" (sizeof(*(ptr)))                      \
>         |                                             ^~~
>   arch/x86/kvm/x86.c:4345:21: note: in expansion of macro ‘put_user’
>    4345 |                 if (put_user(supported_xcr0, (u64 __user *)attr->addr))
> 
> Given that we're collectively 2 for 2 in mishandling {g,s}et_attr(), what about
> a prep pacth like so?  Compile tested only...
>

  Just found those issues are from 32bit kernel build, thanks!
  
  I applied below patch, and did AMX functions test, this patch work well with AMX.

  Thanks!
  Yang

 
> From: Sean Christopherson <seanjc@google.com>
> Date: Thu, 27 Jan 2022 07:31:53 -0800
> Subject: [PATCH] KVM: x86: Add a helper to retrieve userspace address from
>  kvm_device_attr
> 
> Add a helper to handle converting the u64 userspace address embedded in
> struct kvm_device_attr into a userspace pointer, it's all too easy to
> forget the intermediate "unsigned long" cast as well as the truncation
> check.
> 
> No functional change intended.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/x86.c | 28 +++++++++++++++++++++-------
>  1 file changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 8033eca6f3a1..67836f7c71f5 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4335,14 +4335,28 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>  	return r;
>  }
> 
> +static inline void __user *kvm_get_attr_addr(struct kvm_device_attr *attr)
> +{
> +	void __user *uaddr = (void __user*)(unsigned long)attr->addr;
> +
> +	if ((u64)(unsigned long)uaddr != attr->addr)
> +		return ERR_PTR(-EFAULT);
> +	return uaddr;
> +}
> +
>  static int kvm_x86_dev_get_attr(struct kvm_device_attr *attr)
>  {
> +	u64 __user *uaddr = kvm_get_attr_addr(attr);
> +
>  	if (attr->group)
>  		return -ENXIO;
> 
> +	if (IS_ERR(uaddr))
> +		return PTR_ERR(uaddr);
> +
>  	switch (attr->attr) {
>  	case KVM_X86_XCOMP_GUEST_SUPP:
> -		if (put_user(supported_xcr0, (u64 __user *)attr->addr))
> +		if (put_user(supported_xcr0, uaddr))
>  			return -EFAULT;
>  		return 0;
>  	default:
> @@ -5070,11 +5084,11 @@ static int kvm_arch_tsc_has_attr(struct kvm_vcpu *vcpu,
>  static int kvm_arch_tsc_get_attr(struct kvm_vcpu *vcpu,
>  				 struct kvm_device_attr *attr)
>  {
> -	u64 __user *uaddr = (u64 __user *)(unsigned long)attr->addr;
> +	u64 __user *uaddr = kvm_get_attr_addr(attr);
>  	int r;
> 
> -	if ((u64)(unsigned long)uaddr != attr->addr)
> -		return -EFAULT;
> +	if (IS_ERR(uaddr))
> +		return PTR_ERR(uaddr);
> 
>  	switch (attr->attr) {
>  	case KVM_VCPU_TSC_OFFSET:
> @@ -5093,12 +5107,12 @@ static int kvm_arch_tsc_get_attr(struct kvm_vcpu *vcpu,
>  static int kvm_arch_tsc_set_attr(struct kvm_vcpu *vcpu,
>  				 struct kvm_device_attr *attr)
>  {
> -	u64 __user *uaddr = (u64 __user *)(unsigned long)attr->addr;
> +	u64 __user *uaddr = kvm_get_attr_addr(attr);
>  	struct kvm *kvm = vcpu->kvm;
>  	int r;
> 
> -	if ((u64)(unsigned long)uaddr != attr->addr)
> -		return -EFAULT;
> +	if (IS_ERR(uaddr))
> +		return PTR_ERR(uaddr);
> 
>  	switch (attr->attr) {
>  	case KVM_VCPU_TSC_OFFSET: {
> --
> 
> 
> 
> [*] https://lore.kernel.org/all/20211007231647.3553604-1-seanjc@google.com
> 
> 
> > +			return -EFAULT;
> > +		return 0;
> > +	default:
> > +		return -ENXIO;
> > +		break;
> > +	}
> > +}
> > +

Re: [PATCH 2/3] KVM: x86: add system attribute to retrieve full set of supported xsave states

[Paolo Bonzini]

On 1/27/22 16:35, Sean Christopherson wrote:
> On Wed, Jan 26, 2022, Paolo Bonzini wrote:
>> +static int kvm_x86_dev_get_attr(struct kvm_device_attr *attr)
>> +{
>> +	if (attr->group)
>> +		return -ENXIO;
>> +
>> +	switch (attr->attr) {
>> +	case KVM_X86_XCOMP_GUEST_SUPP:
>> +		if (put_user(supported_xcr0, (u64 __user *)attr->addr))
> 
> Deja vu[*].
> 
>    arch/x86/kvm/x86.c: In function ‘kvm_x86_dev_get_attr’:
>    arch/x86/kvm/x86.c:4345:46: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
>     4345 |                 if (put_user(supported_xcr0, (u64 __user *)attr->addr))
>          |                                              ^
>    arch/x86/include/asm/uaccess.h:221:31: note: in definition of macro ‘do_put_user_call’
>      221 |         register __typeof__(*(ptr)) __val_pu asm("%"_ASM_AX);           \
>          |                               ^~~
>    arch/x86/kvm/x86.c:4345:21: note: in expansion of macro ‘put_user’
>     4345 |                 if (put_user(supported_xcr0, (u64 __user *)attr->addr))
>          |                     ^~~~~~~~
>    arch/x86/kvm/x86.c:4345:46: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
>     4345 |                 if (put_user(supported_xcr0, (u64 __user *)attr->addr))
>          |                                              ^
>    arch/x86/include/asm/uaccess.h:223:21: note: in definition of macro ‘do_put_user_call’
>      223 |         __ptr_pu = (ptr);                                               \
>          |                     ^~~
>    arch/x86/kvm/x86.c:4345:21: note: in expansion of macro ‘put_user’
>     4345 |                 if (put_user(supported_xcr0, (u64 __user *)attr->addr))
>          |                     ^~~~~~~~
>    arch/x86/kvm/x86.c:4345:46: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
>     4345 |                 if (put_user(supported_xcr0, (u64 __user *)attr->addr))
>          |                                              ^
>    arch/x86/include/asm/uaccess.h:230:45: note: in definition of macro ‘do_put_user_call’
>      230 |                        [size] "i" (sizeof(*(ptr)))                      \
>          |                                             ^~~
>    arch/x86/kvm/x86.c:4345:21: note: in expansion of macro ‘put_user’
>     4345 |                 if (put_user(supported_xcr0, (u64 __user *)attr->addr))
> 
> Given that we're collectively 2 for 2 in mishandling {g,s}et_attr(), what about
> a prep pacth like so?  Compile tested only...
> 
> From: Sean Christopherson <seanjc@google.com>
> Date: Thu, 27 Jan 2022 07:31:53 -0800
> Subject: [PATCH] KVM: x86: Add a helper to retrieve userspace address from
>   kvm_device_attr
> 
> Add a helper to handle converting the u64 userspace address embedded in
> struct kvm_device_attr into a userspace pointer, it's all too easy to
> forget the intermediate "unsigned long" cast as well as the truncation
> check.
> 
> No functional change intended.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>   arch/x86/kvm/x86.c | 28 +++++++++++++++++++++-------
>   1 file changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 8033eca6f3a1..67836f7c71f5 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4335,14 +4335,28 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>   	return r;
>   }
> 
> +static inline void __user *kvm_get_attr_addr(struct kvm_device_attr *attr)
> +{
> +	void __user *uaddr = (void __user*)(unsigned long)attr->addr;
> +
> +	if ((u64)(unsigned long)uaddr != attr->addr)
> +		return ERR_PTR(-EFAULT);
> +	return uaddr;
> +}
> +
>   static int kvm_x86_dev_get_attr(struct kvm_device_attr *attr)
>   {
> +	u64 __user *uaddr = kvm_get_attr_addr(attr);
> +
>   	if (attr->group)
>   		return -ENXIO;
> 
> +	if (IS_ERR(uaddr))
> +		return PTR_ERR(uaddr);
> +
>   	switch (attr->attr) {
>   	case KVM_X86_XCOMP_GUEST_SUPP:
> -		if (put_user(supported_xcr0, (u64 __user *)attr->addr))
> +		if (put_user(supported_xcr0, uaddr))
>   			return -EFAULT;
>   		return 0;
>   	default:
> @@ -5070,11 +5084,11 @@ static int kvm_arch_tsc_has_attr(struct kvm_vcpu *vcpu,
>   static int kvm_arch_tsc_get_attr(struct kvm_vcpu *vcpu,
>   				 struct kvm_device_attr *attr)
>   {
> -	u64 __user *uaddr = (u64 __user *)(unsigned long)attr->addr;
> +	u64 __user *uaddr = kvm_get_attr_addr(attr);
>   	int r;
> 
> -	if ((u64)(unsigned long)uaddr != attr->addr)
> -		return -EFAULT;
> +	if (IS_ERR(uaddr))
> +		return PTR_ERR(uaddr);
> 
>   	switch (attr->attr) {
>   	case KVM_VCPU_TSC_OFFSET:
> @@ -5093,12 +5107,12 @@ static int kvm_arch_tsc_get_attr(struct kvm_vcpu *vcpu,
>   static int kvm_arch_tsc_set_attr(struct kvm_vcpu *vcpu,
>   				 struct kvm_device_attr *attr)
>   {
> -	u64 __user *uaddr = (u64 __user *)(unsigned long)attr->addr;
> +	u64 __user *uaddr = kvm_get_attr_addr(attr);
>   	struct kvm *kvm = vcpu->kvm;
>   	int r;
> 
> -	if ((u64)(unsigned long)uaddr != attr->addr)
> -		return -EFAULT;
> +	if (IS_ERR(uaddr))
> +		return PTR_ERR(uaddr);
> 
>   	switch (attr->attr) {
>   	case KVM_VCPU_TSC_OFFSET: {
> --
> 

Nice, I applied it.

Paolo

[PATCH 3/3] selftests: kvm: check dynamic bits against KVM_X86_XCOMP_GUEST_SUPP

[Paolo Bonzini]

Provide coverage for the new API.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 tools/arch/x86/include/uapi/asm/kvm.h             |  3 +++
 tools/include/uapi/linux/kvm.h                    |  1 +
 .../testing/selftests/kvm/lib/x86_64/processor.c  | 15 +++++++++++++++
 3 files changed, 19 insertions(+)

diff --git a/tools/arch/x86/include/uapi/asm/kvm.h b/tools/arch/x86/include/uapi/asm/kvm.h
index 2da3316bb559..bf6e96011dfe 100644
--- a/tools/arch/x86/include/uapi/asm/kvm.h
+++ b/tools/arch/x86/include/uapi/asm/kvm.h
@@ -452,6 +452,9 @@ struct kvm_sync_regs {
 
 #define KVM_STATE_VMX_PREEMPTION_TIMER_DEADLINE	0x00000001
 
+/* attributes for system fd (group 0) */
+#define KVM_X86_XCOMP_GUEST_SUPP	0
+
 struct kvm_vmx_nested_state_data {
 	__u8 vmcs12[KVM_STATE_NESTED_VMX_VMCS_SIZE];
 	__u8 shadow_vmcs12[KVM_STATE_NESTED_VMX_VMCS_SIZE];
diff --git a/tools/include/uapi/linux/kvm.h b/tools/include/uapi/linux/kvm.h
index 9563d294f181..b46bcdb0cab1 100644
--- a/tools/include/uapi/linux/kvm.h
+++ b/tools/include/uapi/linux/kvm.h
@@ -1133,6 +1133,7 @@ struct kvm_ppc_resize_hpt {
 #define KVM_CAP_VM_MOVE_ENC_CONTEXT_FROM 206
 #define KVM_CAP_VM_GPA_BITS 207
 #define KVM_CAP_XSAVE2 208
+#define KVM_CAP_SYS_ATTRIBUTES 209
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
index c1d1c195a838..9f000dfb5594 100644
--- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
+++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
@@ -667,8 +667,23 @@ static bool is_xfd_supported(void)
 
 void vm_xsave_req_perm(int bit)
 {
+	int kvm_fd;
 	u64 bitmask;
 	long rc;
+	struct kvm_device_attr attr = {
+		.group = 0,
+		.attr = KVM_X86_XCOMP_GUEST_SUPP,
+		.addr = (unsigned long) &bitmask
+	};
+
+	kvm_fd = open_kvm_dev_path_or_exit();
+	rc = ioctl(kvm_fd, KVM_GET_DEVICE_ATTR, &attr);
+	close(kvm_fd);
+	if (rc == -1 && (errno == ENXIO || errno == EINVAL))
+		exit(KSFT_SKIP);
+	TEST_ASSERT(rc == 0, "KVM_GET_DEVICE_ATTR(0, KVM_X86_XCOMP_GUEST_SUPP) error: %ld", rc);
+	if (!(bitmask & (1ULL << bit)))
+		exit(KSFT_SKIP);
 
 	if (!is_xfd_supported())
 		exit(KSFT_SKIP);
-- 
2.31.1

Re: [PATCH 0/3] KVM: x86: export supported_xcr0 via UAPI

[Yang Zhong]

On Wed, Jan 26, 2022 at 10:22:07AM -0500, Paolo Bonzini wrote:
> While working on the QEMU support for AMX, I noticed that there is no
> equivalent of ARCH_GET_XCOMP_SUPP in the KVM API.  This is important
> because KVM_GET_SUPPORTED_CPUID is meant to be passed (by simple-minded
> VMMs) to KVM_SET_CPUID2, and therefore it cannot include any dynamic
> xsave states that have not been enabled.  Probing the availability of
> dynamic xsave states therefore, requires a new ioctl or arch_prctl.
> 
> In order to avoid moving supported_xcr0 to the kernel from the KVM
> module just for this use, and to ensure that the value can only be
> probed if/after the KVM module has been loaded, this series goes
> for the former option.
> 
> KVM_CHECK_EXTENSION cannot be used because it only has 32 bits of
> output; in order to limit the growth of capabilities and ioctls, the
> series adds a /dev/kvm variant of KVM_{GET,HAS}_DEVICE_ATTR that
> can be used in the future and by other architectures.  It then
> implements it in x86 with just one group (0) and attribute
> (KVM_X86_XCOMP_GUEST_SUPP).
> 
> The corresponding changes to the tests, in patches 1 and 3, are
> designed so that the code will be covered (to the possible extent)
> even when running the tests on systems that do not support AMX.
> However, the patches have not been tested with AMX.
>

  Paolo, thanks for this patchset. I applied this patchset into latest
  Linux release, and verified it from kvm selftest tool and Qemu side
  (In order to verify this easily, I reused the older request permission
   function like kvm selftest did), all work well. thanks!

  Yang
 
> Thanks,
> 
> Paolo
> 
> 
> Paolo Bonzini (3):
>   selftests: kvm: move vm_xsave_req_perm call to amx_test
>   KVM: x86: add system attribute to retrieve full set of supported xsave
>     states
>   selftests: kvm: check dynamic bits against KVM_X86_XCOMP_GUEST_SUPP
> 
>  Documentation/virt/kvm/api.rst                |  4 +-
>  arch/x86/include/uapi/asm/kvm.h               |  3 ++
>  arch/x86/kvm/x86.c                            | 45 +++++++++++++++++++
>  include/uapi/linux/kvm.h                      |  1 +
>  tools/arch/x86/include/uapi/asm/kvm.h         |  3 ++
>  tools/include/uapi/linux/kvm.h                |  1 +
>  .../selftests/kvm/include/kvm_util_base.h     |  1 -
>  .../selftests/kvm/include/x86_64/processor.h  |  1 +
>  tools/testing/selftests/kvm/lib/kvm_util.c    |  7 ---
>  .../selftests/kvm/lib/x86_64/processor.c      | 27 ++++++++---
>  tools/testing/selftests/kvm/x86_64/amx_test.c |  2 +
>  11 files changed, 80 insertions(+), 15 deletions(-)
> 
> -- 
> 2.31.1