* [PATCH v2 2/4] KVM: ARM: vgic: register kvm_device_ops dynamically
2014-07-01 14:45 [PATCH v2 1/4] KVM: device: add simple registration mechanism for kvm_device_ops Will Deacon
@ 2014-07-01 14:45 ` Will Deacon
2014-07-09 16:05 ` Paolo Bonzini
2014-07-31 12:10 ` Christoffer Dall
2014-07-01 14:45 ` [PATCH v2 3/4] KVM: s390: register flic ops dynamically Will Deacon
` (3 subsequent siblings)
4 siblings, 2 replies; 17+ messages in thread
From: Will Deacon @ 2014-07-01 14:45 UTC (permalink / raw)
To: kvm, kvmarm
Cc: cornelia.huck, Alex.Williamson, agraf, gleb, pbonzini,
marc.zyngier, christoffer.dall, 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>
---
include/linux/kvm_host.h | 1 -
virt/kvm/arm/vgic.c | 156 +++++++++++++++++++++++------------------------
virt/kvm/kvm_main.c | 4 --
3 files changed, 78 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..d9d0bcebad2b 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,80 @@ 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 a8539ae10247..7eab47222dcf 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
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v2 2/4] KVM: ARM: vgic: register kvm_device_ops dynamically
2014-07-01 14:45 ` [PATCH v2 2/4] KVM: ARM: vgic: register kvm_device_ops dynamically Will Deacon
@ 2014-07-09 16:05 ` Paolo Bonzini
2014-07-09 16:13 ` Marc Zyngier
2014-07-31 12:10 ` Christoffer Dall
1 sibling, 1 reply; 17+ messages in thread
From: Paolo Bonzini @ 2014-07-09 16:05 UTC (permalink / raw)
To: Will Deacon, kvm, kvmarm
Cc: cornelia.huck, Alex.Williamson, agraf, gleb, marc.zyngier,
christoffer.dall
Il 01/07/2014 16:45, Will Deacon ha scritto:
> 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>
> ---
> include/linux/kvm_host.h | 1 -
> virt/kvm/arm/vgic.c | 156 +++++++++++++++++++++++------------------------
> virt/kvm/kvm_main.c | 4 --
> 3 files changed, 78 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..d9d0bcebad2b 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,80 @@ 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 a8539ae10247..7eab47222dcf 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
>
Christopher, Marc, can you review/ack this?
Paolo
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v2 2/4] KVM: ARM: vgic: register kvm_device_ops dynamically
2014-07-09 16:05 ` Paolo Bonzini
@ 2014-07-09 16:13 ` Marc Zyngier
0 siblings, 0 replies; 17+ messages in thread
From: Marc Zyngier @ 2014-07-09 16:13 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Will Deacon, kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu,
cornelia.huck@de.ibm.com, Alex.Williamson@redhat.com,
agraf@suse.de, gleb@kernel.org, christoffer.dall@linaro.org
On Wed, Jul 09 2014 at 5:05:20 pm BST, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 01/07/2014 16:45, Will Deacon ha scritto:
>> 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>
>> ---
>> include/linux/kvm_host.h | 1 -
>> virt/kvm/arm/vgic.c | 156 +++++++++++++++++++++++------------------------
>> virt/kvm/kvm_main.c | 4 --
>> 3 files changed, 78 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..d9d0bcebad2b 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,80 @@ 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 a8539ae10247..7eab47222dcf 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
>>
>
> Christopher, Marc, can you review/ack this?
I though I had replied already, but apparently not... The proximity
effect, probably! ;-)
Acked-by: Marc Zyngier <marc.zyngier@arm.com>
M.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/4] KVM: ARM: vgic: register kvm_device_ops dynamically
2014-07-01 14:45 ` [PATCH v2 2/4] KVM: ARM: vgic: register kvm_device_ops dynamically Will Deacon
2014-07-09 16:05 ` Paolo Bonzini
@ 2014-07-31 12:10 ` Christoffer Dall
2014-07-31 13:25 ` Will Deacon
1 sibling, 1 reply; 17+ messages in thread
From: Christoffer Dall @ 2014-07-31 12:10 UTC (permalink / raw)
To: Will Deacon
Cc: kvm, kvmarm, cornelia.huck, Alex.Williamson, agraf, gleb,
pbonzini, marc.zyngier
On Tue, Jul 01, 2014 at 03:45:16PM +0100, Will Deacon wrote:
> 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>
> ---
> include/linux/kvm_host.h | 1 -
> virt/kvm/arm/vgic.c | 156 +++++++++++++++++++++++------------------------
> virt/kvm/kvm_main.c | 4 --
> 3 files changed, 78 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..d9d0bcebad2b 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,80 @@ 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);
close to ridiculous nit: but I would add a newline here since the
comment doesn't really apply to the kvm_register_device_ops, but please
don't make another revision just for this.
> + 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 a8539ae10247..7eab47222dcf 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
>
Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v2 2/4] KVM: ARM: vgic: register kvm_device_ops dynamically
2014-07-31 12:10 ` Christoffer Dall
@ 2014-07-31 13:25 ` Will Deacon
0 siblings, 0 replies; 17+ messages in thread
From: Will Deacon @ 2014-07-31 13:25 UTC (permalink / raw)
To: Christoffer Dall
Cc: kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu,
cornelia.huck@de.ibm.com, Alex.Williamson@redhat.com,
agraf@suse.de, gleb@kernel.org, pbonzini@redhat.com, Marc Zyngier
Hi Christoffer,
On Thu, Jul 31, 2014 at 01:10:15PM +0100, Christoffer Dall wrote:
> On Tue, Jul 01, 2014 at 03:45:16PM +0100, Will Deacon wrote:
> > 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>
> > ---
> > include/linux/kvm_host.h | 1 -
> > virt/kvm/arm/vgic.c | 156 +++++++++++++++++++++++------------------------
> > virt/kvm/kvm_main.c | 4 --
> > 3 files changed, 78 insertions(+), 83 deletions(-)
[...]
> > + 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);
>
> close to ridiculous nit: but I would add a newline here since the
> comment doesn't really apply to the kvm_register_device_ops, but please
> don't make another revision just for this.
I'll repost this after the merge window anyway, so there's plenty of room
for pedantic, cosmetic changes!
Thanks,
Will
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 3/4] KVM: s390: register flic ops dynamically
2014-07-01 14:45 [PATCH v2 1/4] KVM: device: add simple registration mechanism for kvm_device_ops Will Deacon
2014-07-01 14:45 ` [PATCH v2 2/4] KVM: ARM: vgic: register kvm_device_ops dynamically Will Deacon
@ 2014-07-01 14:45 ` Will Deacon
2014-07-01 14:45 ` [PATCH v2 4/4] KVM: VFIO: register kvm_device_ops dynamically Will Deacon
` (2 subsequent siblings)
4 siblings, 0 replies; 17+ messages in thread
From: Will Deacon @ 2014-07-01 14:45 UTC (permalink / raw)
To: kvm, kvmarm
Cc: cornelia.huck, Alex.Williamson, agraf, gleb, pbonzini,
marc.zyngier, christoffer.dall, Will Deacon
From: Cornelia Huck <cornelia.huck@de.ibm.com>
Using the new kvm_register_device_ops() interface makes us get rid of
an #ifdef in common code.
Cc: Gleb Natapov <gleb@kernel.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Signed-off-by: Will Deacon <will.deacon@arm.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 | 4 ----
4 files changed, 3 insertions(+), 6 deletions(-)
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 2f3e14fe91a4..d1d5296e6cdb 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 a8655ed31616..b236a8ab7c6e 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 4317fdd10696..422e55ac8a13 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_flic_ops;
#ifdef CONFIG_HAVE_KVM_CPU_RELAX_INTERCEPT
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 7eab47222dcf..c9181db8abdd 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2270,10 +2270,6 @@ static struct kvm_device_ops *kvm_device_ops_table[KVM_DEV_TYPE_MAX] = {
#ifdef CONFIG_KVM_VFIO
[KVM_DEV_TYPE_VFIO] = &kvm_vfio_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)
--
2.0.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v2 4/4] KVM: VFIO: register kvm_device_ops dynamically
2014-07-01 14:45 [PATCH v2 1/4] KVM: device: add simple registration mechanism for kvm_device_ops Will Deacon
2014-07-01 14:45 ` [PATCH v2 2/4] KVM: ARM: vgic: register kvm_device_ops dynamically Will Deacon
2014-07-01 14:45 ` [PATCH v2 3/4] KVM: s390: register flic ops dynamically Will Deacon
@ 2014-07-01 14:45 ` Will Deacon
2014-07-09 16:06 ` Paolo Bonzini
` (2 more replies)
2014-07-02 9:17 ` [PATCH v2 1/4] KVM: device: add simple registration mechanism for kvm_device_ops Cornelia Huck
2014-07-31 12:10 ` Christoffer Dall
4 siblings, 3 replies; 17+ messages in thread
From: Will Deacon @ 2014-07-01 14:45 UTC (permalink / raw)
To: kvm, kvmarm
Cc: cornelia.huck, Alex.Williamson, agraf, gleb, pbonzini,
marc.zyngier, christoffer.dall, Will Deacon
Now that we have a dynamic means to register kvm_device_ops, use that
for the VFIO kvm device, instead of relying on the static table.
This is achieved by a module_init call to register the ops with KVM.
Cc: Gleb Natapov <gleb@kernel.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Alex Williamson <Alex.Williamson@redhat.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
include/linux/kvm_host.h | 1 -
virt/kvm/kvm_main.c | 4 ----
virt/kvm/vfio.c | 22 +++++++++++++++-------
3 files changed, 15 insertions(+), 12 deletions(-)
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 422e55ac8a13..c04d58754263 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -1087,7 +1087,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;
#ifdef CONFIG_HAVE_KVM_CPU_RELAX_INTERCEPT
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index c9181db8abdd..c5f646e846ba 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2266,10 +2266,6 @@ static struct kvm_device_ops *kvm_device_ops_table[KVM_DEV_TYPE_MAX] = {
#ifdef CONFIG_KVM_XICS
[KVM_DEV_TYPE_XICS] = &kvm_xics_ops,
#endif
-
-#ifdef CONFIG_KVM_VFIO
- [KVM_DEV_TYPE_VFIO] = &kvm_vfio_ops,
-#endif
};
int kvm_register_device_ops(struct kvm_device_ops *ops, u32 type)
diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
index ba1a93f935c7..bb11b36ee8a2 100644
--- a/virt/kvm/vfio.c
+++ b/virt/kvm/vfio.c
@@ -246,6 +246,16 @@ static void kvm_vfio_destroy(struct kvm_device *dev)
kfree(dev); /* alloc by kvm_ioctl_create_device, free by .destroy */
}
+static int kvm_vfio_create(struct kvm_device *dev, u32 type);
+
+static struct kvm_device_ops kvm_vfio_ops = {
+ .name = "kvm-vfio",
+ .create = kvm_vfio_create,
+ .destroy = kvm_vfio_destroy,
+ .set_attr = kvm_vfio_set_attr,
+ .has_attr = kvm_vfio_has_attr,
+};
+
static int kvm_vfio_create(struct kvm_device *dev, u32 type)
{
struct kvm_device *tmp;
@@ -268,10 +278,8 @@ static int kvm_vfio_create(struct kvm_device *dev, u32 type)
return 0;
}
-struct kvm_device_ops kvm_vfio_ops = {
- .name = "kvm-vfio",
- .create = kvm_vfio_create,
- .destroy = kvm_vfio_destroy,
- .set_attr = kvm_vfio_set_attr,
- .has_attr = kvm_vfio_has_attr,
-};
+static int __init kvm_vfio_ops_init(void)
+{
+ return kvm_register_device_ops(&kvm_vfio_ops, KVM_DEV_TYPE_VFIO);
+}
+module_init(kvm_vfio_ops_init);
--
2.0.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v2 4/4] KVM: VFIO: register kvm_device_ops dynamically
2014-07-01 14:45 ` [PATCH v2 4/4] KVM: VFIO: register kvm_device_ops dynamically Will Deacon
@ 2014-07-09 16:06 ` Paolo Bonzini
2014-07-09 16:19 ` Alex Williamson
2014-07-09 16:56 ` Alex Williamson
2 siblings, 0 replies; 17+ messages in thread
From: Paolo Bonzini @ 2014-07-09 16:06 UTC (permalink / raw)
To: Will Deacon, kvm, kvmarm
Cc: cornelia.huck, Alex.Williamson, agraf, gleb, marc.zyngier,
christoffer.dall
Il 01/07/2014 16:45, Will Deacon ha scritto:
> Now that we have a dynamic means to register kvm_device_ops, use that
> for the VFIO kvm device, instead of relying on the static table.
>
> This is achieved by a module_init call to register the ops with KVM.
>
> Cc: Gleb Natapov <gleb@kernel.org>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Alex Williamson <Alex.Williamson@redhat.com>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
> include/linux/kvm_host.h | 1 -
> virt/kvm/kvm_main.c | 4 ----
> virt/kvm/vfio.c | 22 +++++++++++++++-------
> 3 files changed, 15 insertions(+), 12 deletions(-)
>
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 422e55ac8a13..c04d58754263 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -1087,7 +1087,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;
>
> #ifdef CONFIG_HAVE_KVM_CPU_RELAX_INTERCEPT
>
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index c9181db8abdd..c5f646e846ba 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -2266,10 +2266,6 @@ static struct kvm_device_ops *kvm_device_ops_table[KVM_DEV_TYPE_MAX] = {
> #ifdef CONFIG_KVM_XICS
> [KVM_DEV_TYPE_XICS] = &kvm_xics_ops,
> #endif
> -
> -#ifdef CONFIG_KVM_VFIO
> - [KVM_DEV_TYPE_VFIO] = &kvm_vfio_ops,
> -#endif
> };
>
> int kvm_register_device_ops(struct kvm_device_ops *ops, u32 type)
> diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
> index ba1a93f935c7..bb11b36ee8a2 100644
> --- a/virt/kvm/vfio.c
> +++ b/virt/kvm/vfio.c
> @@ -246,6 +246,16 @@ static void kvm_vfio_destroy(struct kvm_device *dev)
> kfree(dev); /* alloc by kvm_ioctl_create_device, free by .destroy */
> }
>
> +static int kvm_vfio_create(struct kvm_device *dev, u32 type);
> +
> +static struct kvm_device_ops kvm_vfio_ops = {
> + .name = "kvm-vfio",
> + .create = kvm_vfio_create,
> + .destroy = kvm_vfio_destroy,
> + .set_attr = kvm_vfio_set_attr,
> + .has_attr = kvm_vfio_has_attr,
> +};
> +
> static int kvm_vfio_create(struct kvm_device *dev, u32 type)
> {
> struct kvm_device *tmp;
> @@ -268,10 +278,8 @@ static int kvm_vfio_create(struct kvm_device *dev, u32 type)
> return 0;
> }
>
> -struct kvm_device_ops kvm_vfio_ops = {
> - .name = "kvm-vfio",
> - .create = kvm_vfio_create,
> - .destroy = kvm_vfio_destroy,
> - .set_attr = kvm_vfio_set_attr,
> - .has_attr = kvm_vfio_has_attr,
> -};
> +static int __init kvm_vfio_ops_init(void)
> +{
> + return kvm_register_device_ops(&kvm_vfio_ops, KVM_DEV_TYPE_VFIO);
> +}
> +module_init(kvm_vfio_ops_init);
>
Alex, can you review/ack this?
Paolo
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v2 4/4] KVM: VFIO: register kvm_device_ops dynamically
2014-07-01 14:45 ` [PATCH v2 4/4] KVM: VFIO: register kvm_device_ops dynamically Will Deacon
2014-07-09 16:06 ` Paolo Bonzini
@ 2014-07-09 16:19 ` Alex Williamson
2014-07-09 16:47 ` Will Deacon
2014-07-09 16:56 ` Alex Williamson
2 siblings, 1 reply; 17+ messages in thread
From: Alex Williamson @ 2014-07-09 16:19 UTC (permalink / raw)
To: Will Deacon
Cc: kvm, kvmarm, cornelia.huck, agraf, gleb, pbonzini, marc.zyngier,
christoffer.dall
On Tue, 2014-07-01 at 15:45 +0100, Will Deacon wrote:
> Now that we have a dynamic means to register kvm_device_ops, use that
> for the VFIO kvm device, instead of relying on the static table.
>
> This is achieved by a module_init call to register the ops with KVM.
>
> Cc: Gleb Natapov <gleb@kernel.org>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Alex Williamson <Alex.Williamson@redhat.com>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
> include/linux/kvm_host.h | 1 -
> virt/kvm/kvm_main.c | 4 ----
> virt/kvm/vfio.c | 22 +++++++++++++++-------
> 3 files changed, 15 insertions(+), 12 deletions(-)
>
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 422e55ac8a13..c04d58754263 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -1087,7 +1087,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;
>
> #ifdef CONFIG_HAVE_KVM_CPU_RELAX_INTERCEPT
>
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index c9181db8abdd..c5f646e846ba 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -2266,10 +2266,6 @@ static struct kvm_device_ops *kvm_device_ops_table[KVM_DEV_TYPE_MAX] = {
> #ifdef CONFIG_KVM_XICS
> [KVM_DEV_TYPE_XICS] = &kvm_xics_ops,
> #endif
> -
> -#ifdef CONFIG_KVM_VFIO
> - [KVM_DEV_TYPE_VFIO] = &kvm_vfio_ops,
> -#endif
> };
>
> int kvm_register_device_ops(struct kvm_device_ops *ops, u32 type)
> diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
> index ba1a93f935c7..bb11b36ee8a2 100644
> --- a/virt/kvm/vfio.c
> +++ b/virt/kvm/vfio.c
> @@ -246,6 +246,16 @@ static void kvm_vfio_destroy(struct kvm_device *dev)
> kfree(dev); /* alloc by kvm_ioctl_create_device, free by .destroy */
> }
>
> +static int kvm_vfio_create(struct kvm_device *dev, u32 type);
> +
> +static struct kvm_device_ops kvm_vfio_ops = {
> + .name = "kvm-vfio",
> + .create = kvm_vfio_create,
> + .destroy = kvm_vfio_destroy,
> + .set_attr = kvm_vfio_set_attr,
> + .has_attr = kvm_vfio_has_attr,
> +};
> +
Why move the struct? We wouldn't need the prototype if it was left in
place and it seems like the only change we're making to set it static.
Functionally the change is fine, but the ordering was cleaner before
imho. Thanks,
Alex
> static int kvm_vfio_create(struct kvm_device *dev, u32 type)
> {
> struct kvm_device *tmp;
> @@ -268,10 +278,8 @@ static int kvm_vfio_create(struct kvm_device *dev, u32 type)
> return 0;
> }
>
> -struct kvm_device_ops kvm_vfio_ops = {
> - .name = "kvm-vfio",
> - .create = kvm_vfio_create,
> - .destroy = kvm_vfio_destroy,
> - .set_attr = kvm_vfio_set_attr,
> - .has_attr = kvm_vfio_has_attr,
> -};
> +static int __init kvm_vfio_ops_init(void)
> +{
> + return kvm_register_device_ops(&kvm_vfio_ops, KVM_DEV_TYPE_VFIO);
> +}
> +module_init(kvm_vfio_ops_init);
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v2 4/4] KVM: VFIO: register kvm_device_ops dynamically
2014-07-09 16:19 ` Alex Williamson
@ 2014-07-09 16:47 ` Will Deacon
2014-07-09 16:56 ` Alex Williamson
0 siblings, 1 reply; 17+ messages in thread
From: Will Deacon @ 2014-07-09 16:47 UTC (permalink / raw)
To: Alex Williamson
Cc: kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu,
cornelia.huck@de.ibm.com, agraf@suse.de, gleb@kernel.org,
pbonzini@redhat.com, Marc Zyngier, christoffer.dall@linaro.org
Hi Alex,
On Wed, Jul 09, 2014 at 05:19:24PM +0100, Alex Williamson wrote:
> On Tue, 2014-07-01 at 15:45 +0100, Will Deacon wrote:
> > diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
> > index ba1a93f935c7..bb11b36ee8a2 100644
> > --- a/virt/kvm/vfio.c
> > +++ b/virt/kvm/vfio.c
> > @@ -246,6 +246,16 @@ static void kvm_vfio_destroy(struct kvm_device *dev)
> > kfree(dev); /* alloc by kvm_ioctl_create_device, free by .destroy */
> > }
> >
> > +static int kvm_vfio_create(struct kvm_device *dev, u32 type);
> > +
> > +static struct kvm_device_ops kvm_vfio_ops = {
> > + .name = "kvm-vfio",
> > + .create = kvm_vfio_create,
> > + .destroy = kvm_vfio_destroy,
> > + .set_attr = kvm_vfio_set_attr,
> > + .has_attr = kvm_vfio_has_attr,
> > +};
> > +
>
> Why move the struct? We wouldn't need the prototype if it was left in
> place and it seems like the only change we're making to set it static.
> Functionally the change is fine, but the ordering was cleaner before
> imho. Thanks,
The problem is that kvm_vfio_create takes the address of kvm_vfio_ops:
/* Only one VFIO "device" per VM */
list_for_each_entry(tmp, &dev->kvm->devices, vm_node)
if (tmp->ops == &kvm_vfio_ops)
return -EBUSY;
so you have a circular dependency, which I just resolved in the most obvious
way to me. I'm happy to solve it a better way, if you have a preference?
Will
--->8
arch/arm64/kvm/../../../virt/kvm/vfio.c: In function ‘kvm_vfio_create’:
arch/arm64/kvm/../../../virt/kvm/vfio.c:256:20: error: ‘kvm_vfio_ops’ undeclared (first use in this function)
if (tmp->ops == &kvm_vfio_ops)
^
arch/arm64/kvm/../../../virt/kvm/vfio.c:256:20: note: each undeclared identifier is reported only once for each function it appears in
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v2 4/4] KVM: VFIO: register kvm_device_ops dynamically
2014-07-09 16:47 ` Will Deacon
@ 2014-07-09 16:56 ` Alex Williamson
0 siblings, 0 replies; 17+ messages in thread
From: Alex Williamson @ 2014-07-09 16:56 UTC (permalink / raw)
To: Will Deacon
Cc: kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu,
cornelia.huck@de.ibm.com, agraf@suse.de, gleb@kernel.org,
pbonzini@redhat.com, Marc Zyngier, christoffer.dall@linaro.org
On Wed, 2014-07-09 at 17:47 +0100, Will Deacon wrote:
> Hi Alex,
>
> On Wed, Jul 09, 2014 at 05:19:24PM +0100, Alex Williamson wrote:
> > On Tue, 2014-07-01 at 15:45 +0100, Will Deacon wrote:
> > > diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
> > > index ba1a93f935c7..bb11b36ee8a2 100644
> > > --- a/virt/kvm/vfio.c
> > > +++ b/virt/kvm/vfio.c
> > > @@ -246,6 +246,16 @@ static void kvm_vfio_destroy(struct kvm_device *dev)
> > > kfree(dev); /* alloc by kvm_ioctl_create_device, free by .destroy */
> > > }
> > >
> > > +static int kvm_vfio_create(struct kvm_device *dev, u32 type);
> > > +
> > > +static struct kvm_device_ops kvm_vfio_ops = {
> > > + .name = "kvm-vfio",
> > > + .create = kvm_vfio_create,
> > > + .destroy = kvm_vfio_destroy,
> > > + .set_attr = kvm_vfio_set_attr,
> > > + .has_attr = kvm_vfio_has_attr,
> > > +};
> > > +
> >
> > Why move the struct? We wouldn't need the prototype if it was left in
> > place and it seems like the only change we're making to set it static.
> > Functionally the change is fine, but the ordering was cleaner before
> > imho. Thanks,
>
> The problem is that kvm_vfio_create takes the address of kvm_vfio_ops:
>
> /* Only one VFIO "device" per VM */
> list_for_each_entry(tmp, &dev->kvm->devices, vm_node)
> if (tmp->ops == &kvm_vfio_ops)
> return -EBUSY;
>
> so you have a circular dependency, which I just resolved in the most obvious
> way to me. I'm happy to solve it a better way, if you have a preference?
Ok, that seems reasonable then. Thanks,
Alex
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 4/4] KVM: VFIO: register kvm_device_ops dynamically
2014-07-01 14:45 ` [PATCH v2 4/4] KVM: VFIO: register kvm_device_ops dynamically Will Deacon
2014-07-09 16:06 ` Paolo Bonzini
2014-07-09 16:19 ` Alex Williamson
@ 2014-07-09 16:56 ` Alex Williamson
2 siblings, 0 replies; 17+ messages in thread
From: Alex Williamson @ 2014-07-09 16:56 UTC (permalink / raw)
To: Will Deacon
Cc: kvm, kvmarm, cornelia.huck, agraf, gleb, pbonzini, marc.zyngier,
christoffer.dall
On Tue, 2014-07-01 at 15:45 +0100, Will Deacon wrote:
> Now that we have a dynamic means to register kvm_device_ops, use that
> for the VFIO kvm device, instead of relying on the static table.
>
> This is achieved by a module_init call to register the ops with KVM.
>
> Cc: Gleb Natapov <gleb@kernel.org>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Alex Williamson <Alex.Williamson@redhat.com>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
> include/linux/kvm_host.h | 1 -
> virt/kvm/kvm_main.c | 4 ----
> virt/kvm/vfio.c | 22 +++++++++++++++-------
> 3 files changed, 15 insertions(+), 12 deletions(-)
Acked-by: Alex Williamson <alex.williamson@redhat.com>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/4] KVM: device: add simple registration mechanism for kvm_device_ops
2014-07-01 14:45 [PATCH v2 1/4] KVM: device: add simple registration mechanism for kvm_device_ops Will Deacon
` (2 preceding siblings ...)
2014-07-01 14:45 ` [PATCH v2 4/4] KVM: VFIO: register kvm_device_ops dynamically Will Deacon
@ 2014-07-02 9:17 ` Cornelia Huck
2014-07-02 9:32 ` Will Deacon
2014-07-31 12:10 ` Christoffer Dall
4 siblings, 1 reply; 17+ messages in thread
From: Cornelia Huck @ 2014-07-02 9:17 UTC (permalink / raw)
To: Will Deacon
Cc: kvm, kvmarm, Alex.Williamson, agraf, gleb, pbonzini, marc.zyngier,
christoffer.dall
On Tue, 1 Jul 2014 15:45:15 +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 think that last paragraph should rather go into a cover letter :)
>
> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> Cc: Alex Williamson <Alex.Williamson@redhat.com>
> Cc: Alex Graf <agraf@suse.de>
> 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>
> ---
>
> v1 -> v2: Added enum for KVM_DEV_TYPE* IDs, changed limits to ARRAY_SIZE,
> removed stray semicolon, had a crack at porting VFIO, included
> Cornelia's s390 FLIC patch.
...and the changelog as well (or keep changelogs for individual
patches).
>
> include/linux/kvm_host.h | 1 +
> include/uapi/linux/kvm.h | 22 +++++++++++-----
> virt/kvm/kvm_main.c | 65 ++++++++++++++++++++++++++++--------------------
> 3 files changed, 55 insertions(+), 33 deletions(-)
>
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index e11d8f170a62..6875cc225dff 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -940,15 +940,25 @@ struct kvm_device_attr {
> __u64 addr; /* userspace address of attr data */
> };
>
> -#define KVM_DEV_TYPE_FSL_MPIC_20 1
> -#define KVM_DEV_TYPE_FSL_MPIC_42 2
> -#define KVM_DEV_TYPE_XICS 3
> -#define KVM_DEV_TYPE_VFIO 4
> #define KVM_DEV_VFIO_GROUP 1
> #define KVM_DEV_VFIO_GROUP_ADD 1
> #define KVM_DEV_VFIO_GROUP_DEL 2
> -#define KVM_DEV_TYPE_ARM_VGIC_V2 5
> -#define KVM_DEV_TYPE_FLIC 6
> +
> +enum kvm_device_type {
> + KVM_DEV_TYPE_FSL_MPIC_20 = 1,
> +#define KVM_DEV_TYPE_FSL_MPIC_20 KVM_DEV_TYPE_FSL_MPIC_20
> + KVM_DEV_TYPE_FSL_MPIC_42,
> +#define KVM_DEV_TYPE_FSL_MPIC_42 KVM_DEV_TYPE_FSL_MPIC_42
> + KVM_DEV_TYPE_XICS,
> +#define KVM_DEV_TYPE_XICS KVM_DEV_TYPE_XICS
> + KVM_DEV_TYPE_VFIO,
> +#define KVM_DEV_TYPE_VFIO KVM_DEV_TYPE_VFIO
> + KVM_DEV_TYPE_ARM_VGIC_V2,
> +#define KVM_DEV_TYPE_ARM_VGIC_V2 KVM_DEV_TYPE_ARM_VGIC_V2
> + KVM_DEV_TYPE_FLIC,
> +#define KVM_DEV_TYPE_FLIC KVM_DEV_TYPE_FLIC
> + KVM_DEV_TYPE_MAX,
Do you want to add the individual values to the enum? A removal of a
type would be an API break (so we should be safe against renumbering),
but it's sometimes helpful if one can get the numeric value at a glance.
> +};
>
> /*
> * ioctls for VM fds
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v2 1/4] KVM: device: add simple registration mechanism for kvm_device_ops
2014-07-02 9:17 ` [PATCH v2 1/4] KVM: device: add simple registration mechanism for kvm_device_ops Cornelia Huck
@ 2014-07-02 9:32 ` Will Deacon
2014-07-02 10:18 ` Cornelia Huck
0 siblings, 1 reply; 17+ messages in thread
From: Will Deacon @ 2014-07-02 9:32 UTC (permalink / raw)
To: Cornelia Huck
Cc: kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu,
Alex.Williamson@redhat.com, agraf@suse.de, gleb@kernel.org,
pbonzini@redhat.com, Marc Zyngier, christoffer.dall@linaro.org
On Wed, Jul 02, 2014 at 10:17:20AM +0100, Cornelia Huck wrote:
> On Tue, 1 Jul 2014 15:45:15 +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 think that last paragraph should rather go into a cover letter :)
>
> >
> > Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> > Cc: Alex Williamson <Alex.Williamson@redhat.com>
> > Cc: Alex Graf <agraf@suse.de>
> > 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>
> > ---
> >
> > v1 -> v2: Added enum for KVM_DEV_TYPE* IDs, changed limits to ARRAY_SIZE,
> > removed stray semicolon, had a crack at porting VFIO, included
> > Cornelia's s390 FLIC patch.
>
> ...and the changelog as well (or keep changelogs for individual
> patches).
Yeah... this has grown to be bigger than one patch now. I can include that
for v3.
> > diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> > index e11d8f170a62..6875cc225dff 100644
> > --- a/include/uapi/linux/kvm.h
> > +++ b/include/uapi/linux/kvm.h
> > @@ -940,15 +940,25 @@ struct kvm_device_attr {
> > __u64 addr; /* userspace address of attr data */
> > };
> >
> > -#define KVM_DEV_TYPE_FSL_MPIC_20 1
> > -#define KVM_DEV_TYPE_FSL_MPIC_42 2
> > -#define KVM_DEV_TYPE_XICS 3
> > -#define KVM_DEV_TYPE_VFIO 4
> > #define KVM_DEV_VFIO_GROUP 1
> > #define KVM_DEV_VFIO_GROUP_ADD 1
> > #define KVM_DEV_VFIO_GROUP_DEL 2
> > -#define KVM_DEV_TYPE_ARM_VGIC_V2 5
> > -#define KVM_DEV_TYPE_FLIC 6
> > +
> > +enum kvm_device_type {
> > + KVM_DEV_TYPE_FSL_MPIC_20 = 1,
> > +#define KVM_DEV_TYPE_FSL_MPIC_20 KVM_DEV_TYPE_FSL_MPIC_20
> > + KVM_DEV_TYPE_FSL_MPIC_42,
> > +#define KVM_DEV_TYPE_FSL_MPIC_42 KVM_DEV_TYPE_FSL_MPIC_42
> > + KVM_DEV_TYPE_XICS,
> > +#define KVM_DEV_TYPE_XICS KVM_DEV_TYPE_XICS
> > + KVM_DEV_TYPE_VFIO,
> > +#define KVM_DEV_TYPE_VFIO KVM_DEV_TYPE_VFIO
> > + KVM_DEV_TYPE_ARM_VGIC_V2,
> > +#define KVM_DEV_TYPE_ARM_VGIC_V2 KVM_DEV_TYPE_ARM_VGIC_V2
> > + KVM_DEV_TYPE_FLIC,
> > +#define KVM_DEV_TYPE_FLIC KVM_DEV_TYPE_FLIC
> > + KVM_DEV_TYPE_MAX,
>
> Do you want to add the individual values to the enum? A removal of a
> type would be an API break (so we should be safe against renumbering),
> but it's sometimes helpful if one can get the numeric value at a glance.
Could do, but then I think the advantage of the enum is questionable over
the #defines, since you could just as easily have two entries in the enum
with the same ID value as forgetting to update a KVM_DEV_TYPE_MAX #define
(which was the reason for the enum in the first place).
So I'd be inclined to keep the patch as-is, unless you have really strong
objections?
Will
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v2 1/4] KVM: device: add simple registration mechanism for kvm_device_ops
2014-07-02 9:32 ` Will Deacon
@ 2014-07-02 10:18 ` Cornelia Huck
0 siblings, 0 replies; 17+ messages in thread
From: Cornelia Huck @ 2014-07-02 10:18 UTC (permalink / raw)
To: Will Deacon
Cc: kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu,
Alex.Williamson@redhat.com, agraf@suse.de, gleb@kernel.org,
pbonzini@redhat.com, Marc Zyngier, christoffer.dall@linaro.org
On Wed, 2 Jul 2014 10:32:41 +0100
Will Deacon <will.deacon@arm.com> wrote:
> On Wed, Jul 02, 2014 at 10:17:20AM +0100, Cornelia Huck wrote:
> > On Tue, 1 Jul 2014 15:45:15 +0100
> > Will Deacon <will.deacon@arm.com> wrote:
> > > diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> > > index e11d8f170a62..6875cc225dff 100644
> > > --- a/include/uapi/linux/kvm.h
> > > +++ b/include/uapi/linux/kvm.h
> > > @@ -940,15 +940,25 @@ struct kvm_device_attr {
> > > __u64 addr; /* userspace address of attr data */
> > > };
> > >
> > > -#define KVM_DEV_TYPE_FSL_MPIC_20 1
> > > -#define KVM_DEV_TYPE_FSL_MPIC_42 2
> > > -#define KVM_DEV_TYPE_XICS 3
> > > -#define KVM_DEV_TYPE_VFIO 4
> > > #define KVM_DEV_VFIO_GROUP 1
> > > #define KVM_DEV_VFIO_GROUP_ADD 1
> > > #define KVM_DEV_VFIO_GROUP_DEL 2
> > > -#define KVM_DEV_TYPE_ARM_VGIC_V2 5
> > > -#define KVM_DEV_TYPE_FLIC 6
> > > +
> > > +enum kvm_device_type {
> > > + KVM_DEV_TYPE_FSL_MPIC_20 = 1,
> > > +#define KVM_DEV_TYPE_FSL_MPIC_20 KVM_DEV_TYPE_FSL_MPIC_20
> > > + KVM_DEV_TYPE_FSL_MPIC_42,
> > > +#define KVM_DEV_TYPE_FSL_MPIC_42 KVM_DEV_TYPE_FSL_MPIC_42
> > > + KVM_DEV_TYPE_XICS,
> > > +#define KVM_DEV_TYPE_XICS KVM_DEV_TYPE_XICS
> > > + KVM_DEV_TYPE_VFIO,
> > > +#define KVM_DEV_TYPE_VFIO KVM_DEV_TYPE_VFIO
> > > + KVM_DEV_TYPE_ARM_VGIC_V2,
> > > +#define KVM_DEV_TYPE_ARM_VGIC_V2 KVM_DEV_TYPE_ARM_VGIC_V2
> > > + KVM_DEV_TYPE_FLIC,
> > > +#define KVM_DEV_TYPE_FLIC KVM_DEV_TYPE_FLIC
> > > + KVM_DEV_TYPE_MAX,
> >
> > Do you want to add the individual values to the enum? A removal of a
> > type would be an API break (so we should be safe against renumbering),
> > but it's sometimes helpful if one can get the numeric value at a glance.
>
> Could do, but then I think the advantage of the enum is questionable over
> the #defines, since you could just as easily have two entries in the enum
> with the same ID value as forgetting to update a KVM_DEV_TYPE_MAX #define
> (which was the reason for the enum in the first place).
>
> So I'd be inclined to keep the patch as-is, unless you have really strong
> objections?
I don't really have a strong opinion on that, so
Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/4] KVM: device: add simple registration mechanism for kvm_device_ops
2014-07-01 14:45 [PATCH v2 1/4] KVM: device: add simple registration mechanism for kvm_device_ops Will Deacon
` (3 preceding siblings ...)
2014-07-02 9:17 ` [PATCH v2 1/4] KVM: device: add simple registration mechanism for kvm_device_ops Cornelia Huck
@ 2014-07-31 12:10 ` Christoffer Dall
4 siblings, 0 replies; 17+ messages in thread
From: Christoffer Dall @ 2014-07-31 12:10 UTC (permalink / raw)
To: Will Deacon
Cc: kvm, kvmarm, cornelia.huck, Alex.Williamson, agraf, gleb,
pbonzini, marc.zyngier
On Tue, Jul 01, 2014 at 03:45:15PM +0100, Will Deacon 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.
>
> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> Cc: Alex Williamson <Alex.Williamson@redhat.com>
> Cc: Alex Graf <agraf@suse.de>
> 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>
> ---
>
Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
^ permalink raw reply [flat|nested] 17+ messages in thread