Message ID | 20231106195352.301038-5-dwmw2@infradead.org |
---|---|
State | Handled Elsewhere |
Headers | show |
Series | Rework matching of network devices to -nic options | expand |
Hi David, On 6/11/23 20:49, David Woodhouse wrote: > From: David Woodhouse <dwmw@amazon.co.uk> > > The loop over nd_table[] to add PCI NICs is repeated in quite a few > places. Add a helper function to do it. > > Some platforms also try to instantiate a specific model in a specific > slot, to match the real hardware. Add pci_init_nic_in_slot() for that > purpose. > > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk> > Reviewed-by: Paul Durrant <paul@xen.org> > --- > hw/pci/pci.c | 45 ++++++++++++++++++++++++++++++++++++++++++++ > include/hw/pci/pci.h | 4 +++- > 2 files changed, 48 insertions(+), 1 deletion(-) > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index 885c04b6f5..5703266c0b 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -1925,6 +1925,51 @@ PCIDevice *pci_nic_init_nofail(NICInfo *nd, PCIBus *rootbus, > return pci_dev; > } > > +void pci_init_nic_devices(PCIBus *bus, const char *default_model) > +{ > + qemu_create_nic_bus_devices(&bus->qbus, TYPE_PCI_DEVICE, default_model, > + "virtio", "virtio-net-pci"); > +} > + > +bool pci_init_nic_in_slot(PCIBus *rootbus, const char *model, > + const char *alias, const char *devaddr) > +{ > + NICInfo *nd = qemu_find_nic_info(model, true, alias); > + int dom, busnr, devfn; > + PCIDevice *pci_dev; > + unsigned slot; > + PCIBus *bus; > + > + if (!nd) { > + return false; > + } > + > + if (!devaddr || pci_parse_devaddr(devaddr, &dom, &busnr, &slot, NULL) < 0) { > + error_report("Invalid PCI device address %s for device %s", > + devaddr, model); > + exit(1); > + } > + > + if (dom != 0) { > + error_report("No support for non-zero PCI domains"); > + exit(1); > + } > + > + devfn = PCI_DEVFN(slot, 0); > + > + bus = pci_find_bus_nr(rootbus, busnr); > + if (!bus) { > + error_report("Invalid PCI device address %s for device %s", > + devaddr, model); > + exit(1); > + } > + > + pci_dev = pci_new(devfn, model); > + qdev_set_nic_properties(&pci_dev->qdev, nd); > + pci_realize_and_unref(pci_dev, bus, &error_fatal); Could these functions be used with hotplug devices? If so we should propagate the error, not make it fatal. > + return true; > +} > + > PCIDevice *pci_vga_init(PCIBus *bus) > { > vga_interface_created = true; > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h > index ea5aff118b..684d49bdcd 100644 > --- a/include/hw/pci/pci.h > +++ b/include/hw/pci/pci.h > @@ -317,7 +317,9 @@ void pci_device_reset(PCIDevice *dev); > PCIDevice *pci_nic_init_nofail(NICInfo *nd, PCIBus *rootbus, > const char *default_model, > const char *default_devaddr); > - > +void pci_init_nic_devices(PCIBus *bus, const char *default_model); > +bool pci_init_nic_in_slot(PCIBus *rootbus, const char *default_model, > + const char *alias, const char *devaddr); > PCIDevice *pci_vga_init(PCIBus *bus); > > static inline PCIBus *pci_get_bus(const PCIDevice *dev)
On Fri, 2023-11-10 at 08:31 +0100, Philippe Mathieu-Daudé wrote: > > > + pci_dev = pci_new(devfn, model); > > + qdev_set_nic_properties(&pci_dev->qdev, nd); > > + pci_realize_and_unref(pci_dev, bus, &error_fatal); > > Could these functions be used with hotplug devices? > > If so we should propagate the error, not make it fatal. Hm, not sure. Mostly, I'm trying not to change existing behaviour with this series (except in carefully noted cases where the minutiæ of the existing behaviour appear to be both unintended and unimportant, and it would be unnecessarily complex to preserve the gratuitous differences between the way that platforms have open-coded things). I don't think it makes much sense *even* to use the new qemu_configure_nic_device() with hotplug devices. The user might create a *netdev* at startup, for later hotplug devices to use. But they wouldn't use `-nic` for that, and any devices explicitly added through hotplug will have an explicitly specified netdev, won't they? I don't think we want to change that model and allow hotplug devices to magically get config from -nic on the command line... do we? We even have a warning for the case where NIC configurations are provided with -nic but aren't consumed by the time the platform is instantiated (although that doesn't *prevent* them from being used later by hotplug). But that's answering your question which was about "these functions". For *this* particular function, pci_init_nic_in_slot(), it makes even less sense to consider hotplug. This is for platforms to handle the special cases, like "this board had an RTL8139 in slot 3", and make that NIC appear in the appropriate slot. It's done as a special case before processing the rest of the NICs which land in dynamically assigned slots — which may even be on a *different* bus, in the case of sun4u.
diff --git a/hw/pci/pci.c b/hw/pci/pci.c index 885c04b6f5..5703266c0b 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -1925,6 +1925,51 @@ PCIDevice *pci_nic_init_nofail(NICInfo *nd, PCIBus *rootbus, return pci_dev; } +void pci_init_nic_devices(PCIBus *bus, const char *default_model) +{ + qemu_create_nic_bus_devices(&bus->qbus, TYPE_PCI_DEVICE, default_model, + "virtio", "virtio-net-pci"); +} + +bool pci_init_nic_in_slot(PCIBus *rootbus, const char *model, + const char *alias, const char *devaddr) +{ + NICInfo *nd = qemu_find_nic_info(model, true, alias); + int dom, busnr, devfn; + PCIDevice *pci_dev; + unsigned slot; + PCIBus *bus; + + if (!nd) { + return false; + } + + if (!devaddr || pci_parse_devaddr(devaddr, &dom, &busnr, &slot, NULL) < 0) { + error_report("Invalid PCI device address %s for device %s", + devaddr, model); + exit(1); + } + + if (dom != 0) { + error_report("No support for non-zero PCI domains"); + exit(1); + } + + devfn = PCI_DEVFN(slot, 0); + + bus = pci_find_bus_nr(rootbus, busnr); + if (!bus) { + error_report("Invalid PCI device address %s for device %s", + devaddr, model); + exit(1); + } + + pci_dev = pci_new(devfn, model); + qdev_set_nic_properties(&pci_dev->qdev, nd); + pci_realize_and_unref(pci_dev, bus, &error_fatal); + return true; +} + PCIDevice *pci_vga_init(PCIBus *bus) { vga_interface_created = true; diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h index ea5aff118b..684d49bdcd 100644 --- a/include/hw/pci/pci.h +++ b/include/hw/pci/pci.h @@ -317,7 +317,9 @@ void pci_device_reset(PCIDevice *dev); PCIDevice *pci_nic_init_nofail(NICInfo *nd, PCIBus *rootbus, const char *default_model, const char *default_devaddr); - +void pci_init_nic_devices(PCIBus *bus, const char *default_model); +bool pci_init_nic_in_slot(PCIBus *rootbus, const char *default_model, + const char *alias, const char *devaddr); PCIDevice *pci_vga_init(PCIBus *bus); static inline PCIBus *pci_get_bus(const PCIDevice *dev)