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

[David Woodhouse <dwmw2@infradead.org>]

[-- Attachment #1: Type: text/plain, Size: 4044 bytes --]

On Wed, 2023-10-18 at 11:52 +0100, David Woodhouse wrote:
> 
> 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@infradead.org/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?"

I had a look through the network setup.

There are a bunch of platforms adding specific devices to their own
internal system bus, which often use nd_table[] directly. Sometimes
they do so whether it's been set up or now.

They can mostly be divided into two camps. Some of them will create
their NIC anyway, and will use a matching -nic configuration if it
exists. Others will only create their NIC if a corresponding -nic
configuration does exist.

This is fairly random, and perhaps platforms should be more consistent,
but it's best to avoid user-visible changes as much as possible while
doing the cleanup, so I've kept them as they were.

I've created qemu_configure_nic_device() and qemu_create_nic_device()
functions for those two use cases respectively, and most code which
directly accesses nd_table[] can be converted to those fairly simply:
https://git.infradead.org/users/dwmw2/qemu.git/commitdiff/7b4fb6fc10a4

It means I can throw away the horrid parts of -nic support for the Xen
network device, which were in the pc and xenfv platforms, and replace
it with a trivial loop in xenbus_init():

+    /* This is a bus-generic "configure all NICs on this bus type" */
+    while ((qnic = qemu_create_nic_device("xen-net-device", true, "xen"))) {
+            qdev_realize_and_unref(qnic, bus, &error_fatal);

Other than that one (which is cheating because there's only one type of
network device that can be instantiated on the XenBus), the only
remaining case is PCI. Most platforms just iterate over the -nic
configurations adding devices to a PCI bus of the platform's choice.
In some cases there's a special case, the first one goes at a specific
devfn and/or on a different bus.

There was a little more variation here... for example fuloong2e would
put the first nic (nd_table[0]) in slot 7 only if it's an rtl8139 (if
the model is unspecified). But PReP would put the first PCI NIC in slot
3 *regardless* of whether it's the expected PCNET or not.

I didn't faithfully preserve the behaviour there, because I don't think
it matters. They mostly look like this now (e.g. hw/sh4/r2d):

+    nd = qemu_find_nic_info(mc->default_nic, true, NULL);
+    if (nd) {
+        pci_nic_init_nofail(nd, pci_bus, mc->default_nic, "2");
+    }
+    pci_init_nic_devices(pci_bus, mc->default_nic);

So they'll take the first NIC configuration which is of the expected
model (again, or unspecified model) and place that in the special slot,
and then put the rest of the devices wherever they land.

For the change in behaviour to *matter*, the user would have to
explicitly specify a NIC of the *non-default* type first, and then a
NIC of the default type. My new code will put the default-type NIC in
the "right place", while the old code mostly wouldn't. I think that's
an OK change to make.

My plan is to *remember* which NIC models are used for lookups during
the board in, so that qemu_show_nic_devices() can print them all at the
end.

Current WIP if anyone wants to take a quick look at
https://git.infradead.org/users/dwmw2/qemu.git/shortlog/refs/heads/xenfv-net
but the weekend arrived quicker than I'd hoped, so I haven't quite got
it into shape to post yet. Hopefully it makes sense as an approach
though?



[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]