This fix should work, in a more elegant way. Could you confirm ?
I'm sorry I don't have any means to reproduce the bug on my side ...
Thanks,
Baptiste
diff --git a/drivers/vfio/platform/vfio_platform_irq.c
b/drivers/vfio/platform/vfio_platform_irq.c
index 6ade36b..f5f3de0 100644
--- a/drivers/vfio/platform/vfio_platform_irq.c
+++ b/drivers/vfio/platform/vfio_platform_irq.c
@@ -208,6 +208,7 @@ static int vfio_set_trigger(struct vfio_platform_device
*vdev, int index,
irq->trigger = trigger;
+ irq_set_status_flags(irq->hwirq, IRQ_NOAUTOEN);
ret = request_irq(irq->hwirq, handler, 0, irq->name, irq);
if (ret) {
kfree(irq->name);
@@ -216,12 +217,8 @@ static int vfio_set_trigger(struct
vfio_platform_device *vdev, int index,
return ret;
}
- /* if the IRQ has been masked by the user before setting an eventfd,
- * then we need to make sure it is properly disabled */
- spin_lock_irqsave(&irq->lock, flags);
- if (irq->masked)
- disable_irq_nosync(irq->hwirq);
- spin_unlock_irqrestore(&irq->lock, flags);
+ if (!irq->masked)
+ enable_irq(irq->hwirq);
return 0;
}
On Mon, Jan 19, 2015 at 7:09 PM, Eric Auger <[email protected]> wrote:
> Hi Baptiste,
>
> yes it fixes the issue in my use case.
>
> Best Regards
>
> Eric
>
> On 01/19/2015 06:00 PM, Baptiste Reynal wrote:
> > Hi Eric,
> >
> > Thanks for taking time about this issue. I agree with you, there is a
> > problem here. While I think on a better fix and to be sure the problem
> > is here, may you try this patch and tell me if the problem is solved ?
> > (This should work as the automasked_irq_handler doesn't do anything if
> > the IRQ is masked).
> >
> > If you have a suggestion on a fix, you're welcome :)
> >
> > diff --git a/drivers/vfio/platform/vfio_platform_irq.c
> > b/drivers/vfio/platform/vfio_platform_irq.c
> > index 6ade36b..c9bac80 100644
> > --- a/drivers/vfio/platform/vfio_platform_irq.c
> > +++ b/drivers/vfio/platform/vfio_platform_irq.c
> > @@ -184,6 +184,7 @@ static int vfio_set_trigger(struct
> > vfio_platform_device *vdev, int index,
> > struct eventfd_ctx *trigger;
> > unsigned long flags;
> > int ret;
> > + bool masked;
> >
> > if (irq->trigger) {
> > free_irq(irq->hwirq, irq);
> > @@ -208,6 +209,8 @@ static int vfio_set_trigger(struct
> > vfio_platform_device *vdev, int index,
> >
> > irq->trigger = trigger;
> >
> > + masked = irq->masked;
> > +
> > ret = request_irq(irq->hwirq, handler, 0, irq->name, irq);
> > if (ret) {
> > kfree(irq->name);
> > @@ -219,7 +222,7 @@ static int vfio_set_trigger(struct
> > vfio_platform_device *vdev, int index,
> > /* if the IRQ has been masked by the user before setting an
> eventfd,
> > * then we need to make sure it is properly disabled */
> > spin_lock_irqsave(&irq->lock, flags);
> > - if (irq->masked)
> > + if (masked)
> > disable_irq_nosync(irq->hwirq);
> > spin_unlock_irqrestore(&irq->lock, flags);
> >
> > On Mon, Jan 19, 2015 at 1:27 PM, Eric Auger <[email protected]
> > <mailto:[email protected]>> wrote:
> >
> > Hi Baptiste,
> >
> > I think what happens on the second qemu run is:
> >
> > an IRQ hits immediatly after request_irq
> > automasked handler sets masked = true
> > in vfio_set_trigger following condition becomes true
> > if (irq->masked)
> > disable_irq_nosync(irq->hwirq);
> > IRQ is disabled twice, in handler and in vfio_set_trigger while
> there is
> > a single enable in resamplefd handler.
> > To me you should prevent from entering ISR between request_irq and
> > disable_irq.
> >
> > It does not happen on the first run because no IRQ hit.
> >
> > Please let me know if you share this understanding.
> >
> > Best Regards
> >
> > Eric
> >
> > On 01/19/2015 11:17 AM, Eric Auger wrote:
> > > Hi Baptiste,
> > >
> > > sorry I was off on Friday. you're right I missed the masked field
> was
> > > reset on init. Nethertheless with current QEMU VFIO code, IRQ runs
> on
> > > the first run and not on the second one. I investigate on my side
> ...
> > >
> > > Best Regards
> > >
> > > Eric
> > >
> > >
> > >
> > >
> > > On 01/16/2015 02:25 PM, Baptiste Reynal wrote:
> > >> Hello Eric,
> > >>
> > >> I'm not sure I understand the issue here. I tried to reproduce
> > the bug
> > >> by triggering an interrupt without unmasking it, but the
> interrupt is
> > >> unmasked when the program access to the device (vfio_platform_open
> > >> reinit IRQs).
> > >>
> > >> May I have more details on the bug ?
> > >>
> > >> Thanks
> > >>
> > >> On Fri, Jan 9, 2015 at 12:33 PM, Antonios Motakis
> > >> <[email protected]
> > <mailto:[email protected]>
> > >> <mailto:[email protected]
> > <mailto:[email protected]>>> wrote:
> > >>
> > >> On Fri, Jan 9, 2015 at 9:39 AM, Eric Auger
> > <[email protected] <mailto:[email protected]>
> > >> <mailto:[email protected]
> > <mailto:[email protected]>>> wrote:
> > >> > Hi Antonios,
> > >> >
> > >> > when moving to 3.19rc3 I observe a regression with my xgmac
> > use case
> > >> > (real-time change?).
> > >> >
> > >> > I guess what happens is when I kill a first qemu session,
> > guest does not
> > >> > have time to complete the virtual IRQ and the unmask is not
> > performed by
> > >> > the virqfd handler. When starting a new QEMU session, the
> irqfd
> > >> > signalling is put in place again but since the masked field
> > is set, the
> > >> > IRQ is left unmasked (v11 modification in vfio_set_trigger).
> > >> >
> > >> > The problem is that we don't discriminate between user mask
> > action and
> > >> > automasked handler action. In case the user did not mask, I
> > think we
> > >> > should reset the masked field when doing the free_irq. What
> > do you think?
> > >>
> > >> Hello Eric,
> > >>
> > >> I agree, we should reset the masked field for future users.
> > >>
> > >> Best regards
> > >> Antonios
> > >>
> > >> >
> > >> > Best Regards
> > >> >
> > >> > Eric
> > >> >
> > >> > On 01/06/2015 11:48 AM, Antonios Motakis wrote:
> > >> >> This patch series aims to implement VFIO support for
> platform
> > >> devices that
> > >> >> reside behind an IOMMU. Examples of such devices are
> devices
> > >> behind an ARM
> > >> >> SMMU, or behind a Samsung Exynos System MMU.
> > >> >>
> > >> >> The API used is based on the existing VFIO API that is
> > also used
> > >> with PCI
> > >> >> devices. Only devices that include a basic set of IRQs and
> > memory
> > >> regions are
> > >> >> targeted; devices with complex relationships with other
> > devices
> > >> on a device
> > >> >> tree are not taken into account at this stage.
> > >> >>
> > >> >> This patch series may be applied on the following
> > series/patches:
> > >> >> - [PATCH] driver core: amba: add device binding path
> > >> 'driver_override'
> > >> >> - [PATCH v3 0/6] vfio: type1: support for ARM SMMUS with
> > >> VFIO_IOMMU_TYPE1
> > >> >>
> > >> >> A copy can be cloned from the branch vfio-platform-v11 at:
> > >> >> [email protected]:virtualopensystems/linux-kvm-arm.git
> > >> >>
> > >> >> Changes since v10:
> > >> >> - Check if interrupt is already masked when setting a new
> > trigger
> > >> >> - Fixed kasprintf with unchecked return value in VFIO
> > AMBA driver
> > >> >> Changes since v9:
> > >> >> - Reworked the splitting of the patches that decouple
> virqfd
> > >> from PCI
> > >> >> - Some styling issues and typos
> > >> >> - Removed superfluous includes
> > >> >> - AMBA devices are now named vfio-amba- suffixed by the
> AMBA
> > >> device id
> > >> >> - Several other cleanups and fixes
> > >> >> Changes since v8:
> > >> >> - Separate irq handler for edge and level triggered
> > interrupts
> > >> >> - Mutex based lock for VFIO fd open/release
> > >> >> - Fixed bug where the first region of a platform device
> > wasn't
> > >> exposed
> > >> >> - Read only regions can be MMAPed only read only
> > >> >> - Code cleanups
> > >> >> Changes since v7:
> > >> >> - Some initial placeholder functionality for PIO resources
> > >> >> - Cleaned up code for IRQ triggering, masking and
> unmasking
> > >> >> - Some functionality has been removed from this series and
> > >> posted separately:
> > >> >> - VFIO_IOMMU_TYPE1 support for ARM SMMUs
> > >> >> - IOMMU NOEXEC patches
> > >> >> - driver_override functionality for AMBA devices
> > >> >> - Several fixes
> > >> >> Changes since v6:
> > >> >> - Integrated support for AMBA devices
> > >> >> - Numerous cleanups and fixes
> > >> >> Changes since v5:
> > >> >> - Full eventfd support for IRQ masking and unmasking.
> > >> >> - Changed IOMMU_EXEC to IOMMU_NOEXEC, along with related
> > flags
> > >> in VFIO.
> > >> >> - Other fixes based on reviewer comments.
> > >> >> Changes since v4:
> > >> >> - Use static offsets for each region in the VFIO device fd
> > >> >> - Include patch in the series for the ARM SMMU to expose
> > IOMMU_EXEC
> > >> >> availability via IOMMU_CAP_DMA_EXEC
> > >> >> - Rebased on VFIO multi domain support:
> > >> >> - IOMMU_EXEC is now available if at least one IOMMU in
> the
> > >> container
> > >> >> supports it
> > >> >> - Expose IOMMU_EXEC if available via the capability
> > >> VFIO_IOMMU_PROT_EXEC
> > >> >> - Some bug fixes
> > >> >> Changes since v3:
> > >> >> - Use Kim Phillips' driver_probe_device()
> > >> >> Changes since v2:
> > >> >> - Fixed Read/Write and MMAP on device regions
> > >> >> - Removed dependency on Device Tree
> > >> >> - Interrupts support
> > >> >> - Interrupt masking/unmasking
> > >> >> - Automask level sensitive interrupts
> > >> >> - Introduced VFIO_DMA_MAP_FLAG_EXEC
> > >> >> - Code clean ups
> > >> >>
> > >> >> Antonios Motakis (20):
> > >> >> vfio/platform: initial skeleton of VFIO support for
> platform
> > >> devices
> > >> >> vfio: platform: probe to devices on the platform bus
> > >> >> vfio: platform: add the VFIO PLATFORM module to Kconfig
> > >> >> vfio: amba: VFIO support for AMBA devices
> > >> >> vfio: amba: add the VFIO for AMBA devices module to
> Kconfig
> > >> >> vfio/platform: return info for bound device
> > >> >> vfio/platform: return info for device memory mapped IO
> > regions
> > >> >> vfio/platform: read and write support for the device fd
> > >> >> vfio/platform: support MMAP of MMIO regions
> > >> >> vfio/platform: return IRQ info
> > >> >> vfio/platform: initial interrupts support code
> > >> >> vfio/platform: trigger an interrupt via eventfd
> > >> >> vfio/platform: support for level sensitive interrupts
> > >> >> vfio: add a vfio_ prefix to virqfd_enable and
> > virqfd_disable and
> > >> >> export
> > >> >> vfio: virqfd: rename vfio_pci_virqfd_init and
> > vfio_pci_virqfd_exit
> > >> >> vfio: add local lock for virqfd instead of depending on
> > VFIO PCI
> > >> >> vfio: pass an opaque pointer on virqfd initialization
> > >> >> vfio: move eventfd support code for VFIO_PCI to a
> > separate file
> > >> >> vfio: initialize the virqfd workqueue in VFIO generic
> code
> > >> >> vfio/platform: implement IRQ masking/unmasking via an
> > eventfd
> > >> >>
> > >> >> drivers/vfio/Kconfig | 1 +
> > >> >> drivers/vfio/Makefile | 5 +-
> > >> >> drivers/vfio/pci/vfio_pci.c | 8 -
> > >> >> drivers/vfio/pci/vfio_pci_intrs.c | 238
> > +-----------
> > >> >> drivers/vfio/pci/vfio_pci_private.h | 3 -
> > >> >> drivers/vfio/platform/Kconfig | 19 +
> > >> >> drivers/vfio/platform/Makefile | 8 +
> > >> >> drivers/vfio/platform/vfio_amba.c | 115 ++++++
> > >> >> drivers/vfio/platform/vfio_platform.c | 103 +++++
> > >> >> drivers/vfio/platform/vfio_platform_common.c | 520
> > >> ++++++++++++++++++++++++++
> > >> >> drivers/vfio/platform/vfio_platform_irq.c | 340
> > >> +++++++++++++++++
> > >> >> drivers/vfio/platform/vfio_platform_private.h | 82 ++++
> > >> >> drivers/vfio/vfio.c | 8 +
> > >> >> drivers/vfio/virqfd.c | 213
> > +++++++++++
> > >> >> include/linux/vfio.h | 27 ++
> > >> >> include/uapi/linux/vfio.h | 2 +
> > >> >> 16 files changed, 1456 insertions(+), 236 deletions(-)
> > >> >> create mode 100644 drivers/vfio/platform/Kconfig
> > >> >> create mode 100644 drivers/vfio/platform/Makefile
> > >> >> create mode 100644 drivers/vfio/platform/vfio_amba.c
> > >> >> create mode 100644 drivers/vfio/platform/vfio_platform.c
> > >> >> create mode 100644
> > drivers/vfio/platform/vfio_platform_common.c
> > >> >> create mode 100644
> drivers/vfio/platform/vfio_platform_irq.c
> > >> >> create mode 100644
> > drivers/vfio/platform/vfio_platform_private.h
> > >> >> create mode 100644 drivers/vfio/virqfd.c
> > >> >>
> > >> >
> > >>
> > >>
> > >
> >
> >
>
>
_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu
