On Wednesday, January 12, 2022 10:51 AM, Zeng, Guang wrote:
> To: Tian, Kevin <kevin.t...@intel.com>; Zhong, Yang <yang.zh...@intel.com>;
> qemu-devel@nongnu.org
> Cc: pbonz...@redhat.com; Christopherson,, Sean <sea...@google.com>;
> jing2....@linux.intel.com; Wang, Wei W <wei.w.w...@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.z...@intel.com>
> >> Sent: Monday, January 10, 2022 5:47 PM
> >>
> >> On 1/10/2022 4:40 PM, Tian, Kevin wrote:
> >>>> From: Zhong, Yang <yang.zh...@intel.com>
> >>>> Sent: Friday, January 7, 2022 5:32 PM
> >>>>
> >>>> From: Jing Liu <jing2....@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;
}
