Re: [PATCH v3 05/46] hw/i386/pc: use qemu_get_nic_info() and pci_init_nic_devices()
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). smime.p7s Description: S/MIME cryptographic signature
Re: [PATCH v3 05/46] hw/i386/pc: use qemu_get_nic_info() and pci_init_nic_devices()
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 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 Reviewed-by: Paul Durrant --- 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-4afdc461a...@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
Re: [PATCH v3 05/46] hw/i386/pc: use qemu_get_nic_info() and pci_init_nic_devices()
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 > > > > > > > > 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 > > > > Reviewed-by: Paul Durrant > > > > --- > > > > 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-4afdc461a...@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. smime.p7s Description: S/MIME cryptographic signature
Re: [PATCH v3 05/46] hw/i386/pc: use qemu_get_nic_info() and pci_init_nic_devices()
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 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 Reviewed-by: Paul Durrant --- 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-4afdc461a...@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
Re: [PATCH v3 05/46] hw/i386/pc: use qemu_get_nic_info() and pci_init_nic_devices()
On Fri, 2024-01-26 at 11:43 +0100, Thomas Huth wrote: > On 08/01/2024 21.26, David Woodhouse wrote: > > From: David Woodhouse > > > > 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 > > Reviewed-by: Paul Durrant > > --- > > 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-4afdc461a...@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. smime.p7s Description: S/MIME cryptographic signature
Re: [PATCH v3 05/46] hw/i386/pc: use qemu_get_nic_info() and pci_init_nic_devices()
On 08/01/2024 21.26, David Woodhouse wrote: From: David Woodhouse 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 Reviewed-by: Paul Durrant --- 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++;
[PATCH v3 05/46] hw/i386/pc: use qemu_get_nic_info() and pci_init_nic_devices()
From: David Woodhouse 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 Reviewed-by: Paul Durrant --- 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; +} 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); -- 2.41.0