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.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
