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

[Marcel Apfelbaum <marcel@redhat.com>]

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 <marcel@redhat.com>
>>>>>
>>>>> 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 <ehabkost@redhat.com>
>>>>
>>>> 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