| Message ID | 20240108204909.564514-6-dwmw2@infradead.org |
|---|---|
| State | New |
| Headers | show |
| Series | Rework matching of network devices to -nic options | expand |
On 08/01/2024 21.26, 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 496498df3a..d80c536d88 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -658,8 +658,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; > + } error_setg(&error_fatal, ...) quits QEMU, so the "return;" does not make much sense anymore. Now, according to include/qapi/error.h : * Please don't error_setg(&error_fatal, ...), use error_report() and * exit(), because that's more obvious. So I'd suggest to do that instead. Thanks, Thomas > isa_ne2000_init(bus, ne2000_io[nb_ne2k], > ne2000_irq[nb_ne2k], nd); > nb_ne2k++;
On Fri, 2024-01-26 at 11:43 +0100, Thomas Huth wrote: > On 08/01/2024 21.26, 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 496498df3a..d80c536d88 100644 > > --- a/hw/i386/pc.c > > +++ b/hw/i386/pc.c > > @@ -658,8 +658,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; > > + } > > error_setg(&error_fatal, ...) quits QEMU, so the "return;" does not make > much sense anymore. > Now, according to include/qapi/error.h : > > * Please don't error_setg(&error_fatal, ...), use error_report() and > * exit(), because that's more obvious. > > So I'd suggest to do that instead. It's going slightly in the opposite direction to what's requested in https://lore.kernel.org/qemu-devel/34e2c0c6-4e04-486a-8e1f-4afdc461a5d4@linaro.org/ I was thinking that a future patch would let the &error_fatal be an Error** passed in by the caller, and not actually hard-coded to be fatal at all. But sure, unless Philippe objects I'm happy to do it as you show above.
On 26/01/2024 12.13, David Woodhouse wrote: > On Fri, 2024-01-26 at 11:43 +0100, Thomas Huth wrote: >> On 08/01/2024 21.26, 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 496498df3a..d80c536d88 100644 >>> --- a/hw/i386/pc.c >>> +++ b/hw/i386/pc.c >>> @@ -658,8 +658,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; >>> + } >> >> error_setg(&error_fatal, ...) quits QEMU, so the "return;" does not make >> much sense anymore. >> Now, according to include/qapi/error.h : >> >> * Please don't error_setg(&error_fatal, ...), use error_report() and >> * exit(), because that's more obvious. >> >> So I'd suggest to do that instead. > > It's going slightly in the opposite direction to what's requested in > https://lore.kernel.org/qemu-devel/34e2c0c6-4e04-486a-8e1f-4afdc461a5d4@linaro.org/ > > I was thinking that a future patch would let the &error_fatal be an > Error** passed in by the caller, and not actually hard-coded to be > fatal at all. > > But sure, unless Philippe objects I'm happy to do it as you show above. Now that you mention it, I'd also prefer having an Error** parameter to the function instead, that's certainly cleaner. So if you don't mind, please follow Philippe's suggestion instead! Thanks, Thomas
On Fri, 2024-01-26 at 12:20 +0100, Thomas Huth wrote: > On 26/01/2024 12.13, David Woodhouse wrote: > > On Fri, 2024-01-26 at 11:43 +0100, Thomas Huth wrote: > > > On 08/01/2024 21.26, 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 496498df3a..d80c536d88 100644 > > > > --- a/hw/i386/pc.c > > > > +++ b/hw/i386/pc.c > > > > @@ -658,8 +658,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; > > > > + } > > > > > > error_setg(&error_fatal, ...) quits QEMU, so the "return;" does not make > > > much sense anymore. > > > Now, according to include/qapi/error.h : > > > > > > * Please don't error_setg(&error_fatal, ...), use error_report() and > > > * exit(), because that's more obvious. > > > > > > So I'd suggest to do that instead. > > > > It's going slightly in the opposite direction to what's requested in > > https://lore.kernel.org/qemu-devel/34e2c0c6-4e04-486a-8e1f-4afdc461a5d4@linaro.org/ > > > > I was thinking that a future patch would let the &error_fatal be an > > Error** passed in by the caller, and not actually hard-coded to be > > fatal at all. > > > > But sure, unless Philippe objects I'm happy to do it as you show above. > > Now that you mention it, I'd also prefer having an Error** parameter to the > function instead, that's certainly cleaner. So if you don't mind, please > follow Philippe's suggestion instead! Right. There's a whole bunch of functions to untangle, that take an Error** but don't return success/failure independently as they should. Or don't even take the Error**. Rather than trying to fix that as part of this series, this was my compromise — making it easy to switch that explicit &error_fatal out for a function parameter, but not trying to shave that part of the yak myself just yet.
On 26/01/2024 12.25, David Woodhouse wrote: > On Fri, 2024-01-26 at 12:20 +0100, Thomas Huth wrote: >> On 26/01/2024 12.13, David Woodhouse wrote: >>> On Fri, 2024-01-26 at 11:43 +0100, Thomas Huth wrote: >>>> On 08/01/2024 21.26, 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 496498df3a..d80c536d88 100644 >>>>> --- a/hw/i386/pc.c >>>>> +++ b/hw/i386/pc.c >>>>> @@ -658,8 +658,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; >>>>> + } >>>> >>>> error_setg(&error_fatal, ...) quits QEMU, so the "return;" does not make >>>> much sense anymore. >>>> Now, according to include/qapi/error.h : >>>> >>>> * Please don't error_setg(&error_fatal, ...), use error_report() and >>>> * exit(), because that's more obvious. >>>> >>>> So I'd suggest to do that instead. >>> >>> It's going slightly in the opposite direction to what's requested in >>> https://lore.kernel.org/qemu-devel/34e2c0c6-4e04-486a-8e1f-4afdc461a5d4@linaro.org/ >>> >>> I was thinking that a future patch would let the &error_fatal be an >>> Error** passed in by the caller, and not actually hard-coded to be >>> fatal at all. >>> >>> But sure, unless Philippe objects I'm happy to do it as you show above. >> >> Now that you mention it, I'd also prefer having an Error** parameter to the >> function instead, that's certainly cleaner. So if you don't mind, please >> follow Philippe's suggestion instead! > > Right. There's a whole bunch of functions to untangle, that take an > Error** but don't return success/failure independently as they should. > Or don't even take the Error**. > > Rather than trying to fix that as part of this series, this was my > compromise — making it easy to switch that explicit &error_fatal out > for a function parameter, but not trying to shave that part of the yak > myself just yet. I think the nicest compromise is to add the "Error **errp" to the pc_init_ne2k_isa() and change the caller to pass in &error_fatal there ... further clean-up (passing the error even up further in the stack) is out of scope of this series, indeed. Thomas
On Fri, 2024-01-26 at 14:45 +0100, Thomas Huth wrote: > I think the nicest compromise is to add the "Error **errp" to the > pc_init_ne2k_isa() and change the caller to pass in &error_fatal there ... > further clean-up (passing the error even up further in the stack) is out of > scope of this series, indeed. Yep. That's what I've done in my tree at https://git.infradead.org/users/dwmw2/qemu.git/commitdiff/ec5be4aaf6d2 I've fixed up the LASI NIC support, and I'm now looking into what you said about nd->model (I don't think it matters, but I'm checking).
diff --git a/hw/i386/pc.c b/hw/i386/pc.c index 496498df3a..d80c536d88 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -658,8 +658,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++; @@ -1297,23 +1300,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);