Re: [PATCH v2 4/4] KVM: VFIO: register kvm_device_ops dynamically

[Alex Williamson <alex.williamson@redhat.com>]

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);