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

[Eduardo Habkost]

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? If not, that sounds good to me. But I would move
the field to PCMachineClass instead of PCMachineState.

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

-- 
Eduardo