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

[Marcel Apfelbaum]

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