Re: [PATCH 09/11] qdev: Avoid QemuOpts in QMP device_add

[Laurent Vivier <lvivier@redhat.com>]

On 06/10/2021 12:53, Kevin Wolf wrote:
> Am 06.10.2021 um 11:20 hat Laurent Vivier geschrieben:
>> On 06/10/2021 10:21, Juan Quintela wrote:
>>> Kevin Wolf <kwolf@redhat.com> wrote:
>>>> Am 05.10.2021 um 17:52 hat Damien Hedde geschrieben:
>>>
>>> Hi
>>>
>>>>>> Usage
>>>>>> -----
>>>>>>
>>>>>> The primary device can be hotplugged or be part of the startup
>>>>>> configuration
>>>>>>
>>>>>>     -device virtio-net-pci,netdev=hostnet1,id=net1,
>>>>>>             mac=52:54:00:6f:55:cc,bus=root2,failover=on
>>>>>>
>>>>>> With the parameter failover=on the VIRTIO_NET_F_STANDBY feature
>>>>>> will be enabled.
>>>>>>
>>>>>> -device vfio-pci,host=5e:00.2,id=hostdev0,bus=root1,
>>>>>>           failover_pair_id=net1
>>>>>>
>>>>>> failover_pair_id references the id of the virtio-net standby device.
>>>>>> This is only for pairing the devices within QEMU. The guest kernel
>>>>>> module net_failover will match devices with identical MAC addresses.
>>>>>>
>>>>>> Hotplug
>>>>>> -------
>>>>>>
>>>>>> Both primary and standby device can be hotplugged via the QEMU
>>>>>> monitor.  Note that if the virtio-net device is plugged first a
>>>>>> warning will be issued that it couldn't find the primary device.
>>>>>
>>>>> So maybe this whole primary device lookup can happen during the -device CLI
>>>>> option creation loop. And we can indeed have un-created devices still in the
>>>>> list ?
>>>>
>>>> Yes, that's the only case for which I could imagine for an inconsistency
>>>> between the qdev tree and QemuOpts, but failover_add_primary() is only
>>>> called after feature negotiation with the guest driver, so we can be
>>>> sure that the -device loop has completed long ago.
>>>>
>>>> And even if it hadn't completed yet, the paragraph also says that even
>>>> hotplugging the device later is supported, so creating devices in the
>>>> wrong order should still succeed.
>>>>
>>>> I hope that some of the people I added to CC have some more hints.
>>>
>>> Failover is ... interesting.
>>>
>>> You have two devices: primary and seconday.
>>> seconday is virtio-net, primary can be vfio and some other emulated
>>> devices.
>>>
>>> In the command line, devices can appear on any order, primary then
>>> secondary, secondary then primary, or only one of them.
>>> You can add (any of them) later in the toplevel.
>>>
>>> And now, what all this mess is about.  We only enable the primary if the
>>> guest knows about failover.  Otherwise we use only the virtio device
>>> (*).  The important bit here is that we need to wait until the guest is
>>> booted, and the virtio-net driver is loaded, and then it tells us if it
>>> understands failover (or not).  At that point we decide if we want to
>>> "really" create the primary.
>>>
>>> I know that it abuses device_add() as much as it can be, but I can't see
>>> any better way to handle it.  We need to be able to "create" a device
>>> without showing it to the guest.  And later, when we create a different
>>> device, and depending of driver support on the guest, we "finish" the
>>> creation of the primary device.
>>>
>>> Any good idea?
> 
> Hm, the naive idea would be creating the device without attaching it to
> any bus. But I suppose qdev doesn't let you do that.
> 
> Anyway, the part that I missed yesterday is that qdev_device_add()
> already skips creating the device if qdev_should_hide_device(), which
> explains how the inconsistency is created.
> 
> (As an aside, it then returns NULL without setting an error to
> indicate success, which is an awkward interface, and sure enough,
> qmp_device_add() gets it wrong and deletes the QemuOpts again. So
> hotplugging the virtio-net standby device doesn't even seem to work?)
> 
> Could we just save the configuration in the .hide_device callback (i.e.
> failover_hide_primary_device() in virtio-net) to a new field in
> VirtIONet and then use that when actually creating the device instead of
> accessing the command line state in the QemuOptsList?
> 
> It seems that we can currently add two primary devices that are then
> both hidden. failover_add_primary() adds only one of them, leaving the
> other one hidden. Is this a bug and we should reject such a
> configuration or do we need to support keeping configurations for
> multiple primary devices in a single standby device?
> 
> This would still be ugly because the configuration is only really
> validated when the primary device is actually added instead of
> immediately on -device/device_add, but at least it would keep the
> ugliness more local and wouldn't block the move away from QemuOpts (the
> config would just be stored as a QDict after my patches).
> 
>> I don't know if it can help the discussion, but I'm reformatting the
>> failover code to move all the PCI stuff to pci files.
>>
>> And there is a lot of inconsistencies regarding the device_add and --device
>> option so I've been in the end to add a list of of hidden devices rather
>> than relying on the command line.
>>
>> See PATCH 8 of series "[RFC PATCH v2 0/8] virtio-net failover cleanup and new features"
>>
>> https://patchew.org/QEMU/20210820142002.152994-1-lvivier@redhat.com/
> 
> While it's certainly an improvement over the current state, we really
> should move away from QemuOpts and I think using global state for this

I totally agree with that.

> is wrong anyway. So it feels like it's not the change we need here, but
> more a step sideways.

Yes, I wanted to fix the problem without modifying to much the existing code.

> But thanks for mentioning this series here, we might get some merge
> conflicts there. I'll try to remember to CC you for v2 of this series.

Thank you. I'll try to find a better solution based on your series.

Laurent