RE: [RFC PATCH 6/7] x86: Use new XSAVE ioctls handling

[Wang, Wei W]

On Wednesday, January 12, 2022 10:51 AM, Zeng, Guang wrote:
> To: Tian, Kevin <kevin.tian@intel.com>; Zhong, Yang <yang.zhong@intel.com>;
> qemu-devel@nongnu.org
> Cc: pbonzini@redhat.com; Christopherson,, Sean <seanjc@google.com>;
> jing2.liu@linux.intel.com; Wang, Wei W <wei.w.wang@intel.com>
> Subject: Re: [RFC PATCH 6/7] x86: Use new XSAVE ioctls handling
> 
> On 1/11/2022 10:30 AM, Tian, Kevin wrote:
> >> From: Zeng, Guang <guang.zeng@intel.com>
> >> Sent: Monday, January 10, 2022 5:47 PM
> >>
> >> On 1/10/2022 4:40 PM, Tian, Kevin wrote:
> >>>> From: Zhong, Yang <yang.zhong@intel.com>
> >>>> Sent: Friday, January 7, 2022 5:32 PM
> >>>>
> >>>> From: Jing Liu <jing2.liu@intel.com>
> >>>>
> >>>> Extended feature has large state while current kvm_xsave only
> >>>> allows 4KB. Use new XSAVE ioctls if the xstate size is large than
> >>>> kvm_xsave.
> >>> shouldn't we always use the new xsave ioctls as long as
> >>> CAP_XSAVE2 is available?
> >>
> >> CAP_XSAVE2 may return legacy xsave size or 0 working with old kvm
> >> version in which it's not available.
> >> QEMU just use the new xsave ioctls only when the return value of
> >> CAP_XSAVE2 is larger than legacy xsave size.
> > CAP_XSAVE2  is the superset of CAP_XSAVE. If available it can support
> > both legacy 4K size or bigger.
> 
> Got your point now. We can use new ioctl once CAP_XSAVE2 is available.
> As your suggestion, I'd like to change commit log as follows:
> 
> "x86: Use new XSAVE ioctls handling
> 
>    Extended feature has large state while current
>    kvm_xsave only allows 4KB. Use new XSAVE ioctls
>    if check extension of CAP_XSAVE2 is available."
> 
> And introduce has_xsave2 to indicate the valid of CAP_XSAVE2 with following
> change:
> 
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c index
> 97520e9dff..c8dae88ced 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -116,6 +116,7 @@ static bool has_msr_ucode_rev;
>   static bool has_msr_vmx_procbased_ctls2;
>   static bool has_msr_perf_capabs;
>   static bool has_msr_pkrs;
> +static bool has_xsave2 = false;

It's 0-initialized, so I think no need for the "false" assignment.
Probably better to use "int" (like has_xsave), and improved it a bit:

diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 3fb3ddbe2b..dee40ad0ad 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -122,6 +122,7 @@ static uint32_t num_architectural_pmu_gp_counters;
 static uint32_t num_architectural_pmu_fixed_counters;

 static int has_xsave;
+static int has_xsave2;
 static int has_xcrs;
 static int has_pit_state2;
 static int has_exception_payload;
@@ -1564,6 +1565,26 @@ static Error *invtsc_mig_blocker;

 #define KVM_MAX_CPUID_ENTRIES  100

+static void kvm_init_xsave(CPUX86State *env)
+{
+    if (has_xsave2) {
+        env->xsave_buf_len = QEMU_ALIGN_UP(has_xsave2, 4096);;
+    } else if (has_xsave) {
+        env->xsave_buf_len = sizeof(struct kvm_xsave);
+    } else {
+        return;
+    }
+
+    env->xsave_buf = qemu_memalign(4096, env->xsave_buf_len);
+    memset(env->xsave_buf, 0, env->xsave_buf_len);
+     /*
+      * The allocated storage must be large enough for all of the
+      * possible XSAVE state components.
+      */
+    assert(kvm_arch_get_supported_cpuid(kvm_state, 0xd, 0, R_ECX) <=
+           env->xsave_buf_len);
+}
+
 int kvm_arch_init_vcpu(CPUState *cs)
 {
     struct {
@@ -1982,18 +2003,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
         goto fail;
     }

-    if (has_xsave) {
-        env->xsave_buf_len = sizeof(struct kvm_xsave);
-        env->xsave_buf = qemu_memalign(4096, env->xsave_buf_len);
-        memset(env->xsave_buf, 0, env->xsave_buf_len);
-
-        /*
-         * The allocated storage must be large enough for all of the
-         * possible XSAVE state components.
-         */
-        assert(kvm_arch_get_supported_cpuid(kvm_state, 0xd, 0, R_ECX)
-               <= env->xsave_buf_len);
-    }
+    kvm_init_xsave(env);

     max_nested_state_len = kvm_max_nested_state_length();
     if (max_nested_state_len > 0) {
@@ -2323,6 +2333,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
     }

     has_xsave = kvm_check_extension(s, KVM_CAP_XSAVE);
+    has_xsave2 = kvm_check_extension(s, KVM_CAP_XSAVE2);
     has_xcrs = kvm_check_extension(s, KVM_CAP_XCRS);
     has_pit_state2 = kvm_check_extension(s, KVM_CAP_PIT_STATE2);

@@ -3241,13 +3252,14 @@ static int kvm_get_xsave(X86CPU *cpu)
 {
     CPUX86State *env = &cpu->env;
     void *xsave = env->xsave_buf;
-    int ret;
+    int type, ret;

     if (!has_xsave) {
         return kvm_get_fpu(cpu);
     }

-    ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_XSAVE, xsave);
+    type = has_xsave2 ? KVM_GET_XSAVE2: KVM_GET_XSAVE;
+    ret = kvm_vcpu_ioctl(CPU(cpu), type, xsave);
     if (ret < 0) {
         return ret;
     }