qemu-ppc
[Top][All Lists]
Advanced

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

Re: [Qemu-ppc] [PATCH qemu v10 13/14] vfio: spapr: Add SPAPR IOMMU v2 su


From: Alexey Kardashevskiy
Subject: Re: [Qemu-ppc] [PATCH qemu v10 13/14] vfio: spapr: Add SPAPR IOMMU v2 support (DMA memory preregistering)
Date: Tue, 7 Jul 2015 22:11:46 +1000
User-agent: Mozilla/5.0 (X11; Linux i686 on x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.0.1

On 07/07/2015 02:13 AM, Alex Williamson wrote:
On Tue, 2015-07-07 at 01:34 +1000, Alexey Kardashevskiy wrote:
On 07/06/2015 11:42 PM, Alex Williamson wrote:
On Mon, 2015-07-06 at 12:11 +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>
---
Changes:
v9:
* since there is no more SPAPR-specific data in container::iommu_data,
the memory preregistration fields are common and potentially can be used
by other architectures

v7:
* in vfio_spapr_ram_listener_region_del(), do unref() after ioctl()
* s'ramlistener'register_listener'

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              | 109 
++++++++++++++++++++++++++++++++++++++----
   include/hw/vfio/vfio-common.h |   3 ++
   trace-events                  |   1 +
   3 files changed, 104 insertions(+), 9 deletions(-)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 8eacfd7..0c7ba8c 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -488,6 +488,76 @@ static void vfio_listener_release(VFIOContainer *container)
       memory_listener_unregister(&container->iommu_data.type1.listener);
   }

+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) };

This function is not as general as the name would imply, it's spapr
specific due to this.  How about vfio_spapr_register_memory() with a
bool parameter toggling register vs unregister so we're not passing an
arbitrary ioctl number?

Ok. Although I am quite often asked not to do such a thing and rather add 2
helpers (reg/unreg, do/undo, etc) instead and reuse common bits.

I'm not a fan of functions that do the reverse process based on a bool
arg either, but I dislike them less than passing an arbitrary ioctl
number for a parameter.  The former is ugly, but the latter is difficult
to use and difficult to maintain because it would be subtle later to
spot an unsupported ioctl being passed to the function.

+
+    if (!memory_region_is_ram(section->mr) ||
+        memory_region_is_skip_dump(section->mr)) {
+        return;
+    }
+
+    if (unlikely((section->offset_within_region & (getpagesize() - 1)))) {

s/getpagesize()/qemu_real_host_page_size/?


Oh, right, I guess it reached upstream now.


+        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.ram_reg_initialized) {
+        if (!container->iommu_data.ram_reg_error) {
+            container->iommu_data.ram_reg_error = -errno;
+        }
+    } else {
+        hw_error("vfio: RAM registering failed, unable to continue");
+    }

I'd rather see:

if (ret) {
    if (!container...) {
      ...
    } else {
      ...
    }
}

Exiting early on success and otherwise falling into error handling is a
strange code flow.

Ok... vfio_dma_map() does not follow this rule so I thought it is not that
strict :)

It would be nice to clean it up there too.

+}
+
+static void vfio_ram_listener_region_add(MemoryListener *listener,
+                                         MemoryRegionSection *section)
+{
+    VFIOContainer *container = container_of(listener, VFIOContainer,
+                                            iommu_data.register_listener);
+    memory_region_ref(section->mr);
+    vfio_ram_do_region(container, section, VFIO_IOMMU_SPAPR_REGISTER_MEMORY);

vfio_spapr_register_memory(container, section, true);

+}
+
+static void vfio_ram_listener_region_del(MemoryListener *listener,
+                                         MemoryRegionSection *section)
+{
+    VFIOContainer *container = container_of(listener, VFIOContainer,
+                                            iommu_data.register_listener);
+    vfio_ram_do_region(container, section, VFIO_IOMMU_SPAPR_UNREGISTER_MEMORY);

vfio_spapr_register_memory(container, section, false);

+    memory_region_unref(section->mr);
+}
+
+static const MemoryListener vfio_ram_memory_listener = {
+    .region_add = vfio_ram_listener_region_add,
+    .region_del = vfio_ram_listener_region_del,
+};

These are all spapr specific, please reflect that in the name;
vfio_spapr_v2_memory_listener, vfio_spapr_v2_listener_add/del.

ok.


Actually, can't we determine what type of IOMMU we have and make the
existing MemoryListener handle either type1 or spapr or spapr-v2?


Sorry, I do not follow you here. How? The existing listener listens on PCI
address space (at least, on pseries), new one listens on RAM address space
(address_space_memory). What do I miss?

Isn't that simply a difference of the address space the listener is
attached to?  Type1 maps RAM, spapr-v1 maps guest IOMMU space and these
are already both handled by the same listener.


Ok, I tried merging 2 listeners and realized that the PCI listener works with TARGET_PAGE_SIZE granularity (which is 4K and actually it should be using an IOMMU page size which is not easily available there but this is a different story) and RAM listener with the qemu_real_host_page_size granularity (64K for my case) so depending on the address space type, vfio_listener_region_add() will have to use different page sizes. I like the idea of merging less now...



--
Alexey



reply via email to

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