Re: [Qemu-devel] [PATCH v2 1/3] hw/pci: delay bus_master_enable_region initialization

[Marcel Apfelbaum]

On 06/08/2016 02:16 PM, Paolo Bonzini wrote:
> ----- Original Message -----
> > From: "Marcel Apfelbaum" <address@hidden>
> > To: address@hidden
> > Cc: address@hidden, address@hidden, address@hidden, address@hidden, 
> > address@hidden,
> > address@hidden, "jan kiszka" <address@hidden>, "bd aviv" <address@hidden>, "alex 
> > williamson"
> > <address@hidden>
> > Sent: Thursday, June 2, 2016 10:15:53 PM
> > Subject: [Qemu-devel] [PATCH v2 1/3] hw/pci: delay bus_master_enable_region 
> > initialization
> >
> > Skip bus_master_enable region creation on PCI device init
> > in order to be sure the IOMMU device (if present) would
> > be created in advance. Add this memory region at machine_done time.
> >
> > Signed-off-by: Marcel Apfelbaum <address@hidden>
> > ---
> >   hw/pci/pci.c             | 46 ++++++++++++++++++++++++++++++++++++----------
> >   include/hw/pci/pci_bus.h |  2 ++
> >   2 files changed, 38 insertions(+), 10 deletions(-)
> >
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index bb605ef..29dcbcf 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -127,11 +127,44 @@ static void pci_bus_class_init(ObjectClass *klass, void
> > *data)
> >       pbc->numa_node = pcibus_numa_node;
> >   }
> >
> > +static void pci_init_bus_master(PCIDevice *pci_dev)
> > +{
> > +    AddressSpace *dma_as = pci_device_iommu_address_space(pci_dev);
> > +
> > +    memory_region_init_alias(&pci_dev->bus_master_enable_region,
> > +                             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);
> > +}
> > +
> > +static void pcibus_machine_done(Notifier *notifier, void *data)
> > +{
> > +    PCIBus *bus = container_of(notifier, PCIBus, machine_done);
> > +    int i;
> > +
> > +    for (i = 0; i < ARRAY_SIZE(bus->devices); ++i) {
> > +        if (bus->devices[i]) {
> > +            pci_init_bus_master(bus->devices[i]);
> > +        }
> > +    }
> > +}
> > +
> > +static void pcibus_initfn(Object *obj)
> > +{
> > +    PCIBus *bus = PCI_BUS(obj);
> > +
> > +    bus->machine_done.notify = pcibus_machine_done;
> > +    qemu_add_machine_init_done_notifier(&bus->machine_done);
> > +}
>
> This should be done at realize time, otherwise
>
>      object_unref(object_new(TYPE_PCI_BUS));
>
> leaves a dangling reference.
>
> On one hand I think it's fair to assume that there's no unrealize
> between realize and machine done; if something fails to realize
> QEMU will exit.
>
> On the other hand it might break in weird ways in the future.
> So if you could add a qemu_remove_machine_init_done_notifier and
> call it from bus unrealize, it would be best.

I can do that, sure. I'll also move the pcibus_initfn code in the realize 
function.
Thanks,
Marcel

> Thanks,
>
> Paolo
>
> >   static const TypeInfo pci_bus_info = {
> >       .name = TYPE_PCI_BUS,
> >       .parent = TYPE_BUS,
> >       .instance_size = sizeof(PCIBus),
> >       .class_size = sizeof(PCIBusClass),
> > +    .instance_init = pcibus_initfn,
> >       .class_init = pci_bus_class_init,
> >   };
> >
> > @@ -845,7 +878,6 @@ static PCIDevice *do_pci_register_device(PCIDevice
> > *pci_dev, PCIBus *bus,
> >       PCIConfigReadFunc *config_read = pc->config_read;
> >       PCIConfigWriteFunc *config_write = pc->config_write;
> >       Error *local_err = NULL;
> > -    AddressSpace *dma_as;
> >       DeviceState *dev = DEVICE(pci_dev);
> >
> >       pci_dev->bus = bus;
> > @@ -885,15 +917,9 @@ static PCIDevice *do_pci_register_device(PCIDevice
> > *pci_dev, PCIBus *bus,
> >       }
> >
> >       pci_dev->devfn = devfn;
> > -    dma_as = pci_device_iommu_address_space(pci_dev);
> > -
> > -    memory_region_init_alias(&pci_dev->bus_master_enable_region,
> > -                             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,
> > -                       name);
> > -
> > +    if (qdev_hotplug) {
> > +        pci_init_bus_master(pci_dev);
> > +    }
> >       pstrcpy(pci_dev->name, sizeof(pci_dev->name), name);
> >       pci_dev->irq_state = 0;
> >       pci_config_alloc(pci_dev);
> > diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
> > index 403fec6..5484a9b 100644
> > --- a/include/hw/pci/pci_bus.h
> > +++ b/include/hw/pci/pci_bus.h
> > @@ -39,6 +39,8 @@ struct PCIBus {
> >          Keep a count of the number of devices with raised IRQs.  */
> >       int nirq;
> >       int *irq_count;
> > +
> > +    Notifier machine_done;
> >   };
> >
> >   typedef struct PCIBridgeWindows PCIBridgeWindows;
> > --
> > 2.4.3