[PATCH 1/2] KVM: device: add simple registration mechanism for kvm_device_ops

[PATCH 1/2] KVM: device: add simple registration mechanism for kvm_device_ops

[Will Deacon]

kvm_ioctl_create_device currently has knowledge of all the device types
and their associated ops. This is fairly inflexible when adding support
for new in-kernel device emulations, so move what we currently have out
into a table, which can support dynamic registration of ops by new
drivers for virtual hardware.

I didn't try to port all current drivers over, as it's not always clear
which initialisation hook the ops should be registered from.

Cc: Gleb Natapov <gleb@kernel.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Christoffer Dall <christoffer.dall@linaro.org>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---

Hi guys,

I've just started writing a virtual IOMMU for the ARM SMMU and figured a
registration mechanism for kvm_device_ops would be a nice cleanup for
that.

Will

 include/linux/kvm_host.h |  1 +
 include/uapi/linux/kvm.h |  1 +
 virt/kvm/kvm_main.c      | 65 ++++++++++++++++++++++++++++--------------------
 3 files changed, 40 insertions(+), 27 deletions(-)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index ec4e3bd83d47..b75faaf0d76d 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -1083,6 +1083,7 @@ struct kvm_device_ops {
 void kvm_device_get(struct kvm_device *dev);
 void kvm_device_put(struct kvm_device *dev);
 struct kvm_device *kvm_device_from_filp(struct file *filp);
+int kvm_register_device_ops(struct kvm_device_ops *ops, u32 type);
 
 extern struct kvm_device_ops kvm_mpic_ops;
 extern struct kvm_device_ops kvm_xics_ops;
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index e11d8f170a62..3b368166286f 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -949,6 +949,7 @@ struct kvm_device_attr {
 #define   KVM_DEV_VFIO_GROUP_DEL			2
 #define KVM_DEV_TYPE_ARM_VGIC_V2	5
 #define KVM_DEV_TYPE_FLIC		6
+#define KVM_DEV_TYPE_MAX		7
 
 /*
  * ioctls for VM fds
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 4b6c01b477f9..a0c6cc31a1f6 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2257,44 +2257,55 @@ struct kvm_device *kvm_device_from_filp(struct file *filp)
 	return filp->private_data;
 }
 
-static int kvm_ioctl_create_device(struct kvm *kvm,
-				   struct kvm_create_device *cd)
-{
-	struct kvm_device_ops *ops = NULL;
-	struct kvm_device *dev;
-	bool test = cd->flags & KVM_CREATE_DEVICE_TEST;
-	int ret;
-
-	switch (cd->type) {
+static struct kvm_device_ops *kvm_device_ops_table[KVM_DEV_TYPE_MAX] = {
 #ifdef CONFIG_KVM_MPIC
-	case KVM_DEV_TYPE_FSL_MPIC_20:
-	case KVM_DEV_TYPE_FSL_MPIC_42:
-		ops = &kvm_mpic_ops;
-		break;
+	[KVM_DEV_TYPE_FSL_MPIC_20]	= &kvm_mpic_ops,
+	[KVM_DEV_TYPE_FSL_MPIC_42]	= &kvm_mpic_ops,
 #endif
+
 #ifdef CONFIG_KVM_XICS
-	case KVM_DEV_TYPE_XICS:
-		ops = &kvm_xics_ops;
-		break;
+	[KVM_DEV_TYPE_XICS]		= &kvm_xics_ops,
 #endif
+
 #ifdef CONFIG_KVM_VFIO
-	case KVM_DEV_TYPE_VFIO:
-		ops = &kvm_vfio_ops;
-		break;
+	[KVM_DEV_TYPE_VFIO]		= &kvm_vfio_ops,
 #endif
+
 #ifdef CONFIG_KVM_ARM_VGIC
-	case KVM_DEV_TYPE_ARM_VGIC_V2:
-		ops = &kvm_arm_vgic_v2_ops;
-		break;
+	[KVM_DEV_TYPE_ARM_VGIC_V2]	= &kvm_arm_vgic_v2_ops,
 #endif
+
 #ifdef CONFIG_S390
-	case KVM_DEV_TYPE_FLIC:
-		ops = &kvm_flic_ops;
-		break;
+	[KVM_DEV_TYPE_FLIC]		= &kvm_flic_ops;
 #endif
-	default:
+};
+
+int kvm_register_device_ops(struct kvm_device_ops *ops, u32 type)
+{
+	if (type >= KVM_DEV_TYPE_MAX)
+		return -ENOSPC;
+
+	if (kvm_device_ops_table[type] != NULL)
+		return -EEXIST;
+
+	kvm_device_ops_table[type] = ops;
+	return 0;
+}
+
+static int kvm_ioctl_create_device(struct kvm *kvm,
+				   struct kvm_create_device *cd)
+{
+	struct kvm_device_ops *ops = NULL;
+	struct kvm_device *dev;
+	bool test = cd->flags & KVM_CREATE_DEVICE_TEST;
+	int ret;
+
+	if (cd->type >= KVM_DEV_TYPE_MAX)
+		return -ENODEV;
+
+	ops = kvm_device_ops_table[cd->type];
+	if (ops == NULL)
 		return -ENODEV;
-	}
 
 	if (test)
 		return 0;
-- 
2.0.0

[PATCH 2/2] KVM: ARM: vgic: register kvm_device_ops dynamically

[Will Deacon]

Now that we have a dynamic means to register kvm_device_ops, use that
for the ARM VGIC, instead of relying on the static table.

Cc: Gleb Natapov <gleb@kernel.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Christoffer Dall <christoffer.dall@linaro.org>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---

Applies against Marc's latest GICv3 patches, so it's probably best if he
takes this once patch 1 has been applied.

 include/linux/kvm_host.h |   1 -
 virt/kvm/arm/vgic.c      | 157 ++++++++++++++++++++++++-----------------------
 virt/kvm/kvm_main.c      |   4 --
 3 files changed, 79 insertions(+), 83 deletions(-)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index b75faaf0d76d..4317fdd10696 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -1088,7 +1088,6 @@ int kvm_register_device_ops(struct kvm_device_ops *ops, u32 type);
 extern struct kvm_device_ops kvm_mpic_ops;
 extern struct kvm_device_ops kvm_xics_ops;
 extern struct kvm_device_ops kvm_vfio_ops;
-extern struct kvm_device_ops kvm_arm_vgic_v2_ops;
 extern struct kvm_device_ops kvm_flic_ops;
 
 #ifdef CONFIG_HAVE_KVM_CPU_RELAX_INTERCEPT
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index 795ab482333d..50c6a74f0e3a 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -1502,83 +1502,6 @@ int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
 	return 0;
 }
 
-static void vgic_init_maintenance_interrupt(void *info)
-{
-	enable_percpu_irq(vgic->maint_irq, 0);
-}
-
-static int vgic_cpu_notify(struct notifier_block *self,
-			   unsigned long action, void *cpu)
-{
-	switch (action) {
-	case CPU_STARTING:
-	case CPU_STARTING_FROZEN:
-		vgic_init_maintenance_interrupt(NULL);
-		break;
-	case CPU_DYING:
-	case CPU_DYING_FROZEN:
-		disable_percpu_irq(vgic->maint_irq);
-		break;
-	}
-
-	return NOTIFY_OK;
-}
-
-static struct notifier_block vgic_cpu_nb = {
-	.notifier_call = vgic_cpu_notify,
-};
-
-static const struct of_device_id vgic_ids[] = {
-	{ .compatible = "arm,cortex-a15-gic", .data = vgic_v2_probe, },
-	{ .compatible = "arm,gic-v3", .data = vgic_v3_probe, },
-	{},
-};
-
-int kvm_vgic_hyp_init(void)
-{
-	const struct of_device_id *matched_id;
-	int (*vgic_probe)(struct device_node *,const struct vgic_ops **,
-			  const struct vgic_params **);
-	struct device_node *vgic_node;
-	int ret;
-
-	vgic_node = of_find_matching_node_and_match(NULL,
-						    vgic_ids, &matched_id);
-	if (!vgic_node) {
-		kvm_err("error: no compatible GIC node found\n");
-		return -ENODEV;
-	}
-
-	vgic_probe = matched_id->data;
-	ret = vgic_probe(vgic_node, &vgic_ops, &vgic);
-	if (ret)
-		return ret;
-
-	ret = request_percpu_irq(vgic->maint_irq, vgic_maintenance_handler,
-				 "vgic", kvm_get_running_vcpus());
-	if (ret) {
-		kvm_err("Cannot register interrupt %d\n", vgic->maint_irq);
-		return ret;
-	}
-
-	ret = __register_cpu_notifier(&vgic_cpu_nb);
-	if (ret) {
-		kvm_err("Cannot register vgic CPU notifier\n");
-		goto out_free_irq;
-	}
-
-	on_each_cpu(vgic_init_maintenance_interrupt, NULL, 1);
-
-	/* Callback into for arch code for setup */
-	vgic_arch_setup(vgic);
-
-	return 0;
-
-out_free_irq:
-	free_percpu_irq(vgic->maint_irq, kvm_get_running_vcpus());
-	return ret;
-}
-
 /**
  * kvm_vgic_init - Initialize global VGIC state before running any VCPUs
  * @kvm: pointer to the kvm struct
@@ -2042,7 +1965,7 @@ static int vgic_create(struct kvm_device *dev, u32 type)
 	return kvm_vgic_create(dev->kvm);
 }
 
-struct kvm_device_ops kvm_arm_vgic_v2_ops = {
+static struct kvm_device_ops kvm_arm_vgic_v2_ops = {
 	.name = "kvm-arm-vgic",
 	.create = vgic_create,
 	.destroy = vgic_destroy,
@@ -2050,3 +1973,81 @@ struct kvm_device_ops kvm_arm_vgic_v2_ops = {
 	.get_attr = vgic_get_attr,
 	.has_attr = vgic_has_attr,
 };
+
+static void vgic_init_maintenance_interrupt(void *info)
+{
+	enable_percpu_irq(vgic->maint_irq, 0);
+}
+
+static int vgic_cpu_notify(struct notifier_block *self,
+			   unsigned long action, void *cpu)
+{
+	switch (action) {
+	case CPU_STARTING:
+	case CPU_STARTING_FROZEN:
+		vgic_init_maintenance_interrupt(NULL);
+		break;
+	case CPU_DYING:
+	case CPU_DYING_FROZEN:
+		disable_percpu_irq(vgic->maint_irq);
+		break;
+	}
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block vgic_cpu_nb = {
+	.notifier_call = vgic_cpu_notify,
+};
+
+static const struct of_device_id vgic_ids[] = {
+	{ .compatible = "arm,cortex-a15-gic", .data = vgic_v2_probe, },
+	{ .compatible = "arm,gic-v3", .data = vgic_v3_probe, },
+	{},
+};
+
+int kvm_vgic_hyp_init(void)
+{
+	const struct of_device_id *matched_id;
+	int (*vgic_probe)(struct device_node *,const struct vgic_ops **,
+			  const struct vgic_params **);
+	struct device_node *vgic_node;
+	int ret;
+
+	vgic_node = of_find_matching_node_and_match(NULL,
+						    vgic_ids, &matched_id);
+	if (!vgic_node) {
+		kvm_err("error: no compatible GIC node found\n");
+		return -ENODEV;
+	}
+
+	vgic_probe = matched_id->data;
+	ret = vgic_probe(vgic_node, &vgic_ops, &vgic);
+	if (ret)
+		return ret;
+
+	ret = request_percpu_irq(vgic->maint_irq, vgic_maintenance_handler,
+				 "vgic", kvm_get_running_vcpus());
+	if (ret) {
+		kvm_err("Cannot register interrupt %d\n", vgic->maint_irq);
+		return ret;
+	}
+
+	ret = __register_cpu_notifier(&vgic_cpu_nb);
+	if (ret) {
+		kvm_err("Cannot register vgic CPU notifier\n");
+		goto out_free_irq;
+	}
+
+	on_each_cpu(vgic_init_maintenance_interrupt, NULL, 1);
+
+	/* Callback into for arch code for setup */
+	vgic_arch_setup(vgic);
+	return kvm_register_device_ops(&kvm_arm_vgic_v2_ops,
+				       KVM_DEV_TYPE_ARM_VGIC_V2);
+
+out_free_irq:
+	free_percpu_irq(vgic->maint_irq, kvm_get_running_vcpus());
+	return ret;
+}
+
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index a0c6cc31a1f6..dccb979bb224 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2271,10 +2271,6 @@ static struct kvm_device_ops *kvm_device_ops_table[KVM_DEV_TYPE_MAX] = {
 	[KVM_DEV_TYPE_VFIO]		= &kvm_vfio_ops,
 #endif
 
-#ifdef CONFIG_KVM_ARM_VGIC
-	[KVM_DEV_TYPE_ARM_VGIC_V2]	= &kvm_arm_vgic_v2_ops,
-#endif
-
 #ifdef CONFIG_S390
 	[KVM_DEV_TYPE_FLIC]		= &kvm_flic_ops;
 #endif
-- 
2.0.0

Re: [PATCH 1/2] KVM: device: add simple registration mechanism for kvm_device_ops

[Paolo Bonzini]

Il 26/06/2014 19:30, Will Deacon ha scritto:
> kvm_ioctl_create_device currently has knowledge of all the device types
> and their associated ops. This is fairly inflexible when adding support
> for new in-kernel device emulations, so move what we currently have out
> into a table, which can support dynamic registration of ops by new
> drivers for virtual hardware.
>
> I didn't try to port all current drivers over, as it's not always clear
> which initialisation hook the ops should be registered from.

Conny, Alex (Graf & Williamson),

can you help Will here?  The idea looks sane, but I'd rather merge it 
with all devices converted.

Paolo

Re: [PATCH 1/2] KVM: device: add simple registration mechanism for kvm_device_ops

[Cornelia Huck]

On Thu, 26 Jun 2014 18:30:16 +0100
Will Deacon <will.deacon@arm.com> wrote:

> kvm_ioctl_create_device currently has knowledge of all the device types
> and their associated ops. This is fairly inflexible when adding support
> for new in-kernel device emulations, so move what we currently have out
> into a table, which can support dynamic registration of ops by new
> drivers for virtual hardware.
> 
> I didn't try to port all current drivers over, as it's not always clear
> which initialisation hook the ops should be registered from.

I like the general idea of registering the ops dynamically, some
comments below.

> 
> Cc: Gleb Natapov <gleb@kernel.org>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Cc: Christoffer Dall <christoffer.dall@linaro.org>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
> 
> Hi guys,
> 
> I've just started writing a virtual IOMMU for the ARM SMMU and figured a
> registration mechanism for kvm_device_ops would be a nice cleanup for
> that.
> 
> Will
> 
>  include/linux/kvm_host.h |  1 +
>  include/uapi/linux/kvm.h |  1 +
>  virt/kvm/kvm_main.c      | 65 ++++++++++++++++++++++++++++--------------------
>  3 files changed, 40 insertions(+), 27 deletions(-)
> 

> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index e11d8f170a62..3b368166286f 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -949,6 +949,7 @@ struct kvm_device_attr {
>  #define   KVM_DEV_VFIO_GROUP_DEL			2
>  #define KVM_DEV_TYPE_ARM_VGIC_V2	5
>  #define KVM_DEV_TYPE_FLIC		6
> +#define KVM_DEV_TYPE_MAX		7

This means we always need to move this value once we introduce a new
kvm device type. Can't you keep it in a dynamic list instead of a
table? We just need to do the lookup during device creation anyway.

>  
>  /*
>   * ioctls for VM fds

> +int kvm_register_device_ops(struct kvm_device_ops *ops, u32 type)
> +{
> +	if (type >= KVM_DEV_TYPE_MAX)
> +		return -ENOSPC;
> +
> +	if (kvm_device_ops_table[type] != NULL)
> +		return -EEXIST;

Checking for type collisions would be a bit more expensive with a list,
but I don't think it matters.

> +
> +	kvm_device_ops_table[type] = ops;
> +	return 0;
> +}

Re: [PATCH 1/2] KVM: device: add simple registration mechanism for kvm_device_ops

[Will Deacon]

Hello,

On Mon, Jun 30, 2014 at 10:21:14AM +0100, Cornelia Huck wrote:
> On Thu, 26 Jun 2014 18:30:16 +0100
> Will Deacon <will.deacon@arm.com> wrote:
> 
> > kvm_ioctl_create_device currently has knowledge of all the device types
> > and their associated ops. This is fairly inflexible when adding support
> > for new in-kernel device emulations, so move what we currently have out
> > into a table, which can support dynamic registration of ops by new
> > drivers for virtual hardware.
> > 
> > I didn't try to port all current drivers over, as it's not always clear
> > which initialisation hook the ops should be registered from.
> 
> I like the general idea of registering the ops dynamically, some
> comments below.

Great, thanks.

> > diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> > index e11d8f170a62..3b368166286f 100644
> > --- a/include/uapi/linux/kvm.h
> > +++ b/include/uapi/linux/kvm.h
> > @@ -949,6 +949,7 @@ struct kvm_device_attr {
> >  #define   KVM_DEV_VFIO_GROUP_DEL			2
> >  #define KVM_DEV_TYPE_ARM_VGIC_V2	5
> >  #define KVM_DEV_TYPE_FLIC		6
> > +#define KVM_DEV_TYPE_MAX		7
> 
> This means we always need to move this value once we introduce a new
> kvm device type. Can't you keep it in a dynamic list instead of a
> table? We just need to do the lookup during device creation anyway.

Well, we do need the fixed IDs in order for userspace to create these
devices via the ioctl. If it's the fixed size you're worried about, the
easiest thing is to replace the array with an idr. I actually started off
with that, but it felt a bit overkill (since we never need dynamic ID
allocation). I can bring it back if you prefer?

At the end of the day, we can't get around the fact that the IDs need to
added with some caution (e.g. not assigning an ID twice).

> > +int kvm_register_device_ops(struct kvm_device_ops *ops, u32 type)
> > +{
> > +	if (type >= KVM_DEV_TYPE_MAX)
> > +		return -ENOSPC;
> > +
> > +	if (kvm_device_ops_table[type] != NULL)
> > +		return -EEXIST;
> 
> Checking for type collisions would be a bit more expensive with a list,
> but I don't think it matters.

I'd be *really* surprised if this was a fast-path in KVM!

Will

Re: [PATCH 1/2] KVM: device: add simple registration mechanism for kvm_device_ops

[Cornelia Huck]

On Mon, 30 Jun 2014 10:36:19 +0100
Will Deacon <will.deacon@arm.com> wrote:

> Hello,
> 
> On Mon, Jun 30, 2014 at 10:21:14AM +0100, Cornelia Huck wrote:
> > On Thu, 26 Jun 2014 18:30:16 +0100
> > Will Deacon <will.deacon@arm.com> wrote:

> > > diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> > > index e11d8f170a62..3b368166286f 100644
> > > --- a/include/uapi/linux/kvm.h
> > > +++ b/include/uapi/linux/kvm.h
> > > @@ -949,6 +949,7 @@ struct kvm_device_attr {
> > >  #define   KVM_DEV_VFIO_GROUP_DEL			2
> > >  #define KVM_DEV_TYPE_ARM_VGIC_V2	5
> > >  #define KVM_DEV_TYPE_FLIC		6
> > > +#define KVM_DEV_TYPE_MAX		7
> > 
> > This means we always need to move this value once we introduce a new
> > kvm device type. Can't you keep it in a dynamic list instead of a
> > table? We just need to do the lookup during device creation anyway.
> 
> Well, we do need the fixed IDs in order for userspace to create these
> devices via the ioctl. If it's the fixed size you're worried about, the
> easiest thing is to replace the array with an idr. I actually started off
> with that, but it felt a bit overkill (since we never need dynamic ID
> allocation). I can bring it back if you prefer?
> 
> At the end of the day, we can't get around the fact that the IDs need to
> added with some caution (e.g. not assigning an ID twice).

Ah, just to make this clear, I was only worried about the _MAX value;
the other ids obviously need to be known by userspace :)

So what this basically boils down to is an internal implementation
detail: Do we keep a static table that needs to be grown each time we
add a new device type, or do we keep a dynamic list that can have a
variable number of entries? The dynamic list appeals to me, but it is
slightly more complex code, and we probably won't be adding new device
types left and right anyway. So if your idr code is short and sweet,
I'd say go for it, but not if we end up with a mountain of code.

Re: [PATCH 1/2] KVM: device: add simple registration mechanism for kvm_device_ops

[Will Deacon]

On Mon, Jun 30, 2014 at 11:25:01AM +0100, Cornelia Huck wrote:
> On Mon, 30 Jun 2014 10:36:19 +0100
> Will Deacon <will.deacon@arm.com> wrote:
> > On Mon, Jun 30, 2014 at 10:21:14AM +0100, Cornelia Huck wrote:
> > > On Thu, 26 Jun 2014 18:30:16 +0100
> > > Will Deacon <will.deacon@arm.com> wrote:
> > > > diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> > > > index e11d8f170a62..3b368166286f 100644
> > > > --- a/include/uapi/linux/kvm.h
> > > > +++ b/include/uapi/linux/kvm.h
> > > > @@ -949,6 +949,7 @@ struct kvm_device_attr {
> > > >  #define   KVM_DEV_VFIO_GROUP_DEL			2
> > > >  #define KVM_DEV_TYPE_ARM_VGIC_V2	5
> > > >  #define KVM_DEV_TYPE_FLIC		6
> > > > +#define KVM_DEV_TYPE_MAX		7
> > > 
> > > This means we always need to move this value once we introduce a new
> > > kvm device type. Can't you keep it in a dynamic list instead of a
> > > table? We just need to do the lookup during device creation anyway.
> > 
> > Well, we do need the fixed IDs in order for userspace to create these
> > devices via the ioctl. If it's the fixed size you're worried about, the
> > easiest thing is to replace the array with an idr. I actually started off
> > with that, but it felt a bit overkill (since we never need dynamic ID
> > allocation). I can bring it back if you prefer?
> > 
> > At the end of the day, we can't get around the fact that the IDs need to
> > added with some caution (e.g. not assigning an ID twice).
> 
> Ah, just to make this clear, I was only worried about the _MAX value;
> the other ids obviously need to be known by userspace :)

Agreed.

> So what this basically boils down to is an internal implementation
> detail: Do we keep a static table that needs to be grown each time we
> add a new device type, or do we keep a dynamic list that can have a
> variable number of entries? The dynamic list appeals to me, but it is
> slightly more complex code, and we probably won't be adding new device
> types left and right anyway. So if your idr code is short and sweet,
> I'd say go for it, but not if we end up with a mountain of code.

Okey doke, I'll take a look for v2 (I need to remove a stray semi-colon from
the patch anyway).

Thanks for the feedback,

Will

Re: [PATCH 1/2] KVM: device: add simple registration mechanism for kvm_device_ops

[Will Deacon]

On Fri, Jun 27, 2014 at 03:17:57PM +0100, Paolo Bonzini wrote:
> Il 26/06/2014 19:30, Will Deacon ha scritto:
> > kvm_ioctl_create_device currently has knowledge of all the device types
> > and their associated ops. This is fairly inflexible when adding support
> > for new in-kernel device emulations, so move what we currently have out
> > into a table, which can support dynamic registration of ops by new
> > drivers for virtual hardware.
> >
> > I didn't try to port all current drivers over, as it's not always clear
> > which initialisation hook the ops should be registered from.
> 
> Conny, Alex (Graf & Williamson),
> 
> can you help Will here?  The idea looks sane, but I'd rather merge it 
> with all devices converted.

Also, if we want to use an IDR instead of an array then dynamic registration
will be required, since I can't initialise the former statically.

I can convert kvm_vfio_ops with an initcall, but the others (mpic, xics and
flic) should probably hang off the host irqchip initialisation, no?

All help appreciated! I'm happy to collate diffs :) Failing that, I'll have
a crack at it myself, but I've got no way of testing the result.

Will

Re: [PATCH 1/2] KVM: device: add simple registration mechanism for kvm_device_ops

[Cornelia Huck]

On Fri, 27 Jun 2014 16:17:57 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 26/06/2014 19:30, Will Deacon ha scritto:
> > kvm_ioctl_create_device currently has knowledge of all the device types
> > and their associated ops. This is fairly inflexible when adding support
> > for new in-kernel device emulations, so move what we currently have out
> > into a table, which can support dynamic registration of ops by new
> > drivers for virtual hardware.
> >
> > I didn't try to port all current drivers over, as it's not always clear
> > which initialisation hook the ops should be registered from.
> 
> Conny, Alex (Graf & Williamson),
> 
> can you help Will here?  The idea looks sane, but I'd rather merge it 
> with all devices converted.

FWIW, the following patch on top works for me on s390:

>From 7cc0bf5b143c2e2c1971a65ef785050ece35faf3 Mon Sep 17 00:00:00 2001
From: Cornelia Huck <cornelia.huck@de.ibm.com>
Date: Mon, 30 Jun 2014 12:47:35 +0200
Subject: [PATCH] KVM: s390: register flic ops dynamically

Using the new kvm_register_device_ops() interface makes us get rid of
an #ifdef in commom code.

Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
 arch/s390/kvm/kvm-s390.c | 3 ++-
 arch/s390/kvm/kvm-s390.h | 1 +
 include/linux/kvm_host.h | 1 -
 virt/kvm/kvm_main.c      | 3 ---
 4 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 2f3e14f..d1d5296 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -130,7 +130,8 @@ void kvm_arch_check_processor_compat(void *rtn)
 
 int kvm_arch_init(void *opaque)
 {
-	return 0;
+	/* Register floating interrupt controller interface. */
+	return kvm_register_device_ops(&kvm_flic_ops, KVM_DEV_TYPE_FLIC);
 }
 
 void kvm_arch_exit(void)
diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
index a8655ed..b236a8a 100644
--- a/arch/s390/kvm/kvm-s390.h
+++ b/arch/s390/kvm/kvm-s390.h
@@ -222,6 +222,7 @@ int kvm_cpu_has_interrupt(struct kvm_vcpu *vcpu);
 int psw_extint_disabled(struct kvm_vcpu *vcpu);
 void kvm_s390_destroy_adapters(struct kvm *kvm);
 int kvm_s390_si_ext_call_pending(struct kvm_vcpu *vcpu);
+extern struct kvm_device_ops kvm_flic_ops;
 
 /* implemented in guestdbg.c */
 void kvm_s390_backup_guest_per_regs(struct kvm_vcpu *vcpu);
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index b75faaf..72e12dd 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -1089,7 +1089,6 @@ extern struct kvm_device_ops kvm_mpic_ops;
 extern struct kvm_device_ops kvm_xics_ops;
 extern struct kvm_device_ops kvm_vfio_ops;
 extern struct kvm_device_ops kvm_arm_vgic_v2_ops;
-extern struct kvm_device_ops kvm_flic_ops;
 
 #ifdef CONFIG_HAVE_KVM_CPU_RELAX_INTERCEPT
 
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index a0c6cc3..c2a562b 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2275,9 +2275,6 @@ static struct kvm_device_ops *kvm_device_ops_table[KVM_DEV_TYPE_MAX] = {
 	[KVM_DEV_TYPE_ARM_VGIC_V2]	= &kvm_arm_vgic_v2_ops,
 #endif
 
-#ifdef CONFIG_S390
-	[KVM_DEV_TYPE_FLIC]		= &kvm_flic_ops;
-#endif
 };
 
 int kvm_register_device_ops(struct kvm_device_ops *ops, u32 type)
-- 
1.8.5.5

Re: [PATCH 1/2] KVM: device: add simple registration mechanism for kvm_device_ops

[Will Deacon]

On Mon, Jun 30, 2014 at 12:14:27PM +0100, Cornelia Huck wrote:
> On Fri, 27 Jun 2014 16:17:57 +0200
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> > Il 26/06/2014 19:30, Will Deacon ha scritto:
> > > kvm_ioctl_create_device currently has knowledge of all the device types
> > > and their associated ops. This is fairly inflexible when adding support
> > > for new in-kernel device emulations, so move what we currently have out
> > > into a table, which can support dynamic registration of ops by new
> > > drivers for virtual hardware.
> > >
> > > I didn't try to port all current drivers over, as it's not always clear
> > > which initialisation hook the ops should be registered from.
> > 
> > Conny, Alex (Graf & Williamson),
> > 
> > can you help Will here?  The idea looks sane, but I'd rather merge it 
> > with all devices converted.
> 
> FWIW, the following patch on top works for me on s390:
> 
> From 7cc0bf5b143c2e2c1971a65ef785050ece35faf3 Mon Sep 17 00:00:00 2001
> From: Cornelia Huck <cornelia.huck@de.ibm.com>
> Date: Mon, 30 Jun 2014 12:47:35 +0200
> Subject: [PATCH] KVM: s390: register flic ops dynamically
> 
> Using the new kvm_register_device_ops() interface makes us get rid of
> an #ifdef in commom code.
> 
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>

[...]

Thanks Cornelia! I've fixed the minor conflicts against the vgic patch
and added this to my series.

Will

Re: [PATCH 1/2] KVM: device: add simple registration mechanism for kvm_device_ops

[Paolo Bonzini]

Il 30/06/2014 11:21, Cornelia Huck ha scritto:
> On Thu, 26 Jun 2014 18:30:16 +0100
> Will Deacon <will.deacon@arm.com> wrote:
>
>> kvm_ioctl_create_device currently has knowledge of all the device types
>> and their associated ops. This is fairly inflexible when adding support
>> for new in-kernel device emulations, so move what we currently have out
>> into a table, which can support dynamic registration of ops by new
>> drivers for virtual hardware.
>>
>> I didn't try to port all current drivers over, as it's not always clear
>> which initialisation hook the ops should be registered from.
>
> I like the general idea of registering the ops dynamically, some
> comments below.
>
>>
>> Cc: Gleb Natapov <gleb@kernel.org>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>> Cc: Christoffer Dall <christoffer.dall@linaro.org>
>> Signed-off-by: Will Deacon <will.deacon@arm.com>
>> ---
>>
>> Hi guys,
>>
>> I've just started writing a virtual IOMMU for the ARM SMMU and figured a
>> registration mechanism for kvm_device_ops would be a nice cleanup for
>> that.
>>
>> Will
>>
>>  include/linux/kvm_host.h |  1 +
>>  include/uapi/linux/kvm.h |  1 +
>>  virt/kvm/kvm_main.c      | 65 ++++++++++++++++++++++++++++--------------------
>>  3 files changed, 40 insertions(+), 27 deletions(-)
>>
>
>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>> index e11d8f170a62..3b368166286f 100644
>> --- a/include/uapi/linux/kvm.h
>> +++ b/include/uapi/linux/kvm.h
>> @@ -949,6 +949,7 @@ struct kvm_device_attr {
>>  #define   KVM_DEV_VFIO_GROUP_DEL			2
>>  #define KVM_DEV_TYPE_ARM_VGIC_V2	5
>>  #define KVM_DEV_TYPE_FLIC		6
>> +#define KVM_DEV_TYPE_MAX		7
>
> This means we always need to move this value once we introduce a new
> kvm device type. Can't you keep it in a dynamic list instead of a
> table? We just need to do the lookup during device creation anyway.

There's also this wonderful thing called enum. ;)

It would let Will keep the simpler code with an array, and autogenerate 
KVM_DEV_TYPE_MAX.

>>
>>  /*
>>   * ioctls for VM fds
>
>> +int kvm_register_device_ops(struct kvm_device_ops *ops, u32 type)
>> +{
>> +	if (type >= KVM_DEV_TYPE_MAX)

... then you can make this ARRAY_SIZE, which makes the code quite nice & 
obvious.

Paolo

>> +		return -ENOSPC;
>> +
>> +	if (kvm_device_ops_table[type] != NULL)
>> +		return -EEXIST;
>
> Checking for type collisions would be a bit more expensive with a list,
> but I don't think it matters.
>
>> +
>> +	kvm_device_ops_table[type] = ops;
>> +	return 0;
>> +}
>

Re: [PATCH 1/2] KVM: device: add simple registration mechanism for kvm_device_ops

[Cornelia Huck]

On Mon, 30 Jun 2014 19:22:55 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 30/06/2014 11:21, Cornelia Huck ha scritto:
> > On Thu, 26 Jun 2014 18:30:16 +0100
> > Will Deacon <will.deacon@arm.com> wrote:
> >
> >> kvm_ioctl_create_device currently has knowledge of all the device types
> >> and their associated ops. This is fairly inflexible when adding support
> >> for new in-kernel device emulations, so move what we currently have out
> >> into a table, which can support dynamic registration of ops by new
> >> drivers for virtual hardware.
> >>
> >> I didn't try to port all current drivers over, as it's not always clear
> >> which initialisation hook the ops should be registered from.
> >
> > I like the general idea of registering the ops dynamically, some
> > comments below.
> >
> >>
> >> Cc: Gleb Natapov <gleb@kernel.org>
> >> Cc: Paolo Bonzini <pbonzini@redhat.com>
> >> Cc: Marc Zyngier <marc.zyngier@arm.com>
> >> Cc: Christoffer Dall <christoffer.dall@linaro.org>
> >> Signed-off-by: Will Deacon <will.deacon@arm.com>
> >> ---
> >>
> >> Hi guys,
> >>
> >> I've just started writing a virtual IOMMU for the ARM SMMU and figured a
> >> registration mechanism for kvm_device_ops would be a nice cleanup for
> >> that.
> >>
> >> Will
> >>
> >>  include/linux/kvm_host.h |  1 +
> >>  include/uapi/linux/kvm.h |  1 +
> >>  virt/kvm/kvm_main.c      | 65 ++++++++++++++++++++++++++++--------------------
> >>  3 files changed, 40 insertions(+), 27 deletions(-)
> >>
> >
> >> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> >> index e11d8f170a62..3b368166286f 100644
> >> --- a/include/uapi/linux/kvm.h
> >> +++ b/include/uapi/linux/kvm.h
> >> @@ -949,6 +949,7 @@ struct kvm_device_attr {
> >>  #define   KVM_DEV_VFIO_GROUP_DEL			2
> >>  #define KVM_DEV_TYPE_ARM_VGIC_V2	5
> >>  #define KVM_DEV_TYPE_FLIC		6
> >> +#define KVM_DEV_TYPE_MAX		7
> >
> > This means we always need to move this value once we introduce a new
> > kvm device type. Can't you keep it in a dynamic list instead of a
> > table? We just need to do the lookup during device creation anyway.
> 
> There's also this wonderful thing called enum. ;)
> 
> It would let Will keep the simpler code with an array, and autogenerate 
> KVM_DEV_TYPE_MAX.

Or this :)

It means we statically grow the table with each new device type, but we
probably don't want bazillions of kvm device types anyway.

Re: [PATCH 1/2] KVM: device: add simple registration mechanism for kvm_device_ops

[Will Deacon]

On Mon, Jun 30, 2014 at 06:22:55PM +0100, Paolo Bonzini wrote:
> Il 30/06/2014 11:21, Cornelia Huck ha scritto:
> > On Thu, 26 Jun 2014 18:30:16 +0100
> > Will Deacon <will.deacon@arm.com> wrote:
> >> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> >> index e11d8f170a62..3b368166286f 100644
> >> --- a/include/uapi/linux/kvm.h
> >> +++ b/include/uapi/linux/kvm.h
> >> @@ -949,6 +949,7 @@ struct kvm_device_attr {
> >>  #define   KVM_DEV_VFIO_GROUP_DEL			2
> >>  #define KVM_DEV_TYPE_ARM_VGIC_V2	5
> >>  #define KVM_DEV_TYPE_FLIC		6
> >> +#define KVM_DEV_TYPE_MAX		7
> >
> > This means we always need to move this value once we introduce a new
> > kvm device type. Can't you keep it in a dynamic list instead of a
> > table? We just need to do the lookup during device creation anyway.
> 
> There's also this wonderful thing called enum. ;)
> 
> It would let Will keep the simpler code with an array, and autogenerate 
> KVM_DEV_TYPE_MAX.

Although this is uapi, so we may need to #define the symbols anyway to avoid
breaking userspace #ifndef tests.

What do you reckon; is this an ABI break?

Will

Re: [PATCH 1/2] KVM: device: add simple registration mechanism for kvm_device_ops

[Paolo Bonzini]

Il 30/06/2014 19:31, Will Deacon ha scritto:
>> > It would let Will keep the simpler code with an array, and autogenerate
>> > KVM_DEV_TYPE_MAX.
> Although this is uapi, so we may need to #define the symbols anyway to avoid
> breaking userspace #ifndef tests.
>
> What do you reckon; is this an ABI break?

Yes, it would be an API break.  But you can also do this:

#define FOO FOO

I think there are other occurrences of this in uapi/.

Paolo