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@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?" 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:
smime.p7s
Description: S/MIME cryptographic signature
