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

[Marcel Apfelbaum]

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 <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 ?
>
> 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 <address@hidden>

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 */