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

[Marcel Apfelbaum <marcel@redhat.com>]

On 06/08/2016 02:16 PM, Paolo Bonzini wrote:
>
>
> ----- Original Message -----
>> From: "Marcel Apfelbaum" <marcel@redhat.com>
>> To: qemu-devel@nongnu.org
>> Cc: marcel@redhat.com, mst@redhat.com, pbonzini@redhat.com, ehabkost@redhat.com, peterx@redhat.com,
>> davidkiarie4@gmail.com, "jan kiszka" <jan.kiszka@web.de>, "bd aviv" <bd.aviv@gmail.com>, "alex williamson"
>> <alex.williamson@redhat.com>
>> 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 <marcel@redhat.com>
>> ---
>>   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
>>
>>