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/02/2009 06:20 PM, Jan Kiszka wrote:
>> Add a new IOCTL pair to retrieve or set the VCPU state in one chunk.
>> More precisely, the IOCTL is able to process a list of substates to be
>> read or written. This list is easily extensible without breaking the
>> existing ABI, thus we will no longer have to add new IOCTLs when we
>> discover a missing VCPU state field or want to support new hardware
>> features.
>>    
> 
> 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 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.

>      },
> };
> 
> (btw, the new stuff would also benefit from this).

Right.

> 
> So, what's the real justification for the new ABI?

The remaining differences are:
 - single kernel call possible
 - slightly higher regularity (the IOCTL space is rather chaotic)

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

Jan

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