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

[Kevin Wolf <kwolf@redhat.com>]

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
is wrong anyway. So it feels like it's not the change we need here, but
more a step sideways.

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.

Kevin