Re: [Qemu-devel] [PATCH qemu v17 07/12] vfio: spapr: Add DMA memory preregistering (SPAPR IOMMU v2)

[Alex Williamson <alex.williamson@redhat.com>]

On Mon, 6 Jun 2016 16:04:57 +1000
Alexey Kardashevskiy <aik@ozlabs.ru> wrote:

> On 04/06/16 02:13, Alex Williamson wrote:
> > On Wed,  1 Jun 2016 18:57:38 +1000
> > Alexey Kardashevskiy <aik@ozlabs.ru> 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 prereg memory listener which listens on address_space_memory
> >> and notifies a VFIO container about memory which needs to be
> >> pinned/unpinned. VFIO MMIO regions (i.e. "skip dump" regions) are skipped.
> >>
> >> As there is no per-IOMMU-type release() callback anymore, this stores
> >> the IOMMU type in the container so vfio_listener_release() can determine
> >> if it needs to unregister @prereg_listener.
> >>
> >> 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 enforces guest RAM blocks to be host page size aligned; however
> >> this is not new as KVM already requires memory slots to be host page
> >> size aligned.
> >>
> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >> ---
> >> Changes:
> >> v17:
> >> * s/prereg\.c/spapr.c/
> >> * s/vfio_prereg_gpa_to_ua/vfio_prereg_gpa_to_vaddr/
> >> * vfio_prereg_listener_skipped_section does hw_error() on IOMMUs
> >>
> >> v16:
> >> * switched to 64bit math everywhere as there is no chance to see
> >> region_add on RAM blocks even remotely close to 1<<64bytes.
> >>
> >> v15:
> >> * banned unaligned sections
> >> * added an vfio_prereg_gpa_to_ua() helper
> >>
> >> v14:
> >> * s/free_container_exit/listener_release_exit/g
> >> * added "if memory_region_is_iommu()" to vfio_prereg_listener_skipped_section
> >> ---
> >>  hw/vfio/Makefile.objs         |   1 +
> >>  hw/vfio/common.c              |  38 +++++++++---
> >>  hw/vfio/spapr.c               | 137 ++++++++++++++++++++++++++++++++++++++++++
> >>  include/hw/vfio/vfio-common.h |   4 ++
> >>  trace-events                  |   2 +
> >>  5 files changed, 172 insertions(+), 10 deletions(-)
> >>  create mode 100644 hw/vfio/spapr.c
> >>
> >> diff --git a/hw/vfio/Makefile.objs b/hw/vfio/Makefile.objs
> >> index ceddbb8..c25e32b 100644
> >> --- a/hw/vfio/Makefile.objs
> >> +++ b/hw/vfio/Makefile.objs
> >> @@ -4,4 +4,5 @@ obj-$(CONFIG_PCI) += pci.o pci-quirks.o
> >>  obj-$(CONFIG_SOFTMMU) += platform.o
> >>  obj-$(CONFIG_SOFTMMU) += calxeda-xgmac.o
> >>  obj-$(CONFIG_SOFTMMU) += amd-xgbe.o
> >> +obj-$(CONFIG_SOFTMMU) += spapr.o
> >>  endif
> >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> >> index f1a12b0..770f630 100644
> >> --- a/hw/vfio/common.c
> >> +++ b/hw/vfio/common.c
> >> @@ -504,6 +504,9 @@ static const MemoryListener vfio_memory_listener = {
> >>  static void vfio_listener_release(VFIOContainer *container)
> >>  {
> >>      memory_listener_unregister(&container->listener);
> >> +    if (container->iommu_type == VFIO_SPAPR_TCE_v2_IOMMU) {
> >> +        memory_listener_unregister(&container->prereg_listener);
> >> +    }
> >>  }
> >>  
> >>  static struct vfio_info_cap_header *
> >> @@ -862,8 +865,8 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
> >>              goto free_container_exit;
> >>          }
> >>  
> >> -        ret = ioctl(fd, VFIO_SET_IOMMU,
> >> -                    v2 ? VFIO_TYPE1v2_IOMMU : VFIO_TYPE1_IOMMU);
> >> +        container->iommu_type = v2 ? VFIO_TYPE1v2_IOMMU : VFIO_TYPE1_IOMMU;
> >> +        ret = ioctl(fd, VFIO_SET_IOMMU, container->iommu_type);
> >>          if (ret) {
> >>              error_report("vfio: failed to set iommu for container: %m");
> >>              ret = -errno;
> >> @@ -888,8 +891,10 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
> >>          if ((ret == 0) && (info.flags & VFIO_IOMMU_INFO_PGSIZES)) {
> >>              container->iova_pgsizes = info.iova_pgsizes;
> >>          }
> >> -    } 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)) {
> >>          struct vfio_iommu_spapr_tce_info info;
> >> +        bool v2 = !!ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_SPAPR_TCE_v2_IOMMU);
> >>  
> >>          ret = ioctl(group->fd, VFIO_GROUP_SET_CONTAINER, &fd);
> >>          if (ret) {
> >> @@ -897,7 +902,9 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
> >>              ret = -errno;
> >>              goto free_container_exit;
> >>          }
> >> -        ret = ioctl(fd, VFIO_SET_IOMMU, VFIO_SPAPR_TCE_IOMMU);
> >> +        container->iommu_type =
> >> +            v2 ? VFIO_SPAPR_TCE_v2_IOMMU : VFIO_SPAPR_TCE_IOMMU;
> >> +        ret = ioctl(fd, VFIO_SET_IOMMU, container->iommu_type);
> >>          if (ret) {
> >>              error_report("vfio: failed to set iommu for container: %m");
> >>              ret = -errno;
> >> @@ -909,11 +916,22 @@ 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;
> >> +            }
> >> +        } else {
> >> +            container->prereg_listener = vfio_prereg_listener;
> >> +
> >> +            memory_listener_register(&container->prereg_listener,
> >> +                                     &address_space_memory);
> >> +            if (container->error) {
> >> +                error_report("vfio: RAM memory listener initialization failed for container");
> >> +                goto listener_release_exit;  
> > 
> > Why doesn't this goto free_container_exit?  registration failure should
> > not need an unregister.  
> 
> 
> The listener registration cannot possibly fail, it adds a listener into the
> memory_listeners list, no matter what region_add() does.

Oops, right.

> 
> I'll add an explicit
> memory_listener_unregister(&container->prereg_listener) here.

Ok.

> >   
> >> +            }
> >>          }
> >>  
> >>          /*
> >> @@ -926,7 +944,7 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
> >>          if (ret) {
> >>              error_report("vfio: VFIO_IOMMU_SPAPR_TCE_GET_INFO failed: %m");
> >>              ret = -errno;
> >> -            goto free_container_exit;
> >> +            goto listener_release_exit;  
> > 
> > Looks like this will cause much badness when we try to do
> > memory_listener_unregister() on an empty listener struct for the main
> > listener.  
> 
> 
> Oh. Bug. I'll add
>  memory_listener_unregister(&container->prereg_listener) here.
> 
> 
> >   
> >>          }
> >>          container->min_iova = info.dma32_window_start;
> >>          container->max_iova = container->min_iova + info.dma32_window_size - 1;
> >> diff --git a/hw/vfio/spapr.c b/hw/vfio/spapr.c
> >> new file mode 100644
> >> index 0000000..f339472
> >> --- /dev/null
> >> +++ b/hw/vfio/spapr.c
> >> @@ -0,0 +1,137 @@
> >> +/*
> >> + * DMA memory preregistration
> >> + *
> >> + * Authors:
> >> + *  Alexey Kardashevskiy <aik@ozlabs.ru>
> >> + *
> >> + * This work is licensed under the terms of the GNU GPL, version 2.  See
> >> + * the COPYING file in the top-level directory.
> >> + */
> >> +
> >> +#include "qemu/osdep.h"
> >> +#include "cpu.h"
> >> +#include <sys/ioctl.h>
> >> +#include <linux/vfio.h>
> >> +
> >> +#include "hw/vfio/vfio-common.h"
> >> +#include "hw/hw.h"
> >> +#include "qemu/error-report.h"
> >> +#include "trace.h"
> >> +
> >> +static bool vfio_prereg_listener_skipped_section(MemoryRegionSection *section)
> >> +{
> >> +    if (memory_region_is_iommu(section->mr)) {
> >> +        hw_error("Cannot possibly preregister IOMMU memory");
> >> +    }
> >> +
> >> +    return !memory_region_is_ram(section->mr) ||
> >> +            memory_region_is_skip_dump(section->mr);
> >> +}
> >> +
> >> +static void *vfio_prereg_gpa_to_vaddr(MemoryRegionSection *section, hwaddr gpa)
> >> +{
> >> +    return memory_region_get_ram_ptr(section->mr) +
> >> +        section->offset_within_region +
> >> +        (gpa - section->offset_within_address_space);
> >> +}
> >> +
> >> +static void vfio_prereg_listener_region_add(MemoryListener *listener,
> >> +                                            MemoryRegionSection *section)
> >> +{
> >> +    VFIOContainer *container = container_of(listener, VFIOContainer,
> >> +                                            prereg_listener);
> >> +    const hwaddr gpa = section->offset_within_address_space;
> >> +    hwaddr end;
> >> +    int ret;
> >> +    hwaddr page_mask = qemu_real_host_page_mask;
> >> +    struct vfio_iommu_spapr_register_memory reg = {
> >> +        .argsz = sizeof(reg),
> >> +        .flags = 0,
> >> +    };
> >> +
> >> +    if (vfio_prereg_listener_skipped_section(section)) {
> >> +        trace_vfio_listener_region_add_skip(
> >> +                section->offset_within_address_space,
> >> +                section->offset_within_address_space +
> >> +                int128_get64(int128_sub(section->size, int128_one())));  
> > 
> > How will we know if this trace is related to the main listener or the
> > prereg listener?  
> 
> 
> By addresses it prints :)
> 
> Fair point, one question though:
> 
> trace_vfio_prereg_listener_region_add_skip or
> trace_vfio_spapr_listener_region_add_skip ?
> 
> Should all symbols in this file get "spapr" instead of "prereg"?

I prefer the trace match the function name.  I'm not convinced that
prereg won't become more pervasive, possibly used for some future type1
variant, but the current code is only partially generic in that sense,
hard coding spapr ioctls, which is why I objected to trying to pass it
off as generic.  However, I'm not sure it's worth spending much more
time renaming each function that can be done when a second user arrives
and we try harder to really make it a general interface.
 
> >> +        return;
> >> +    }
> >> +
> >> +    if (unlikely((section->offset_within_address_space & ~page_mask) ||
> >> +                 (section->offset_within_region & ~page_mask) ||
> >> +                 (int128_get64(section->size) & ~page_mask))) {
> >> +        error_report("%s received unaligned region", __func__);
> >> +        return;
> >> +    }
> >> +
> >> +    end = section->offset_within_address_space + int128_get64(section->size);
> >> +    g_assert(gpa < end);  
> > 
> > This would imply a zero-sized region, can't you simply return?  
> 
> Zero-sized region or overflow, no?

Yes, but doesn't that imply a bogus MemoryRegionSection from the memory
API?  Can that happen or are we pointlessly re-sanitizing a condition
that cannot occur?

> When I copied this from vfio_listener_region_add(), I thought it is an
> overflow check (which imho should have been assert() or hwerror(), is not
> it? What do I miss?

That sort of consistency test that would justify an assert or hwerror
doesn't seem like it belongs in a consumer of the API, the API should
enforce it elsewhere.
 
> >   
> >> +
> >> +    memory_region_ref(section->mr);
> >> +
> >> +    reg.vaddr = (__u64) vfio_prereg_gpa_to_vaddr(section, gpa);
> >> +    reg.size = end - gpa;
> >> +
> >> +    ret = ioctl(container->fd, VFIO_IOMMU_SPAPR_REGISTER_MEMORY, &reg);
> >> +    trace_vfio_ram_register(reg.vaddr, reg.size, ret ? -errno : 0);
> >> +    if (ret) {
> >> +        /*
> >> +         * 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->initialized) {
> >> +            if (!container->error) {
> >> +                container->error = ret;
> >> +            }
> >> +        } else {
> >> +            hw_error("vfio: Memory registering failed, unable to continue");
> >> +        }
> >> +    }
> >> +}
> >> +
> >> +static void vfio_prereg_listener_region_del(MemoryListener *listener,
> >> +                                            MemoryRegionSection *section)
> >> +{
> >> +    VFIOContainer *container = container_of(listener, VFIOContainer,
> >> +                                            prereg_listener);
> >> +    const hwaddr gpa = section->offset_within_address_space;
> >> +    hwaddr end;
> >> +    int ret;
> >> +    hwaddr page_mask = qemu_real_host_page_mask;
> >> +    struct vfio_iommu_spapr_register_memory reg = {
> >> +        .argsz = sizeof(reg),
> >> +        .flags = 0,
> >> +    };
> >> +
> >> +    if (vfio_prereg_listener_skipped_section(section)) {
> >> +        trace_vfio_listener_region_del_skip(
> >> +                section->offset_within_address_space,
> >> +                section->offset_within_address_space +
> >> +                int128_get64(int128_sub(section->size, int128_one())));  
> > 
> > Again, indistinguishable from main listener trace.
> >   
> >> +        return;
> >> +    }
> >> +
> >> +    if (unlikely((section->offset_within_address_space & ~page_mask) ||
> >> +                 (section->offset_within_region & ~page_mask) ||
> >> +                 (int128_get64(section->size) & ~page_mask))) {
> >> +        error_report("%s received unaligned region", __func__);
> >> +        return;
> >> +    }
> >> +
> >> +    end = section->offset_within_address_space + int128_get64(section->size);
> >> +    if (gpa >= end) {
> >> +        return;  
> > 
> > We simply return here, not sure why we need to g_assert above.  
> 
> Well, we won't get this far if this is the case - region_add() would fail
> first.

Then why test it at all or why not make it an assert?  The point is if
we can skip it here, why couldn't we skip it above.  If we assert
above, why can we skip it here even though seeing it here would be
another unexpected inconsistency.  IMO, we can skip it in both places
just like the existing listener does.

> >   
> >> +    }
> >> +
> >> +    reg.vaddr = (__u64) vfio_prereg_gpa_to_vaddr(section, gpa);
> >> +    reg.size = end - gpa;
> >> +
> >> +    ret = ioctl(container->fd, VFIO_IOMMU_SPAPR_UNREGISTER_MEMORY, &reg);
> >> +    trace_vfio_ram_unregister(reg.vaddr, reg.size, ret ? -errno : 0);
> >> +}
> >> +
> >> +const MemoryListener vfio_prereg_listener = {
> >> +    .region_add = vfio_prereg_listener_region_add,
> >> +    .region_del = vfio_prereg_listener_region_del,
> >> +};
> >> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> >> index 0610377..405c3b2 100644
> >> --- a/include/hw/vfio/vfio-common.h
> >> +++ b/include/hw/vfio/vfio-common.h
> >> @@ -73,6 +73,8 @@ typedef struct VFIOContainer {
> >>      VFIOAddressSpace *space;
> >>      int fd; /* /dev/vfio/vfio, empowered by the attached groups */
> >>      MemoryListener listener;
> >> +    MemoryListener prereg_listener;
> >> +    unsigned iommu_type;
> >>      int error;
> >>      bool initialized;
> >>      /*
> >> @@ -158,4 +160,6 @@ int vfio_get_region_info(VFIODevice *vbasedev, int index,
> >>  int vfio_get_dev_region_info(VFIODevice *vbasedev, uint32_t type,
> >>                               uint32_t subtype, struct vfio_region_info **info);
> >>  #endif
> >> +extern const MemoryListener vfio_prereg_listener;
> >> +
> >>  #endif /* !HW_VFIO_VFIO_COMMON_H */
> >> diff --git a/trace-events b/trace-events
> >> index de42012..ddb8676 100644
> >> --- a/trace-events
> >> +++ b/trace-events
> >> @@ -1766,6 +1766,8 @@ vfio_region_mmaps_set_enabled(const char *name, bool enabled) "Region %s mmaps e
> >>  vfio_region_sparse_mmap_header(const char *name, int index, int nr_areas) "Device %s region %d: %d sparse mmap entries"
> >>  vfio_region_sparse_mmap_entry(int i, unsigned long start, unsigned long end) "sparse entry %d [0x%lx - 0x%lx]"
> >>  vfio_get_dev_region(const char *name, int index, uint32_t type, uint32_t subtype) "%s index %d, %08x/%0x8"
> >> +vfio_ram_register(uint64_t va, uint64_t size, int ret) "va=%"PRIx64" size=%"PRIx64" ret=%d"
> >> +vfio_ram_unregister(uint64_t va, uint64_t size, int ret) "va=%"PRIx64" size=%"PRIx64" ret=%d"  
> > 
> > This file loosely calls out which file the trace is in, these are not
> > in common.c.
> >   
> >>  
> >>  # hw/vfio/platform.c
> >>  vfio_platform_base_device_init(char *name, int groupid) "%s belongs to group #%d"  
> >   
> 
>