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

[Marcel Apfelbaum]

On 12/01/2015 04:48 PM, Eduardo Habkost wrote:
> On Tue, Dec 01, 2015 at 04:07:33PM +0200, Marcel Apfelbaum wrote:
> > 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.S
>
> There's no existing field or method in PCIBus that can be already
> used for that?

Hmm, you can derive the info you need from pci_bus_is_express.
If express-> no acpi_hotplug. This is not 100% true, but since
we don't support acpi hotplug on PCIe machines, it should be OK for now.

 If not, that sounds good to me. But I would move
> the field to PCMachineClass instead of PCMachineState.

Sure.

> Would you like me to do it? I am planning to submit other changes
> to remove q35/piix dependencies from acpi-build.c.

If you have the time, go for it :)
Is a "one liner", if applied on top of this series.

Thanks,
Marcel