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

[Paolo Bonzini]

Il 09/09/2013 11:18, Gleb Natapov ha scritto:
> On Mon, Sep 09, 2013 at 10:51:58AM +0200, Paolo Bonzini wrote:
>> 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.
>>
> That's different from what you do here though. If xstate_bv was in its
> separate subsection things would be easier, but it is not.

I agree.  And also if YMM was in its separate subsections; future XSAVE
states will likely use subsections (whose presence is keyed off bits in
env->xstate_bv).

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

Because this is not talking to the guest, it is talking to userspace.

The VCPU state is more than what is visible to the guest, and returning
all of it seems more consistent with the rest of the KVM API.  For
example, KVM_GET_FPU always returns SSE state even if the CPUID lacks
SSE and/or FXSR.

>> 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
>>
> The I fail to see what is the purpose of the patch. I see two cases:
> 1. Each extended state has separate CPUID bit (is this guarantied?)

Not guaranteed, but it has always happened so far (AVX, AVX-512, MPX).

>   - In this case, as you say, by matching CPUID on src and dst we guaranty
>     that migration data is good.

But we don't match CPUID on src and destination.  This is something that
the user should do, but it's better if we can test it too.  Subsections
do that for us; I am, in some sense, emulating subsections for the XSAVE
states that are not stored in subsections.

>> 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.
> 
> The kernel should not even return state that is not visible in CPUID.

That's an interesting point of view that I hadn't considered.  But just
like you asked me why it should return state that is not visible in
CPUID, I'm asking you why it should not...

Paolo

>>
>> 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
>>>
> 
> --
>                       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
>