Re: [Qemu-devel] [PATCH uq/master 2/2] KVM: make XSAVE support more robust

[Paolo Bonzini]

Il 08/09/2013 13:52, Gleb Natapov ha scritto:
> On Thu, Sep 05, 2013 at 03:06:22PM +0200, Paolo Bonzini wrote:
>> QEMU moves state from CPUArchState to struct kvm_xsave and back when it
>> invokes the KVM_*_XSAVE ioctls.  Because it doesn't treat the XSAVE
>> region as an opaque blob, it might be impossible to set some state on
>> the destination if migrating to an older version.
>>
>> This patch blocks migration if it finds that unsupported bits are set
>> in the XSTATE_BV header field.  To make this work robustly, QEMU should
>> only report in env->xstate_bv those fields that will actually end up
>> in the migration stream.
> 
> We usually handle host cpu differences in cpuid layer, not by trying to
> validate migration data.

Actually we do both.  QEMU for example detects invalid subsections and
blocks migration, and CPU differences also result in subsections that
the destination does not know.

But as far as QEMU is concerned, setting an unknown bit in XSTATE_BV is
not a CPU difference, it is simply invalid migration data.

> i.e CPUID.0D should be configurable and
> management should be able to query QEMU what is supported and prevent
> migration attempt accordingly.

Management is already able to query QEMU of what is supported, because
new XSAVE state is always attached to new CPUID bits in leaves other
than 0Dh (e.g. EAX=07h, ECX=0h returns AVX512 and MPX support in EBX).
QEMU should compute 0Dh data based on those bits indeed.

However, KVM_GET/SET_XSAVE should still return all values supported by
the hypervisor, independent of the supported CPUID bits.

>>
>> Signed-off-by: Paolo Bonzini <address@hidden>
>> ---
>>  target-i386/kvm.c     | 3 ++-
>>  target-i386/machine.c | 4 ++++
>>  2 files changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
>> index 749aa09..df08a4b 100644
>> --- a/target-i386/kvm.c
>> +++ b/target-i386/kvm.c
>> @@ -1291,7 +1291,8 @@ static int kvm_get_xsave(X86CPU *cpu)
>>              sizeof env->fpregs);
>>      memcpy(env->xmm_regs, &xsave->region[XSAVE_XMM_SPACE],
>>              sizeof env->xmm_regs);
>> -    env->xstate_bv = *(uint64_t *)&xsave->region[XSAVE_XSTATE_BV];
>> +    env->xstate_bv = *(uint64_t *)&xsave->region[XSAVE_XSTATE_BV] &
>> +            XSTATE_SUPPORTED;
> Don't we just drop state here that will not be restored on the
> destination and destination will not be able to tell since we masked
> unsupported bits?

A well-behaved guest should not have modified that state anyway, since:

* the source and destination machines should have the same CPU

* since the destination QEMU does not support the feature, the source
should have masked it as well

* the guest should always probe CPUID before using a feature

There will be only one change for well-behaved guests with this patch
(and the change will be invisible to them since they behave well).
After the patch, KVM_SET_XSAVE will set the extended states to the
processor-reset state instead of all-zeros.  However, all
currently-defined states have a processor-reset state that is equal to
all-zeroes, so this change is theoretical.

In fact, perhaps even XSTATE_SUPPORTED is not restrictive enough here,
and we should hide all features that are not visible in CPUID.  It is
okay, however, to test it in cpu_post_load.

Paolo

>>      memcpy(env->ymmh_regs, &xsave->region[XSAVE_YMMH_SPACE],
>>              sizeof env->ymmh_regs);
>>      return 0;
>> diff --git a/target-i386/machine.c b/target-i386/machine.c
>> index dc81cde..9e2cfcf 100644
>> --- a/target-i386/machine.c
>> +++ b/target-i386/machine.c
>> @@ -278,6 +278,10 @@ static int cpu_post_load(void *opaque, int version_id)
>>      CPUX86State *env = &cpu->env;
>>      int i;
>>  
>> +    if (env->xstate_bv & ~XSTATE_SUPPORTED) {
>> +        return -EINVAL;
>> +    }
>> + 
>>      /*
>>       * Real mode guest segments register DPL should be zero.
>>       * Older KVM version were setting it wrongly.
>> -- 
>> 1.8.3.1
> 
> --
>                       Gleb.
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to address@hidden
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>