Re: [PATCH v2 6/8] x86: add support for KVM_CAP_XSAVE2 and AMX state migration

[David Edmondson]

On Wednesday, 2022-02-16 at 22:04:32 -08, Yang Zhong wrote:

> From: Jing Liu <jing2.liu@intel.com>
>
> When dynamic xfeatures (e.g. AMX) are used by the guest, the xsave
> area would be larger than 4KB. KVM_GET_XSAVE2 and KVM_SET_XSAVE
> under KVM_CAP_XSAVE2 works with a xsave buffer larger than 4KB.
> Always use the new ioctls under KVM_CAP_XSAVE2 when KVM supports it.
>
> Signed-off-by: Jing Liu <jing2.liu@intel.com>
> Signed-off-by: Zeng Guang <guang.zeng@intel.com>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> Signed-off-by: Yang Zhong <yang.zhong@intel.com>
> ---
>  target/i386/cpu.h          |  4 ++++
>  target/i386/kvm/kvm.c      | 42 ++++++++++++++++++++++++--------------
>  target/i386/xsave_helper.c | 33 ++++++++++++++++++++++++++++++
>  3 files changed, 64 insertions(+), 15 deletions(-)
>
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index f7fc2e97a6..de9da38e42 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -1528,6 +1528,10 @@ typedef struct CPUX86State {
>      uint64_t opmask_regs[NB_OPMASK_REGS];
>      YMMReg zmmh_regs[CPU_NB_REGS];
>      ZMMReg hi16_zmm_regs[CPU_NB_REGS];
> +#ifdef TARGET_X86_64
> +    uint8_t xtilecfg[64];
> +    uint8_t xtiledata[8192];
> +#endif

Can we have defined constants for these sizes? They also appear in patch
2.

>
>      /* sysenter registers */
>      uint32_t sysenter_cs;
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index 8562d3d138..ff064e3d8f 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_sregs2;
> @@ -1585,6 +1586,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);

Idle curiosity - why do we round this up?

> +    } 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 {
> @@ -1614,6 +1635,8 @@ int kvm_arch_init_vcpu(CPUState *cs)
>
>      cpuid_i = 0;
>
> +    has_xsave2 = kvm_check_extension(cs->kvm_state, KVM_CAP_XSAVE2);
> +
>      r = kvm_arch_set_tsc_khz(cs);
>      if (r < 0) {
>          return r;
> @@ -2003,19 +2026,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
>      if (r) {
>          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) {
> @@ -3319,13 +3330,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;
>      }
> diff --git a/target/i386/xsave_helper.c b/target/i386/xsave_helper.c
> index ac61a96344..b6a004505f 100644
> --- a/target/i386/xsave_helper.c
> +++ b/target/i386/xsave_helper.c
> @@ -5,6 +5,7 @@
>  #include "qemu/osdep.h"
>
>  #include "cpu.h"
> +#include <asm/kvm.h>
>
>  void x86_cpu_xsave_all_areas(X86CPU *cpu, void *buf, uint32_t buflen)
>  {
> @@ -126,6 +127,22 @@ void x86_cpu_xsave_all_areas(X86CPU *cpu, void *buf, 
> uint32_t buflen)
>
>          memcpy(pkru, &env->pkru, sizeof(env->pkru));
>      }
> +
> +    e = &x86_ext_save_areas[XSTATE_XTILE_CFG_BIT];
> +    if (e->size && e->offset) {
> +        XSaveXTILECFG *tilecfg = buf + e->offset;
> +
> +        memcpy(tilecfg, &env->xtilecfg, sizeof(env->xtilecfg));
> +    }
> +
> +    if (buflen > sizeof(struct kvm_xsave)) {
> +        e = &x86_ext_save_areas[XSTATE_XTILE_DATA_BIT];
> +        if (e->size && e->offset && buflen >= e->size + e->offset) {
> +            XSaveXTILEDATA *tiledata = buf + e->offset;
> +
> +            memcpy(tiledata, &env->xtiledata, sizeof(env->xtiledata));
> +        }
> +    }
>  #endif
>  }
>
> @@ -247,5 +264,21 @@ void x86_cpu_xrstor_all_areas(X86CPU *cpu, const void 
> *buf, uint32_t buflen)
>          pkru = buf + e->offset;
>          memcpy(&env->pkru, pkru, sizeof(env->pkru));
>      }
> +
> +    e = &x86_ext_save_areas[XSTATE_XTILE_CFG_BIT];
> +    if (e->size && e->offset) {
> +        const XSaveXTILECFG *tilecfg = buf + e->offset;
> +
> +        memcpy(&env->xtilecfg, tilecfg, sizeof(env->xtilecfg));
> +    }
> +
> +    if (buflen > sizeof(struct kvm_xsave)) {
> +        e = &x86_ext_save_areas[XSTATE_XTILE_DATA_BIT];
> +        if (e->size && e->offset && buflen >= e->size + e->offset) {
> +            const XSaveXTILEDATA *tiledata = buf + e->offset;
> +
> +            memcpy(&env->xtiledata, tiledata, sizeof(env->xtiledata));
> +        }
> +    }
>  #endif
>  }

dme.
-- 
Why does it have to be like this? I can never tell.