Re: [PATCH 11/12] hw/xen: automatically assign device index to block devices

[David Woodhouse <dwmw2@xxxxxxxxxxxxx>]

On Wed, 2023-10-18 at 10:52 +0200, Kevin Wolf wrote:
> Am 16.10.2023 um 17:19 hat David Woodhouse geschrieben:
> > From: David Woodhouse <dwmw@xxxxxxxxxxxx>
> > 
> > There's no need to force the user to assign a vdev. We can automatically
> > assign one, starting at xvda and searching until we find the first disk
> > name that's unused.
> > 
> > This means we can now allow '-drive if=xen,file=xxx' to work without an
> > explicit separate -driver argument, just like if=virtio.
> > 
> > Signed-off-by: David Woodhouse <dwmw@xxxxxxxxxxxx>
> 
> Actually, how does this play together with xen_config_dev_blk()? This
> looks like it tried to implement a very similar thing (which is IF_XEN
> even already existed).

Ah yes, thanks for spotting that! I hadn't been looking at the xenfv

> Are we now trying to attach each if=xen disk twice in the 'xenpv'
> machine? Or if something prevents this, is it dead code.

I suspect we end up creating them twice (and probably thus failing to
lock the backing file).

The old Xen PV device drivers in Qemu, before Paul's XenDevice
conversion, were purely reactive. If they see nodes in XenStore
describing a backend which they should implement, they'd do so.

The code in xen_config_dev_blk() is *creating* those nodes for the
frontend (guest) and backend (host/qemu) to see, which prompts the
xen-block and xen-net drivers into action.

In the new XenDevice model, we can just instantiate a device (e.g.
qdev_new(TYPE_XEN_DISK_DEVICE) and its realize() method will create the
corresponding XenStore nodes (with some help from the generic XenBus
code for the common ones).

The new XenDevice/XenBus model will *also* react to nodes which it
discovers in XenStore, and instantiate the corresponding devices.

So I suspect what'll happen is xen_config_dev_blk() will create the
nodes starting at xvda (int vdev = 202 * 256 + 16 * disk->unit), and
later my new code will create a new set starting from xvdb or wherever
the next free one is.

> I think in both cases, we would want to delete that function and remove
> the loop that calls it in xen_init_pv()?

Yes, I think so. The xen_config_dev_blk() one can just die, as can
xen_config_dev_console() which isn't called from anywhere anyway.

The vkbd/vfb ones can stay until/unless those drivers are converted to
the new XenDevice model.

And xen_config_dev_nic() probably just needs to loop doing the same as
I did in pc_init_nic() in
https://lore.kernel.org/qemu-devel/20231017182545.97973-5-dwmw2@xxxxxxxxxxxxx/T/#u

+        if (xen_bus && (!nd->model || g_str_equal(model, "xen-net-device"))) {
+            DeviceState *dev = qdev_new("xen-net-device");
+            qdev_set_nic_properties(dev, nd);
+            qdev_realize_and_unref(dev, xen_bus, &error_fatal);


... but this just reinforces what I said there about "if
qmp_device_add() can find the damn bus and do this right, why do we
have to litter it through platform code?"

Attachment: smime.p7s
Description: S/MIME cryptographic signature