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

[Jan Kiszka <jan.kiszka@siemens.com>]

Avi Kivity wrote:
> 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 redundant, therefore I considered it pointless.

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

Regarding CPU states, there is actually only the MSR interface.

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

And debugging. But, true, this is all fairly uncritical.

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

So the new roadmap shall be like this:

 o Add KVM_X86_GET/SET_EVENT_STATES (instead of
   KVM_X86_VCPU_STATE_EVENTS)

 o Refactor in-kernel VCPU state IOCTLs to use table-driven dispatching
   and shared argument passing

 o Maybe refactor user space as well towards a table-driven state sync
   (need to think about this a bit more)

Any other comments or does everyone agree?

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux