Re: [Qemu-devel] [PATCH V2 0/4] hw/pcie: Multi-root support for Q35

[Marcel Apfelbaum <marcel@redhat.com>]

On 11/17/2015 02:26 PM, Markus Armbruster wrote:
> Marcel Apfelbaum <marcel@redhat.com> writes:
>
>> On 11/17/2015 10:15 AM, Markus Armbruster wrote:
>>> Marcel Apfelbaum <marcel@redhat.com> writes:
>>>
>>>> On 11/16/2015 12:11 PM, Paolo Bonzini wrote:
>>>>>
>>>>>
>>>>> On 16/11/2015 11:10, Marcel Apfelbaum wrote:
>>>>>>> What would you lose?  Hotplug?
>>>>>>
>>>>>> Without the bridge? Yes. However the user can add it manually the
>>>>>> pci-bridge and have it anyway.
>>>>>
>>>>> Ok, I guess that's more or less acceptable.  It's still ugly however, to
>>>>> the point that I wonder if we should rename the device and call the old
>>>>> one a failed experiment.
>>>>>
>>>>
>>>> I guess we can rename the pxb to extra-root or something, but in this way
>>>> will have a deprecated/duplicated device to support and kill in the future.
>>>>
>>>> Why not use the compat property as it is?
>>>> Again, the command line *remains* the same, the difference is where the
>>>> devices associated with the pxb will land: on the secondary bus
>>>> (for QEMU < 2.5)
>>>> or on the root bus itself (QEMU >= 2.5).
>>>>
>>>> I know is guest visible, but the guest will see one of them depending
>>>> on the machine type.
>>>>
>>>> Regarding the splitting of pxb into 2 devices (pci/pcie), I have
>>>> nothing against it,
>>>> but because the implementation is *exactly* the same I think we
>>>> should gain more
>>>> by maintaining one device.
>>>
>>> I have no opinion on two devices vs. one device + property in this
>>> particular case, I just want to interject that I'd expect the difference
>>> in maintaince to be negligible.
>>>
>>> A second device basically takes a copy of the TypeInfo with some
>>> (trivial) init function to make it different.  Might be a few more lines
>>> of code than adding a property, but in complexity, it's a wash.
>>>
>>> In case you plan to get rid of the old variant: with two devices, you
>>> deprecate and later delete the old device.  With device + property, you
>>> deprecate setting the property, and later delete it.  The former might
>>> be a bit easier to document.
>>
>> Hi Markus,
>> Thank you for the review.
>>
>> Following all the comments I have received, I am going to:
>>
>> 1. Leave the old device (pxb) as is and mark it as deprecated. Maybe
>> you can point me on the cleanest way to mark a device as deprecated?
>
> First, we need to decide whether they're deprecated or legacy.
>
> Deprecated means their use is discouraged and they may go away
> eventually.  Users are advised to switch to the replacement.
>
> Legacy means we keep them around just for compatibility.  Existing uses
> are just fine, but new use is discouraged.
>
> We usually remove deprecated interfaces from help and documentation.
> Sometimes we mark them deprecated for a while before we remove them.
>
> Legacy interfaces stay documented, but should be clearly marked.
>
> Right now, -device help shows all pluggable devices, i.e. the devices
> that don't have cannot_instantiate_with_device_add_yet set.  In other
> words, there's no way to suppress deprecated devices, yet.
>
> Two ways to mark them:
>
> 1. Set DeviceClass member desc to a string containing "(deprecated)" or
>     "(legacy)", respectively.
>
> 2. Create a new DeviceCategory, and move the device there.
>
> I prefer 2.  You can easily turn it into a way to suppress deprecated
> devices: just skip the "deprecated" category.

Thanks Markus for the detailed information!

>
>> 2. Create a new device (pci-expander) that will behave as a:
>>     - PCI root, if the machine's bus 0 is legacy PCI.
>>     - PCI express root (Root Complex), if machine's bus 0 is PCI Express.
>>
>> This way I will not need two device and not even a property for (pci/pcie).
>> We don't really need an extra PCI root on a PCIe machine and vice versa.
>>
>> By the way, following the same concept I converted the virtio devices
>> to PCIe devices.
>> I didn't want a new set of devices.
>
> Trust your judgement :)

Re-thinking this whole thing, I have a new plan :)

We have the pxb device which is working just fine. Having the internal pci-bridge
by default is a good idea (hot-plug support, smaller ACPI tables).

Adding a new device, say "root-complex", that re-uses some of the pxb code
is the best way to go, and it will be easier to maintain when those devices
will diverge.

Thanks,
Marcel

>
> [...]
>