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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
