1. Patchew
  2. QEMU
  3. [PATCH v3 00/46] Rework matching of network devices to -nic options
  4. View patch

[PATCH v3 05/46] hw/i386/pc: use qemu_get_nic_info() and pci_init_nic_devices()

David Woodhouse posted 46 patches 10 months, 3 weeks ago
Maintainers: Richard Henderson <richard.henderson@linaro.org>, Beniamino Galvani <b.galvani@gmail.com>, Peter Maydell <peter.maydell@linaro.org>, Strahinja Jankovic <strahinja.p.jankovic@gmail.com>, Niek Linnenbank <nieklinnenbank@gmail.com>, "Cédric Le Goater" <clg@kaod.org>, Andrew Jeffery <andrew@codeconstruct.com.au>, Joel Stanley <joel@jms.id.au>, Igor Mitsyanko <i.mitsyanko@gmail.com>, Jean-Christophe Dubois <jcd@tribudubois.net>, Andrey Smirnov <andrew.smirnov@gmail.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Rob Herring <robh@kernel.org>, Subbaraya Sundeep <sundeep.lkml@gmail.com>, Jan Kiszka <jan.kiszka@web.de>, Tyrone Ting <kfting@nuvoton.com>, Hao Wu <wuhaotsh@google.com>, Radoslaw Biernacki <rad@semihalf.com>, Leif Lindholm <quic_llindhol@quicinc.com>, Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>, "Edgar E. Iglesias" <edgar.iglesias@gmail.com>, Alistair Francis <alistair@alistair23.me>, Helge Deller <deller@gmx.de>, "Michael S. Tsirkin" <mst@redhat.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Paolo Bonzini <pbonzini@redhat.com>, Eduardo Habkost <eduardo@habkost.net>, Song Gao <gaosong@loongson.cn>, Thomas Huth <huth@tuxfamily.org>, Laurent Vivier <laurent@vivier.eu>, Huacai Chen <chenhuacai@kernel.org>, Jiaxun Yang <jiaxun.yang@flygoat.com>, "Hervé Poussineau" <hpoussin@reactos.org>, Aleksandar Rikalo <aleksandar.rikalo@syrmia.com>, Aurelien Jarno <aurelien@aurel32.net>, Jason Wang <jasowang@redhat.com>, Jia Liu <proljc@gmail.com>, Stafford Horne <shorne@gmail.com>, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>, Nicholas Piggin <npiggin@gmail.com>, Daniel Henrique Barboza <danielhb413@gmail.com>, David Gibson <david@gibson.dropbear.id.au>, Harsh Prateek Bora <harshpb@linux.ibm.com>, Bin Meng <bin.meng@windriver.com>, Palmer Dabbelt <palmer@dabbelt.com>, Weiwei Li <liwei1518@gmail.com>, Liu Zhiwei <zhiwei_liu@linux.alibaba.com>, Halil Pasic <pasic@linux.ibm.com>, Christian Borntraeger <borntraeger@linux.ibm.com>, Eric Farman <farman@linux.ibm.com>, David Hildenbrand <david@redhat.com>, Ilya Leoshkevich <iii@linux.ibm.com>, Yoshinori Sato <ysato@users.sourceforge.jp>, Magnus Damm <magnus.damm@gmail.com>, Artyom Tarasenko <atar4qemu@gmail.com>, Stefano Stabellini <sstabellini@kernel.org>, Anthony Perard <anthony.perard@citrix.com>, Paul Durrant <paul@xen.org>, Max Filippov <jcmvbkbc@gmail.com>
There is a newer version of this series
[PATCH v3 05/46] hw/i386/pc: use qemu_get_nic_info() and pci_init_nic_devices()
Posted by David Woodhouse 10 months, 3 weeks ago 
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;
+    }
     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
Re: [PATCH v3 05/46] hw/i386/pc: use qemu_get_nic_info() and pci_init_nic_devices()
Posted by Thomas Huth 10 months ago 
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++;
Re: [PATCH v3 05/46] hw/i386/pc: use qemu_get_nic_info() and pci_init_nic_devices()
Posted by David Woodhouse 10 months ago 
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.
Re: [PATCH v3 05/46] hw/i386/pc: use qemu_get_nic_info() and pci_init_nic_devices()
Posted by Thomas Huth 10 months ago 
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
Re: [PATCH v3 05/46] hw/i386/pc: use qemu_get_nic_info() and pci_init_nic_devices()
Posted by David Woodhouse 10 months ago 
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.
Re: [PATCH v3 05/46] hw/i386/pc: use qemu_get_nic_info() and pci_init_nic_devices()
Posted by Thomas Huth 10 months ago 
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
Re: [PATCH v3 05/46] hw/i386/pc: use qemu_get_nic_info() and pci_init_nic_devices()
Posted by David Woodhouse 10 months ago 
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).

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.