From: David Woodhouse <dwmw@amazon.co.uk>

Eliminate direct access to nd_table[] and nb_nics by processing the the
Xen and ISA NICs first and then calling pci_init_nic_devices() for the
rest.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Reviewed-by: Paul Durrant <paul@xen.org>
---
 hw/i386/pc.c                | 26 ++++++++++++++++----------
 include/hw/net/ne2000-isa.h |  2 --
 2 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index c2bc3fa52d..4078d2d231 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -652,8 +652,11 @@ static void pc_init_ne2k_isa(ISABus *bus, NICInfo *nd)
 {
     static int nb_ne2k = 0;
 
-    if (nb_ne2k == NE2000_NB_MAX)
+    if (nb_ne2k == NE2000_NB_MAX) {
+        error_setg(&error_fatal,
+                   "maximum number of ISA NE2000 devices exceeded");
         return;
+    }
     isa_ne2000_init(bus, ne2000_io[nb_ne2k],
                     ne2000_irq[nb_ne2k], nd);
     nb_ne2k++;
@@ -1291,23 +1294,26 @@ void pc_nic_init(PCMachineClass *pcmc, ISABus *isa_bus, PCIBus *pci_bus,
                  BusState *xen_bus)
 {
     MachineClass *mc = MACHINE_CLASS(pcmc);
-    int i;
+    bool default_is_ne2k = g_str_equal(mc->default_nic, TYPE_ISA_NE2000);
+    NICInfo *nd;
 
     rom_set_order_override(FW_CFG_ORDER_OVERRIDE_NIC);
-    for (i = 0; i < nb_nics; i++) {
-        NICInfo *nd = &nd_table[i];
-        const char *model = nd->model ? nd->model : mc->default_nic;
 
-        if (xen_bus && (!nd->model || g_str_equal(model, "xen-net-device"))) {
+    if (xen_bus) {
+        while (nc = qemu_find_nic_info("xen-net-device", true, NULL)) {
             DeviceState *dev = qdev_new("xen-net-device");
             qdev_set_nic_properties(dev, nd);
             qdev_realize_and_unref(dev, xen_bus, &error_fatal);
-        } else if (g_str_equal(model, "ne2k_isa")) {
-            pc_init_ne2k_isa(isa_bus, nd);
-        } else {
-            pci_nic_init_nofail(nd, pci_bus, model, NULL);
         }
     }
+
+    while ((nd = qemu_find_nic_info(TYPE_ISA_NE2000, default_is_ne2k, NULL))) {
+        pc_init_ne2k_isa(isa_bus, nd);
+    }
+
+    /* Anything remaining should be a PCI NIC */
+    pci_init_nic_devices(pci_bus, mc->default_nic);
+
     rom_reset_order_override();
 }
 
diff --git a/include/hw/net/ne2000-isa.h b/include/hw/net/ne2000-isa.h
index af59ee0b02..73bae10ad1 100644
--- a/include/hw/net/ne2000-isa.h
+++ b/include/hw/net/ne2000-isa.h
@@ -22,8 +22,6 @@ static inline ISADevice *isa_ne2000_init(ISABus *bus, int base, int irq,
 {
     ISADevice *d;
 
-    qemu_check_nic_model(nd, "ne2k_isa");
-
     d = isa_try_new(TYPE_ISA_NE2000);
     if (d) {
         DeviceState *dev = DEVICE(d);
-- 
2.41.0

Hi David,

+Markus/Bernhard

On 6/11/23 20:49, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> Eliminate direct access to nd_table[] and nb_nics by processing the the
> Xen and ISA NICs first and then calling pci_init_nic_devices() for the
> rest.
> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> Reviewed-by: Paul Durrant <paul@xen.org>
> ---
>   hw/i386/pc.c                | 26 ++++++++++++++++----------
>   include/hw/net/ne2000-isa.h |  2 --
>   2 files changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index c2bc3fa52d..4078d2d231 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -652,8 +652,11 @@ static void pc_init_ne2k_isa(ISABus *bus, NICInfo *nd)
>   {
>       static int nb_ne2k = 0;
>   
> -    if (nb_ne2k == NE2000_NB_MAX)
> +    if (nb_ne2k == NE2000_NB_MAX) {
> +        error_setg(&error_fatal,

In the context of dynamically created machines I'd rather have
this function,

> +                   "maximum number of ISA NE2000 devices exceeded");
>           return;
> +    }
>       isa_ne2000_init(bus, ne2000_io[nb_ne2k],
>                       ne2000_irq[nb_ne2k], nd);
>       nb_ne2k++;
> @@ -1291,23 +1294,26 @@ void pc_nic_init(PCMachineClass *pcmc, ISABus *isa_bus, PCIBus *pci_bus,
>                    BusState *xen_bus)
>   {
>       MachineClass *mc = MACHINE_CLASS(pcmc);
> -    int i;
> +    bool default_is_ne2k = g_str_equal(mc->default_nic, TYPE_ISA_NE2000);
> +    NICInfo *nd;
>   
>       rom_set_order_override(FW_CFG_ORDER_OVERRIDE_NIC);
> -    for (i = 0; i < nb_nics; i++) {
> -        NICInfo *nd = &nd_table[i];
> -        const char *model = nd->model ? nd->model : mc->default_nic;
>   
> -        if (xen_bus && (!nd->model || g_str_equal(model, "xen-net-device"))) {
> +    if (xen_bus) {
> +        while (nc = qemu_find_nic_info("xen-net-device", true, NULL)) {
>               DeviceState *dev = qdev_new("xen-net-device");
>               qdev_set_nic_properties(dev, nd);
>               qdev_realize_and_unref(dev, xen_bus, &error_fatal);

and this one non-fatal (primarily for API example). But this is pending
on a discussion on another thread, see:
https://lore.kernel.org/qemu-devel/c1322f3b-2ae2-4ca7-9a76-a2a434dc8315@linaro.org/
so no changed requested so far.

> -        } else if (g_str_equal(model, "ne2k_isa")) {
> -            pc_init_ne2k_isa(isa_bus, nd);
> -        } else {
> -            pci_nic_init_nofail(nd, pci_bus, model, NULL);
>           }
>       }

Regards,

Phil.

On Fri, 2023-11-10 at 08:40 +0100, Philippe Mathieu-Daudé wrote:
> Hi David,
> 
> +Markus/Bernhard
> 
> On 6/11/23 20:49, David Woodhouse wrote:
> > From: David Woodhouse <dwmw@amazon.co.uk>
> > 
> > Eliminate direct access to nd_table[] and nb_nics by processing the the
> > Xen and ISA NICs first and then calling pci_init_nic_devices() for the
> > rest.
> > 
> > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> > Reviewed-by: Paul Durrant <paul@xen.org>
> > ---
> >   hw/i386/pc.c                | 26 ++++++++++++++++----------
> >   include/hw/net/ne2000-isa.h |  2 --
> >   2 files changed, 16 insertions(+), 12 deletions(-)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index c2bc3fa52d..4078d2d231 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -652,8 +652,11 @@ static void pc_init_ne2k_isa(ISABus *bus, NICInfo *nd)
> >   {
> >       static int nb_ne2k = 0;
> >   
> > -    if (nb_ne2k == NE2000_NB_MAX)
> > +    if (nb_ne2k == NE2000_NB_MAX) {
> > +        error_setg(&error_fatal,
> 
> In the context of dynamically created machines I'd rather have
> this function,
> 
> > +                   "maximum number of ISA NE2000 devices exceeded");
> >           return;
> > +    }
> >       isa_ne2000_init(bus, ne2000_io[nb_ne2k],
> >                       ne2000_irq[nb_ne2k], nd);
> >       nb_ne2k++;
> > @@ -1291,23 +1294,26 @@ void pc_nic_init(PCMachineClass *pcmc, ISABus *isa_bus, PCIBus *pci_bus,
> >                    BusState *xen_bus)
> >   {
> >       MachineClass *mc = MACHINE_CLASS(pcmc);
> > -    int i;
> > +    bool default_is_ne2k = g_str_equal(mc->default_nic, TYPE_ISA_NE2000);
> > +    NICInfo *nd;
> >   
> >       rom_set_order_override(FW_CFG_ORDER_OVERRIDE_NIC);
> > -    for (i = 0; i < nb_nics; i++) {
> > -        NICInfo *nd = &nd_table[i];
> > -        const char *model = nd->model ? nd->model : mc->default_nic;
> >   
> > -        if (xen_bus && (!nd->model || g_str_equal(model, "xen-net-device"))) {
> > +    if (xen_bus) {
> > +        while (nc = qemu_find_nic_info("xen-net-device", true, NULL)) {
> >               DeviceState *dev = qdev_new("xen-net-device");
> >               qdev_set_nic_properties(dev, nd);
> >               qdev_realize_and_unref(dev, xen_bus, &error_fatal);
> 
> and this one non-fatal (primarily for API example). But this is pending
> on a discussion on another thread, see:
> https://lore.kernel.org/qemu-devel/c1322f3b-2ae2-4ca7-9a76-a2a434dc8315@linaro.org/
> so no changed requested so far.

Thanks for the reference.

I'm happy to make pc_init_ne2k_isa() and even pc_nic_init() take an
'Error **' argument and use that instead of &error_fatal... and for the
*caller* to pass &error_fatal for now until/unless that discussion is
resolved? Not sure it helps much?

Then again... I do not favour the "my caller cannot *currently*
handle/propagate an error therefore I won't bother to return one
coherently" approach. That leads to someone else thinking "my callee
does not return an error coherently therefore I won't bother to handle
it", and nothing ever gets fixed.

I'll go fix it to take an errp argument.