1. Patchew
  2. QEMU
  3. View series

[PATCH] target/i386: Return right size value after dynamic xfeature enabled

Yang Zhong posted 1 patch 2 years, 1 month ago
Failed in applying to current master (apply log)
target/i386/cpu.h     |  3 +++
target/i386/kvm/kvm.c | 15 +++++++++++++--
2 files changed, 16 insertions(+), 2 deletions(-)
[PATCH] target/i386: Return right size value after dynamic xfeature enabled
Posted by Yang Zhong 2 years, 1 month ago 
The kvm_arch_get_supported_cpuid() only call KVM_GET_SUPPORTED_CPUID one
time, so the cpuid buffer information still keep older value. Once Qemu
enable new dynamic xfeature, like XTILEDATA, the cpuid[0D,0].{EBX,ECX}
still return older value.

This patch can return right size value in kvm_init_xsave() if XTILEDATA
has been enabled by arch_prctl.

assert(kvm_arch_get_supported_cpuid(kvm_state, 0xd, 0, R_ECX) <=
           env->xsave_buf_len);

Signed-off-by: Yang Zhong <yang.zhong@intel.com>
---
 target/i386/cpu.h     |  3 +++
 target/i386/kvm/kvm.c | 15 +++++++++++++--
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 5e406088a9..814ba4020b 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -565,6 +565,9 @@ typedef enum X86Seg {
 #define ESA_FEATURE_ALIGN64_MASK        (1U << ESA_FEATURE_ALIGN64_BIT)
 #define ESA_FEATURE_XFD_MASK            (1U << ESA_FEATURE_XFD_BIT)
 
+#define ARCH_GET_XCOMP_GUEST_PERM       0x1024
+#define ARCH_REQ_XCOMP_GUEST_PERM       0x1025
+
 
 /* CPUID feature words */
 typedef enum FeatureWord {
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 06901c2a43..312d4fccf8 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -46,6 +46,7 @@
 #include "hw/i386/intel_iommu.h"
 #include "hw/i386/x86-iommu.h"
 #include "hw/i386/e820_memory_layout.h"
+#include "target/i386/cpu.h"
 
 #include "hw/pci/pci.h"
 #include "hw/pci/msi.h"
@@ -437,6 +438,18 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function,
             return ret;
         }
         ret = (reg == R_EAX) ? bitmask : bitmask >> 32;
+    } else if (function == 0xd && index == 0 &&
+               (reg == R_EBX || reg == R_ECX)) {
+        /*
+         * The value returned by KVM_GET_SUPPORTED_CPUID does not include
+         * features that already be enabled with the arch_prctl system call.
+         */
+        int rc = syscall(SYS_arch_prctl, ARCH_GET_XCOMP_GUEST_PERM, &bitmask);
+        if (rc) {
+            warn_report("prctl(ARCH_GET_XCOMP_GUEST_PERM) error: %d", rc);
+        } else if (bitmask & XSTATE_XTILE_DATA_MASK) {
+            ret += sizeof(XSaveXTILEDATA);
+        }
     } else if (function == 0x80000001 && reg == R_ECX) {
         /*
          * It's safe to enable TOPOEXT even if it's not returned by
@@ -5214,8 +5227,6 @@ bool kvm_arch_cpu_check_are_resettable(void)
     return !sev_es_enabled();
 }
 
-#define ARCH_REQ_XCOMP_GUEST_PERM       0x1025
-
 void kvm_request_xsave_components(X86CPU *cpu, uint64_t mask)
 {
     KVMState *s = kvm_state;
Re: [PATCH] target/i386: Return right size value after dynamic xfeature enabled
Posted by Paolo Bonzini 2 years, 1 month ago 
On 3/24/22 04:18, Yang Zhong wrote:
> The kvm_arch_get_supported_cpuid() only call KVM_GET_SUPPORTED_CPUID one
> time, so the cpuid buffer information still keep older value. Once Qemu
> enable new dynamic xfeature, like XTILEDATA, the cpuid[0D,0].{EBX,ECX}
> still return older value.
> 
> This patch can return right size value in kvm_init_xsave() if XTILEDATA
> has been enabled by arch_prctl.
> 
> assert(kvm_arch_get_supported_cpuid(kvm_state, 0xd, 0, R_ECX) <=
>             env->xsave_buf_len);
> 
> Signed-off-by: Yang Zhong <yang.zhong@intel.com>

I don't understand, is this a bugfix for an assertion failure or just a 
cleanup?

Either way, while I like the idea of modifying 
kvm_arch_get_supported_cpuid, I think the right thing to do is to just 
use has_xsave2 as the return value if it is nonzero.  And then 
kvm_init_xsave can just do

if (!has_xsave) {
     return;
}
env->xsave_buf_len = kvm_arch_get_supported_cpuid(kvm_state, 0xd, 0, R_ECX);

without the assertion that is now obvious.

Paolo

> ---
>   target/i386/cpu.h     |  3 +++
>   target/i386/kvm/kvm.c | 15 +++++++++++++--
>   2 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index 5e406088a9..814ba4020b 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -565,6 +565,9 @@ typedef enum X86Seg {
>   #define ESA_FEATURE_ALIGN64_MASK        (1U << ESA_FEATURE_ALIGN64_BIT)
>   #define ESA_FEATURE_XFD_MASK            (1U << ESA_FEATURE_XFD_BIT)
>   
> +#define ARCH_GET_XCOMP_GUEST_PERM       0x1024
> +#define ARCH_REQ_XCOMP_GUEST_PERM       0x1025
> +
>   
>   /* CPUID feature words */
>   typedef enum FeatureWord {
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index 06901c2a43..312d4fccf8 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -46,6 +46,7 @@
>   #include "hw/i386/intel_iommu.h"
>   #include "hw/i386/x86-iommu.h"
>   #include "hw/i386/e820_memory_layout.h"
> +#include "target/i386/cpu.h"
>   
>   #include "hw/pci/pci.h"
>   #include "hw/pci/msi.h"
> @@ -437,6 +438,18 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function,
>               return ret;
>           }
>           ret = (reg == R_EAX) ? bitmask : bitmask >> 32;
> +    } else if (function == 0xd && index == 0 &&
> +               (reg == R_EBX || reg == R_ECX)) {
> +        /*
> +         * The value returned by KVM_GET_SUPPORTED_CPUID does not include
> +         * features that already be enabled with the arch_prctl system call.
> +         */
> +        int rc = syscall(SYS_arch_prctl, ARCH_GET_XCOMP_GUEST_PERM, &bitmask);
> +        if (rc) {
> +            warn_report("prctl(ARCH_GET_XCOMP_GUEST_PERM) error: %d", rc);
> +        } else if (bitmask & XSTATE_XTILE_DATA_MASK) {
> +            ret += sizeof(XSaveXTILEDATA);
> +        }
>       } else if (function == 0x80000001 && reg == R_ECX) {
>           /*
>            * It's safe to enable TOPOEXT even if it's not returned by
> @@ -5214,8 +5227,6 @@ bool kvm_arch_cpu_check_are_resettable(void)
>       return !sev_es_enabled();
>   }
>   
> -#define ARCH_REQ_XCOMP_GUEST_PERM       0x1025
> -
>   void kvm_request_xsave_components(X86CPU *cpu, uint64_t mask)
>   {
>       KVMState *s = kvm_state;
>
Re: [PATCH] target/i386: Return right size value after dynamic xfeature enabled
Posted by Yang Zhong 2 years, 1 month ago 
On Thu, Mar 24, 2022 at 08:35:10AM +0100, Paolo Bonzini wrote:
> On 3/24/22 04:18, Yang Zhong wrote:
> >The kvm_arch_get_supported_cpuid() only call KVM_GET_SUPPORTED_CPUID one
> >time, so the cpuid buffer information still keep older value. Once Qemu
> >enable new dynamic xfeature, like XTILEDATA, the cpuid[0D,0].{EBX,ECX}
> >still return older value.
> >
> >This patch can return right size value in kvm_init_xsave() if XTILEDATA
> >has been enabled by arch_prctl.
> >
> >assert(kvm_arch_get_supported_cpuid(kvm_state, 0xd, 0, R_ECX) <=
> >            env->xsave_buf_len);
> >
> >Signed-off-by: Yang Zhong <yang.zhong@intel.com>
> 
> I don't understand, is this a bugfix for an assertion failure or
> just a cleanup?
> 

  In fact, no assert issue here.
  The issue is after we enable dynamic xfeature, and if we still use
  kvm_arch_get_supported_cpuid(kvm_state, 0xd, 0, R_ECX) to get size,
  the size is older value(size:2816), not the size(11008) we expected.

  The code for cpuid[0D,0].{EBX,ECX} by kvm_arch_get_supported_cpuid()
  need cleanup here, or we can't get the real value here. thanks!

  Yang



> Either way, while I like the idea of modifying
> kvm_arch_get_supported_cpuid, I think the right thing to do is to
> just use has_xsave2 as the return value if it is nonzero.  And then
> kvm_init_xsave can just do
> 
> if (!has_xsave) {
>     return;
> }
> env->xsave_buf_len = kvm_arch_get_supported_cpuid(kvm_state, 0xd, 0, R_ECX);
> 
> without the assertion that is now obvious.
> 
> Paolo
> 
> >---
> >  target/i386/cpu.h     |  3 +++
> >  target/i386/kvm/kvm.c | 15 +++++++++++++--
> >  2 files changed, 16 insertions(+), 2 deletions(-)
> >
> >diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> >index 5e406088a9..814ba4020b 100644
> >--- a/target/i386/cpu.h
> >+++ b/target/i386/cpu.h
> >@@ -565,6 +565,9 @@ typedef enum X86Seg {
> >  #define ESA_FEATURE_ALIGN64_MASK        (1U << ESA_FEATURE_ALIGN64_BIT)
> >  #define ESA_FEATURE_XFD_MASK            (1U << ESA_FEATURE_XFD_BIT)
> >+#define ARCH_GET_XCOMP_GUEST_PERM       0x1024
> >+#define ARCH_REQ_XCOMP_GUEST_PERM       0x1025
> >+
> >  /* CPUID feature words */
> >  typedef enum FeatureWord {
> >diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> >index 06901c2a43..312d4fccf8 100644
> >--- a/target/i386/kvm/kvm.c
> >+++ b/target/i386/kvm/kvm.c
> >@@ -46,6 +46,7 @@
> >  #include "hw/i386/intel_iommu.h"
> >  #include "hw/i386/x86-iommu.h"
> >  #include "hw/i386/e820_memory_layout.h"
> >+#include "target/i386/cpu.h"
> >  #include "hw/pci/pci.h"
> >  #include "hw/pci/msi.h"
> >@@ -437,6 +438,18 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function,
> >              return ret;
> >          }
> >          ret = (reg == R_EAX) ? bitmask : bitmask >> 32;
> >+    } else if (function == 0xd && index == 0 &&
> >+               (reg == R_EBX || reg == R_ECX)) {
> >+        /*
> >+         * The value returned by KVM_GET_SUPPORTED_CPUID does not include
> >+         * features that already be enabled with the arch_prctl system call.
> >+         */
> >+        int rc = syscall(SYS_arch_prctl, ARCH_GET_XCOMP_GUEST_PERM, &bitmask);
> >+        if (rc) {
> >+            warn_report("prctl(ARCH_GET_XCOMP_GUEST_PERM) error: %d", rc);
> >+        } else if (bitmask & XSTATE_XTILE_DATA_MASK) {
> >+            ret += sizeof(XSaveXTILEDATA);
> >+        }
> >      } else if (function == 0x80000001 && reg == R_ECX) {
> >          /*
> >           * It's safe to enable TOPOEXT even if it's not returned by
> >@@ -5214,8 +5227,6 @@ bool kvm_arch_cpu_check_are_resettable(void)
> >      return !sev_es_enabled();
> >  }
> >-#define ARCH_REQ_XCOMP_GUEST_PERM       0x1025
> >-
> >  void kvm_request_xsave_components(X86CPU *cpu, uint64_t mask)
> >  {
> >      KVMState *s = kvm_state;
> >

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.