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 13:03:29 +0100 Message-ID: <4AF95691.70705@siemens.com> References: <20091102162028.19049.34651.stgit@mchn012c.ww002.siemens.net> <20091102162028.19049.64564.stgit@mchn012c.ww002.siemens.net> <4AF93CFF.5080102@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 thoth.sbs.de ([192.35.17.2]:18088 "EHLO thoth.sbs.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750923AbZKJMDW (ORCPT ); Tue, 10 Nov 2009 07:03:22 -0500 In-Reply-To: <4AF93CFF.5080102@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: 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