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);
+    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)
-- 
2.41.0

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.