Re: [PATCH 03/10] VFIO: platform: Direct EOI irq bypass for ARM/ARM64

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

On Wed, 24 May 2017 22:13:16 +0200
Eric Auger <eric.auger@redhat.com> wrote:

> This patch adds the registration/unregistration of an
> irq_bypass_producer for vfio platform device interrupts.
> 
> Its callbacks handle the direct EOI modality on VFIO side.
> 
> - stop/start: disable/enable the host irq
> - add/del consumer: set the VFIO Direct EOI mode, ie. select the
>   adapted physical IRQ handler (automasked or not automasked).
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> 
> ---
> ---
>  drivers/vfio/platform/Kconfig                    |   5 +
>  drivers/vfio/platform/Makefile                   |   2 +-
>  drivers/vfio/platform/vfio_platform_irq.c        |  19 ++++
>  drivers/vfio/platform/vfio_platform_irq_bypass.c | 114 +++++++++++++++++++++++
>  drivers/vfio/platform/vfio_platform_private.h    |  23 +++++
>  5 files changed, 162 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/vfio/platform/vfio_platform_irq_bypass.c
> 
> diff --git a/drivers/vfio/platform/Kconfig b/drivers/vfio/platform/Kconfig
> index bb30128..33ec3d9 100644
> --- a/drivers/vfio/platform/Kconfig
> +++ b/drivers/vfio/platform/Kconfig
> @@ -2,6 +2,7 @@ config VFIO_PLATFORM
>  	tristate "VFIO support for platform devices"
>  	depends on VFIO && EVENTFD && (ARM || ARM64)
>  	select VFIO_VIRQFD
> +	select IRQ_BYPASS_MANAGER
>  	help
>  	  Support for platform devices with VFIO. This is required to make
>  	  use of platform devices present on the system using the VFIO
> @@ -19,4 +20,8 @@ config VFIO_AMBA
>  
>  	  If you don't know what to do here, say N.
>  
> +config VFIO_PLATFORM_IRQ_BYPASS_DEOI
> +	depends on VFIO_PLATFORM
> +	def_bool y
> +
>  source "drivers/vfio/platform/reset/Kconfig"
> diff --git a/drivers/vfio/platform/Makefile b/drivers/vfio/platform/Makefile
> index 41a6224..324f3e7 100644
> --- a/drivers/vfio/platform/Makefile
> +++ b/drivers/vfio/platform/Makefile
> @@ -1,4 +1,4 @@
> -vfio-platform-base-y := vfio_platform_common.o vfio_platform_irq.o
> +vfio-platform-base-y := vfio_platform_common.o vfio_platform_irq.o vfio_platform_irq_bypass.o
>  vfio-platform-y := vfio_platform.o
>  
>  obj-$(CONFIG_VFIO_PLATFORM) += vfio-platform.o
> diff --git a/drivers/vfio/platform/vfio_platform_irq.c b/drivers/vfio/platform/vfio_platform_irq.c
> index 2f82459..5b70c8e 100644
> --- a/drivers/vfio/platform/vfio_platform_irq.c
> +++ b/drivers/vfio/platform/vfio_platform_irq.c
> @@ -20,6 +20,7 @@
>  #include <linux/types.h>
>  #include <linux/vfio.h>
>  #include <linux/irq.h>
> +#include <linux/irqbypass.h>
>  
>  #include "vfio_platform_private.h"
>  
> @@ -186,6 +187,19 @@ static irqreturn_t vfio_wrapper_handler(int irq, void *dev_id)
>  	return ret;
>  }
>  
> +/* must be called with irq_ctx->lock held */
> +int vfio_platform_set_deoi(struct vfio_platform_irq *irq_ctx, bool deoi)
> +{
> +	irq_ctx->deoi = deoi;
> +
> +	if (!deoi && (irq_ctx->flags & VFIO_IRQ_INFO_AUTOMASKED))
> +		irq_ctx->handler = vfio_automasked_irq_handler;
> +	else
> +		irq_ctx->handler = vfio_irq_handler;
> +
> +	return 0;
> +}
> +
>  static int vfio_set_trigger(struct vfio_platform_device *vdev, int index,
>  			    int fd, irq_handler_t handler)
>  {
> @@ -196,6 +210,7 @@ static int vfio_set_trigger(struct vfio_platform_device *vdev, int index,
>  	if (irq->trigger) {
>  		irq_clear_status_flags(irq->hwirq, IRQ_NOAUTOEN);
>  		free_irq(irq->hwirq, irq);
> +		irq_bypass_unregister_producer(&irq->producer);
>  		kfree(irq->name);
>  		eventfd_ctx_put(irq->trigger);
>  		irq->trigger = NULL;
> @@ -227,6 +242,10 @@ static int vfio_set_trigger(struct vfio_platform_device *vdev, int index,
>  		return ret;
>  	}
>  
> +	if (vfio_platform_has_deoi())
> +		vfio_platform_register_deoi_producer(vdev, irq,
> +						     trigger, irq->hwirq);
> +
>  	if (!irq->masked)
>  		enable_irq(irq->hwirq);
>  
> diff --git a/drivers/vfio/platform/vfio_platform_irq_bypass.c b/drivers/vfio/platform/vfio_platform_irq_bypass.c
> new file mode 100644
> index 0000000..436902c
> --- /dev/null
> +++ b/drivers/vfio/platform/vfio_platform_irq_bypass.c
> @@ -0,0 +1,114 @@
> +/*
> + * VFIO platform device irqbypass callback implementation for DEOI
> + *
> + * Copyright (C) 2017 Red Hat, Inc.  All rights reserved.
> + * Author: Eric Auger <eric.auger@redhat.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License, version 2, as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/device.h>
> +#include <linux/irq.h>
> +#include <linux/irqbypass.h>
> +#include "vfio_platform_private.h"
> +
> +#ifdef CONFIG_VFIO_PLATFORM_IRQ_BYPASS_DEOI
> +
> +static void irq_bypass_deoi_start(struct irq_bypass_producer *prod)
> +{
> +	enable_irq(prod->irq);
> +}
> +
> +static void irq_bypass_deoi_stop(struct irq_bypass_producer *prod)
> +{
> +	disable_irq(prod->irq);
> +}
> +
> +/**
> + * irq_bypass_deoi_add_consumer - turns irq direct EOI on
> + *
> + * The linux irq is disabled when the function is called.
> + * The operation succeeds only if the irq is not active at irqchip level
> + * and the irq is not automasked at VFIO level, meaning the IRQ is under
> + * injection into the guest.
> + */
> +static int irq_bypass_deoi_add_consumer(struct irq_bypass_producer *prod,
> +					struct irq_bypass_consumer *cons)
> +{
> +	struct vfio_platform_irq *irq_ctx =
> +		container_of(prod, struct vfio_platform_irq, producer);
> +	unsigned long flags;
> +	bool active;
> +	int ret;
> +
> +	spin_lock_irqsave(&irq_ctx->lock, flags);
> +
> +	ret = irq_get_irqchip_state(irq_ctx->hwirq, IRQCHIP_STATE_ACTIVE,
> +				    &active);
> +	if (ret)
> +		goto out;
> +
> +	if (active || irq_ctx->automasked) {
> +		ret = -EAGAIN;
> +		goto out;
> +	}
> +
> +	if (!(irq_get_trigger_type(irq_ctx->hwirq) & IRQ_TYPE_LEVEL_MASK))
> +		goto out;

Not sure why this wouldn't return -EINVAL;

> +
> +	ret = vfio_platform_set_deoi(irq_ctx, true);
> +out:
> +	spin_unlock_irqrestore(&irq_ctx->lock, flags);
> +	return ret;
> +}
> +
> +static void irq_bypass_deoi_del_consumer(struct irq_bypass_producer *prod,
> +					 struct irq_bypass_consumer *cons)
> +{
> +	struct vfio_platform_irq *irq_ctx =
> +		container_of(prod, struct vfio_platform_irq, producer);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&irq_ctx->lock, flags);
> +	if (irq_get_trigger_type(irq_ctx->hwirq) & IRQ_TYPE_LEVEL_MASK)
> +		vfio_platform_set_deoi(irq_ctx, false);
> +	spin_unlock_irqrestore(&irq_ctx->lock, flags);
> +}
> +
> +bool vfio_platform_has_deoi(void)
> +{
> +	return true;
> +}
> +
> +void vfio_platform_register_deoi_producer(struct vfio_platform_device *vdev,
> +					  struct vfio_platform_irq *irq,
> +					  struct eventfd_ctx *trigger,
> +					  unsigned int host_irq)
> +{
> +	struct irq_bypass_producer *prod = &irq->producer;
> +	int ret;
> +
> +	prod->token =		trigger;
> +	prod->irq =		host_irq;
> +	prod->add_consumer =	irq_bypass_deoi_add_consumer;
> +	prod->del_consumer =	irq_bypass_deoi_del_consumer;
> +	prod->stop =		irq_bypass_deoi_stop;
> +	prod->start =		irq_bypass_deoi_start;
> +
> +	ret = irq_bypass_register_producer(prod);
> +	if (unlikely(ret))
> +		dev_info(vdev->device,
> +			 "irq bypass producer (token %p) registration fails: %d\n",
> +			 prod->token, ret);
> +}
> +
> +#endif
> +
> diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
> index b80a380..bfa2675 100644
> --- a/drivers/vfio/platform/vfio_platform_private.h
> +++ b/drivers/vfio/platform/vfio_platform_private.h
> @@ -17,6 +17,7 @@
>  
>  #include <linux/types.h>
>  #include <linux/interrupt.h>
> +#include <linux/irqbypass.h>
>  
>  #define VFIO_PLATFORM_OFFSET_SHIFT   40
>  #define VFIO_PLATFORM_OFFSET_MASK (((u64)(1) << VFIO_PLATFORM_OFFSET_SHIFT) - 1)
> @@ -40,6 +41,7 @@ struct vfio_platform_irq {
>  	struct virqfd		*mask;
>  	bool			deoi;
>  	irqreturn_t		(*handler)(int irq, void *dev_id);
> +	struct irq_bypass_producer producer;
>  };
>  
>  struct vfio_platform_region {
> @@ -102,9 +104,30 @@ extern int vfio_platform_set_irqs_ioctl(struct vfio_platform_device *vdev,
>  					unsigned start, unsigned count,
>  					void *data);
>  
> +extern int vfio_platform_set_deoi(struct vfio_platform_irq *irq_ctx, bool deoi);
> +
>  extern void __vfio_platform_register_reset(struct vfio_platform_reset_node *n);
>  extern void vfio_platform_unregister_reset(const char *compat,
>  					   vfio_platform_reset_fn_t fn);
> +
> +#ifdef CONFIG_VFIO_PLATFORM_IRQ_BYPASS_DEOI
> +bool vfio_platform_has_deoi(void);
> +void vfio_platform_register_deoi_producer(struct vfio_platform_device *vdev,
> +					  struct vfio_platform_irq *irq,
> +					  struct eventfd_ctx *trigger,
> +					  unsigned int host_irq);
> +#else
> +static inline bool vfio_platform_has_deoi(void)
> +{
> +	return false;
> +}
> +static inline
> +void vfio_platform_register_deoi_producer(struct vfio_platform_device *vdev,
> +					  struct vfio_platform_irq *irq,
> +					  struct eventfd_ctx *trigger,
> +					  unsigned int host_irq) {}
> +#endif
> +
>  #define vfio_platform_register_reset(__compat, __reset)		\
>  static struct vfio_platform_reset_node __reset ## _node = {	\
>  	.owner = THIS_MODULE,					\