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

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.

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 */
     },
};

(btw, the new stuff would also benefit from this).

So, what's the real justification for the new ABI?

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.

-- 
error compiling committee.c: too many arguments to function