[Qemu-devel] [RfC PATCH v2 0/5] qdev-ify network cards

[Qemu-devel] [RfC PATCH v2 0/5] qdev-ify network cards

[Gerd Hoffmann]

  Hi,

Short RfC patch series to get the discussion rolling.  We really need to
get the nic drivers qdev-ified properly, so qemu stops segfaulting on
'-device $any_nic_here'.

New in v2:
  * Fix some style nits and minor issues from review.
  * Don't hook up the nic to vlan 0 by default, in preparation to
    other ways coming to hook up nics.
  * Add pxe property for option rom loading.

To keep things small simple I've started with the ne2k_isa.  All the PCI
nics share the initialization path and thus I can't simply pick a single
one as example and convert it.

The network card can now be created using ...

  -device ne2k_isa,mac=00:11:22:33:44:55,vlan=1,irq=3,id=foo

'info qtree' shows ...

	[ ... ]
        bus: isa.0
          type ISA
          dev: ne2k_isa, id "foo"
            dev-prop: iobase = 0x300
            dev-prop: irq = 3
            dev-prop: vlan = 1
            dev-prop: mac = 00:11:22:33:44:55
            dev-prop: pxe = 0
	[ ... ]

'info network' shows:

	[ ... ]
	VLAN 1 devices:
	  foo: model=ne2k_isa,macaddr=00:11:22:33:44:55

With a vlan specified the nic initialization code calls
qemu_new_vlan_client() with the vlan specified using the vlan property.
Likewise the device cleanup code should call qemu_del_vlan_client.  You
don't see that in the patches though as ISA devices are not
hot-pluggable ;)

struct NICInfo is not needed at all here.  I hope we can kill it long-term.

This patch series is also available here:
  http://git.et.redhat.com/?p=qemu-kraxel.git;a=shortlog;h=refs/heads/nic.v2

cheers,
  Gerd

[Qemu-devel] [RFC PATCH v2 1/5] net: macaddr tweaks.

[Gerd Hoffmann]

Add new type for mac addresses.

Add function which sets the qemu default mac address if it finds the mac
address uninitialized (i.e. all zeros).

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 net.c |   15 +++++++++++++++
 net.h |    2 ++
 2 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/net.c b/net.c
index 2e4dd58..4dc910b 100644
--- a/net.c
+++ b/net.c
@@ -281,6 +281,21 @@ void qemu_format_nic_info_str(VLANClientState *vc, uint8_t macaddr[6])
              macaddr[3], macaddr[4], macaddr[5]);
 }
 
+void qemu_macaddr_default_if_unset(macaddr_t macaddr)
+{
+    static int index = 0;
+    static const macaddr_t zero = { 0,0,0,0,0,0 };
+
+    if (memcmp(macaddr, zero, sizeof(zero)) != 0)
+        return;
+    macaddr[0] = 0x52;
+    macaddr[1] = 0x54;
+    macaddr[2] = 0x00;
+    macaddr[3] = 0x12;
+    macaddr[4] = 0x34;
+    macaddr[5] = 0x56 + index++;
+}
+
 static char *assign_name(VLANClientState *vc1, const char *model)
 {
     VLANState *vlan;
diff --git a/net.h b/net.h
index 2b0ed9b..7aefc51 100644
--- a/net.h
+++ b/net.h
@@ -8,6 +8,7 @@
 
 /* VLANs support */
 
+typedef uint8_t macaddr_t[6];
 typedef struct VLANClientState VLANClientState;
 
 typedef int (NetCanReceive)(VLANClientState *);
@@ -76,6 +77,7 @@ ssize_t qemu_send_packet_async(VLANClientState *vc, const uint8_t *buf,
 void qemu_purge_queued_packets(VLANClientState *vc);
 void qemu_flush_queued_packets(VLANClientState *vc);
 void qemu_format_nic_info_str(VLANClientState *vc, uint8_t macaddr[6]);
+void qemu_macaddr_default_if_unset(macaddr_t macaddr);
 int qemu_show_nic_models(const char *arg, const char *const *models);
 void qemu_check_nic_model(NICInfo *nd, const char *model);
 int qemu_find_nic_model(NICInfo *nd, const char * const *models,
-- 
1.6.2.5

Re: [Qemu-devel] [RFC PATCH v2 1/5] net: macaddr tweaks.

[Anthony Liguori]

Gerd Hoffmann wrote:
> Add new type for mac addresses.
>
> Add function which sets the qemu default mac address if it finds the mac
> address uninitialized (i.e. all zeros).
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  net.c |   15 +++++++++++++++
>  net.h |    2 ++
>  2 files changed, 17 insertions(+), 0 deletions(-)
>
> diff --git a/net.c b/net.c
> index 2e4dd58..4dc910b 100644
> --- a/net.c
> +++ b/net.c
> @@ -281,6 +281,21 @@ void qemu_format_nic_info_str(VLANClientState *vc, uint8_t macaddr[6])
>               macaddr[3], macaddr[4], macaddr[5]);
>  }
>  
> +void qemu_macaddr_default_if_unset(macaddr_t macaddr)
> +{
> +    static int index = 0;
> +    static const macaddr_t zero = { 0,0,0,0,0,0 };
> +
> +    if (memcmp(macaddr, zero, sizeof(zero)) != 0)
> +        return;
> +    macaddr[0] = 0x52;
> +    macaddr[1] = 0x54;
> +    macaddr[2] = 0x00;
> +    macaddr[3] = 0x12;
> +    macaddr[4] = 0x34;
> +    macaddr[5] = 0x56 + index++;
> +}
> +
>  static char *assign_name(VLANClientState *vc1, const char *model)
>  {
>      VLANState *vlan;
> diff --git a/net.h b/net.h
> index 2b0ed9b..7aefc51 100644
> --- a/net.h
> +++ b/net.h
> @@ -8,6 +8,7 @@
>  
>  /* VLANs support */
>  
> +typedef uint8_t macaddr_t[6];
>   

Let's make it a stronger type and avoid introducing more _t types.  I'd 
suggest:

typedef struct MacAddress
{
    uint8_t addr[6];
} MacAddress

>  typedef struct VLANClientState VLANClientState;
>  
>  typedef int (NetCanReceive)(VLANClientState *);
> @@ -76,6 +77,7 @@ ssize_t qemu_send_packet_async(VLANClientState *vc, const uint8_t *buf,
>  void qemu_purge_queued_packets(VLANClientState *vc);
>  void qemu_flush_queued_packets(VLANClientState *vc);
>  void qemu_format_nic_info_str(VLANClientState *vc, uint8_t macaddr[6]);
> +void qemu_macaddr_default_if_unset(macaddr_t macaddr);
>   

Couldn't we set the default based on a qdev default property?  A #define 
could be used to ensure there was a global default.

Regards,

Anthony Liguori

Re: [Qemu-devel] [RFC PATCH v2 1/5] net: macaddr tweaks.

[Gerd Hoffmann]

>> +typedef uint8_t macaddr_t[6];
>
> Let's make it a stronger type and avoid introducing more _t types. I'd
> suggest:

Oh yes, killing that _t was on the todo list anyway, forgot it.
Will do for the next respin.

> typedef struct MacAddress
> {
> uint8_t addr[6];
> } MacAddress

Why the extra struct?

cheers,
   Gerd

Re: [Qemu-devel] [RFC PATCH v2 1/5] net: macaddr tweaks.

[Anthony Liguori]

Gerd Hoffmann wrote:
>>> +typedef uint8_t macaddr_t[6];
>>
>> Let's make it a stronger type and avoid introducing more _t types. I'd
>> suggest:
>
> Oh yes, killing that _t was on the todo list anyway, forgot it.
> Will do for the next respin.
>
>> typedef struct MacAddress
>> {
>> uint8_t addr[6];
>> } MacAddress
>
> Why the extra struct?

To make the type strong so that type checking actually works.

Regards,

Anthony Liguori

[Qemu-devel] [RFC PATCH v2 2/5] qdev: mac addr property fixups

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/qdev-properties.c |   31 +++++++++++++++++++++----------
 hw/qdev.h            |    3 ++-
 2 files changed, 23 insertions(+), 11 deletions(-)

diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index 5c627fa..c231f74 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -1,4 +1,5 @@
 #include "sysemu.h"
+#include "net.h"
 #include "qdev.h"
 
 void *qdev_get_prop_ptr(DeviceState *dev, Property *prop)
@@ -274,7 +275,7 @@ PropertyInfo qdev_prop_ptr = {
  */
 static int parse_mac(DeviceState *dev, Property *prop, const char *str)
 {
-    uint8_t *mac = qdev_get_prop_ptr(dev, prop);
+    macaddr_t *mac = qdev_get_prop_ptr(dev, prop);
     int i, pos;
     char *p;
 
@@ -283,26 +284,31 @@ static int parse_mac(DeviceState *dev, Property *prop, const char *str)
             return -1;
         if (!qemu_isxdigit(str[pos+1]))
             return -1;
-        if (i == 5 && str[pos+2] != '\0')
-            return -1;
-        if (str[pos+2] != ':' && str[pos+2] != '-')
-            return -1;
-        mac[i] = strtol(str+pos, &p, 16);
+        if (i == 5) {
+            if (str[pos+2] != '\0')
+                return -1;
+        } else {
+            if (str[pos+2] != ':' && str[pos+2] != '-')
+                return -1;
+        }
+        (*mac)[i] = strtol(str+pos, &p, 16);
     }
     return 0;
 }
 
 static int print_mac(DeviceState *dev, Property *prop, char *dest, size_t len)
 {
-    uint8_t *mac = qdev_get_prop_ptr(dev, prop);
+    macaddr_t *mac = qdev_get_prop_ptr(dev, prop);
+
     return snprintf(dest, len, "%02x:%02x:%02x:%02x:%02x:%02x",
-                    mac[0], mac[1], mac[2], mac[3], mac[4], mac[5]);
+                    (*mac)[0], (*mac)[1], (*mac)[2],
+                    (*mac)[3], (*mac)[4], (*mac)[5]);
 }
 
 PropertyInfo qdev_prop_macaddr = {
-    .name  = "mac-addr",
+    .name  = "macaddr",
     .type  = PROP_TYPE_MACADDR,
-    .size  = 6,
+    .size  = sizeof(macaddr_t),
     .parse = parse_mac,
     .print = print_mac,
 };
@@ -454,6 +460,11 @@ void qdev_prop_set_chr(DeviceState *dev, const char *name, CharDriverState *valu
     qdev_prop_set(dev, name, &value, PROP_TYPE_CHR);
 }
 
+void qdev_prop_set_macaddr(DeviceState *dev, const char *name, macaddr_t value)
+{
+    qdev_prop_set(dev, name, value, PROP_TYPE_MACADDR);
+}
+
 void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value)
 {
     qdev_prop_set(dev, name, &value, PROP_TYPE_PTR);
diff --git a/hw/qdev.h b/hw/qdev.h
index 893ae92..9c66501 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -229,7 +229,7 @@ extern PropertyInfo qdev_prop_pci_devfn;
 #define DEFINE_PROP_DRIVE(_n, _s, _f)             \
     DEFINE_PROP(_n, _s, _f, qdev_prop_drive, DriveInfo*)
 #define DEFINE_PROP_MACADDR(_n, _s, _f)         \
-    DEFINE_PROP(_n, _s, _f, qdev_prop_macaddr, uint8_t[6])
+    DEFINE_PROP(_n, _s, _f, qdev_prop_macaddr, macaddr_t)
 
 #define DEFINE_PROP_END_OF_LIST()               \
     {}
@@ -245,6 +245,7 @@ void qdev_prop_set_int32(DeviceState *dev, const char *name, int32_t value);
 void qdev_prop_set_uint64(DeviceState *dev, const char *name, uint64_t value);
 void qdev_prop_set_chr(DeviceState *dev, const char *name, CharDriverState *value);
 void qdev_prop_set_drive(DeviceState *dev, const char *name, DriveInfo *value);
+void qdev_prop_set_macaddr(DeviceState *dev, const char *name, uint8_t *value);
 /* FIXME: Remove opaque pointer properties.  */
 void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value);
 void qdev_prop_set_defaults(DeviceState *dev, Property *props);
-- 
1.6.2.5

[Qemu-devel] [RFC PATCH v2 3/5] ne2k: work without vlan.

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/ne2000.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/hw/ne2000.c b/hw/ne2000.c
index 87f1e59..0ed6eec 100644
--- a/hw/ne2000.c
+++ b/hw/ne2000.c
@@ -322,7 +322,8 @@ void ne2000_ioport_write(void *opaque, uint32_t addr, uint32_t val)
                     index -= NE2000_PMEM_SIZE;
                 /* fail safe: check range on the transmitted length  */
                 if (index + s->tcnt <= NE2000_PMEM_END) {
-                    qemu_send_packet(s->vc, s->mem + index, s->tcnt);
+                    if (s->vc)
+                        qemu_send_packet(s->vc, s->mem + index, s->tcnt);
                 }
                 /* signal end of transfer */
                 s->tsr = ENTSR_PTX;
-- 
1.6.2.5

Re: [Qemu-devel] [RFC PATCH v2 3/5] ne2k: work without vlan.

[Anthony Liguori]

Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/ne2000.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/hw/ne2000.c b/hw/ne2000.c
> index 87f1e59..0ed6eec 100644
> --- a/hw/ne2000.c
> +++ b/hw/ne2000.c
> @@ -322,7 +322,8 @@ void ne2000_ioport_write(void *opaque, uint32_t addr, uint32_t val)
>                      index -= NE2000_PMEM_SIZE;
>                  /* fail safe: check range on the transmitted length  */
>                  if (index + s->tcnt <= NE2000_PMEM_END) {
> -                    qemu_send_packet(s->vc, s->mem + index, s->tcnt);
> +                    if (s->vc)
> +                        qemu_send_packet(s->vc, s->mem + index, s->tcnt);
>                  }
>   

Perhaps we should just push the s->vc check into qemu_send_packet since 
we want this behavior consistently.

Regards,

Anthony Liguori

Re: [Qemu-devel] [RFC PATCH v2 3/5] ne2k: work without vlan.

[Gerd Hoffmann]

On 10/07/09 15:06, Anthony Liguori wrote:
>> - qemu_send_packet(s->vc, s->mem + index, s->tcnt);
>> + if (s->vc)
>> + qemu_send_packet(s->vc, s->mem + index, s->tcnt);
>> }
>
> Perhaps we should just push the s->vc check into qemu_send_packet since
> we want this behavior consistently.

Makes sense indeed.  Will change it.

cheers,
   Gerd

[Qemu-devel] [RFC PATCH v2 4/5] ne2k_isa: qdev-ify.

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/ne2000-isa.c |   25 +++++++++++++++++--------
 hw/ne2000.h     |    3 ++-
 2 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/hw/ne2000-isa.c b/hw/ne2000-isa.c
index 54c0478..7a24088 100644
--- a/hw/ne2000-isa.c
+++ b/hw/ne2000-isa.c
@@ -65,13 +65,18 @@ static int isa_ne2000_initfn(ISADevice *dev)
 
     isa_init_irq(dev, &s->irq, isa->isairq);
 
-    qdev_get_macaddr(&dev->qdev, s->macaddr);
+    qemu_macaddr_default_if_unset(s->macaddr);
     ne2000_reset(s);
 
-    s->vc = qdev_get_vlan_client(&dev->qdev,
-                                 ne2000_can_receive, ne2000_receive, NULL,
-                                 isa_ne2000_cleanup, s);
-    qemu_format_nic_info_str(s->vc, s->macaddr);
+    if (s->vlan != -1) {
+        s->vc = qemu_new_vlan_client(qemu_find_vlan(s->vlan, 1),
+                                     dev->qdev.info->name, dev->qdev.id,
+                                     ne2000_can_receive, ne2000_receive, NULL,
+                                     isa_ne2000_cleanup, s);
+        qemu_format_nic_info_str(s->vc, s->macaddr);
+    } else {
+        qemu_error("warning: no vlan specfied, ne2k_isa is unconnected\n");
+    }
 
     register_savevm("ne2000", -1, 2, ne2000_save, ne2000_load, s);
     return 0;
@@ -84,9 +89,11 @@ void isa_ne2000_init(int base, int irq, NICInfo *nd)
     qemu_check_nic_model(nd, "ne2k_isa");
 
     dev = isa_create("ne2k_isa");
-    dev->qdev.nd = nd; /* hack alert */
     qdev_prop_set_uint32(&dev->qdev, "iobase", base);
     qdev_prop_set_uint32(&dev->qdev, "irq",    irq);
+    qdev_prop_set_macaddr(&dev->qdev, "mac",   nd->macaddr);
+    if (nd->vlan)
+        qdev_prop_set_int32(&dev->qdev, "vlan",   nd->vlan->id);
     qdev_init(&dev->qdev);
 }
 
@@ -95,8 +102,10 @@ static ISADeviceInfo ne2000_isa_info = {
     .qdev.size  = sizeof(ISANE2000State),
     .init       = isa_ne2000_initfn,
     .qdev.props = (Property[]) {
-        DEFINE_PROP_HEX32("iobase", ISANE2000State, iobase, 0x300),
-        DEFINE_PROP_UINT32("irq",   ISANE2000State, isairq, 9),
+        DEFINE_PROP_HEX32("iobase", ISANE2000State, iobase,       0x300),
+        DEFINE_PROP_UINT32("irq",   ISANE2000State, isairq,       9),
+        DEFINE_PROP_MACADDR("mac",  ISANE2000State, ne2000.macaddr),
+        DEFINE_PROP_INT32("vlan",   ISANE2000State, ne2000.vlan,  -1),
         DEFINE_PROP_END_OF_LIST(),
     },
 };
diff --git a/hw/ne2000.h b/hw/ne2000.h
index 92a2ddb..7936fb2 100644
--- a/hw/ne2000.h
+++ b/hw/ne2000.h
@@ -22,8 +22,9 @@ typedef struct NE2000State {
     uint8_t curpag;
     uint8_t mult[8]; /* multicast mask array */
     qemu_irq irq;
+    int32_t vlan;
     VLANClientState *vc;
-    uint8_t macaddr[6];
+    macaddr_t macaddr;
     uint8_t mem[NE2000_MEM_SIZE];
 } NE2000State;
 
-- 
1.6.2.5

Re: [Qemu-devel] [RFC PATCH v2 4/5] ne2k_isa: qdev-ify.

[Mark McLoughlin]

On Wed, 2009-10-07 at 14:36 +0200, Gerd Hoffmann wrote:

> -    s->vc = qdev_get_vlan_client(&dev->qdev,
> -                                 ne2000_can_receive, ne2000_receive, NULL,
> -                                 isa_ne2000_cleanup, s);
> -    qemu_format_nic_info_str(s->vc, s->macaddr);
> +    if (s->vlan != -1) {
> +        s->vc = qemu_new_vlan_client(qemu_find_vlan(s->vlan, 1),
> +                                     dev->qdev.info->name, dev->qdev.id,
> +                                     ne2000_can_receive, ne2000_receive, NULL,
> +                                     isa_ne2000_cleanup, s);
> +        qemu_format_nic_info_str(s->vc, s->macaddr);
> +    } else {
> +        qemu_error("warning: no vlan specfied, ne2k_isa is unconnected\n");
> +    }

I'll be posting a patch later which allows qemu_new_vlan_client() to
take a NULL vlan thereafter drop any packets passed to it.

I think that makes more sense, as VLANClientState should become a
NetClient structure which can represent a NIC, backend or even a vlan
itself.

Cheers,
Mark.

Re: [Qemu-devel] [RFC PATCH v2 4/5] ne2k_isa: qdev-ify.

[Gerd Hoffmann]

On 10/07/09 15:50, Mark McLoughlin wrote:
> On Wed, 2009-10-07 at 14:36 +0200, Gerd Hoffmann wrote:
>
>> -    s->vc = qdev_get_vlan_client(&dev->qdev,
>> -                                 ne2000_can_receive, ne2000_receive, NULL,
>> -                                 isa_ne2000_cleanup, s);
>> -    qemu_format_nic_info_str(s->vc, s->macaddr);
>> +    if (s->vlan != -1) {
>> +        s->vc = qemu_new_vlan_client(qemu_find_vlan(s->vlan, 1),
>> +                                     dev->qdev.info->name, dev->qdev.id,
>> +                                     ne2000_can_receive, ne2000_receive, NULL,
>> +                                     isa_ne2000_cleanup, s);
>> +        qemu_format_nic_info_str(s->vc, s->macaddr);
>> +    } else {
>> +        qemu_error("warning: no vlan specfied, ne2k_isa is unconnected\n");
>> +    }
>
> I'll be posting a patch later which allows qemu_new_vlan_client() to
> take a NULL vlan thereafter drop any packets passed to it.
>
> I think that makes more sense, as VLANClientState should become a
> NetClient structure which can represent a NIC, backend or even a vlan
> itself.

Fine with me.  I just had to pull out something out of thin air as 
placeholder for net changes which are in the pipeline but not posted 
yet.  Will adjust to whatever comes with for the next respin ;)

cheers,
   Gerd

[Qemu-devel] [RFC PATCH v2 5/5] ne2k_isa: add property for option rom loading.

[Gerd Hoffmann]

Moving option rom loading from machine init to the (nic) drivers.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/ne2000-isa.c |    9 +++++++++
 hw/ne2000.h     |    1 +
 2 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/hw/ne2000-isa.c b/hw/ne2000-isa.c
index 7a24088..1d296a0 100644
--- a/hw/ne2000-isa.c
+++ b/hw/ne2000-isa.c
@@ -27,6 +27,7 @@
 #include "qdev.h"
 #include "net.h"
 #include "ne2000.h"
+#include "loader.h"
 
 typedef struct ISANE2000State {
     ISADevice dev;
@@ -78,6 +79,13 @@ static int isa_ne2000_initfn(ISADevice *dev)
         qemu_error("warning: no vlan specfied, ne2k_isa is unconnected\n");
     }
 
+    if (s->pxe) {
+        if (rom_add_option("pxe-ne2k_isa.bin") != 0) {
+            qemu_error("warning: loading rom pxe-ne2k_isa.bin failed\n");
+            s->pxe = 0;
+        }
+    }
+
     register_savevm("ne2000", -1, 2, ne2000_save, ne2000_load, s);
     return 0;
 }
@@ -104,6 +112,7 @@ static ISADeviceInfo ne2000_isa_info = {
     .qdev.props = (Property[]) {
         DEFINE_PROP_HEX32("iobase", ISANE2000State, iobase,       0x300),
         DEFINE_PROP_UINT32("irq",   ISANE2000State, isairq,       9),
+        DEFINE_PROP_UINT32("pxe",   ISANE2000State, ne2000.pxe,   0),
         DEFINE_PROP_MACADDR("mac",  ISANE2000State, ne2000.macaddr),
         DEFINE_PROP_INT32("vlan",   ISANE2000State, ne2000.vlan,  -1),
         DEFINE_PROP_END_OF_LIST(),
diff --git a/hw/ne2000.h b/hw/ne2000.h
index 7936fb2..febaf6a 100644
--- a/hw/ne2000.h
+++ b/hw/ne2000.h
@@ -23,6 +23,7 @@ typedef struct NE2000State {
     uint8_t mult[8]; /* multicast mask array */
     qemu_irq irq;
     int32_t vlan;
+    uint32_t pxe;
     VLANClientState *vc;
     macaddr_t macaddr;
     uint8_t mem[NE2000_MEM_SIZE];
-- 
1.6.2.5

Re: [Qemu-devel] [RFC PATCH v2 5/5] ne2k_isa: add property for option rom loading.

[Anthony Liguori]

Gerd Hoffmann wrote:
> Moving option rom loading from machine init to the (nic) drivers.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/ne2000-isa.c |    9 +++++++++
>  hw/ne2000.h     |    1 +
>  2 files changed, 10 insertions(+), 0 deletions(-)
>
> diff --git a/hw/ne2000-isa.c b/hw/ne2000-isa.c
> index 7a24088..1d296a0 100644
> --- a/hw/ne2000-isa.c
> +++ b/hw/ne2000-isa.c
> @@ -27,6 +27,7 @@
>  #include "qdev.h"
>  #include "net.h"
>  #include "ne2000.h"
> +#include "loader.h"
>  
>  typedef struct ISANE2000State {
>      ISADevice dev;
> @@ -78,6 +79,13 @@ static int isa_ne2000_initfn(ISADevice *dev)
>          qemu_error("warning: no vlan specfied, ne2k_isa is unconnected\n");
>      }
>  
> +    if (s->pxe) {
> +        if (rom_add_option("pxe-ne2k_isa.bin") != 0) {
> +            qemu_error("warning: loading rom pxe-ne2k_isa.bin failed\n");
> +            s->pxe = 0;
> +        }
> +    }
> +
>   

Maybe we should make the filename a property instead of adding a pxe option?

Regards,

Anthony Liguori

Re: [Qemu-devel] [RFC PATCH v2 5/5] ne2k_isa: add property for option rom loading.

[Gerd Hoffmann]

On 10/07/09 15:08, Anthony Liguori wrote:
> Gerd Hoffmann wrote:
>> + if (s->pxe) {
>> + if (rom_add_option("pxe-ne2k_isa.bin") != 0) {
>> + qemu_error("warning: loading rom pxe-ne2k_isa.bin failed\n");
>> + s->pxe = 0;
>> + }
>> + }
>> +
>
> Maybe we should make the filename a property instead of adding a pxe
> option?

No.  The user should not need to know the file name of the option rom 
just to enable pxe booting for the nic.

When making the filename configurable it should be a separate property 
like "rom-name" or simliar.  I would suggest to NOT implement it unless 
users actually ask for it ;)

cheers,
   Gerd

Re: [Qemu-devel] [RFC PATCH v2 5/5] ne2k_isa: add property for option rom loading.

[Anthony Liguori]

Gerd Hoffmann wrote:
> On 10/07/09 15:08, Anthony Liguori wrote:
>> Gerd Hoffmann wrote:
>>> + if (s->pxe) {
>>> + if (rom_add_option("pxe-ne2k_isa.bin") != 0) {
>>> + qemu_error("warning: loading rom pxe-ne2k_isa.bin failed\n");
>>> + s->pxe = 0;
>>> + }
>>> + }
>>> +
>>
>> Maybe we should make the filename a property instead of adding a pxe
>> option?
>
> No.  The user should not need to know the file name of the option rom 
> just to enable pxe booting for the nic.

Having a pxe flag is somewhat odd.  Real network devices always have 
roms and they always get loaded.  They register themselves as BEV 
devices and the normal boot selection is used to determine whether a 
particular NIC gets network booted or not.

Our roms do expose themselves as BEV roms so there's really no harm in 
loading an option rom while booting from disk.

Any PCI device can have a rom and it probably should be a generic 
property of any PCI device.  There's really nothing specific about 
network adapters.

> When making the filename configurable it should be a separate property 
> like "rom-name" or simliar.  I would suggest to NOT implement it 
> unless users actually ask for it ;)

Quite a few users today replace the standard etherboot roms with gPXE roms.

Regards,

Anthony Liguori

Re: [Qemu-devel] [RFC PATCH v2 5/5] ne2k_isa: add property for option rom loading.

[Gerd Hoffmann]

On 10/07/09 15:28, Anthony Liguori wrote:
> Having a pxe flag is somewhat odd. Real network devices always have roms
> and they always get loaded. They register themselves as BEV devices and
> the normal boot selection is used to determine whether a particular NIC
> gets network booted or not.
>
> Our roms do expose themselves as BEV roms so there's really no harm in
> loading an option rom while booting from disk.

Wrong.  Loading a pxe rom makes qemu trying to boot from it, even with 
-boot c (using the roms shipped in pc-bios/).

Maybe SeaBIOS has better BEV support and handles things differently, so 
we could load them unconditionally once we made the switch.

> Any PCI device can have a rom and it probably should be a generic
> property of any PCI device. There's really nothing specific about
> network adapters.

It's pc-specific though, so when we go the route of loading roms 
unconditionally we need to wrap that into a machine-specific helper 
function so it happes on TARGET_I386 only.

>> When making the filename configurable it should be a separate property
>> like "rom-name" or simliar. I would suggest to NOT implement it unless
>> users actually ask for it ;)
>
> Quite a few users today replace the standard etherboot roms with gPXE roms.

Usually with the same file names though, so a simple '-boot n' continues 
to work.

cheers,
   Gerd

Re: [Qemu-devel] [RFC PATCH v2 5/5] ne2k_isa: add property for option rom loading.

[Anthony Liguori]

Gerd Hoffmann wrote:
> On 10/07/09 15:28, Anthony Liguori wrote:
>> Having a pxe flag is somewhat odd. Real network devices always have roms
>> and they always get loaded. They register themselves as BEV devices and
>> the normal boot selection is used to determine whether a particular NIC
>> gets network booted or not.
>>
>> Our roms do expose themselves as BEV roms so there's really no harm in
>> loading an option rom while booting from disk.
>
> Wrong.  Loading a pxe rom makes qemu trying to boot from it, even with 
> -boot c (using the roms shipped in pc-bios/).

Only with the e1000 because the rom is misconfig.  Try the ne2k or the 
rtl8139.

>> Any PCI device can have a rom and it probably should be a generic
>> property of any PCI device. There's really nothing specific about
>> network adapters.
>
> It's pc-specific though, so when we go the route of loading roms 
> unconditionally we need to wrap that into a machine-specific helper 
> function so it happes on TARGET_I386 only.

No, it's not pc-specific.  An e1000 card on a PPC still has an x86 
option rom.  Whether it gets loaded and how it gets loaded depends on 
the target, but not the existence of the rom on the device.

Regards,

Anthony Liguori

Re: [Qemu-devel] [RFC PATCH v2 5/5] ne2k_isa: add property for option rom loading.

[Gerd Hoffmann]

>> Wrong. Loading a pxe rom makes qemu trying to boot from it, even with
>> -boot c (using the roms shipped in pc-bios/).
>
> Only with the e1000 because the rom is misconfig. Try the ne2k or the
> rtl8139.

rtl8139 works indeed (and shows up in the F12 menu as it should).

>> It's pc-specific though, so when we go the route of loading roms
>> unconditionally we need to wrap that into a machine-specific helper
>> function so it happes on TARGET_I386 only.
>
> No, it's not pc-specific. An e1000 card on a PPC still has an x86 option
> rom. Whether it gets loaded and how it gets loaded depends on the
> target, but not the existence of the rom on the device.

Yep, the *loading* is what I was referring to (see $subject) ...

cheers,
   Gerd

Re: [Qemu-devel] [RFC PATCH v2 5/5] ne2k_isa: add property for option rom loading.

[Anthony Liguori]

Gerd Hoffmann wrote:
>>> Wrong. Loading a pxe rom makes qemu trying to boot from it, even with
>>> -boot c (using the roms shipped in pc-bios/).
>>
>> Only with the e1000 because the rom is misconfig. Try the ne2k or the
>> rtl8139.
>
> rtl8139 works indeed (and shows up in the F12 menu as it should).
>
>>> It's pc-specific though, so when we go the route of loading roms
>>> unconditionally we need to wrap that into a machine-specific helper
>>> function so it happes on TARGET_I386 only.
>>
>> No, it's not pc-specific. An e1000 card on a PPC still has an x86 option
>> rom. Whether it gets loaded and how it gets loaded depends on the
>> target, but not the existence of the rom on the device.
>
> Yep, the *loading* is what I was referring to (see $subject) ...

Well, I guess I'm confused about where we stand.

Are you suggesting that we drop the pxe property and load the roms 
unconditionally?  The tricky thing here is that we only want to load a 
particular rom once.  There's no need to load the rtl8139 multiple times 
for multiple nics.

Regards,

Anthony Liguori

Re: [Qemu-devel] [RFC PATCH v2 5/5] ne2k_isa: add property for option rom loading.

[Gerd Hoffmann]

>>>> It's pc-specific though, so when we go the route of loading roms
>>>> unconditionally we need to wrap that into a machine-specific helper
>>>> function so it happes on TARGET_I386 only.
>>>
>>> No, it's not pc-specific. An e1000 card on a PPC still has an x86 option
>>> rom. Whether it gets loaded and how it gets loaded depends on the
>>> target, but not the existence of the rom on the device.
>>
>> Yep, the *loading* is what I was referring to (see $subject) ...
>
> Well, I guess I'm confused about where we stand.

loading the rom is x86 specific ...

> Are you suggesting that we drop the pxe property and load the roms
> unconditionally?

Yes, I think we should do that (on x86), given BEV works nicely.
The e1000 rom needs fixing first of course ;)

> The tricky thing here is that we only want to load a
> particular rom once. There's no need to load the rtl8139 multiple times
> for multiple nics.

As hw/loader.c keeps track of the roms this should be easy to do.

The rom_add_option() macro in hw/loader.h should become a function which 
loads the rom on x86 and does nothing on other archs.  Then the nic 
drivers can simply call rom_add_option("pxe-${driver}.bin") unconditionally.

cheers,
   Gerd