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

[Gleb Natapov <gleb@redhat.com>]

On Mon, Feb 18, 2013 at 09:50:44PM -0800, Christoffer Dall wrote:
> >> > +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?
> >
> 
> The fact that you don't have to create static inlines for the
> architectures that don't define the functions that get called or have
> to similar #ifdef tricks, and I also think it's easier to read the
> arch-specific bits of the code that way, instead of some arbitrary
> function that you have to trace through to figure out where it's
> called from.
> 
> > 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).
> 
> I would say that's exactly what you're going to need with your approach:
> 
> switch (cd->type) {
> case KVM_ARM_VGIC_V2_0:
>     kvm_arm_vgic_v2_0_create(...);
> }
> 
> 
> are you going to ifdef here in this function, or? I think it's cleaner
> to have the single arch-specific hooks and handle the cases there.
> 
That is exactly what last patch is doing:
+#ifdef CONFIG_KVM_MPIC
+       case KVM_DEV_TYPE_FSL_MPIC_20:
+       case KVM_DEV_TYPE_FSL_MPIC_42: {
+               if (test) {
+                       r = 0;
+                       break;
+               }
+
+               r = kvm_create_mpic(kvm, cd->type, &dev);
+               break;
+       }
+#endif

> The use case of having a single device which is so central to the
> system that we emulate it inside the kernel and is shared across
> multiple archs is pretty far fetched to me.
> 
There is (or should I say was) one such device: IOAPIC. It is shared
between ia64 and x86.

Unless we have device that is shared between all/some arches I am with
Christoffer on this one. If such device will appear we ca do:

kvm_ioctl_create_device()
{
	switch (cd->type) {
         case DEVICEA_SHARED_BY_ALL_ARCHS:
                r = createa()
         break;
         case DEVICEB_SHARED_BY_ALL_ARCHS:
	        r = createb()
	 break;
         default:
           r = kvm_ioctl_arch_create_device();
       }
}

--
			Gleb.