Re: [PATCH v11 00/20] VFIO support for platform and ARM AMBA devices

[Baptiste Reynal <b.reynal-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org>]

[-- Attachment #1.1: Type: text/plain, Size: 15098 bytes --]

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 <eric.auger-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> 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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org
> > <mailto:eric.auger-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>> 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-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org
> >     <mailto:a.motakis-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org>
> >     >> <mailto:a.motakis-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org
> >     <mailto:a.motakis-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org>>> wrote:
> >     >>
> >     >>     On Fri, Jan 9, 2015 at 9:39 AM, Eric Auger
> >     <eric.auger-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org <mailto:eric.auger-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >     >>     <mailto:eric.auger-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org
> >     <mailto:eric.auger-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>>> 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-9UaJU3cA/F/QT0dZR+AlfA@public.gmane.org: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
> >     >>     >>
> >     >>     >
> >     >>
> >     >>
> >     >
> >
> >
>
>

[-- Attachment #1.2: Type: text/html, Size: 21982 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]