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

[Markus Armbruster]

Marcel Apfelbaum <address@hidden> writes:

> On 11/17/2015 10:15 AM, Markus Armbruster wrote:
>> Marcel Apfelbaum <address@hidden> 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.

> 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 :)

[...]