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

[Scott Wood <scottwood@freescale.com>]

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