| Message ID | 1462344751-28281-15-git-send-email-aik@ozlabs.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 4 May 2016 16:52:26 +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: > 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/prereg.c | 137 ++++++++++++++++++++++++++++++++++++++++++ > include/hw/vfio/vfio-common.h | 4 ++ > trace-events | 2 + > 5 files changed, 172 insertions(+), 10 deletions(-) > create mode 100644 hw/vfio/prereg.c > > diff --git a/hw/vfio/Makefile.objs b/hw/vfio/Makefile.objs > index ceddbb8..5800e0e 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) += prereg.o > endif > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > index 2050040..496eb82 100644 > --- a/hw/vfio/common.c > +++ b/hw/vfio/common.c > @@ -501,6 +501,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); > + } > } > > int vfio_region_setup(Object *obj, VFIODevice *vbasedev, VFIORegion *region, > @@ -808,8 +811,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; > @@ -834,8 +837,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) { > @@ -843,7 +848,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; > @@ -855,11 +862,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; > + } > } > > /* > @@ -872,7 +890,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; > } > container->min_iova = info.dma32_window_start; > container->max_iova = container->min_iova + info.dma32_window_size - 1; > diff --git a/hw/vfio/prereg.c b/hw/vfio/prereg.c > new file mode 100644 > index 0000000..d0e4728 > --- /dev/null > +++ b/hw/vfio/prereg.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 <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)) { > + error_report("Cannot possibly preregister IOMMU memory"); What is a user supposed to do with this error_report()? Is it continue-able? How is it possible? What should they do differently? > + return true; > + } > + > + return !memory_region_is_ram(section->mr) || > + memory_region_is_skip_dump(section->mr); > +} > + > +static void *vfio_prereg_gpa_to_ua(MemoryRegionSection *section, hwaddr gpa) What's "ua"? > +{ > + 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, > + }; So we're just pretending that this spapr specific code is some sort of generic pre-registration interface? > + > + 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()))); > + 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); > + > + memory_region_ref(section->mr); > + > + reg.vaddr = (__u64) vfio_prereg_gpa_to_ua(section, gpa); Hmm, why wasn't that simply gpa_to_vaddr? > + reg.size = end - gpa; > + > + ret = ioctl(container->fd, VFIO_IOMMU_SPAPR_REGISTER_MEMORY, ®); > + 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()))); > + 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; > + } > + > + reg.vaddr = (__u64) vfio_prereg_gpa_to_ua(section, gpa); > + reg.size = end - gpa; > + > + ret = ioctl(container->fd, VFIO_IOMMU_SPAPR_UNREGISTER_MEMORY, ®); > + 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 c9b6622..c72e45a 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; > /* > @@ -156,4 +158,6 @@ extern QLIST_HEAD(vfio_as_head, VFIOAddressSpace) vfio_address_spaces; > int vfio_get_region_info(VFIODevice *vbasedev, int index, > 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 dd50005..d0d8615 100644 > --- a/trace-events > +++ b/trace-events > @@ -1737,6 +1737,8 @@ vfio_region_mmap(const char *name, unsigned long offset, unsigned long end) "Reg > vfio_region_exit(const char *name, int index) "Device %s, region %d" > vfio_region_finalize(const char *name, int index) "Device %s, region %d" > vfio_region_mmaps_set_enabled(const char *name, bool enabled) "Region %s mmaps enabled: %d" > +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" > > # hw/vfio/platform.c > vfio_platform_base_device_init(char *name, int groupid) "%s belongs to group #%d"
On 05/14/2016 08:25 AM, Alex Williamson wrote: > On Wed, 4 May 2016 16:52:26 +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: >> 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/prereg.c | 137 ++++++++++++++++++++++++++++++++++++++++++ >> include/hw/vfio/vfio-common.h | 4 ++ >> trace-events | 2 + >> 5 files changed, 172 insertions(+), 10 deletions(-) >> create mode 100644 hw/vfio/prereg.c >> >> diff --git a/hw/vfio/Makefile.objs b/hw/vfio/Makefile.objs >> index ceddbb8..5800e0e 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) += prereg.o >> endif >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >> index 2050040..496eb82 100644 >> --- a/hw/vfio/common.c >> +++ b/hw/vfio/common.c >> @@ -501,6 +501,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); >> + } >> } >> >> int vfio_region_setup(Object *obj, VFIODevice *vbasedev, VFIORegion *region, >> @@ -808,8 +811,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; >> @@ -834,8 +837,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) { >> @@ -843,7 +848,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; >> @@ -855,11 +862,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; >> + } >> } >> >> /* >> @@ -872,7 +890,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; >> } >> container->min_iova = info.dma32_window_start; >> container->max_iova = container->min_iova + info.dma32_window_size - 1; >> diff --git a/hw/vfio/prereg.c b/hw/vfio/prereg.c >> new file mode 100644 >> index 0000000..d0e4728 >> --- /dev/null >> +++ b/hw/vfio/prereg.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 <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)) { >> + error_report("Cannot possibly preregister IOMMU memory"); > > What is a user supposed to do with this error_report()? Is it > continue-able? How is it possible? What should they do differently? If I remember correctly, David did have theories where this may be possible, not today with the existing code though. Could be assert() or abort(), what is better here? >> + return true; >> + } >> + >> + return !memory_region_is_ram(section->mr) || >> + memory_region_is_skip_dump(section->mr); >> +} >> + >> +static void *vfio_prereg_gpa_to_ua(MemoryRegionSection *section, hwaddr gpa) > > What's "ua"? Userspace address. > >> +{ >> + 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, >> + }; > > So we're just pretending that this spapr specific code is some sort of > generic pre-registration interface? Yes. >> + >> + 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()))); >> + 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); >> + >> + memory_region_ref(section->mr); >> + >> + reg.vaddr = (__u64) vfio_prereg_gpa_to_ua(section, gpa); > > Hmm, why wasn't that simply gpa_to_vaddr? I wanted to keep a prefix in all functions, even if they are static, easier to grep. Bad idea? >> + reg.size = end - gpa; >> + >> + ret = ioctl(container->fd, VFIO_IOMMU_SPAPR_REGISTER_MEMORY, ®); >> + 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()))); >> + 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; >> + } >> + >> + reg.vaddr = (__u64) vfio_prereg_gpa_to_ua(section, gpa); >> + reg.size = end - gpa; >> + >> + ret = ioctl(container->fd, VFIO_IOMMU_SPAPR_UNREGISTER_MEMORY, ®); >> + 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 c9b6622..c72e45a 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; >> /* >> @@ -156,4 +158,6 @@ extern QLIST_HEAD(vfio_as_head, VFIOAddressSpace) vfio_address_spaces; >> int vfio_get_region_info(VFIODevice *vbasedev, int index, >> 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 dd50005..d0d8615 100644 >> --- a/trace-events >> +++ b/trace-events >> @@ -1737,6 +1737,8 @@ vfio_region_mmap(const char *name, unsigned long offset, unsigned long end) "Reg >> vfio_region_exit(const char *name, int index) "Device %s, region %d" >> vfio_region_finalize(const char *name, int index) "Device %s, region %d" >> vfio_region_mmaps_set_enabled(const char *name, bool enabled) "Region %s mmaps enabled: %d" >> +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" >> >> # hw/vfio/platform.c >> vfio_platform_base_device_init(char *name, int groupid) "%s belongs to group #%d"
On Mon, 16 May 2016 11:10:05 +1000 Alexey Kardashevskiy <aik@ozlabs.ru> wrote: > On 05/14/2016 08:25 AM, Alex Williamson wrote: > > On Wed, 4 May 2016 16:52:26 +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: > >> 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/prereg.c | 137 ++++++++++++++++++++++++++++++++++++++++++ > >> include/hw/vfio/vfio-common.h | 4 ++ > >> trace-events | 2 + > >> 5 files changed, 172 insertions(+), 10 deletions(-) > >> create mode 100644 hw/vfio/prereg.c > >> > >> diff --git a/hw/vfio/Makefile.objs b/hw/vfio/Makefile.objs > >> index ceddbb8..5800e0e 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) += prereg.o > >> endif > >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c > >> index 2050040..496eb82 100644 > >> --- a/hw/vfio/common.c > >> +++ b/hw/vfio/common.c > >> @@ -501,6 +501,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); > >> + } > >> } > >> > >> int vfio_region_setup(Object *obj, VFIODevice *vbasedev, VFIORegion *region, > >> @@ -808,8 +811,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; > >> @@ -834,8 +837,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) { > >> @@ -843,7 +848,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; > >> @@ -855,11 +862,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; > >> + } > >> } > >> > >> /* > >> @@ -872,7 +890,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; > >> } > >> container->min_iova = info.dma32_window_start; > >> container->max_iova = container->min_iova + info.dma32_window_size - 1; > >> diff --git a/hw/vfio/prereg.c b/hw/vfio/prereg.c > >> new file mode 100644 > >> index 0000000..d0e4728 > >> --- /dev/null > >> +++ b/hw/vfio/prereg.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 <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)) { > >> + error_report("Cannot possibly preregister IOMMU memory"); > > > > What is a user supposed to do with this error_report()? Is it > > continue-able? How is it possible? What should they do differently? > > > If I remember correctly, David did have theories where this may be > possible, not today with the existing code though. Could be assert() or > abort(), what is better here? If it's a hardware configuration error, then use hw_error(), I prefer not to add either assert() or abort() calls to vfio. > >> + return true; > >> + } > >> + > >> + return !memory_region_is_ram(section->mr) || > >> + memory_region_is_skip_dump(section->mr); > >> +} > >> + > >> +static void *vfio_prereg_gpa_to_ua(MemoryRegionSection *section, hwaddr gpa) > > > > What's "ua"? > > > Userspace address. But we use it to set a vaddr below, so let's just call it vaddr. > > > >> +{ > >> + 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, > >> + }; > > > > So we're just pretending that this spapr specific code is some sort of > > generic pre-registration interface? > > Yes. :-\ > >> + > >> + 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()))); > >> + 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); > >> + > >> + memory_region_ref(section->mr); > >> + > >> + reg.vaddr = (__u64) vfio_prereg_gpa_to_ua(section, gpa); > > > > Hmm, why wasn't that simply gpa_to_vaddr? > > I wanted to keep a prefix in all functions, even if they are static, easier > to grep. Bad idea? My question about "ua" means that it's not obvious what we're returning based on the name of the function alone, so I would avoid such a name. > >> + reg.size = end - gpa; > >> + > >> + ret = ioctl(container->fd, VFIO_IOMMU_SPAPR_REGISTER_MEMORY, ®); > >> + 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()))); > >> + 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; > >> + } > >> + > >> + reg.vaddr = (__u64) vfio_prereg_gpa_to_ua(section, gpa); > >> + reg.size = end - gpa; > >> + > >> + ret = ioctl(container->fd, VFIO_IOMMU_SPAPR_UNREGISTER_MEMORY, ®); > >> + 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 c9b6622..c72e45a 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; > >> /* > >> @@ -156,4 +158,6 @@ extern QLIST_HEAD(vfio_as_head, VFIOAddressSpace) vfio_address_spaces; > >> int vfio_get_region_info(VFIODevice *vbasedev, int index, > >> 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 dd50005..d0d8615 100644 > >> --- a/trace-events > >> +++ b/trace-events > >> @@ -1737,6 +1737,8 @@ vfio_region_mmap(const char *name, unsigned long offset, unsigned long end) "Reg > >> vfio_region_exit(const char *name, int index) "Device %s, region %d" > >> vfio_region_finalize(const char *name, int index) "Device %s, region %d" > >> vfio_region_mmaps_set_enabled(const char *name, bool enabled) "Region %s mmaps enabled: %d" > >> +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" > >> > >> # hw/vfio/platform.c > >> vfio_platform_base_device_init(char *name, int groupid) "%s belongs to group #%d" > >
On Mon, May 16, 2016 at 02:20:33PM -0600, Alex Williamson wrote: > On Mon, 16 May 2016 11:10:05 +1000 > Alexey Kardashevskiy <aik@ozlabs.ru> wrote: > > > On 05/14/2016 08:25 AM, Alex Williamson wrote: > > > On Wed, 4 May 2016 16:52:26 +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: > > >> 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/prereg.c | 137 ++++++++++++++++++++++++++++++++++++++++++ > > >> include/hw/vfio/vfio-common.h | 4 ++ > > >> trace-events | 2 + > > >> 5 files changed, 172 insertions(+), 10 deletions(-) > > >> create mode 100644 hw/vfio/prereg.c > > >> > > >> diff --git a/hw/vfio/Makefile.objs b/hw/vfio/Makefile.objs > > >> index ceddbb8..5800e0e 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) += prereg.o > > >> endif > > >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c > > >> index 2050040..496eb82 100644 > > >> --- a/hw/vfio/common.c > > >> +++ b/hw/vfio/common.c > > >> @@ -501,6 +501,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); > > >> + } > > >> } > > >> > > >> int vfio_region_setup(Object *obj, VFIODevice *vbasedev, VFIORegion *region, > > >> @@ -808,8 +811,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; > > >> @@ -834,8 +837,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) { > > >> @@ -843,7 +848,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; > > >> @@ -855,11 +862,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; > > >> + } > > >> } > > >> > > >> /* > > >> @@ -872,7 +890,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; > > >> } > > >> container->min_iova = info.dma32_window_start; > > >> container->max_iova = container->min_iova + info.dma32_window_size - 1; > > >> diff --git a/hw/vfio/prereg.c b/hw/vfio/prereg.c > > >> new file mode 100644 > > >> index 0000000..d0e4728 > > >> --- /dev/null > > >> +++ b/hw/vfio/prereg.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 <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)) { > > >> + error_report("Cannot possibly preregister IOMMU memory"); > > > > > > What is a user supposed to do with this error_report()? Is it > > > continue-able? How is it possible? What should they do differently? > > > > > > If I remember correctly, David did have theories where this may be > > possible, not today with the existing code though. Could be assert() or > > abort(), what is better here? > > If it's a hardware configuration error, then use hw_error(), I prefer > not to add either assert() or abort() calls to vfio. Personally I would have gone with assert() - hitting this is _almost_ certainly an indication of a bug in the code (an IOMMU translating into an AS that itself has IOMMUs). In theory a combination of a wierd platform with a multi-layered IOMMU and the wrong config options could trigger it, but it's very unlikely. hw_error() is fine by me too. > > >> + return true; > > >> + } > > >> + > > >> + return !memory_region_is_ram(section->mr) || > > >> + memory_region_is_skip_dump(section->mr); > > >> +} > > >> + > > >> +static void *vfio_prereg_gpa_to_ua(MemoryRegionSection *section, hwaddr gpa) > > > > > > What's "ua"? > > > > > > Userspace address. > > But we use it to set a vaddr below, so let's just call it vaddr. > > > > > > >> +{ > > >> + 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, > > >> + }; > > > > > > So we're just pretending that this spapr specific code is some sort of > > > generic pre-registration interface? > > > > Yes. > > :-\ It's a bit of an odd mix because the actual caps and ioctls are spapr specific, but there's nothing inherently spapr specific about the concept of pre-registration. It's only implemented for spapr now, but pre-reg would probably be a good idea for performance on any platform where the guest expects to actively manage an IOMMU with a reasonably small window. > > > >> + > > >> + 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()))); > > >> + 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); > > >> + > > >> + memory_region_ref(section->mr); > > >> + > > >> + reg.vaddr = (__u64) vfio_prereg_gpa_to_ua(section, gpa); > > > > > > Hmm, why wasn't that simply gpa_to_vaddr? > > > > I wanted to keep a prefix in all functions, even if they are static, easier > > to grep. Bad idea? > > My question about "ua" means that it's not obvious what we're returning > based on the name of the function alone, so I would avoid such a name. > > > >> + reg.size = end - gpa; > > >> + > > >> + ret = ioctl(container->fd, VFIO_IOMMU_SPAPR_REGISTER_MEMORY, ®); > > >> + 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()))); > > >> + 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; > > >> + } > > >> + > > >> + reg.vaddr = (__u64) vfio_prereg_gpa_to_ua(section, gpa); > > >> + reg.size = end - gpa; > > >> + > > >> + ret = ioctl(container->fd, VFIO_IOMMU_SPAPR_UNREGISTER_MEMORY, ®); > > >> + 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 c9b6622..c72e45a 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; > > >> /* > > >> @@ -156,4 +158,6 @@ extern QLIST_HEAD(vfio_as_head, VFIOAddressSpace) vfio_address_spaces; > > >> int vfio_get_region_info(VFIODevice *vbasedev, int index, > > >> 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 dd50005..d0d8615 100644 > > >> --- a/trace-events > > >> +++ b/trace-events > > >> @@ -1737,6 +1737,8 @@ vfio_region_mmap(const char *name, unsigned long offset, unsigned long end) "Reg > > >> vfio_region_exit(const char *name, int index) "Device %s, region %d" > > >> vfio_region_finalize(const char *name, int index) "Device %s, region %d" > > >> vfio_region_mmaps_set_enabled(const char *name, bool enabled) "Region %s mmaps enabled: %d" > > >> +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" > > >> > > >> # hw/vfio/platform.c > > >> vfio_platform_base_device_init(char *name, int groupid) "%s belongs to group #%d" > > > > >
diff --git a/hw/vfio/Makefile.objs b/hw/vfio/Makefile.objs index ceddbb8..5800e0e 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) += prereg.o endif diff --git a/hw/vfio/common.c b/hw/vfio/common.c index 2050040..496eb82 100644 --- a/hw/vfio/common.c +++ b/hw/vfio/common.c @@ -501,6 +501,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); + } } int vfio_region_setup(Object *obj, VFIODevice *vbasedev, VFIORegion *region, @@ -808,8 +811,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; @@ -834,8 +837,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) { @@ -843,7 +848,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; @@ -855,11 +862,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; + } } /* @@ -872,7 +890,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; } container->min_iova = info.dma32_window_start; container->max_iova = container->min_iova + info.dma32_window_size - 1; diff --git a/hw/vfio/prereg.c b/hw/vfio/prereg.c new file mode 100644 index 0000000..d0e4728 --- /dev/null +++ b/hw/vfio/prereg.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 <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)) { + error_report("Cannot possibly preregister IOMMU memory"); + return true; + } + + return !memory_region_is_ram(section->mr) || + memory_region_is_skip_dump(section->mr); +} + +static void *vfio_prereg_gpa_to_ua(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()))); + 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); + + memory_region_ref(section->mr); + + reg.vaddr = (__u64) vfio_prereg_gpa_to_ua(section, gpa); + reg.size = end - gpa; + + ret = ioctl(container->fd, VFIO_IOMMU_SPAPR_REGISTER_MEMORY, ®); + 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()))); + 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; + } + + reg.vaddr = (__u64) vfio_prereg_gpa_to_ua(section, gpa); + reg.size = end - gpa; + + ret = ioctl(container->fd, VFIO_IOMMU_SPAPR_UNREGISTER_MEMORY, ®); + 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 c9b6622..c72e45a 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; /* @@ -156,4 +158,6 @@ extern QLIST_HEAD(vfio_as_head, VFIOAddressSpace) vfio_address_spaces; int vfio_get_region_info(VFIODevice *vbasedev, int index, 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 dd50005..d0d8615 100644 --- a/trace-events +++ b/trace-events @@ -1737,6 +1737,8 @@ vfio_region_mmap(const char *name, unsigned long offset, unsigned long end) "Reg vfio_region_exit(const char *name, int index) "Device %s, region %d" vfio_region_finalize(const char *name, int index) "Device %s, region %d" vfio_region_mmaps_set_enabled(const char *name, bool enabled) "Region %s mmaps enabled: %d" +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" # hw/vfio/platform.c vfio_platform_base_device_init(char *name, int groupid) "%s belongs to group #%d"
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: 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/prereg.c | 137 ++++++++++++++++++++++++++++++++++++++++++ include/hw/vfio/vfio-common.h | 4 ++ trace-events | 2 + 5 files changed, 172 insertions(+), 10 deletions(-) create mode 100644 hw/vfio/prereg.c