* [Qemu-devel] [PATCH 1/2] Revert "virtio: unbreak virtio-pci with IOMMU after caching ring translations" @ 2017-03-13 3:29 Jason Wang 2017-03-13 3:29 ` [Qemu-devel] [PATCH 2/2] pci: introduce a bus master container Jason Wang 0 siblings, 1 reply; 6+ messages in thread From: Jason Wang @ 2017-03-13 3:29 UTC (permalink / raw) To: mst, marcel, qemu-devel, peterx, pbonzini; +Cc: Jason Wang This reverts commit 96a8821d21411f10d77ea994af369c6e5c35a2cc. Following patch will come a better solution which does not require a strict order between virtio and IOMMU. CC: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Jason Wang <jasowang@redhat.com> --- hw/virtio/virtio-pci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index b76f3f6..5ce42af 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c @@ -1153,7 +1153,7 @@ static AddressSpace *virtio_pci_get_dma_as(DeviceState *d) VirtIOPCIProxy *proxy = VIRTIO_PCI(d); PCIDevice *dev = &proxy->pci_dev; - return pci_device_iommu_address_space(dev); + return pci_get_address_space(dev); } static int virtio_pci_add_mem_cap(VirtIOPCIProxy *proxy, -- 2.7.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [Qemu-devel] [PATCH 2/2] pci: introduce a bus master container 2017-03-13 3:29 [Qemu-devel] [PATCH 1/2] Revert "virtio: unbreak virtio-pci with IOMMU after caching ring translations" Jason Wang @ 2017-03-13 3:29 ` Jason Wang 2017-03-13 9:07 ` Paolo Bonzini 2017-03-13 9:38 ` Marcel Apfelbaum 0 siblings, 2 replies; 6+ messages in thread From: Jason Wang @ 2017-03-13 3:29 UTC (permalink / raw) To: mst, marcel, qemu-devel, peterx, pbonzini; +Cc: Jason Wang 96a8821d2141 ("virtio: unbreak virtio-pci with IOMMU after caching ring translations") tries to make IOMMU works with virtio memory region cache, but it requires IOMMU to be created before any virtio devices. This is sub optimal, fixing this by introduce a bus master container to make sure address space can be initialized during device registering, and then we can safely set alias and make bus_master_enable_region as its subregion during bus master initialization. Cc: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Jason Wang <jasowang@redhat.com> --- hw/pci/pci.c | 9 +++++++-- include/hw/pci/pci.h | 1 + 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/hw/pci/pci.c b/hw/pci/pci.c index 273f1e4..ad46390 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -88,8 +88,8 @@ static void pci_init_bus_master(PCIDevice *pci_dev) OBJECT(pci_dev), "bus master", dma_as->root, 0, memory_region_size(dma_as->root)); memory_region_set_enabled(&pci_dev->bus_master_enable_region, false); - address_space_init(&pci_dev->bus_master_as, - &pci_dev->bus_master_enable_region, pci_dev->name); + memory_region_add_subregion(&pci_dev->bus_master_container_region, 0, + &pci_dev->bus_master_enable_region); } static void pcibus_machine_done(Notifier *notifier, void *data) @@ -995,6 +995,11 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus, pci_dev->devfn = devfn; pci_dev->requester_id_cache = pci_req_id_cache_get(pci_dev); + memory_region_init(&pci_dev->bus_master_container_region, OBJECT(pci_dev), + "bus master container", UINT64_MAX); + address_space_init(&pci_dev->bus_master_as, + &pci_dev->bus_master_container_region, pci_dev->name); + if (qdev_hotplug) { pci_init_bus_master(pci_dev); } diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h index 9349acb..713ede0 100644 --- a/include/hw/pci/pci.h +++ b/include/hw/pci/pci.h @@ -284,6 +284,7 @@ struct PCIDevice { char name[64]; PCIIORegion io_regions[PCI_NUM_REGIONS]; AddressSpace bus_master_as; + MemoryRegion bus_master_container_region; MemoryRegion bus_master_enable_region; /* do not access the following fields */ -- 2.7.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] pci: introduce a bus master container 2017-03-13 3:29 ` [Qemu-devel] [PATCH 2/2] pci: introduce a bus master container Jason Wang @ 2017-03-13 9:07 ` Paolo Bonzini 2017-03-13 9:38 ` Marcel Apfelbaum 1 sibling, 0 replies; 6+ messages in thread From: Paolo Bonzini @ 2017-03-13 9:07 UTC (permalink / raw) To: Jason Wang, mst, marcel, qemu-devel, peterx On 13/03/2017 04:29, Jason Wang wrote: > 96a8821d2141 ("virtio: unbreak virtio-pci with IOMMU after caching ring > translations") tries to make IOMMU works with virtio memory region > cache, but it requires IOMMU to be created before any virtio > devices. This is sub optimal, fixing this by introduce a bus master > container to make sure address space can be initialized during device > registering, and then we can safely set alias and make > bus_master_enable_region as its subregion during bus master > initialization. > > Cc: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Jason Wang <jasowang@redhat.com> > --- > hw/pci/pci.c | 9 +++++++-- > include/hw/pci/pci.h | 1 + > 2 files changed, 8 insertions(+), 2 deletions(-) > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index 273f1e4..ad46390 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -88,8 +88,8 @@ static void pci_init_bus_master(PCIDevice *pci_dev) > OBJECT(pci_dev), "bus master", > dma_as->root, 0, memory_region_size(dma_as->root)); > memory_region_set_enabled(&pci_dev->bus_master_enable_region, false); > - address_space_init(&pci_dev->bus_master_as, > - &pci_dev->bus_master_enable_region, pci_dev->name); > + memory_region_add_subregion(&pci_dev->bus_master_container_region, 0, > + &pci_dev->bus_master_enable_region); > } > > static void pcibus_machine_done(Notifier *notifier, void *data) > @@ -995,6 +995,11 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus, > pci_dev->devfn = devfn; > pci_dev->requester_id_cache = pci_req_id_cache_get(pci_dev); > > + memory_region_init(&pci_dev->bus_master_container_region, OBJECT(pci_dev), > + "bus master container", UINT64_MAX); > + address_space_init(&pci_dev->bus_master_as, > + &pci_dev->bus_master_container_region, pci_dev->name); > + > if (qdev_hotplug) { > pci_init_bus_master(pci_dev); > } > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h > index 9349acb..713ede0 100644 > --- a/include/hw/pci/pci.h > +++ b/include/hw/pci/pci.h > @@ -284,6 +284,7 @@ struct PCIDevice { > char name[64]; > PCIIORegion io_regions[PCI_NUM_REGIONS]; > AddressSpace bus_master_as; > + MemoryRegion bus_master_container_region; > MemoryRegion bus_master_enable_region; > > /* do not access the following fields */ > Series Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] pci: introduce a bus master container 2017-03-13 3:29 ` [Qemu-devel] [PATCH 2/2] pci: introduce a bus master container Jason Wang 2017-03-13 9:07 ` Paolo Bonzini @ 2017-03-13 9:38 ` Marcel Apfelbaum 2017-03-13 9:52 ` Paolo Bonzini 1 sibling, 1 reply; 6+ messages in thread From: Marcel Apfelbaum @ 2017-03-13 9:38 UTC (permalink / raw) To: Jason Wang, mst, qemu-devel, peterx, pbonzini On 03/13/2017 05:29 AM, Jason Wang wrote: > 96a8821d2141 ("virtio: unbreak virtio-pci with IOMMU after caching ring > translations") tries to make IOMMU works with virtio memory region > cache, but it requires IOMMU to be created before any virtio > devices. This is sub optimal, fixing this by introduce a bus master > container to make sure address space can be initialized during device > registering, and then we can safely set alias and make > bus_master_enable_region as its subregion during bus master > initialization. > > Cc: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Jason Wang <jasowang@redhat.com> > --- > hw/pci/pci.c | 9 +++++++-- > include/hw/pci/pci.h | 1 + > 2 files changed, 8 insertions(+), 2 deletions(-) > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index 273f1e4..ad46390 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -88,8 +88,8 @@ static void pci_init_bus_master(PCIDevice *pci_dev) > OBJECT(pci_dev), "bus master", > dma_as->root, 0, memory_region_size(dma_as->root)); > memory_region_set_enabled(&pci_dev->bus_master_enable_region, false); > - address_space_init(&pci_dev->bus_master_as, > - &pci_dev->bus_master_enable_region, pci_dev->name); > + memory_region_add_subregion(&pci_dev->bus_master_container_region, 0, > + &pci_dev->bus_master_enable_region); Hi Jason, The patch looks good to me, I only have one question: On hot-unplug, shouldn't we remove the "new" sub-region ? Thanks, Marcel > } > > static void pcibus_machine_done(Notifier *notifier, void *data) > @@ -995,6 +995,11 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus, > pci_dev->devfn = devfn; > pci_dev->requester_id_cache = pci_req_id_cache_get(pci_dev); > > + memory_region_init(&pci_dev->bus_master_container_region, OBJECT(pci_dev), > + "bus master container", UINT64_MAX); > + address_space_init(&pci_dev->bus_master_as, > + &pci_dev->bus_master_container_region, pci_dev->name); > + > if (qdev_hotplug) { > pci_init_bus_master(pci_dev); > } > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h > index 9349acb..713ede0 100644 > --- a/include/hw/pci/pci.h > +++ b/include/hw/pci/pci.h > @@ -284,6 +284,7 @@ struct PCIDevice { > char name[64]; > PCIIORegion io_regions[PCI_NUM_REGIONS]; > AddressSpace bus_master_as; > + MemoryRegion bus_master_container_region; > MemoryRegion bus_master_enable_region; > > /* do not access the following fields */ > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] pci: introduce a bus master container 2017-03-13 9:38 ` Marcel Apfelbaum @ 2017-03-13 9:52 ` Paolo Bonzini 2017-03-13 10:18 ` Marcel Apfelbaum 0 siblings, 1 reply; 6+ messages in thread From: Paolo Bonzini @ 2017-03-13 9:52 UTC (permalink / raw) To: Marcel Apfelbaum, Jason Wang, mst, qemu-devel, peterx On 13/03/2017 10:38, Marcel Apfelbaum wrote: > On 03/13/2017 05:29 AM, Jason Wang wrote: >> 96a8821d2141 ("virtio: unbreak virtio-pci with IOMMU after caching ring >> translations") tries to make IOMMU works with virtio memory region >> cache, but it requires IOMMU to be created before any virtio >> devices. This is sub optimal, fixing this by introduce a bus master >> container to make sure address space can be initialized during device >> registering, and then we can safely set alias and make >> bus_master_enable_region as its subregion during bus master >> initialization. >> >> Cc: Paolo Bonzini <pbonzini@redhat.com> >> Signed-off-by: Jason Wang <jasowang@redhat.com> >> --- >> hw/pci/pci.c | 9 +++++++-- >> include/hw/pci/pci.h | 1 + >> 2 files changed, 8 insertions(+), 2 deletions(-) >> >> diff --git a/hw/pci/pci.c b/hw/pci/pci.c >> index 273f1e4..ad46390 100644 >> --- a/hw/pci/pci.c >> +++ b/hw/pci/pci.c >> @@ -88,8 +88,8 @@ static void pci_init_bus_master(PCIDevice *pci_dev) >> OBJECT(pci_dev), "bus master", >> dma_as->root, 0, >> memory_region_size(dma_as->root)); >> memory_region_set_enabled(&pci_dev->bus_master_enable_region, >> false); >> - address_space_init(&pci_dev->bus_master_as, >> - &pci_dev->bus_master_enable_region, >> pci_dev->name); >> + >> memory_region_add_subregion(&pci_dev->bus_master_container_region, 0, >> + &pci_dev->bus_master_enable_region); > > Hi Jason, > > The patch looks good to me, I only have one question: > On hot-unplug, shouldn't we remove the "new" sub-region ? It should be done automatically: /* We know the region is not visible in any address space (it * does not have a container and cannot be a root either because * it has no references, so we can blindly clear mr->enabled. * memory_region_set_enabled instead could trigger a transaction * and cause an infinite loop. */ mr->enabled = false; memory_region_transaction_begin(); while (!QTAILQ_EMPTY(&mr->subregions)) { MemoryRegion *subregion = QTAILQ_FIRST(&mr->subregions); memory_region_del_subregion(mr, subregion); } memory_region_transaction_commit(); It's relatively new though (QEMU v2.5+). Paolo > Thanks, > Marcel > >> } >> >> static void pcibus_machine_done(Notifier *notifier, void *data) >> @@ -995,6 +995,11 @@ static PCIDevice >> *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus, >> pci_dev->devfn = devfn; >> pci_dev->requester_id_cache = pci_req_id_cache_get(pci_dev); >> >> + memory_region_init(&pci_dev->bus_master_container_region, >> OBJECT(pci_dev), >> + "bus master container", UINT64_MAX); >> + address_space_init(&pci_dev->bus_master_as, >> + &pci_dev->bus_master_container_region, >> pci_dev->name); >> + >> if (qdev_hotplug) { >> pci_init_bus_master(pci_dev); >> } >> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h >> index 9349acb..713ede0 100644 >> --- a/include/hw/pci/pci.h >> +++ b/include/hw/pci/pci.h >> @@ -284,6 +284,7 @@ struct PCIDevice { >> char name[64]; >> PCIIORegion io_regions[PCI_NUM_REGIONS]; >> AddressSpace bus_master_as; >> + MemoryRegion bus_master_container_region; >> MemoryRegion bus_master_enable_region; >> >> /* do not access the following fields */ >> > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] pci: introduce a bus master container 2017-03-13 9:52 ` Paolo Bonzini @ 2017-03-13 10:18 ` Marcel Apfelbaum 0 siblings, 0 replies; 6+ messages in thread From: Marcel Apfelbaum @ 2017-03-13 10:18 UTC (permalink / raw) To: Paolo Bonzini, Jason Wang, mst, qemu-devel, peterx On 03/13/2017 11:52 AM, Paolo Bonzini wrote: > > > On 13/03/2017 10:38, Marcel Apfelbaum wrote: >> On 03/13/2017 05:29 AM, Jason Wang wrote: >>> 96a8821d2141 ("virtio: unbreak virtio-pci with IOMMU after caching ring >>> translations") tries to make IOMMU works with virtio memory region >>> cache, but it requires IOMMU to be created before any virtio >>> devices. This is sub optimal, fixing this by introduce a bus master >>> container to make sure address space can be initialized during device >>> registering, and then we can safely set alias and make >>> bus_master_enable_region as its subregion during bus master >>> initialization. >>> >>> Cc: Paolo Bonzini <pbonzini@redhat.com> >>> Signed-off-by: Jason Wang <jasowang@redhat.com> >>> --- >>> hw/pci/pci.c | 9 +++++++-- >>> include/hw/pci/pci.h | 1 + >>> 2 files changed, 8 insertions(+), 2 deletions(-) >>> >>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c >>> index 273f1e4..ad46390 100644 >>> --- a/hw/pci/pci.c >>> +++ b/hw/pci/pci.c >>> @@ -88,8 +88,8 @@ static void pci_init_bus_master(PCIDevice *pci_dev) >>> OBJECT(pci_dev), "bus master", >>> dma_as->root, 0, >>> memory_region_size(dma_as->root)); >>> memory_region_set_enabled(&pci_dev->bus_master_enable_region, >>> false); >>> - address_space_init(&pci_dev->bus_master_as, >>> - &pci_dev->bus_master_enable_region, >>> pci_dev->name); >>> + >>> memory_region_add_subregion(&pci_dev->bus_master_container_region, 0, >>> + &pci_dev->bus_master_enable_region); >> >> Hi Jason, >> >> The patch looks good to me, I only have one question: >> On hot-unplug, shouldn't we remove the "new" sub-region ? > > It should be done automatically: > > /* We know the region is not visible in any address space (it > * does not have a container and cannot be a root either because > * it has no references, so we can blindly clear mr->enabled. > * memory_region_set_enabled instead could trigger a transaction > * and cause an infinite loop. > */ > mr->enabled = false; > memory_region_transaction_begin(); > while (!QTAILQ_EMPTY(&mr->subregions)) { > MemoryRegion *subregion = QTAILQ_FIRST(&mr->subregions); > memory_region_del_subregion(mr, subregion); > } > memory_region_transaction_commit(); > > It's relatively new though (QEMU v2.5+). > Nice! Not that new, but new to me :) Reviewed-by: Marcel Apfelbaum <marcel@redhat.com> Thanks, Marcel > Paolo > >> Thanks, >> Marcel >> >>> } >>> >>> static void pcibus_machine_done(Notifier *notifier, void *data) >>> @@ -995,6 +995,11 @@ static PCIDevice >>> *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus, >>> pci_dev->devfn = devfn; >>> pci_dev->requester_id_cache = pci_req_id_cache_get(pci_dev); >>> >>> + memory_region_init(&pci_dev->bus_master_container_region, >>> OBJECT(pci_dev), >>> + "bus master container", UINT64_MAX); >>> + address_space_init(&pci_dev->bus_master_as, >>> + &pci_dev->bus_master_container_region, >>> pci_dev->name); >>> + >>> if (qdev_hotplug) { >>> pci_init_bus_master(pci_dev); >>> } >>> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h >>> index 9349acb..713ede0 100644 >>> --- a/include/hw/pci/pci.h >>> +++ b/include/hw/pci/pci.h >>> @@ -284,6 +284,7 @@ struct PCIDevice { >>> char name[64]; >>> PCIIORegion io_regions[PCI_NUM_REGIONS]; >>> AddressSpace bus_master_as; >>> + MemoryRegion bus_master_container_region; >>> MemoryRegion bus_master_enable_region; >>> >>> /* do not access the following fields */ >>> >> ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-03-13 10:18 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-03-13 3:29 [Qemu-devel] [PATCH 1/2] Revert "virtio: unbreak virtio-pci with IOMMU after caching ring translations" Jason Wang 2017-03-13 3:29 ` [Qemu-devel] [PATCH 2/2] pci: introduce a bus master container Jason Wang 2017-03-13 9:07 ` Paolo Bonzini 2017-03-13 9:38 ` Marcel Apfelbaum 2017-03-13 9:52 ` Paolo Bonzini 2017-03-13 10:18 ` Marcel Apfelbaum
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).