| Message ID | 20231106195352.301038-6-dwmw2@infradead.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Rework matching of network devices to -nic options | expand |
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.
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);