Re: [Qemu-devel] [PATCH qemu v17 10/12] vfio/spapr: Create DMA window dynamically (SPAPR IOMMU v2)
On Mon, Jun 06, 2016 at 04:45:50PM +1000, Alexey Kardashevskiy wrote: > On 03/06/16 17:37, David Gibson wrote: > > On Wed, Jun 01, 2016 at 06:57:41PM +1000, Alexey Kardashevskiy wrote: > >> New VFIO_SPAPR_TCE_v2_IOMMU type supports dynamic DMA window management. > >> This adds ability to VFIO common code to dynamically allocate/remove > >> DMA windows in the host kernel when new VFIO container is added/removed. > >> > >> This adds VFIO_IOMMU_SPAPR_TCE_CREATE ioctl to vfio_listener_region_add > >> and adds just created IOMMU into the host IOMMU list; the opposite > >> action is taken in vfio_listener_region_del. > >> > >> When creating a new window, this uses heuristic to decide on the TCE table > >> levels number. > >> > >> This should cause no guest visible change in behavior. > >> > >> Signed-off-by: Alexey Kardashevskiy> >> --- > >> Changes: > >> v17: > >> * moved spapr window create/remove helpers to separate file > >> * added hw_error() if vfio_host_win_del() failed > >> > >> v16: > >> * used memory_region_iommu_get_page_sizes() in vfio_listener_region_add() > >> * enforced no intersections between windows > >> > >> v14: > >> * new to the series > >> --- > >> hw/vfio/common.c | 76 > >> +-- > >> hw/vfio/spapr.c | 70 +++ > >> include/hw/vfio/vfio-common.h | 6 > >> trace-events | 2 ++ > >> 4 files changed, 144 insertions(+), 10 deletions(-) > >> > >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c > >> index 52b08fd..7f55c26 100644 > >> --- a/hw/vfio/common.c > >> +++ b/hw/vfio/common.c > >> @@ -275,6 +275,18 @@ static void vfio_host_win_add(VFIOContainer > >> *container, > >> QLIST_INSERT_HEAD(>hostwin_list, hostwin, hostwin_next); > >> } > >> > >> +static int vfio_host_win_del(VFIOContainer *container, hwaddr min_iova) > >> +{ > >> +VFIOHostDMAWindow *hostwin = vfio_host_win_lookup(container, > >> min_iova, 1); > > > > Hrm.. and for this case I think you want exact match, rather than > > looking for range inclusion. > > I suppose so, I'll change this. > > > >> + > >> +if (!hostwin) { > >> +return -1; > >> +} > >> +QLIST_REMOVE(hostwin, hostwin_next); > >> + > >> +return 0; > >> +} > >> + > >> static bool vfio_listener_skipped_section(MemoryRegionSection *section) > >> { > >> return (!memory_region_is_ram(section->mr) && > >> @@ -388,6 +400,30 @@ static void vfio_listener_region_add(MemoryListener > >> *listener, > >> } > >> end = int128_get64(int128_sub(llend, int128_one())); > >> > >> +if (container->iommu_type == VFIO_SPAPR_TCE_v2_IOMMU) { > >> +VFIOHostDMAWindow *hostwin; > >> +hwaddr pgsize = 0; > >> + > >> +/* For now intersections are not allowed, we may relax this later > >> */ > >> +QLIST_FOREACH(hostwin, >hostwin_list, hostwin_next) { > >> +if (ranges_overlap(hostwin->min_iova, > >> + hostwin->max_iova - hostwin->min_iova + 1, > >> + section->offset_within_address_space, > >> + int128_get64(section->size))) { > >> +goto fail; > >> +} > >> +} > >> + > >> +ret = vfio_spapr_create_window(container, section, ); > >> +if (ret) { > >> +goto fail; > >> +} > >> + > >> +vfio_host_win_add(container, section->offset_within_address_space, > >> + section->offset_within_address_space + > >> + int128_get64(section->size) - 1, pgsize); > >> +} > >> + > >> if (!vfio_host_win_lookup(container, iova, end)) { > >> error_report("vfio: IOMMU container %p can't map guest IOVA > >> region" > >> " 0x%"HWADDR_PRIx"..0x%"HWADDR_PRIx, > >> @@ -523,6 +559,18 @@ static void vfio_listener_region_del(MemoryListener > >> *listener, > >> "0x%"HWADDR_PRIx") = %d (%m)", > >> container, iova, int128_get64(llsize), ret); > >> } > >> + > >> +if (container->iommu_type == VFIO_SPAPR_TCE_v2_IOMMU) { > >> +vfio_spapr_remove_window(container, > >> + section->offset_within_address_space); > > > > Should check for error here. > > > And do what here? vfio_spapr_remove_window() calls error_report() already > and I still want to remove the host window here. Hmm.. yes, I guess so. Probably best to have a comment here saying why it's safe to ignore an error you should usually test for. > > > >> +if (vfio_host_win_del(container, > >> + section->offset_within_address_space) < 0) { > >> +hw_error("%s: Cannot delete missing window at %"HWADDR_PRIx, > >> + __func__, section->offset_within_address_space); > > > > Personally I think assert() would be better here, but
Re: [Qemu-devel] [PATCH qemu v17 10/12] vfio/spapr: Create DMA window dynamically (SPAPR IOMMU v2)
On 03/06/16 17:37, David Gibson wrote: > On Wed, Jun 01, 2016 at 06:57:41PM +1000, Alexey Kardashevskiy wrote: >> New VFIO_SPAPR_TCE_v2_IOMMU type supports dynamic DMA window management. >> This adds ability to VFIO common code to dynamically allocate/remove >> DMA windows in the host kernel when new VFIO container is added/removed. >> >> This adds VFIO_IOMMU_SPAPR_TCE_CREATE ioctl to vfio_listener_region_add >> and adds just created IOMMU into the host IOMMU list; the opposite >> action is taken in vfio_listener_region_del. >> >> When creating a new window, this uses heuristic to decide on the TCE table >> levels number. >> >> This should cause no guest visible change in behavior. >> >> Signed-off-by: Alexey Kardashevskiy>> --- >> Changes: >> v17: >> * moved spapr window create/remove helpers to separate file >> * added hw_error() if vfio_host_win_del() failed >> >> v16: >> * used memory_region_iommu_get_page_sizes() in vfio_listener_region_add() >> * enforced no intersections between windows >> >> v14: >> * new to the series >> --- >> hw/vfio/common.c | 76 >> +-- >> hw/vfio/spapr.c | 70 +++ >> include/hw/vfio/vfio-common.h | 6 >> trace-events | 2 ++ >> 4 files changed, 144 insertions(+), 10 deletions(-) >> >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >> index 52b08fd..7f55c26 100644 >> --- a/hw/vfio/common.c >> +++ b/hw/vfio/common.c >> @@ -275,6 +275,18 @@ static void vfio_host_win_add(VFIOContainer *container, >> QLIST_INSERT_HEAD(>hostwin_list, hostwin, hostwin_next); >> } >> >> +static int vfio_host_win_del(VFIOContainer *container, hwaddr min_iova) >> +{ >> +VFIOHostDMAWindow *hostwin = vfio_host_win_lookup(container, min_iova, >> 1); > > Hrm.. and for this case I think you want exact match, rather than > looking for range inclusion. I suppose so, I'll change this. >> + >> +if (!hostwin) { >> +return -1; >> +} >> +QLIST_REMOVE(hostwin, hostwin_next); >> + >> +return 0; >> +} >> + >> static bool vfio_listener_skipped_section(MemoryRegionSection *section) >> { >> return (!memory_region_is_ram(section->mr) && >> @@ -388,6 +400,30 @@ static void vfio_listener_region_add(MemoryListener >> *listener, >> } >> end = int128_get64(int128_sub(llend, int128_one())); >> >> +if (container->iommu_type == VFIO_SPAPR_TCE_v2_IOMMU) { >> +VFIOHostDMAWindow *hostwin; >> +hwaddr pgsize = 0; >> + >> +/* For now intersections are not allowed, we may relax this later */ >> +QLIST_FOREACH(hostwin, >hostwin_list, hostwin_next) { >> +if (ranges_overlap(hostwin->min_iova, >> + hostwin->max_iova - hostwin->min_iova + 1, >> + section->offset_within_address_space, >> + int128_get64(section->size))) { >> +goto fail; >> +} >> +} >> + >> +ret = vfio_spapr_create_window(container, section, ); >> +if (ret) { >> +goto fail; >> +} >> + >> +vfio_host_win_add(container, section->offset_within_address_space, >> + section->offset_within_address_space + >> + int128_get64(section->size) - 1, pgsize); >> +} >> + >> if (!vfio_host_win_lookup(container, iova, end)) { >> error_report("vfio: IOMMU container %p can't map guest IOVA region" >> " 0x%"HWADDR_PRIx"..0x%"HWADDR_PRIx, >> @@ -523,6 +559,18 @@ static void vfio_listener_region_del(MemoryListener >> *listener, >> "0x%"HWADDR_PRIx") = %d (%m)", >> container, iova, int128_get64(llsize), ret); >> } >> + >> +if (container->iommu_type == VFIO_SPAPR_TCE_v2_IOMMU) { >> +vfio_spapr_remove_window(container, >> + section->offset_within_address_space); > > Should check for error here. And do what here? vfio_spapr_remove_window() calls error_report() already and I still want to remove the host window here. > >> +if (vfio_host_win_del(container, >> + section->offset_within_address_space) < 0) { >> +hw_error("%s: Cannot delete missing window at %"HWADDR_PRIx, >> + __func__, section->offset_within_address_space); > > Personally I think assert() would be better here, but Alex doesn't > like them so I'm ok with this. > >> +} >> + >> + >> trace_vfio_spapr_remove_window(section->offset_within_address_space); >> +} >> } >> >> static const MemoryListener vfio_memory_listener = { >> @@ -960,11 +1008,6 @@ static int vfio_connect_container(VFIOGroup *group, >> AddressSpace *as) >> } >> } >> >> -/* >> - * This only considers the host IOMMU's 32-bit window. At >> -
Re: [Qemu-devel] [PATCH qemu v17 10/12] vfio/spapr: Create DMA window dynamically (SPAPR IOMMU v2)
On Wed, 1 Jun 2016 18:57:41 +1000 Alexey Kardashevskiywrote: > New VFIO_SPAPR_TCE_v2_IOMMU type supports dynamic DMA window management. > This adds ability to VFIO common code to dynamically allocate/remove > DMA windows in the host kernel when new VFIO container is added/removed. > > This adds VFIO_IOMMU_SPAPR_TCE_CREATE ioctl to vfio_listener_region_add > and adds just created IOMMU into the host IOMMU list; the opposite > action is taken in vfio_listener_region_del. > > When creating a new window, this uses heuristic to decide on the TCE table > levels number. > > This should cause no guest visible change in behavior. > > Signed-off-by: Alexey Kardashevskiy > --- > Changes: > v17: > * moved spapr window create/remove helpers to separate file > * added hw_error() if vfio_host_win_del() failed > > v16: > * used memory_region_iommu_get_page_sizes() in vfio_listener_region_add() > * enforced no intersections between windows > > v14: > * new to the series > --- > hw/vfio/common.c | 76 > +-- > hw/vfio/spapr.c | 70 +++ > include/hw/vfio/vfio-common.h | 6 > trace-events | 2 ++ > 4 files changed, 144 insertions(+), 10 deletions(-) > > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > index 52b08fd..7f55c26 100644 > --- a/hw/vfio/common.c > +++ b/hw/vfio/common.c > @@ -275,6 +275,18 @@ static void vfio_host_win_add(VFIOContainer *container, > QLIST_INSERT_HEAD(>hostwin_list, hostwin, hostwin_next); > } > > +static int vfio_host_win_del(VFIOContainer *container, hwaddr min_iova) > +{ > +VFIOHostDMAWindow *hostwin = vfio_host_win_lookup(container, min_iova, > 1); > + +1 David's comment for exact match. > +if (!hostwin) { > +return -1; > +} > +QLIST_REMOVE(hostwin, hostwin_next); > + > +return 0; > +} > + > static bool vfio_listener_skipped_section(MemoryRegionSection *section) > { > return (!memory_region_is_ram(section->mr) && > @@ -388,6 +400,30 @@ static void vfio_listener_region_add(MemoryListener > *listener, > } > end = int128_get64(int128_sub(llend, int128_one())); > > +if (container->iommu_type == VFIO_SPAPR_TCE_v2_IOMMU) { > +VFIOHostDMAWindow *hostwin; > +hwaddr pgsize = 0; > + > +/* For now intersections are not allowed, we may relax this later */ > +QLIST_FOREACH(hostwin, >hostwin_list, hostwin_next) { > +if (ranges_overlap(hostwin->min_iova, > + hostwin->max_iova - hostwin->min_iova + 1, > + section->offset_within_address_space, > + int128_get64(section->size))) { > +goto fail; > +} > +} > + > +ret = vfio_spapr_create_window(container, section, ); > +if (ret) { > +goto fail; > +} > + > +vfio_host_win_add(container, section->offset_within_address_space, > + section->offset_within_address_space + > + int128_get64(section->size) - 1, pgsize); > +} > + > if (!vfio_host_win_lookup(container, iova, end)) { > error_report("vfio: IOMMU container %p can't map guest IOVA region" > " 0x%"HWADDR_PRIx"..0x%"HWADDR_PRIx, > @@ -523,6 +559,18 @@ static void vfio_listener_region_del(MemoryListener > *listener, > "0x%"HWADDR_PRIx") = %d (%m)", > container, iova, int128_get64(llsize), ret); > } > + > +if (container->iommu_type == VFIO_SPAPR_TCE_v2_IOMMU) { > +vfio_spapr_remove_window(container, > + section->offset_within_address_space); > +if (vfio_host_win_del(container, > + section->offset_within_address_space) < 0) { > +hw_error("%s: Cannot delete missing window at %"HWADDR_PRIx, > + __func__, section->offset_within_address_space); > +} > + > +trace_vfio_spapr_remove_window(section->offset_within_address_space); Trace within the function like you do on the create_window side. > +} > } > > static const MemoryListener vfio_memory_listener = { > @@ -960,11 +1008,6 @@ static int vfio_connect_container(VFIOGroup *group, > AddressSpace *as) > } > } > > -/* > - * This only considers the host IOMMU's 32-bit window. At > - * some point we need to add support for the optional 64-bit > - * window and dynamic windows > - */ > info.argsz = sizeof(info); > ret = ioctl(fd, VFIO_IOMMU_SPAPR_TCE_GET_INFO, ); > if (ret) { > @@ -973,11 +1016,24 @@ static int vfio_connect_container(VFIOGroup *group, > AddressSpace *as) > goto listener_release_exit; > } > > -/* The default table uses 4K pages
Re: [Qemu-devel] [PATCH qemu v17 10/12] vfio/spapr: Create DMA window dynamically (SPAPR IOMMU v2)
On Wed, Jun 01, 2016 at 06:57:41PM +1000, Alexey Kardashevskiy wrote: > New VFIO_SPAPR_TCE_v2_IOMMU type supports dynamic DMA window management. > This adds ability to VFIO common code to dynamically allocate/remove > DMA windows in the host kernel when new VFIO container is added/removed. > > This adds VFIO_IOMMU_SPAPR_TCE_CREATE ioctl to vfio_listener_region_add > and adds just created IOMMU into the host IOMMU list; the opposite > action is taken in vfio_listener_region_del. > > When creating a new window, this uses heuristic to decide on the TCE table > levels number. > > This should cause no guest visible change in behavior. > > Signed-off-by: Alexey Kardashevskiy> --- > Changes: > v17: > * moved spapr window create/remove helpers to separate file > * added hw_error() if vfio_host_win_del() failed > > v16: > * used memory_region_iommu_get_page_sizes() in vfio_listener_region_add() > * enforced no intersections between windows > > v14: > * new to the series > --- > hw/vfio/common.c | 76 > +-- > hw/vfio/spapr.c | 70 +++ > include/hw/vfio/vfio-common.h | 6 > trace-events | 2 ++ > 4 files changed, 144 insertions(+), 10 deletions(-) > > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > index 52b08fd..7f55c26 100644 > --- a/hw/vfio/common.c > +++ b/hw/vfio/common.c > @@ -275,6 +275,18 @@ static void vfio_host_win_add(VFIOContainer *container, > QLIST_INSERT_HEAD(>hostwin_list, hostwin, hostwin_next); > } > > +static int vfio_host_win_del(VFIOContainer *container, hwaddr min_iova) > +{ > +VFIOHostDMAWindow *hostwin = vfio_host_win_lookup(container, min_iova, > 1); Hrm.. and for this case I think you want exact match, rather than looking for range inclusion. > + > +if (!hostwin) { > +return -1; > +} > +QLIST_REMOVE(hostwin, hostwin_next); > + > +return 0; > +} > + > static bool vfio_listener_skipped_section(MemoryRegionSection *section) > { > return (!memory_region_is_ram(section->mr) && > @@ -388,6 +400,30 @@ static void vfio_listener_region_add(MemoryListener > *listener, > } > end = int128_get64(int128_sub(llend, int128_one())); > > +if (container->iommu_type == VFIO_SPAPR_TCE_v2_IOMMU) { > +VFIOHostDMAWindow *hostwin; > +hwaddr pgsize = 0; > + > +/* For now intersections are not allowed, we may relax this later */ > +QLIST_FOREACH(hostwin, >hostwin_list, hostwin_next) { > +if (ranges_overlap(hostwin->min_iova, > + hostwin->max_iova - hostwin->min_iova + 1, > + section->offset_within_address_space, > + int128_get64(section->size))) { > +goto fail; > +} > +} > + > +ret = vfio_spapr_create_window(container, section, ); > +if (ret) { > +goto fail; > +} > + > +vfio_host_win_add(container, section->offset_within_address_space, > + section->offset_within_address_space + > + int128_get64(section->size) - 1, pgsize); > +} > + > if (!vfio_host_win_lookup(container, iova, end)) { > error_report("vfio: IOMMU container %p can't map guest IOVA region" > " 0x%"HWADDR_PRIx"..0x%"HWADDR_PRIx, > @@ -523,6 +559,18 @@ static void vfio_listener_region_del(MemoryListener > *listener, > "0x%"HWADDR_PRIx") = %d (%m)", > container, iova, int128_get64(llsize), ret); > } > + > +if (container->iommu_type == VFIO_SPAPR_TCE_v2_IOMMU) { > +vfio_spapr_remove_window(container, > + section->offset_within_address_space); Should check for error here. > +if (vfio_host_win_del(container, > + section->offset_within_address_space) < 0) { > +hw_error("%s: Cannot delete missing window at %"HWADDR_PRIx, > + __func__, section->offset_within_address_space); Personally I think assert() would be better here, but Alex doesn't like them so I'm ok with this. > +} > + > +trace_vfio_spapr_remove_window(section->offset_within_address_space); > +} > } > > static const MemoryListener vfio_memory_listener = { > @@ -960,11 +1008,6 @@ static int vfio_connect_container(VFIOGroup *group, > AddressSpace *as) > } > } > > -/* > - * This only considers the host IOMMU's 32-bit window. At > - * some point we need to add support for the optional 64-bit > - * window and dynamic windows > - */ > info.argsz = sizeof(info); > ret = ioctl(fd, VFIO_IOMMU_SPAPR_TCE_GET_INFO, ); > if (ret) { > @@ -973,11 +1016,24 @@ static int vfio_connect_container(VFIOGroup *group, > AddressSpace
[Qemu-devel] [PATCH qemu v17 10/12] vfio/spapr: Create DMA window dynamically (SPAPR IOMMU v2)
New VFIO_SPAPR_TCE_v2_IOMMU type supports dynamic DMA window management. This adds ability to VFIO common code to dynamically allocate/remove DMA windows in the host kernel when new VFIO container is added/removed. This adds VFIO_IOMMU_SPAPR_TCE_CREATE ioctl to vfio_listener_region_add and adds just created IOMMU into the host IOMMU list; the opposite action is taken in vfio_listener_region_del. When creating a new window, this uses heuristic to decide on the TCE table levels number. This should cause no guest visible change in behavior. Signed-off-by: Alexey Kardashevskiy--- Changes: v17: * moved spapr window create/remove helpers to separate file * added hw_error() if vfio_host_win_del() failed v16: * used memory_region_iommu_get_page_sizes() in vfio_listener_region_add() * enforced no intersections between windows v14: * new to the series --- hw/vfio/common.c | 76 +-- hw/vfio/spapr.c | 70 +++ include/hw/vfio/vfio-common.h | 6 trace-events | 2 ++ 4 files changed, 144 insertions(+), 10 deletions(-) diff --git a/hw/vfio/common.c b/hw/vfio/common.c index 52b08fd..7f55c26 100644 --- a/hw/vfio/common.c +++ b/hw/vfio/common.c @@ -275,6 +275,18 @@ static void vfio_host_win_add(VFIOContainer *container, QLIST_INSERT_HEAD(>hostwin_list, hostwin, hostwin_next); } +static int vfio_host_win_del(VFIOContainer *container, hwaddr min_iova) +{ +VFIOHostDMAWindow *hostwin = vfio_host_win_lookup(container, min_iova, 1); + +if (!hostwin) { +return -1; +} +QLIST_REMOVE(hostwin, hostwin_next); + +return 0; +} + static bool vfio_listener_skipped_section(MemoryRegionSection *section) { return (!memory_region_is_ram(section->mr) && @@ -388,6 +400,30 @@ static void vfio_listener_region_add(MemoryListener *listener, } end = int128_get64(int128_sub(llend, int128_one())); +if (container->iommu_type == VFIO_SPAPR_TCE_v2_IOMMU) { +VFIOHostDMAWindow *hostwin; +hwaddr pgsize = 0; + +/* For now intersections are not allowed, we may relax this later */ +QLIST_FOREACH(hostwin, >hostwin_list, hostwin_next) { +if (ranges_overlap(hostwin->min_iova, + hostwin->max_iova - hostwin->min_iova + 1, + section->offset_within_address_space, + int128_get64(section->size))) { +goto fail; +} +} + +ret = vfio_spapr_create_window(container, section, ); +if (ret) { +goto fail; +} + +vfio_host_win_add(container, section->offset_within_address_space, + section->offset_within_address_space + + int128_get64(section->size) - 1, pgsize); +} + if (!vfio_host_win_lookup(container, iova, end)) { error_report("vfio: IOMMU container %p can't map guest IOVA region" " 0x%"HWADDR_PRIx"..0x%"HWADDR_PRIx, @@ -523,6 +559,18 @@ static void vfio_listener_region_del(MemoryListener *listener, "0x%"HWADDR_PRIx") = %d (%m)", container, iova, int128_get64(llsize), ret); } + +if (container->iommu_type == VFIO_SPAPR_TCE_v2_IOMMU) { +vfio_spapr_remove_window(container, + section->offset_within_address_space); +if (vfio_host_win_del(container, + section->offset_within_address_space) < 0) { +hw_error("%s: Cannot delete missing window at %"HWADDR_PRIx, + __func__, section->offset_within_address_space); +} + +trace_vfio_spapr_remove_window(section->offset_within_address_space); +} } static const MemoryListener vfio_memory_listener = { @@ -960,11 +1008,6 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as) } } -/* - * This only considers the host IOMMU's 32-bit window. At - * some point we need to add support for the optional 64-bit - * window and dynamic windows - */ info.argsz = sizeof(info); ret = ioctl(fd, VFIO_IOMMU_SPAPR_TCE_GET_INFO, ); if (ret) { @@ -973,11 +1016,24 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as) goto listener_release_exit; } -/* The default table uses 4K pages */ -vfio_host_win_add(container, info.dma32_window_start, - info.dma32_window_start + - info.dma32_window_size - 1, - 0x1000); +if (v2) { +/* + * There is a default window in just created container. + * To make region_add/del simpler, we better remove this + * window now and let those iommu_listener