linux-kernel - Re: [PATCH 02/10] VFIO: platform: Introduce direct EOI interrupt handler

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 31 May 2017 12:20:39 -0600
From:   Alex Williamson <alex.williamson@...hat.com>
To:     Eric Auger <eric.auger@...hat.com>
Cc:     eric.auger.pro@...il.com, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org, kvmarm@...ts.cs.columbia.edu,
        pbonzini@...hat.com, marc.zyngier@....com,
        christoffer.dall@...aro.org, drjones@...hat.com, wei@...hat.com
Subject: Re: [PATCH 02/10] VFIO: platform: Introduce direct EOI interrupt
 handler

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

> We add two new fields in vfio_platform_irq: deoi and handler.
> 
> If deoi is set, this means the physical IRQ attached to the virtual
> IRQ is directly deactivated by the guest and the VFIO driver does
> not need to disable the physical IRQ and mask it at VFIO level.
> 
> The handler pointer points to either the automasked or "deoi" handler.
> This latter is the same as the one used for edge sensitive IRQs.
> A wrapper handler is introduced that calls the chosen handler function.
> 
> The irq lock is taken/released in the wrapper handler. eventfd_signal
> can be called in regions not allowed to sleep.
> 
> Signed-off-by: Eric Auger <eric.auger@...hat.com>
> 
> ---
> ---
>  drivers/vfio/platform/vfio_platform_irq.c     | 24 +++++++++++++++++-------
>  drivers/vfio/platform/vfio_platform_private.h |  2 ++
>  2 files changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/vfio/platform/vfio_platform_irq.c b/drivers/vfio/platform/vfio_platform_irq.c
> index 831f0b0..2f82459 100644
> --- a/drivers/vfio/platform/vfio_platform_irq.c
> +++ b/drivers/vfio/platform/vfio_platform_irq.c
> @@ -148,11 +148,8 @@ static int vfio_platform_set_irq_unmask(struct vfio_platform_device *vdev,
>  static irqreturn_t vfio_automasked_irq_handler(int irq, void *dev_id)
>  {
>  	struct vfio_platform_irq *irq_ctx = dev_id;
> -	unsigned long flags;
>  	int ret = IRQ_NONE;
>  
> -	spin_lock_irqsave(&irq_ctx->lock, flags);
> -
>  	if (!irq_ctx->masked && !irq_ctx->automasked) {
>  		ret = IRQ_HANDLED;
>  
> @@ -161,8 +158,6 @@ static irqreturn_t vfio_automasked_irq_handler(int irq, void *dev_id)
>  		irq_ctx->automasked = true;
>  	}
>  
> -	spin_unlock_irqrestore(&irq_ctx->lock, flags);
> -
>  	if (ret == IRQ_HANDLED)
>  		eventfd_signal(irq_ctx->trigger, 1);
>  
> @@ -178,6 +173,19 @@ static irqreturn_t vfio_irq_handler(int irq, void *dev_id)
>  	return IRQ_HANDLED;
>  }
>  
> +static irqreturn_t vfio_wrapper_handler(int irq, void *dev_id)
> +{
> +	struct vfio_platform_irq *irq_ctx = dev_id;
> +	unsigned long flags;
> +	irqreturn_t ret;
> +
> +	spin_lock_irqsave(&irq_ctx->lock, flags);
> +	ret = irq_ctx->handler(irq, dev_id);
> +	spin_unlock_irqrestore(&irq_ctx->lock, flags);
> +
> +	return ret;
> +}
> +
>  static int vfio_set_trigger(struct vfio_platform_device *vdev, int index,
>  			    int fd, irq_handler_t handler)
>  {
> @@ -208,9 +216,10 @@ static int vfio_set_trigger(struct vfio_platform_device *vdev, int index,
>  	}
>  
>  	irq->trigger = trigger;
> +	irq->handler = handler;
>  
>  	irq_set_status_flags(irq->hwirq, IRQ_NOAUTOEN);
> -	ret = request_irq(irq->hwirq, handler, 0, irq->name, irq);
> +	ret = request_irq(irq->hwirq, vfio_wrapper_handler, 0, irq->name, irq);

Noting here that vfio-platform already requests an exclusive interrupt
(flags = 0), which I think is something that's going to need work for
vfio-pci since we'll request a shared interrupt by default if the
device supports DisINTx masking.  If we're doing this special trick at
the GIC, we'll starve other devices if they share an interrupt.

>  	if (ret) {
>  		kfree(irq->name);
>  		eventfd_ctx_put(trigger);
> @@ -232,7 +241,8 @@ static int vfio_platform_set_irq_trigger(struct vfio_platform_device *vdev,
>  	struct vfio_platform_irq *irq = &vdev->irqs[index];
>  	irq_handler_t handler;
>  
> -	if (vdev->irqs[index].flags & VFIO_IRQ_INFO_AUTOMASKED)
> +	if (vdev->irqs[index].flags & VFIO_IRQ_INFO_AUTOMASKED &&
> +			!irq->deoi)

nit, strange linewrap

>  		handler = vfio_automasked_irq_handler;
>  	else
>  		handler = vfio_irq_handler;
> diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
> index 8a3cfa9..b80a380 100644
> --- a/drivers/vfio/platform/vfio_platform_private.h
> +++ b/drivers/vfio/platform/vfio_platform_private.h
> @@ -38,6 +38,8 @@ struct vfio_platform_irq {
>  	spinlock_t		lock;
>  	struct virqfd		*unmask;
>  	struct virqfd		*mask;
> +	bool			deoi;
> +	irqreturn_t		(*handler)(int irq, void *dev_id);
>  };
>  
>  struct vfio_platform_region {