From mboxrd@z Thu Jan 1 00:00:00 1970 From: Scott Wood Subject: Re: [RFC PATCH 1/6] kvm: add device control API Date: Mon, 18 Feb 2013 18:53:43 -0600 Message-ID: <1361235223.25178.5@snotra> References: Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; delsp=Yes; format=Flowed Content-Transfer-Encoding: 8BIT Cc: Alexander Graf , , To: Christoffer Dall Return-path: In-Reply-To: (from cdall@cs.columbia.edu on Mon Feb 18 18:44:20 2013) Content-Disposition: inline Sender: kvm-ppc-owner@vger.kernel.org List-Id: kvm.vger.kernel.org On 02/18/2013 06:44:20 PM, Christoffer Dall wrote: > On Wed, Feb 13, 2013 at 9:49 PM, Scott Wood > wrote: > > index 0350e0d..dbaf012 100644 > > --- a/include/linux/kvm_host.h > > +++ b/include/linux/kvm_host.h > > @@ -335,6 +335,25 @@ struct kvm_memslots { > > short id_to_index[KVM_MEM_SLOTS_NUM]; > > }; > > > > +/* > > + * The worst case number of simultaneous devices will likely be > very low > > + * (usually zero or one) for the forseeable future. If the worst > case > > + * exceeds this, then it can be increased, or we can convert to > idr. > > + */ > > This comment is on the heavy side (if at all needed). If you want to > remind people of idr, just put that in a single line. A define is a > define is a define. OK. > > +#define KVM_CREATE_DEVICE _IOWR(KVMIO, 0xac, struct > kvm_create_device) > > +#define KVM_SET_DEVICE_ATTR _IOW(KVMIO, 0xad, struct > kvm_device_attr) > > +#define KVM_GET_DEVICE_ATTR _IOW(KVMIO, 0xae, struct > kvm_device_attr) > > _IOWR ? struct kvm_device_attr itself is write-only, though the data pointed to by the addr field goes the other way for GET. ONE_REG is in the same situation and also uses _IOW for both. > > +static int kvm_ioctl_create_device(struct kvm *kvm, > > + struct kvm_create_device *cd) > > +{ > > + struct kvm_device *dev = NULL; > > + bool test = cd->flags & KVM_CREATE_DEVICE_TEST; > > + int id; > > + int r; > > + > > + mutex_lock(&kvm->lock); > > + > > + id = kvm->num_devices; > > + if (id >= KVM_MAX_DEVICES && !test) { > > + r = -ENOSPC; > > + goto out; > > + } > > + > > + switch (cd->type) { > > + default: > > + r = -ENODEV; > > + goto out; > > + } > > do we really believe that there will be any arch-generic recognition > of types? shouldn't this be a call to an arch-specific function > instead. Which makes me wonder whether the device type IDs should be > arch specific as well... I prefer to look at it from the other direction -- is there any reason why this *should* be architecture specific? What will that make easier? By doing device recognition here we don't need a separate copy of this per arch (including some #ifdef or modifying every arch at once -- including ARM which I can't modify yet because it's not merged), and *if* we should end up with an in-kernel-emulated device that gets used across multiple architectures, it would be annoying to have to modify all relevant architectures (and worse to deal with per-arch numberspaces). -Scott