qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 3/7] x86: Grant AMX permission for guest


From: Paolo Bonzini
Subject: Re: [PATCH 3/7] x86: Grant AMX permission for guest
Date: Mon, 24 Jan 2022 11:16:36 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.4.0

On 1/24/22 08:55, Yang Zhong wrote:
Kernel allocates 4K xstate buffer by default. For XSAVE features
which require large state component (e.g. AMX), Linux kernel
dynamically expands the xstate buffer only after the process has
acquired the necessary permissions. Those are called dynamically-
enabled XSAVE features (or dynamic xfeatures).

There are separate permissions for native tasks and guests.

Qemu should request the guest permissions for dynamic xfeatures
which will be exposed to the guest. This only needs to be done
once before the first vcpu is created.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Yang Zhong <yang.zhong@intel.com>
Signed-off-by: Jing Liu <jing2.liu@intel.com>
Signed-off-by: Wei Wang <wei.w.wang@intel.com>
---
  target/i386/cpu.h         |  7 +++++++
  target/i386/cpu.c         | 31 +++++++++++++++++++++++++++++++
  target/i386/kvm/kvm-cpu.c | 12 ++++++------
  target/i386/kvm/kvm.c     |  6 ++++++
  4 files changed, 50 insertions(+), 6 deletions(-)

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 06d2d6bccf..d4ad0f56bd 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -549,6 +549,13 @@ typedef enum X86Seg {
  #define XSTATE_ZMM_Hi256_MASK           (1ULL << XSTATE_ZMM_Hi256_BIT)
  #define XSTATE_Hi16_ZMM_MASK            (1ULL << XSTATE_Hi16_ZMM_BIT)
  #define XSTATE_PKRU_MASK                (1ULL << XSTATE_PKRU_BIT)
+#define XSTATE_XTILE_CFG_MASK           (1ULL << XSTATE_XTILE_CFG_BIT)
+#define XSTATE_XTILE_DATA_MASK          (1ULL << XSTATE_XTILE_DATA_BIT)
+#define XFEATURE_XTILE_MASK             (XSTATE_XTILE_CFG_MASK \
+                                         | XSTATE_XTILE_DATA_MASK)
+
+#define ARCH_GET_XCOMP_GUEST_PERM       0x1024
+#define ARCH_REQ_XCOMP_GUEST_PERM       0x1025
#define ESA_FEATURE_ALIGN64_BIT 1 diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 3390820745..29b0348c25 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -43,6 +43,10 @@
  #include "disas/capstone.h"
  #include "cpu-internal.h"
+#include <sys/syscall.h>
+
+bool request_perm;
+
  /* Helpers for building CPUID[2] descriptors: */
struct CPUID2CacheDescriptorInfo {
@@ -6000,6 +6004,27 @@ static void x86_cpu_adjust_feat_level(X86CPU *cpu, 
FeatureWord w)
      }
  }
+static void kvm_request_xsave_components(X86CPU *cpu, uint32_t bit)
+{
+    KVMState *s = CPU(cpu)->kvm_state;
+
+    long rc = syscall(SYS_arch_prctl, ARCH_REQ_XCOMP_GUEST_PERM,
+                      bit);
+    if (rc) {
+        /*
+         * The older kernel version(<5.15) can't support
+         * ARCH_REQ_XCOMP_GUEST_PERM and directly return.
+         */
+        return;
+    }
+
+    rc = kvm_arch_get_supported_cpuid(s, 0xd, 0, R_EAX);
+    if (!(rc & XFEATURE_XTILE_MASK)) {
+        error_report("get cpuid failure and rc=0x%lx", rc);
+        exit(EXIT_FAILURE);
+    }
+}
+
  /* Calculate XSAVE components based on the configured CPU feature flags */
  static void x86_cpu_enable_xsave_components(X86CPU *cpu)
  {
@@ -6021,6 +6046,12 @@ static void x86_cpu_enable_xsave_components(X86CPU *cpu)
          }
      }
+ /* Only request permission from fisrt vcpu. */
+    if (kvm_enabled() && !request_perm) {
+        kvm_request_xsave_components(cpu, XSTATE_XTILE_DATA_BIT);
+        request_perm = true;
+    }

You should pass "mask" here, or "mask & XSTATE_DYNAMIC_MASK" so that the components are only requested if necessary.

      env->features[FEAT_XSAVE_COMP_LO] = mask;
      env->features[FEAT_XSAVE_COMP_HI] = mask >> 32;
  }
diff --git a/target/i386/kvm/kvm-cpu.c b/target/i386/kvm/kvm-cpu.c
index 033ca011ea..5ab6a0b9d2 100644
--- a/target/i386/kvm/kvm-cpu.c
+++ b/target/i386/kvm/kvm-cpu.c
@@ -84,7 +84,7 @@ static void kvm_cpu_max_instance_init(X86CPU *cpu)
  static void kvm_cpu_xsave_init(void)
  {
      static bool first = true;
-    KVMState *s = kvm_state;
+    uint32_t eax, ebx, ecx, edx;
      int i;
if (!first) {
@@ -100,13 +100,13 @@ static void kvm_cpu_xsave_init(void)
          ExtSaveArea *esa = &x86_ext_save_areas[i];
if (esa->size) {
-            int sz = kvm_arch_get_supported_cpuid(s, 0xd, i, R_EAX);
-            if (sz != 0) {
-                assert(esa->size == sz);
-                esa->offset = kvm_arch_get_supported_cpuid(s, 0xd, i, R_EBX);
+            host_cpuid(0xd, i, &eax, &ebx, &ecx, &edx);
+            if (eax != 0) {
+                assert(esa->size == eax);
+                esa->offset = ebx;
              }
- esa->ecx = kvm_arch_get_supported_cpuid(s, 0xd, i, R_ECX);
+            esa->ecx = ecx;

I think esa->ecx should be assigned inside the "if". This is also true in patch 1.

          }
      }
  }
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 2c8feb4a6f..caf1388d8b 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -405,6 +405,12 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, 
uint32_t function,
          if (!has_msr_arch_capabs) {
              ret &= ~CPUID_7_0_EDX_ARCH_CAPABILITIES;
          }
+    } else if (function == 0xd && index == 0 && reg == R_EAX) {
+        /*
+         * We can set the AMX XTILE DATA flag, even if KVM does not
+         * return it on GET_SUPPORTED_CPUID.
+         */
+        ret |= XSTATE_XTILE_DATA_MASK;
      } else if (function == 0x80000001 && reg == R_ECX) {
          /*
           * It's safe to enable TOPOEXT even if it's not returned by


Instead of setting XSTATE_XTILE_DATA_MASK blindly, you need something like arch_prctl(ARCH_GET_XCOMP_GUEST_SUPP). However, this arch_prctl only exists in the bare-metal version ARCH_GET_XCOMP_SUPP, and it would need access to the variable supported_xcr0 that KVM exports. So I think it's better to implement it as a new KVM_CHECK_EXTENSION value instead of a prctl.

Paolo



reply via email to

[Prev in Thread] Current Thread [Next in Thread]