Re: [PATCH 14/15] KVM: Add support for enabling capabilities per-vcpu

[Alexander Graf <agraf@suse.de>]

Avi Kivity wrote:
> On 03/08/2010 03:56 PM, Alexander Graf wrote:
>> Avi Kivity wrote:
>>   
>>> On 03/08/2010 03:51 PM, Alexander Graf wrote:
>>>     
>>>> Avi Kivity wrote:
>>>>
>>>>       
>>>>> On 03/05/2010 06:50 PM, Alexander Graf wrote:
>>>>>
>>>>>         
>>>>>>         }
>>>>>> diff --git a/include/linux/kvm.h b/include/linux/kvm.h
>>>>>> index ce28767..c7ed3cb 100644
>>>>>> --- a/include/linux/kvm.h
>>>>>> +++ b/include/linux/kvm.h
>>>>>> @@ -400,6 +400,12 @@ struct kvm_ioeventfd {
>>>>>>         __u8  pad[36];
>>>>>>     };
>>>>>>
>>>>>> +/* for KVM_ENABLE_CAP */
>>>>>> +struct kvm_enable_cap {
>>>>>> +    /* in */
>>>>>> +    __u32 cap;
>>>>>>
>>>>>>
>>>>>>            
>>>>> Reserve space here.  Add a flags field and check it for zeros.
>>>>>
>>>>>          
>>>> Flags? How about something like
>>>>
>>>> u64 args[4]
>>>>
>>>> That way the capability enabling code could decide what to do with the
>>>> arguments. We don't always only need flags I suppose?.
>>>>
>>>>        
>>> If you interpret these as bit flags anyway, that would be redundant.
>>>
>>>      
>> I think I just don't understand what you're trying to say with "flags".
>> For the OSI enabling we don't need any flags. For later additions we
>> don't know what we'll need.
>>    
>
> When we have reserved fields which are later used for something new,
> the kernel needs a way to know if the reserved fields are known or not
> by userspace.  One way to do this is to assume a value of zero means
> the field is unknown to usespace so ignore it.  Another is to require
> userspace to set a bit in an already-known flags field, and only act
> on the new field if its bit was set.  This has the advantage that the
> old kernel checks for unknown flags and errors out, improving forwards
> and backwards compatibility.
>
> I thought ->cap was already a bit field, so this isn't necessary, but
> if it isn't, then a flags field is helpful.

-> cap is the capability number. So you want something like:

struct kvm_enable_cap {
  __u32 cap;
  __u32 flags;
  __u64 args[4];
  __u8 pad[64];
};

And then check for flags == 0 in the ioctl handler? Flags could later on
define if the padding changed to a different position, adding new fields
in between args and pad?


Alex