Re: [Qemu-devel] [PATCH V3 3/3] hw/i386: extend pxb query for all PC machines

[Marcel Apfelbaum <marcel@redhat.com>]

On 11/27/2015 07:28 PM, Eduardo Habkost wrote:
> On Thu, Nov 26, 2015 at 06:00:28PM +0200, Marcel Apfelbaum wrote:
>> Add bus property to PC machines and use it when looking
>> for primary PCI root bus (bus 0).
>>
>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>
> I can't pretend I have reviewed the q35 part, but the changes are
> an improvement to the existing code that depended on
> find_i440fx().
>
> Acked-by: Eduardo Habkost <ehabkost@redhat.com>

Thanks!

>
> BTW, what's missing to allow us to change acpi_set_pci_info() to
> use PCMachine::bus instead of find_i440fx(), too? How much of the
> PCI hotplug stuff is different in q35?

It is pretty different.
i440fx has acpi based hotplug while q35 has PCIe native hotplug. Since is
"native", no acpi info is necessary.

Having said that, if we have an PCIe-PCI bridge, the pci devices behind it
cannot be hotplugged/unplugged right now.

Once we decide to add hotplug support for this scenario, maybe we can get rid of
find_i440fx().

Thanks,
Marcel

>
>> ---
>>   hw/i386/acpi-build.c | 3 +--
>>   hw/i386/pc.c         | 2 +-
>>   hw/i386/pc_piix.c    | 1 +
>>   hw/i386/pc_q35.c     | 1 +
>>   include/hw/i386/pc.h | 1 +
>>   5 files changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index 736b252..bca3f06 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -950,8 +950,7 @@ build_ssdt(GArray *table_data, GArray *linker,
>>       /* Reserve space for header */
>>       acpi_data_push(ssdt->buf, sizeof(AcpiTableHeader));
>>
>> -    /* Extra PCI root buses are implemented  only for i440fx */
>> -    bus = find_i440fx();
>> +    bus = PC_MACHINE(machine)->bus;
>>       if (bus) {
>>           QLIST_FOREACH(bus, &bus->child, sibling) {
>>               uint8_t bus_num = pci_bus_num(bus);
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index 5e20e07..07697ed 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -1174,7 +1174,7 @@ void pc_guest_info_machine_done(Notifier *notifier, void *data)
>>       PcGuestInfoState *guest_info_state = container_of(notifier,
>>                                                         PcGuestInfoState,
>>                                                         machine_done);
>> -    PCIBus *bus = find_i440fx();
>> +    PCIBus *bus = PC_MACHINE(qdev_get_machine())->bus;
>>
>>       if (bus) {
>>           int extra_hosts = 0;
>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>> index 07d0baa..ca6ef9a 100644
>> --- a/hw/i386/pc_piix.c
>> +++ b/hw/i386/pc_piix.c
>> @@ -197,6 +197,7 @@ static void pc_init1(MachineState *machine,
>>                                 pcms->below_4g_mem_size,
>>                                 pcms->above_4g_mem_size,
>>                                 pci_memory, ram_memory);
>> +        pcms->bus = pci_bus;
>>       } else {
>>           pci_bus = NULL;
>>           i440fx_state = NULL;
>> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>> index 0fdae09..822b3e5 100644
>> --- a/hw/i386/pc_q35.c
>> +++ b/hw/i386/pc_q35.c
>> @@ -192,6 +192,7 @@ static void pc_q35_init(MachineState *machine)
>>       qdev_init_nofail(DEVICE(q35_host));
>>       phb = PCI_HOST_BRIDGE(q35_host);
>>       host_bus = phb->bus;
>> +    pcms->bus = phb->bus;
>>       /* create ISA bus */
>>       lpc = pci_create_simple_multifunction(host_bus, PCI_DEVFN(ICH9_LPC_DEV,
>>                                             ICH9_LPC_FUNC), true,
>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>> index 854c330..e42771c 100644
>> --- a/include/hw/i386/pc.h
>> +++ b/include/hw/i386/pc.h
>> @@ -41,6 +41,7 @@ struct PCMachineState {
>>       OnOffAuto smm;
>>       bool enforce_aligned_dimm;
>>       ram_addr_t below_4g_mem_size, above_4g_mem_size;
>> +    PCIBus *bus;
>>   };
>>
>>   #define PC_MACHINE_ACPI_DEVICE_PROP "acpi-device"
>> --
>> 2.1.0
>>
>