Re: [PATCH V4 1/4] kvm: factor out core eventfd assign/deassign logic

[Cornelia Huck <cornelia.huck@de.ibm.com>]

On Fri, 11 Sep 2015 11:17:34 +0800
Jason Wang <jasowang@redhat.com> wrote:

> This patch factors out core eventfd assign/deassign logic and leave
> the argument checking and bus index selection to callers.
> 
> Cc: Gleb Natapov <gleb@kernel.org>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  virt/kvm/eventfd.c | 83 ++++++++++++++++++++++++++++++++----------------------
>  1 file changed, 49 insertions(+), 34 deletions(-)
> 
> diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
> index 9ff4193..163258d 100644
> --- a/virt/kvm/eventfd.c
> +++ b/virt/kvm/eventfd.c
> @@ -771,40 +771,14 @@ static enum kvm_bus ioeventfd_bus_from_flags(__u32 flags)
>  	return KVM_MMIO_BUS;
>  }
> 
> -static int
> -kvm_assign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
> +static int kvm_assign_ioeventfd_idx(struct kvm *kvm,
> +				enum kvm_bus bus_idx,
> +				struct kvm_ioeventfd *args)
>  {
> -	enum kvm_bus              bus_idx;
> -	struct _ioeventfd        *p;
> -	struct eventfd_ctx       *eventfd;
> -	int                       ret;
> -
> -	bus_idx = ioeventfd_bus_from_flags(args->flags);
> -	/* must be natural-word sized, or 0 to ignore length */
> -	switch (args->len) {
> -	case 0:
> -	case 1:
> -	case 2:
> -	case 4:
> -	case 8:
> -		break;
> -	default:
> -		return -EINVAL;
> -	}
> 
> -	/* check for range overflow */
> -	if (args->addr + args->len < args->addr)
> -		return -EINVAL;
> -
> -	/* check for extra flags that we don't understand */
> -	if (args->flags & ~KVM_IOEVENTFD_VALID_FLAG_MASK)
> -		return -EINVAL;
> -
> -	/* ioeventfd with no length can't be combined with DATAMATCH */
> -	if (!args->len &&
> -	    args->flags & (KVM_IOEVENTFD_FLAG_PIO |
> -			   KVM_IOEVENTFD_FLAG_DATAMATCH))
> -		return -EINVAL;
> +	struct eventfd_ctx *eventfd;
> +	struct _ioeventfd *p;
> +	int ret;
> 
>  	eventfd = eventfd_ctx_fdget(args->fd);
>  	if (IS_ERR(eventfd))
> @@ -873,14 +847,48 @@ fail:
>  }
> 
>  static int
> -kvm_deassign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
> +kvm_assign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)

You'll move this function to below the deassign function in patch 2.
Maybe do it already here?

>  {
>  	enum kvm_bus              bus_idx;
> +
> +	bus_idx = ioeventfd_bus_from_flags(args->flags);
> +	/* must be natural-word sized, or 0 to ignore length */
> +	switch (args->len) {
> +	case 0:
> +	case 1:
> +	case 2:
> +	case 4:
> +	case 8:
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	/* check for range overflow */
> +	if (args->addr + args->len < args->addr)
> +		return -EINVAL;
> +
> +	/* check for extra flags that we don't understand */
> +	if (args->flags & ~KVM_IOEVENTFD_VALID_FLAG_MASK)
> +		return -EINVAL;
> +
> +	/* ioeventfd with no length can't be combined with DATAMATCH */
> +	if (!args->len &&
> +	    args->flags & (KVM_IOEVENTFD_FLAG_PIO |
> +			   KVM_IOEVENTFD_FLAG_DATAMATCH))
> +		return -EINVAL;
> +
> +	return kvm_assign_ioeventfd_idx(kvm, bus_idx, args);
> +}
> +
> +static int
> +kvm_deassign_ioeventfd_idx(struct kvm *kvm, enum kvm_bus bus_idx,
> +			   struct kvm_ioeventfd *args)

While this file uses newline before function name quite often, putting
it on the same line seems more common - don't know which one the
maintainers prefer.

> +{
>  	struct _ioeventfd        *p, *tmp;
>  	struct eventfd_ctx       *eventfd;
>  	int                       ret = -ENOENT;
> 
> -	bus_idx = ioeventfd_bus_from_flags(args->flags);
>  	eventfd = eventfd_ctx_fdget(args->fd);
>  	if (IS_ERR(eventfd))
>  		return PTR_ERR(eventfd);
> @@ -918,6 +926,13 @@ kvm_deassign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
>  	return ret;
>  }
> 
> +static int kvm_deassign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
> +{
> +	enum kvm_bus bus_idx = ioeventfd_bus_from_flags(args->flags);
> +
> +	return kvm_deassign_ioeventfd_idx(kvm, bus_idx, args);
> +}
> +
>  int
>  kvm_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
>  {