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