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 ...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 <eric.auger@xxxxxxxxxx> 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 <eric.auger@xxxxxxxxxx
> >> <mailto:a.motakis@xxxxxxxxxxxxxxxxxxxxxx> <mailto:eric.auger@xxxxxxxxxx>> 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
> >> <a.motakis@xxxxxxxxxxxxxxxxxxxxxx
> <mailto:a.motakis@xxxxxxxxxxxxxxxxxxxxxx>
> <mailto:a.motakis@xxxxxxxxxxxxxxxxxxxxxx>>> wrote:
> >>
> >> On Fri, Jan 9, 2015 at 9:39 AM, Eric Auger
> <eric.auger@xxxxxxxxxx <mailto:eric.auger@xxxxxxxxxx>
> >> <mailto:eric.auger@xxxxxxxxxx
> <mailto:eric.auger@xxxxxxxxxx>>> 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:
> >> >> git@xxxxxxxxxx: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
> >> >>
> >> >
> >>
> >>
> >
>
>
_______________________________________________ kvmarm mailing list kvmarm@xxxxxxxxxxxxxxxxxxxxx https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
