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

[Eduardo Habkost]

On Tue, Dec 01, 2015 at 06:50:15PM +0200, Marcel Apfelbaum wrote:
> On 12/01/2015 05:09 PM, Eduardo Habkost wrote:
> >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?
> >
> 
> Because this contradicts the "do not probe for piix" requirement.
> pcihp_io_base depends on piix query for pm (piix4_pm_find).
> So pcihp_io_base is an i440fx only "artifact".
> 

Yes, but at least the piix4-specific code would be contained
inside acpi_get_pm_info(). (And making acpi_get_pm_info() generic
is also part of my plans.)

However, you have a good point:

> Also, acpi_set_pci_info is called before acpi_build that populates
> acpi_get_pm_info. All of that can be taken care of, of course.

So we need to be careful about ordering, there. But it looks
doable without adding yet another PCMachineState field.

> 
> At the end of the day, as long as the functionality is preserved,
> I personally have no objection in re-factoring.

Working on it. :)

-- 
Eduardo