Re: [RFC PATCH 1/6] kvm: add device control API

Scott Wood
2013-02-19T00:53:43Z

On 02/18/2013 06:44:20 PM, Christoffer Dall wrote:
> On Wed, Feb 13, 2013 at 9:49 PM, Scott Wood <scottwood@freescale.com>
> 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