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

[Marcel Apfelbaum <marcel@redhat.com>]

On 11/30/2015 05:07 PM, Eduardo Habkost wrote:
> On Sun, Nov 29, 2015 at 10:46:03AM +0200, Marcel Apfelbaum wrote:
>> 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 for the explanation. I wonder if there's a better way to
> check if ACPI-based hotplug is needed by looking at
> PCMachineState or PCIBus, so we don't couple the ACPI code to
> piix.c.
>

I suppose we can do something about it, like adding a property to PCMachineState,
lets say bool acpi_hotplug and set it false for Q35.

Then we have:
     pcm = PC_MACHINE(current_machine);
     if(pcm->acpi_hotplug) {
         bus  = pcm->bus;
         ...
     }

Sounds acceptable? If yes, I'll send a patch on top since is not directly related.

Thanks,
Marcel