qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH qemu v6 07/15] vfio: spapr: Add SPAPR IOMMU v2 s


From: Alexey Kardashevskiy
Subject: Re: [Qemu-devel] [PATCH qemu v6 07/15] vfio: spapr: Add SPAPR IOMMU v2 support (DMA memory preregistering)
Date: Wed, 22 Apr 2015 19:38:27 +1000
User-agent: Mozilla/5.0 (X11; Linux i686 on x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0

On 04/22/2015 03:53 PM, David Gibson wrote:
On Sat, Apr 11, 2015 at 01:24:36AM +1000, Alexey Kardashevskiy wrote:
This makes use of the new "memory registering" feature. The idea is
to provide the userspace ability to notify the host kernel about pages
which are going to be used for DMA. Having this information, the host
kernel can pin them all once per user process, do locked pages
accounting (once) and not spent time on doing that in real time with
possible failures which cannot be handled nicely in some cases.

This adds a guest RAM memory listener which notifies a VFIO container
about memory which needs to be pinned/unpinned. VFIO MMIO regions
(i.e. "skip dump" regions) are skipped.

The feature is only enabled for SPAPR IOMMU v2. The host kernel changes
are required. Since v2 does not need/support VFIO_IOMMU_ENABLE, this does
not call it when v2 is detected and enabled.

This does not change the guest visible interface.

Signed-off-by: Alexey Kardashevskiy <address@hidden>

Reviewed-by: David Gibson <address@hidden>

Albeit with some nits described below.

---
Changes:
v6:
* fixed commit log (s/guest/userspace/), added note about no guest visible
change
* fixed error checking if ram registration failed
* added alignment check for section->offset_within_region

v5:
* simplified the patch
* added trace points
* added round_up() for the size
* SPAPR IOMMU v2 used
---
  hw/vfio/common.c              | 26 +++++++++----
  hw/vfio/spapr.c               | 88 ++++++++++++++++++++++++++++++++++++++++++-
  include/hw/vfio/vfio-common.h |  5 ++-
  trace-events                  |  1 +
  4 files changed, 109 insertions(+), 11 deletions(-)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 369e564..9e3e0b0 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -577,14 +577,18 @@ static int vfio_connect_container(VFIOGroup *group, 
AddressSpace *as)

          container->iommu_data.type1.initialized = true;

-    } else if (ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_SPAPR_TCE_IOMMU)) {
+    } else if (ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_SPAPR_TCE_IOMMU) ||
+               ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_SPAPR_TCE_v2_IOMMU)) {
+        bool v2 = !!ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_SPAPR_TCE_v2_IOMMU);
+
          ret = ioctl(group->fd, VFIO_GROUP_SET_CONTAINER, &fd);
          if (ret) {
              error_report("vfio: failed to set group container: %m");
              ret = -errno;
              goto free_container_exit;
          }
-        ret = ioctl(fd, VFIO_SET_IOMMU, VFIO_SPAPR_TCE_IOMMU);
+        ret = ioctl(fd, VFIO_SET_IOMMU,
+                v2 ? VFIO_SPAPR_TCE_v2_IOMMU : VFIO_SPAPR_TCE_IOMMU);
          if (ret) {
              error_report("vfio: failed to set iommu for container: %m");
              ret = -errno;
@@ -596,14 +600,20 @@ static int vfio_connect_container(VFIOGroup *group, 
AddressSpace *as)
           * when container fd is closed so we do not call it explicitly
           * in this file.
           */
-        ret = ioctl(fd, VFIO_IOMMU_ENABLE);
-        if (ret) {
-            error_report("vfio: failed to enable container: %m");
-            ret = -errno;
-            goto free_container_exit;
+        if (!v2) {
+            ret = ioctl(fd, VFIO_IOMMU_ENABLE);
+            if (ret) {
+                error_report("vfio: failed to enable container: %m");
+                ret = -errno;
+                goto free_container_exit;
+            }
          }

-        spapr_memory_listener_register(container);
+        ret = spapr_memory_listener_register(container, v2 ? 2 : 1);
+        if (ret) {
+            error_report("vfio: RAM memory listener initialization failed for 
container");
+            goto listener_release_exit;
+        }

      } else {
          error_report("vfio: No available IOMMU models");
diff --git a/hw/vfio/spapr.c b/hw/vfio/spapr.c
index 5f79194..31353f1 100644
--- a/hw/vfio/spapr.c
+++ b/hw/vfio/spapr.c
@@ -17,6 +17,9 @@
   *  along with this program; if not, see <http://www.gnu.org/licenses/>.
   */

+#include <sys/ioctl.h>
+#include <linux/vfio.h>
+
  #include "hw/vfio/vfio-common.h"
  #include "qemu/error-report.h"
  #include "trace.h"
@@ -211,16 +214,97 @@ static const MemoryListener vfio_spapr_memory_listener = {
      .region_del = vfio_spapr_listener_region_del,
  };

+static void vfio_ram_do_region(VFIOContainer *container,
+                              MemoryRegionSection *section, unsigned long req)
+{
+    int ret;
+    struct vfio_iommu_spapr_register_memory reg = { .argsz = sizeof(reg) };
+
+    if (!memory_region_is_ram(section->mr) ||
+        memory_region_is_skip_dump(section->mr)) {
+        return;
+    }
+
+    if (unlikely((section->offset_within_region & (getpagesize() - 1)))) {
+        error_report("%s received unaligned region", __func__);
+        return;
+    }
+
+    reg.vaddr = (__u64) memory_region_get_ram_ptr(section->mr) +
+        section->offset_within_region;
+    reg.size = ROUND_UP(int128_get64(section->size), TARGET_PAGE_SIZE);
+
+    ret = ioctl(container->fd, req, &reg);
+    trace_vfio_ram_register(_IOC_NR(req) - VFIO_BASE, reg.vaddr, reg.size,
+            ret ? -errno : 0);
+    if (!ret) {
+        return;
+    }
+
+    /*
+     * On the initfn path, store the first error in the container so we
+     * can gracefully fail.  Runtime, there's not much we can do other
+     * than throw a hardware error.
+     */
+    if (!container->iommu_data.spapr.ram_reg_initialized) {
+        if (!container->iommu_data.spapr.ram_reg_error) {
+            container->iommu_data.spapr.ram_reg_error = -errno;
+        }
+    } else {
+        hw_error("vfio: RAM registering failed, unable to continue");
+    }
+}
+
+static void vfio_spapr_ram_listener_region_add(MemoryListener *listener,
+                                               MemoryRegionSection *section)
+{
+    VFIOContainer *container = container_of(listener, VFIOContainer,
+                                            iommu_data.spapr.ramlistener);
+    memory_region_ref(section->mr);
+    vfio_ram_do_region(container, section, VFIO_IOMMU_SPAPR_REGISTER_MEMORY);
+}
+
+static void vfio_spapr_ram_listener_region_del(MemoryListener *listener,
+                                               MemoryRegionSection *section)
+{
+    VFIOContainer *container = container_of(listener, VFIOContainer,
+                                            iommu_data.spapr.ramlistener);
+    memory_region_unref(section->mr);

You're probably safe because of the QBL, but it would be more
obviously correct if you did the ioctl unregister before unreferencing
the MR.


I rather expect the called to pass a referenced section but I'll do what you suggested, cannot hurt.



+    vfio_ram_do_region(container, section, VFIO_IOMMU_SPAPR_UNREGISTER_MEMORY);
+}
+
+static const MemoryListener vfio_spapr_ram_memory_listener = {
+    .region_add = vfio_spapr_ram_listener_region_add,
+    .region_del = vfio_spapr_ram_listener_region_del,
+};
+
  static void vfio_spapr_listener_release(VFIOContainer *container)
  {
      memory_listener_unregister(&container->iommu_data.spapr.listener);
  }

-void spapr_memory_listener_register(VFIOContainer *container)
+static void vfio_spapr_listener_release_v2(VFIOContainer *container)
+{
+    memory_listener_unregister(&container->iommu_data.spapr.listener);
+    vfio_spapr_listener_release(container);
+}
+
+int spapr_memory_listener_register(VFIOContainer *container, int ver)
  {
      container->iommu_data.spapr.listener = vfio_spapr_memory_listener;
      container->iommu_data.release = vfio_spapr_listener_release;
-
      memory_listener_register(&container->iommu_data.spapr.listener,
                               container->space->as);
+    if (ver < 2) {
+        return 0;
+    }

I wonder if it would make sense to store the IOMMU type value into the
VFIOContainer (from non-arch specific code).  It would avoid the
ad-hoc passing of version here and also allow for some sanity checks.


What kind of checks? Cannot think of any which would make sense to do here and not in spapr_(pci|rtas)*.c



+
+    container->iommu_data.spapr.ramlistener = vfio_spapr_ram_memory_listener;
+    container->iommu_data.release = vfio_spapr_listener_release_v2;
+    memory_listener_register(&container->iommu_data.spapr.ramlistener,
+                             &address_space_memory);
+
+    container->iommu_data.spapr.ram_reg_initialized = true;
+
+    return container->iommu_data.spapr.ram_reg_error;
  }
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index d0b831c..b5ef446 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -71,6 +71,9 @@ typedef struct VFIOType1 {

  typedef struct VFIOSPAPR {
      MemoryListener listener;
+    MemoryListener ramlistener;

The names "listener" and "ramlistener" don't exactly give a clear hint
as to what the difference between them is.  Maybe "register_listener"
for the new one since its purpose is to register dma memory.


It is listening for RAM changes. If/when we decide to add something else to this listener, we won't have to change its name :) Still change?


I forget
what the purpose of the old one is, and so what a better name for it
might be.

It is listening on PHB address space for IOMMU table updates and translates those to MAP/UNMAP ioctls to VFIO containers.




+    int ram_reg_error;
+    bool ram_reg_initialized;
  } VFIOSPAPR;

  typedef struct VFIOContainer {
@@ -156,6 +159,6 @@ extern int vfio_dma_unmap(VFIOContainer *container,
                            hwaddr iova, ram_addr_t size);
  bool vfio_listener_skipped_section(MemoryRegionSection *section);

-extern void spapr_memory_listener_register(VFIOContainer *container);
+extern int spapr_memory_listener_register(VFIOContainer *container, int ver);

  #endif /* !HW_VFIO_VFIO_COMMON_H */
diff --git a/trace-events b/trace-events
index 1231ba4..2739140 100644
--- a/trace-events
+++ b/trace-events
@@ -1563,6 +1563,7 @@ vfio_disconnect_container(int fd) "close container->fd=%d"
  vfio_put_group(int fd) "close group->fd=%d"
  vfio_get_device(const char * name, unsigned int flags, unsigned int num_regions, 
unsigned int num_irqs) "Device %s flags: %u, regions: %u, irqs: %u"
  vfio_put_base_device(int fd) "close vdev->fd=%d"
+vfio_ram_register(int req, uint64_t va, uint64_t size, int ret) "req=%d va=%"PRIx64" 
size=%"PRIx64" ret=%d"

  #hw/acpi/memory_hotplug.c
  mhp_acpi_invalid_slot_selected(uint32_t slot) "0x%"PRIx32



--
Alexey



reply via email to

[Prev in Thread] Current Thread [Next in Thread]