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

[Qemu-devel] [RfC PATCH 0/3] 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'.

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=0,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
	[ ... ]

'info network' shows:

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

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.v1

You might find it useful to have a look at the qbus patches.  Especially
the hotplug patches which add device_add and device_del monitor
commands.  The patches are on the qemu-devel list too and also here:
   http://git.et.redhat.com/?p=qemu-kraxel.git;a=shortlog;h=refs/heads/qbus.v2

cheers,
  Gerd

[Qemu-devel] [RfC PATCH 1/3] 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).
---
 net.c |   14 ++++++++++++++
 net.h |    2 ++
 2 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/net.c b/net.c
index 3fdf1e6..c6eb93c 100644
--- a/net.c
+++ b/net.c
@@ -280,6 +280,20 @@ 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)
+{
+    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;
+}
+
 static char *assign_name(VLANClientState *vc1, const char *model)
 {
     VLANState *vlan;
diff --git a/net.h b/net.h
index 1479826..50630a1 100644
--- a/net.h
+++ b/net.h
@@ -7,6 +7,7 @@
 
 /* VLANs support */
 
+typedef uint8_t macaddr_t[6];
 typedef struct VLANClientState VLANClientState;
 
 typedef int (NetCanReceive)(VLANClientState *);
@@ -75,6 +76,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);
 void qemu_check_nic_model(NICInfo *nd, const char *model);
 void qemu_check_nic_model_list(NICInfo *nd, const char * const *models,
                                const char *default_model);
-- 
1.6.2.5

[Qemu-devel] [RfC PATCH 2/3] qdev: mac addr property fixups

[Gerd Hoffmann]

---
 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 2ecb58d..89e45e1 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)
@@ -246,7 +247,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;
 
@@ -255,26 +256,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,
 };
@@ -421,6 +427,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 623ded5..7a081f8 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -203,7 +203,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()               \
     {}
@@ -218,6 +218,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 3/3] ne2k_isa: qdev-ify.

[Gerd Hoffmann]

---
 hw/ne2000-isa.c |   14 +++++++++-----
 hw/ne2000.h     |    3 ++-
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/hw/ne2000-isa.c b/hw/ne2000-isa.c
index 54c0478..a8c2c43 100644
--- a/hw/ne2000-isa.c
+++ b/hw/ne2000-isa.c
@@ -65,10 +65,11 @@ 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,
+    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);
@@ -84,9 +85,10 @@ 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_uint32(&dev->qdev, "vlan",   nd->vlan->id);
+    qdev_prop_set_macaddr(&dev->qdev, "mac",   nd->macaddr);
     qdev_init(&dev->qdev);
 }
 
@@ -95,8 +97,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_UINT32("vlan",  ISANE2000State, ne2000.vlan,  0),
+        DEFINE_PROP_MACADDR("mac",  ISANE2000State, ne2000.macaddr),
         DEFINE_PROP_END_OF_LIST(),
     },
 };
diff --git a/hw/ne2000.h b/hw/ne2000.h
index 92a2ddb..151c78e 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;
+    uint32_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 1/3] net: macaddr tweaks.

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

> 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).
> ---
>  net.c |   14 ++++++++++++++
>  net.h |    2 ++
>  2 files changed, 16 insertions(+), 0 deletions(-)
>
> diff --git a/net.c b/net.c
> index 3fdf1e6..c6eb93c 100644
> --- a/net.c
> +++ b/net.c
> @@ -280,6 +280,20 @@ 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)
> +{
> +    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;
> +}
> +

This will get us the same default MAC address for all NICs, won't it?
The old code provides a different default for each NIC.

Simply increment the default whenever it is used?

>  static char *assign_name(VLANClientState *vc1, const char *model)
>  {
>      VLANState *vlan;
> diff --git a/net.h b/net.h
> index 1479826..50630a1 100644
> --- a/net.h
> +++ b/net.h
> @@ -7,6 +7,7 @@
>  
>  /* VLANs support */
>  
> +typedef uint8_t macaddr_t[6];

Reserved identifier (any POSIX header).  Do we care?

>  typedef struct VLANClientState VLANClientState;
>  
>  typedef int (NetCanReceive)(VLANClientState *);
> @@ -75,6 +76,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);
>  void qemu_check_nic_model(NICInfo *nd, const char *model);
>  void qemu_check_nic_model_list(NICInfo *nd, const char * const *models,
>                                 const char *default_model);

Re: [Qemu-devel] [RfC PATCH 0/3] qdev-ify network cards

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

>   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'.
>
> 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=0,irq=3,id=foo
[...]
>
> 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.

That would be good.

> This patch series is also available here:
>   http://git.et.redhat.com/?p=qemu-kraxel.git;a=shortlog;h=refs/heads/nic.v1
>
> You might find it useful to have a look at the qbus patches.  Especially
> the hotplug patches which add device_add and device_del monitor
> commands.  The patches are on the qemu-devel list too and also here:
>    http://git.et.redhat.com/?p=qemu-kraxel.git;a=shortlog;h=refs/heads/qbus.v2

Patches look fine to me.

Re: [Qemu-devel] [RfC PATCH 1/3] net: macaddr tweaks.

[Gerd Hoffmann]

>> +void qemu_macaddr_default_if_unset(macaddr_t macaddr)
>> +{
>> +    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;
>> +}
>> +
>
> This will get us the same default MAC address for all NICs, won't it?
> The old code provides a different default for each NIC.
>
> Simply increment the default whenever it is used?

Would be an option.  Doesn't fully match the old behavior though.

Old behavior is to use 0x56 + nic index (i.e. nd_table index) for the 
last byte, so nic #1 allways has 0x57, no matter whenever for nic #0 a 
default was specified or not.

The problem we have here is that the nics created via -device don't have 
a nd_table entry and thus no nic index.  We have a related issue with 
pxe boot (-boot n,o,p,q == boot first,second,third,fourth nic).

Didn't have a good idea (yet) how to address that.

>> +typedef uint8_t macaddr_t[6];
>
> Reserved identifier (any POSIX header).  Do we care?

We should prrobably qemu-ify it to something like MACAddr anyway.

cheers,
   Gerd

Re: [Qemu-devel] [RfC PATCH 0/3] qdev-ify network cards

[Mark McLoughlin]

On Fri, 2009-09-25 at 21:43 +0200, Gerd Hoffmann wrote:
> 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'.
> 
> 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=0,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
> 	[ ... ]
> 
> 'info network' shows:
> 
> 	[ ... ]
> 	VLAN 1 devices:
> 	  foo: model=ne2k_isa,macaddr=00:11:22:33:44:55
> 
> The nic initialization code calls qemu_new_vlan_client() with the
> vlan specified using the vlan property.

One thing I'd like to see is that if vlan= isn't specified, the nic
shouldn't be connected to any vlan.

I want to add the option to connect a nic directly to a backend without
any vlan involved. That implies you should be able to create a
disconnected nic and later connect it to a backend. Assuming vlan=0
where none is specified makes that difficult.

Patches look fine otherwise.

Cheers,
Mark.

Re: [Qemu-devel] [RfC PATCH 0/3] qdev-ify network cards

[Gerd Hoffmann]

> One thing I'd like to see is that if vlan= isn't specified, the nic
> shouldn't be connected to any vlan.
>
> I want to add the option to connect a nic directly to a backend without
> any vlan involved. That implies you should be able to create a
> disconnected nic and later connect it to a backend. Assuming vlan=0
> where none is specified makes that difficult.

Easy to do ;)

cheers,
   Gerd

[Qemu-devel] Re: [RfC PATCH 1/3] net: macaddr tweaks.

[Paolo Bonzini]

> +void qemu_macaddr_default_if_unset(macaddr_t macaddr)
> +{
> +    macaddr_t zero = { 0,0,0,0,0,0 };

Very minor since this is not at all a hot path, but please make it just

   static macaddr_t zero;

since otherwise it would be initialized on the stack at run time.

Paolo

Re: [Qemu-devel] Re: [RfC PATCH 1/3] net: macaddr tweaks.

[Blue Swirl]

On Wed, Sep 30, 2009 at 2:57 PM, Paolo Bonzini <bonzini@gnu.org> wrote:
>
>> +void qemu_macaddr_default_if_unset(macaddr_t macaddr)
>> +{
>> +    macaddr_t zero = { 0,0,0,0,0,0 };
>
> Very minor since this is not at all a hot path, but please make it just
>
>  static macaddr_t zero;
>
> since otherwise it would be initialized on the stack at run time.

Even better, static const.