Re: [PATCH v3 2/4] KVM: Add unified KVM_GET/SET_VCPU_STATE IOCTL

[Avi Kivity <avi@redhat.com>]

On 11/10/2009 02:03 PM, Jan Kiszka wrote:
>
>> I'm having some second thoughts about this.
>>
>> What does the new API buy us?  Instead of declaring two new ioctls for
>> new/fixed substates, we only have to declare one.  We still have the
>> capability check.  We still have to declare a structure.
>>      
> Right, we still need CAPs to protect us against undefined types. So
> KVM_CHECK_VCPU_STATES is actually pointless - well, someone asked for it...
>    

It's not pointless - you can do a compile-time check for 
KVM_VCPU_STATE_... and a runtime check using KVM_CHECK_VCPU_STATES.  But 
it does duplicate the existing KVM_CAP_ functionality.

>> It's true that the internals are currently a mess.  We can fix that with
>> a table-driven approach:
>>
>> static struct kvm_state_ioctl state_ioctls[] = {
>>      {
>>           .get_ioctl = KVM_GET_FPU,
>>           .set_ioctl = KVM_SET_FPU,
>>           .get = kvm_get_fpu,
>>           .set = kvm_set_fpu,
>>           .size = sizeof(struct kvm_fpu), /* 0 for variable-size state */
>>      
> Even a variable-sized state has a fixed-size header. The handlers would
> have to deal with this, or we would need to define which field in the
> header holds the extension size, and what is its multiplier.
>    

Since we have very few variable-size states, and their number is 
unlikely to increase, ad-hoc handling should be sufficient.

>> So, what's the real justification for the new ABI?
>>      
> The remaining differences are:
>   - single kernel call possible
>    

Is there a real advantage in this?  It's not a high performance call, 
typically only called during save/restore, reset, and for vmware's 
wonderful ioport interface.

>   - slightly higher regularity (the IOCTL space is rather chaotic)
>    

But still, actually handling the state is not regular either on the 
userspace or kernel side.

>> Jan, my apologies for raising this at such a very late stage in the
>> review, after all the nits have been satisfactorily addressed.  But I
>> want to make sure we don't bloat the interface without very good reasons.
>>      
> I think we came from the idea: "Let's have one new IOCTL that will fit
> it all - now and then." That's obviously not cheaply achievable. So the
> valid question is what our extension concept of the future should be,
> the existing multi-IOCTL approach or the substates? I only have a slight
> bias towards the latter but the strong wish to achieve to a final decision.
>    

It would have been better to start from substates in the first place, 
since there is less duplication: instead of 2 x NR_STATES ioctls, we 
define 2 ioctls + NR_STATES defines.  It's more regular and less chance 
for errors (like misspelling _IOR/_IOW).

But given that we already do have the old interface, perhaps it's best 
to stick with it and concentrate on improving the internals.

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