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:55:57PM +0200, Marcel Apfelbaum wrote:
> 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.

What about just checking if AcpiPmInfo.pcihp_io_base is set?

-- 
Eduardo