Re: [Qemu-devel] [PATCH qemu v10 13/14] vfio: spapr: Add SPAPR IOMMU v2 support (DMA memory preregistering)

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

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 <aik@ozlabs.ru>
> >> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> >> ---
> >> 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.

> >> +
> >> +static void vfio_spapr_listener_release_v2(VFIOContainer *container)
> >> +{
> >> +    memory_listener_unregister(&container->iommu_data.register_listener);
> >> +    vfio_listener_release(container);
> >> +}
> >> +
> >>   int vfio_mmap_region(Object *obj, VFIORegion *region,
> >>                        MemoryRegion *mem, MemoryRegion *submem,
> >>                        void **map, size_t size, off_t offset,
> >> @@ -698,14 +768,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;
> >> @@ -717,19 +791,36 @@ 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;
> >> +            }
> >>           }
> >>
> >>           container->iommu_data.type1.listener = vfio_memory_listener;
> >> -        container->iommu_data.release = vfio_listener_release;
> >> -
> >>           memory_listener_register(&container->iommu_data.type1.listener,
> >>                                    container->space->as);
> >>
> >> +        if (!v2) {
> >> +            container->iommu_data.release = vfio_listener_release;
> >> +        } else {
> >> +            container->iommu_data.release = vfio_spapr_listener_release_v2;
> >> +            container->iommu_data.register_listener =
> >> +                    vfio_ram_memory_listener;
> >> +            memory_listener_register(&container->iommu_data.register_listener,
> >> +                                     &address_space_memory);
> >> +
> >> +            if (container->iommu_data.ram_reg_error) {
> >> +                error_report("vfio: RAM memory listener initialization failed for container");
> >> +                goto listener_release_exit;
> >> +            }
> >> +
> >> +            container->iommu_data.ram_reg_initialized = true;
> >> +        }
> >> +
> >>       } else {
> >>           error_report("vfio: No available IOMMU models");
> >>           ret = -EINVAL;
> >> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> >> index 59a321d..b132248 100644
> >> --- a/include/hw/vfio/vfio-common.h
> >> +++ b/include/hw/vfio/vfio-common.h
> >> @@ -79,6 +79,9 @@ typedef struct VFIOContainer {
> >>               VFIOType1 type1;
> >>           };
> >>           void (*release)(struct VFIOContainer *);
> >> +        MemoryListener register_listener;
> >> +        int ram_reg_error;
> >> +        bool ram_reg_initialized;
> >
> > Isn't this exactly what the union above is for?
> 
> This is a different listener on a different address space and I do not 
> really feel sharing these _error/_initialized between unrelated listeners, 
> should I?

How are they not related?  They're specific to the spapr-v2 IOMMU
backend, which is exactly what that union was intended to abstract.
Thanks,

Alex

> >>       } iommu_data;
> >>       QLIST_HEAD(, VFIOGuestIOMMU) giommu_list;
> >>       QLIST_HEAD(, VFIOGroup) group_list;
> >> diff --git a/trace-events b/trace-events
> >> index a994019..b300e94 100644
> >> --- a/trace-events
> >> +++ b/trace-events
> >> @@ -1584,6 +1584,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/vfio/platform.c
> >>   vfio_platform_populate_regions(int region_index, unsigned long flag, unsigned long size, int fd, unsigned long offset) "- region %d flags = 0x%lx, size = 0x%lx, fd= %d, offset = 0x%lx"
> >
> >
> >
> 
>