From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kiszka Subject: Re: [PATCH v3 2/4] KVM: Add unified KVM_GET/SET_VCPU_STATE IOCTL Date: Tue, 10 Nov 2009 14:22:28 +0100 Message-ID: <4AF96914.6010606@siemens.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> <4AF96342.4060205@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: Marcelo Tosatti , "kvm@vger.kernel.org" To: Avi Kivity Return-path: Received: from goliath.siemens.de ([192.35.17.28]:24614 "EHLO goliath.siemens.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750830AbZKJNWU (ORCPT ); Tue, 10 Nov 2009 08:22:20 -0500 In-Reply-To: <4AF96342.4060205@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: 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