From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH v3 2/4] KVM: Add unified KVM_GET/SET_VCPU_STATE IOCTL Date: Tue, 10 Nov 2009 14:57:38 +0200 Message-ID: <4AF96342.4060205@redhat.com> References: <20091102162028.19049.34651.stgit@mchn012c.ww002.siemens.net> <20091102162028.19049.64564.stgit@mchn012c.ww002.siemens.net> <4AF93CFF.5080102@redhat.com> <4AF95691.70705@siemens.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Cc: Marcelo Tosatti , "kvm@vger.kernel.org" To: Jan Kiszka Return-path: Received: from mx1.redhat.com ([209.132.183.28]:19427 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750912AbZKJM5g (ORCPT ); Tue, 10 Nov 2009 07:57:36 -0500 In-Reply-To: <4AF95691.70705@siemens.com> Sender: kvm-owner@vger.kernel.org List-ID: 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