Re: [PATCH] KVM: x86: XSAVE/XRSTOR live migration support

[Avi Kivity <avi@redhat.com>]

On 05/27/2010 01:33 PM, Sheng Yang wrote:
>
>> If we do that then xcr0 needs to be in a separate structure, say
>> kvm_xcr, with a flags field and reserved space of its own for future xcr
>> growth.
>>      
> I meant to put it into sregs, but found it's already full... How about "extended
> sregs"?
>    

Isn't this what xcr means?  xtended control register?

>>> +static void kvm_vcpu_ioctl_x86_get_xsave(struct kvm_vcpu *vcpu,
>>> +					struct kvm_xsave *guest_xsave)
>>> +{
>>> +	struct xsave_struct *xsave =&vcpu->arch.guest_fpu.state->xsave;
>>> +
>>> +	if (!cpu_has_xsave)
>>> +		return;
>>>        
>> Hm, it would be nice to make it backward compatible and return the
>> legacy fpu instead.  I think the layouts are compatible?
>>      
> Sound good.  But seems we still need KVM_CAP_XSAVE to use this interface, and
> other processors would still go FPU interface. Seems didn't improve much?
>    

I would like the new interface to be used in all cases, this way we can 
deprecate the old one in a few years.

>>> diff --git a/include/linux/kvm.h b/include/linux/kvm.h
>>> index 23ea022..5006761 100644
>>> --- a/include/linux/kvm.h
>>> +++ b/include/linux/kvm.h
>>> @@ -524,6 +524,9 @@ struct kvm_enable_cap {
>>>
>>>    #define KVM_CAP_PPC_OSI 52
>>>    #define KVM_CAP_PPC_UNSET_IRQ 53
>>>    #define KVM_CAP_ENABLE_CAP 54
>>>
>>> +#ifdef __KVM_HAVE_XSAVE
>>> +#define KVM_CAP_XSAVE 55
>>> +#endif
>>>        
>> Might make sense to have a separate KVM_CAP_XCR, just for consistency.
>>      
> Maybe EXTENDED_SREGS? But still every future field in the struct need a CAP...
>    

Might do

struct kvm_xcr {
     __u32 xcr;
     __u32 reserved;
     __u64 value;
};

struct kvm_xcrs {
     __u32 nr_xcrs;
     __u32 flags;
     struct kvm_xcr xcrs[KVM_MAX_XCRS];
     ... reserved;
};

which would allow new xcrs to be added easily.

You'll need to change kvm_set_xcr0() to kvm_set_xcr() for this to work 
though.

-- 
error compiling committee.c: too many arguments to function