[Qemu-devel] [PATCH qemu v5 0/2] vfio-pci: Allow mmap of MSIX BAR

[Qemu-devel] [PATCH qemu v5 0/2] vfio-pci: Allow mmap of MSIX BAR

[Alexey Kardashevskiy]

This is to enable MSIX BAR mapping to the guest and by the same time
avoid failing on p2p DMA mappings. This attempt makes a try to
separate p2p handling from msix.


This is based on sha1
a6506a9 Suraj Jitindar Singh "target/ppc/spapr: Add H-Call H_GET_CPU_CHARACTERISTICS".

Please comment. Thanks.



Alexey Kardashevskiy (2):
  vfio/common: Add 'p2p' property to enable DMA mapping of MMIO regions
  RFC: vfio-pci: Allow mmap of MSIX BAR

 include/hw/vfio/vfio-common.h |  2 ++
 linux-headers/linux/vfio.h    |  5 +++
 hw/ppc/spapr.c                |  7 ++++
 hw/vfio/common.c              | 74 +++++++++++++++++++++++++++++++++++++++----
 hw/vfio/pci.c                 | 11 +++++++
 5 files changed, 92 insertions(+), 7 deletions(-)

-- 
2.11.0

[Qemu-devel] [PATCH qemu v5 1/2] vfio/common: Add 'p2p' property to enable DMA mapping of MMIO regions

[Alexey Kardashevskiy]

At the moment we map all RAM memory regions for possible DMA including
MMIO regions of passed through device as this might be used for P2P PCI.
However if DMA map fails for whatever reason, we fail and exit QEMU.
Since P2P is not widely used and tested, it makes sense to exclude
MMIO regions from DMA mapping by default and add a flag to enable these
when needed.

This adds a "p2p" option for "vfio-pci" device and
vfio_listener_skipped_section() checks for it so region_add/del() skip
these by default. The MMIO region needs initialized mr::owner which
is set anyway.

This avoids DMA map when start and/or size of the area is not aligned
to the minimal IOMMU page size as it is known to fail; the diagnostic
message is printed in this case.

This avoids exiting QEMU if QEMU tried DMA map and failed for some other
reason that misalignment; this should allow to experiment when needed.

This adds necessary checks to the vfio_listener_region_del() hook.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 include/hw/vfio/vfio-common.h |  1 +
 hw/vfio/common.c              | 59 ++++++++++++++++++++++++++++++++++++++-----
 hw/vfio/pci.c                 |  1 +
 3 files changed, 54 insertions(+), 7 deletions(-)

diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index f3a2ac9..8c7ba75 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -121,6 +121,7 @@ typedef struct VFIODevice {
     bool reset_works;
     bool needs_reset;
     bool no_mmap;
+    bool p2p;
     VFIODeviceOps *ops;
     unsigned int num_irqs;
     unsigned int num_regions;
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 3d652c8..8aaed8d 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -305,7 +305,13 @@ static bool vfio_listener_skipped_section(MemoryRegionSection *section)
             * are never accessed by the CPU and beyond the address width of
             * some IOMMU hardware.  TODO: VFIO should tell us the IOMMU width.
             */
-           section->offset_within_address_space & (1ULL << 63);
+           section->offset_within_address_space & (1ULL << 63) ||
+           /*
+            * Allow mapping of MMIO only if the device has p2p=true.
+            */
+           (memory_region_is_ram_device(section->mr) &&
+            (!section->mr->owner ||
+             !object_property_get_bool(section->mr->owner, "p2p", NULL)));
 }
 
 /* Called with rcu_read_lock held.  */
@@ -508,6 +514,18 @@ static void vfio_listener_region_add(MemoryListener *listener,
     }
 
     /* Here we assume that memory_region_is_ram(section->mr)==true */
+    if (memory_region_is_ram_device(section->mr)) {
+        hwaddr pgmask = (1ULL << ctz64(hostwin->iova_pgsizes)) - 1;
+
+        if ((section->offset_within_region & pgmask) ||
+            (int128_getlo(section->size) & pgmask)) {
+            error_report("Region %"HWADDR_PRIx"..%"HWADDR_PRIx" is not aligned to %"HWADDR_PRIx" and cannot be mapped for DMA",
+                         section->offset_within_region,
+                         int128_getlo(section->size),
+                         pgmask + 1);
+            return;
+        }
+    }
 
     vaddr = memory_region_get_ram_ptr(section->mr) +
             section->offset_within_region +
@@ -523,6 +541,9 @@ static void vfio_listener_region_add(MemoryListener *listener,
         error_report("vfio_dma_map(%p, 0x%"HWADDR_PRIx", "
                      "0x%"HWADDR_PRIx", %p) = %d (%m)",
                      container, iova, int128_get64(llsize), vaddr, ret);
+        if (memory_region_is_ram_device(section->mr)) {
+            return;
+        }
         goto fail;
     }
 
@@ -550,6 +571,7 @@ static void vfio_listener_region_del(MemoryListener *listener,
     hwaddr iova, end;
     Int128 llend, llsize;
     int ret;
+    bool try_unmap = true;
 
     if (vfio_listener_skipped_section(section)) {
         trace_vfio_listener_region_del_skip(
@@ -602,13 +624,36 @@ static void vfio_listener_region_del(MemoryListener *listener,
 
     trace_vfio_listener_region_del(iova, end);
 
-    ret = vfio_dma_unmap(container, iova, int128_get64(llsize));
+    if (memory_region_is_ram_device(section->mr)) {
+        hwaddr pgmask;
+        VFIOHostDMAWindow *hostwin;
+        bool hostwin_found;
+
+        hostwin_found = false;
+        QLIST_FOREACH(hostwin, &container->hostwin_list, hostwin_next) {
+            if (hostwin->min_iova <= iova && end <= hostwin->max_iova) {
+                hostwin_found = true;
+                break;
+            }
+        }
+        assert(hostwin_found);
+
+        pgmask = (1ULL << ctz64(hostwin->iova_pgsizes)) - 1;
+
+        try_unmap = !(section->offset_within_region & pgmask) &&
+            !(int128_getlo(section->size) & pgmask);
+    }
+
+    if (try_unmap) {
+        ret = vfio_dma_unmap(container, iova, int128_get64(llsize));
+        if (ret) {
+            error_report("vfio_dma_unmap(%p, 0x%"HWADDR_PRIx", "
+                         "0x%"HWADDR_PRIx") = %d (%m)",
+                         container, iova, int128_get64(llsize), ret);
+        }
+    }
+
     memory_region_unref(section->mr);
-    if (ret) {
-        error_report("vfio_dma_unmap(%p, 0x%"HWADDR_PRIx", "
-                     "0x%"HWADDR_PRIx") = %d (%m)",
-                     container, iova, int128_get64(llsize), ret);
-    }
 
     if (container->iommu_type == VFIO_SPAPR_TCE_v2_IOMMU) {
         vfio_spapr_remove_window(container,
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 2c71295..5b20620 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2999,6 +2999,7 @@ static Property vfio_pci_dev_properties[] = {
     DEFINE_PROP_UNSIGNED_NODEFAULT("x-nv-gpudirect-clique", VFIOPCIDevice,
                                    nv_gpudirect_clique,
                                    qdev_prop_nv_gpudirect_clique, uint8_t),
+    DEFINE_PROP_BOOL("p2p", VFIOPCIDevice, vbasedev.p2p, false),
     /*
      * TODO - support passed fds... is this necessary?
      * DEFINE_PROP_STRING("vfiofd", VFIOPCIDevice, vfiofd_name),
-- 
2.11.0

[Qemu-devel] [PATCH qemu v5 2/2] RFC: vfio-pci: Allow mmap of MSIX BAR

[Alexey Kardashevskiy]

This makes use of a new VFIO_REGION_INFO_CAP_MSIX_MAPPABLE capability
which tells that a region with MSIX data can be mapped entirely, i.e.
the VFIO PCI driver won't prevent MSIX vectors area from being mapped.

With this change, all BARs are mapped in a single chunk and MSIX vectors
are emulated on top unless the machine requests not to by defining and
enabling a new "vfio-no-msix-emulation" property. At the moment only
sPAPR machine does so - it prohibits MSIX emulation and does not allow
enabling it as it does not define the "set" callback for the new property;
the new property also does not appear in "-machine pseries,help".

This requires the kernel change - "vfio-pci: Allow mapping MSIX BAR" -
for the new capability: https://www.spinics.net/lists/kvm/msg160282.html

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
Changes:
v5:
* rebased on top of 'p2p' proposed patch

v4:
* silenced dma map errors if unaligned mapping is attempted - they are going
to fail anyway

v3:
* vfio_listener_region_add() won't make qemu exit if failed on MMIO MR
---
 include/hw/vfio/vfio-common.h |  1 +
 linux-headers/linux/vfio.h    |  5 +++++
 hw/ppc/spapr.c                |  7 +++++++
 hw/vfio/common.c              | 15 +++++++++++++++
 hw/vfio/pci.c                 | 10 ++++++++++
 5 files changed, 38 insertions(+)

diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index 8c7ba75..3ef9f8e 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -172,6 +172,7 @@ int vfio_get_region_info(VFIODevice *vbasedev, int index,
                          struct vfio_region_info **info);
 int vfio_get_dev_region_info(VFIODevice *vbasedev, uint32_t type,
                              uint32_t subtype, struct vfio_region_info **info);
+bool vfio_is_cap_present(VFIODevice *vbasedev, uint16_t cap_type, int region);
 #endif
 extern const MemoryListener vfio_prereg_listener;
 
diff --git a/linux-headers/linux/vfio.h b/linux-headers/linux/vfio.h
index 4312e96..b45182e 100644
--- a/linux-headers/linux/vfio.h
+++ b/linux-headers/linux/vfio.h
@@ -301,6 +301,11 @@ struct vfio_region_info_cap_type {
 #define VFIO_REGION_SUBTYPE_INTEL_IGD_HOST_CFG	(2)
 #define VFIO_REGION_SUBTYPE_INTEL_IGD_LPC_CFG	(3)
 
+/*
+ * The MSIX mappable capability informs that MSIX data of a BAR can be mmapped.
+ */
+#define VFIO_REGION_INFO_CAP_MSIX_MAPPABLE	3
+
 /**
  * VFIO_DEVICE_GET_IRQ_INFO - _IOWR(VFIO_TYPE, VFIO_BASE + 9,
  *				    struct vfio_irq_info)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 32a876b..6d333e2 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2830,6 +2830,11 @@ static void spapr_set_modern_hotplug_events(Object *obj, bool value,
     spapr->use_hotplug_event_source = value;
 }
 
+static bool spapr_get_msix_emulation(Object *obj, Error **errp)
+{
+    return true;
+}
+
 static char *spapr_get_resize_hpt(Object *obj, Error **errp)
 {
     sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
@@ -2911,6 +2916,8 @@ static void spapr_instance_init(Object *obj)
     object_property_set_description(obj, "vsmt",
                                     "Virtual SMT: KVM behaves as if this were"
                                     " the host's SMT mode", &error_abort);
+    object_property_add_bool(obj, "vfio-no-msix-emulation",
+                             spapr_get_msix_emulation, NULL, NULL);
 }
 
 static void spapr_machine_finalizefn(Object *obj)
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 8aaed8d..18b98e8 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1431,6 +1431,21 @@ int vfio_get_dev_region_info(VFIODevice *vbasedev, uint32_t type,
     return -ENODEV;
 }
 
+bool vfio_is_cap_present(VFIODevice *vbasedev, uint16_t cap_type, int region)
+{
+    struct vfio_region_info *info = NULL;
+    bool ret = false;
+
+    if (!vfio_get_region_info(vbasedev, region, &info)) {
+        if (vfio_get_region_info_cap(info, cap_type)) {
+            ret = true;
+        }
+        g_free(info);
+    }
+
+    return ret;
+}
+
 /*
  * Interfaces for IBM EEH (Enhanced Error Handling)
  */
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 5b20620..87a186f 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1289,6 +1289,11 @@ static void vfio_pci_fixup_msix_region(VFIOPCIDevice *vdev)
     off_t start, end;
     VFIORegion *region = &vdev->bars[vdev->msix->table_bar].region;
 
+    if (vfio_is_cap_present(&vdev->vbasedev, VFIO_REGION_INFO_CAP_MSIX_MAPPABLE,
+                            vdev->msix->table_bar)) {
+        return;
+    }
+
     /*
      * We expect to find a single mmap covering the whole BAR, anything else
      * means it's either unsupported or already setup.
@@ -1473,6 +1478,11 @@ static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
      */
     memory_region_set_enabled(&vdev->pdev.msix_pba_mmio, false);
 
+    if (object_property_get_bool(OBJECT(qdev_get_machine()),
+                                 "vfio-no-msix-emulation", NULL)) {
+        memory_region_set_enabled(&vdev->pdev.msix_table_mmio, false);
+    }
+
     return 0;
 }
 
-- 
2.11.0

Re: [Qemu-devel] [PATCH qemu v5 0/2] vfio-pci: Allow mmap of MSIX BAR

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20180130035527.47336-1-aik@ozlabs.ru
Subject: [Qemu-devel] [PATCH qemu v5 0/2] vfio-pci: Allow mmap of MSIX BAR

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
261b4a1327 RFC: vfio-pci: Allow mmap of MSIX BAR
efd447736c vfio/common: Add 'p2p' property to enable DMA mapping of MMIO regions

=== OUTPUT BEGIN ===
Checking PATCH 1/2: vfio/common: Add 'p2p' property to enable DMA mapping of MMIO regions...
ERROR: line over 90 characters
#59: FILE: hw/vfio/common.c:523:
+            error_report("Region %"HWADDR_PRIx"..%"HWADDR_PRIx" is not aligned to %"HWADDR_PRIx" and cannot be mapped for DMA",

total: 1 errors, 0 warnings, 105 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 2/2: RFC: vfio-pci: Allow mmap of MSIX BAR...
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [PATCH qemu v5 1/2] vfio/common: Add 'p2p' property to enable DMA mapping of MMIO regions

[Alex Williamson]

On Tue, 30 Jan 2018 14:55:26 +1100
Alexey Kardashevskiy <aik@ozlabs.ru> wrote:

> At the moment we map all RAM memory regions for possible DMA including
> MMIO regions of passed through device as this might be used for P2P PCI.
> However if DMA map fails for whatever reason, we fail and exit QEMU.
> Since P2P is not widely used and tested, it makes sense to exclude
> MMIO regions from DMA mapping by default and add a flag to enable these
> when needed.

No, it doesn't.  You're disabling an intentionally added feature with
no evidence that it doesn't work and isn't in use and placing the
burden on the user to discover this change and re-enable it.  This
has really gone off into the weeds.  NAK.  Thanks,

Alex
 
> This adds a "p2p" option for "vfio-pci" device and
> vfio_listener_skipped_section() checks for it so region_add/del() skip
> these by default. The MMIO region needs initialized mr::owner which
> is set anyway.
> 
> This avoids DMA map when start and/or size of the area is not aligned
> to the minimal IOMMU page size as it is known to fail; the diagnostic
> message is printed in this case.
> 
> This avoids exiting QEMU if QEMU tried DMA map and failed for some other
> reason that misalignment; this should allow to experiment when needed.
> 
> This adds necessary checks to the vfio_listener_region_del() hook.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>  include/hw/vfio/vfio-common.h |  1 +
>  hw/vfio/common.c              | 59 ++++++++++++++++++++++++++++++++++++++-----
>  hw/vfio/pci.c                 |  1 +
>  3 files changed, 54 insertions(+), 7 deletions(-)
> 
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index f3a2ac9..8c7ba75 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -121,6 +121,7 @@ typedef struct VFIODevice {
>      bool reset_works;
>      bool needs_reset;
>      bool no_mmap;
> +    bool p2p;
>      VFIODeviceOps *ops;
>      unsigned int num_irqs;
>      unsigned int num_regions;
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 3d652c8..8aaed8d 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -305,7 +305,13 @@ static bool vfio_listener_skipped_section(MemoryRegionSection *section)
>              * are never accessed by the CPU and beyond the address width of
>              * some IOMMU hardware.  TODO: VFIO should tell us the IOMMU width.
>              */
> -           section->offset_within_address_space & (1ULL << 63);
> +           section->offset_within_address_space & (1ULL << 63) ||
> +           /*
> +            * Allow mapping of MMIO only if the device has p2p=true.
> +            */
> +           (memory_region_is_ram_device(section->mr) &&
> +            (!section->mr->owner ||
> +             !object_property_get_bool(section->mr->owner, "p2p", NULL)));
>  }
>  
>  /* Called with rcu_read_lock held.  */
> @@ -508,6 +514,18 @@ static void vfio_listener_region_add(MemoryListener *listener,
>      }
>  
>      /* Here we assume that memory_region_is_ram(section->mr)==true */
> +    if (memory_region_is_ram_device(section->mr)) {
> +        hwaddr pgmask = (1ULL << ctz64(hostwin->iova_pgsizes)) - 1;
> +
> +        if ((section->offset_within_region & pgmask) ||
> +            (int128_getlo(section->size) & pgmask)) {
> +            error_report("Region %"HWADDR_PRIx"..%"HWADDR_PRIx" is not aligned to %"HWADDR_PRIx" and cannot be mapped for DMA",
> +                         section->offset_within_region,
> +                         int128_getlo(section->size),
> +                         pgmask + 1);
> +            return;
> +        }
> +    }
>  
>      vaddr = memory_region_get_ram_ptr(section->mr) +
>              section->offset_within_region +
> @@ -523,6 +541,9 @@ static void vfio_listener_region_add(MemoryListener *listener,
>          error_report("vfio_dma_map(%p, 0x%"HWADDR_PRIx", "
>                       "0x%"HWADDR_PRIx", %p) = %d (%m)",
>                       container, iova, int128_get64(llsize), vaddr, ret);
> +        if (memory_region_is_ram_device(section->mr)) {
> +            return;
> +        }
>          goto fail;
>      }
>  
> @@ -550,6 +571,7 @@ static void vfio_listener_region_del(MemoryListener *listener,
>      hwaddr iova, end;
>      Int128 llend, llsize;
>      int ret;
> +    bool try_unmap = true;
>  
>      if (vfio_listener_skipped_section(section)) {
>          trace_vfio_listener_region_del_skip(
> @@ -602,13 +624,36 @@ static void vfio_listener_region_del(MemoryListener *listener,
>  
>      trace_vfio_listener_region_del(iova, end);
>  
> -    ret = vfio_dma_unmap(container, iova, int128_get64(llsize));
> +    if (memory_region_is_ram_device(section->mr)) {
> +        hwaddr pgmask;
> +        VFIOHostDMAWindow *hostwin;
> +        bool hostwin_found;
> +
> +        hostwin_found = false;
> +        QLIST_FOREACH(hostwin, &container->hostwin_list, hostwin_next) {
> +            if (hostwin->min_iova <= iova && end <= hostwin->max_iova) {
> +                hostwin_found = true;
> +                break;
> +            }
> +        }
> +        assert(hostwin_found);
> +
> +        pgmask = (1ULL << ctz64(hostwin->iova_pgsizes)) - 1;
> +
> +        try_unmap = !(section->offset_within_region & pgmask) &&
> +            !(int128_getlo(section->size) & pgmask);
> +    }
> +
> +    if (try_unmap) {
> +        ret = vfio_dma_unmap(container, iova, int128_get64(llsize));
> +        if (ret) {
> +            error_report("vfio_dma_unmap(%p, 0x%"HWADDR_PRIx", "
> +                         "0x%"HWADDR_PRIx") = %d (%m)",
> +                         container, iova, int128_get64(llsize), ret);
> +        }
> +    }
> +
>      memory_region_unref(section->mr);
> -    if (ret) {
> -        error_report("vfio_dma_unmap(%p, 0x%"HWADDR_PRIx", "
> -                     "0x%"HWADDR_PRIx") = %d (%m)",
> -                     container, iova, int128_get64(llsize), ret);
> -    }
>  
>      if (container->iommu_type == VFIO_SPAPR_TCE_v2_IOMMU) {
>          vfio_spapr_remove_window(container,
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 2c71295..5b20620 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2999,6 +2999,7 @@ static Property vfio_pci_dev_properties[] = {
>      DEFINE_PROP_UNSIGNED_NODEFAULT("x-nv-gpudirect-clique", VFIOPCIDevice,
>                                     nv_gpudirect_clique,
>                                     qdev_prop_nv_gpudirect_clique, uint8_t),
> +    DEFINE_PROP_BOOL("p2p", VFIOPCIDevice, vbasedev.p2p, false),
>      /*
>       * TODO - support passed fds... is this necessary?
>       * DEFINE_PROP_STRING("vfiofd", VFIOPCIDevice, vfiofd_name),