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> 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>> wrote:
>>
>> On Fri, Jan 9, 2015 at 9:39 AM, Eric Auger <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
