Re: [Qemu-devel] [PATCH 2/2] pci: introduce a bus master container

[Marcel Apfelbaum]

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 <address@hidden>
> Signed-off-by: Jason Wang <address@hidden>
> ---
>  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 */