[Qemu-devel] [PULL 0/4] Net patches

[Qemu-devel] [PULL 0/4] Net patches

[Stefan Hajnoczi]

The following changes since commit 1680d485777ecf436d724631ea8722cc0c66990e:

  Merge remote-tracking branch 'rth/tcg-ldst-6' into staging (2013-10-14 09:59:59 -0700)

are available in the git repository at:


  git://github.com/stefanha/qemu.git net

for you to fetch changes up to 6e6247e54da991cbc7e0c05040bd6a14d505fc89:

  net: disallow to specify multicast MAC address (2013-10-18 13:28:09 +0200)

----------------------------------------------------------------
Amos Kong (3):
      net: update nic info during device reset
      net/e1000: update network information when macaddr is changed in guest
      net/rtl8139: update network information when macaddr is changed in guest

Dmitry Krivenok (1):
      net: disallow to specify multicast MAC address

 hw/net/e1000.c   | 9 +++++++++
 hw/net/rtl8139.c | 7 ++++++-
 net/net.c        | 5 +++++
 net/util.c       | 5 +++++
 net/util.h       | 2 ++
 5 files changed, 27 insertions(+), 1 deletion(-)

-- 
1.8.3.1

[Qemu-devel] [PULL 1/4] net: update nic info during device reset

[Stefan Hajnoczi]

From: Amos Kong <akong@redhat.com>

macaddr is reset during device reset, but nic info
isn't updated, this problem exists in e1000 & rtl8139

Signed-off-by: Amos Kong <akong@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/net/e1000.c   | 1 +
 hw/net/rtl8139.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index 151d25e..9a1c46e 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -401,6 +401,7 @@ static void e1000_reset(void *opaque)
         d->mac_reg[RA] |= macaddr[i] << (8 * i);
         d->mac_reg[RA + 1] |= (i < 2) ? macaddr[i + 4] << (8 * i) : 0;
     }
+    qemu_format_nic_info_str(qemu_get_queue(d->nic), macaddr);
 }
 
 static void
diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
index c31199f..9b4a650 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -1214,6 +1214,7 @@ static void rtl8139_reset(DeviceState *d)
 
     /* restore MAC address */
     memcpy(s->phys, s->conf.macaddr.a, 6);
+    qemu_format_nic_info_str(qemu_get_queue(s->nic), s->phys);
 
     /* reset interrupt mask */
     s->IntrStatus = 0;
-- 
1.8.3.1

[Qemu-devel] [PULL 2/4] net/e1000: update network information when macaddr is changed in guest

[Stefan Hajnoczi]

From: Amos Kong <akong@redhat.com>

If we change macaddr in guest by 'ifconfig eth0 hw ether 12:12:12:34:35:36',
the mac register of e1000 is already updated, but we don't update
network information in qemu. Therefor, the information in monitor
is wrong.

This patch updates nic info when the second part of macaddr is written.

Signed-off-by: Amos Kong <akong@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/net/e1000.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index 9a1c46e..70a59fd 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -1106,7 +1106,15 @@ mac_read_clr8(E1000State *s, int index)
 static void
 mac_writereg(E1000State *s, int index, uint32_t val)
 {
+    uint32_t macaddr[2];
+
     s->mac_reg[index] = val;
+
+    if (index == RA + 1) {
+        macaddr[0] = cpu_to_le32(s->mac_reg[RA]);
+        macaddr[1] = cpu_to_le32(s->mac_reg[RA + 1]);
+        qemu_format_nic_info_str(qemu_get_queue(s->nic), (uint8_t *)macaddr);
+    }
 }
 
 static void
-- 
1.8.3.1

[Qemu-devel] [PULL 3/4] net/rtl8139: update network information when macaddr is changed in guest

[Stefan Hajnoczi]

From: Amos Kong <akong@redhat.com>

rtl8139 has same problem as e1000, nic info isn't updated when macaddr
is changed in guest.

This patch updates the nic info when the last bit of macaddr is written.

Signed-off-by: Amos Kong <akong@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/net/rtl8139.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
index 9b4a650..3225f3d 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -2741,8 +2741,12 @@ static void rtl8139_io_writeb(void *opaque, uint8_t addr, uint32_t val)
 
     switch (addr)
     {
-        case MAC0 ... MAC0+5:
+        case MAC0 ... MAC0+4:
+            s->phys[addr - MAC0] = val;
+            break;
+        case MAC0+5:
             s->phys[addr - MAC0] = val;
+            qemu_format_nic_info_str(qemu_get_queue(s->nic), s->phys);
             break;
         case MAC0+6 ... MAC0+7:
             /* reserved */
-- 
1.8.3.1

[Qemu-devel] [PULL 4/4] net: disallow to specify multicast MAC address

[Stefan Hajnoczi]

From: Dmitry Krivenok <krivenok.dmitry@gmail.com>

Added explicit check of MAC address specified via macaddr option.
Multicast MAC addresses are no longer allowed.
This fixes bug lp#495566.

Signed-off-by: Dmitry V. Krivenok <krivenok.dmitry@gmail.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 net/net.c  | 5 +++++
 net/util.c | 5 +++++
 net/util.h | 2 ++
 3 files changed, 12 insertions(+)

diff --git a/net/net.c b/net/net.c
index c330c9a..009dece 100644
--- a/net/net.c
+++ b/net/net.c
@@ -689,6 +689,11 @@ static int net_init_nic(const NetClientOptions *opts, const char *name,
         error_report("invalid syntax for ethernet address");
         return -1;
     }
+    if (nic->has_macaddr &&
+        net_macaddr_is_multicast(nd->macaddr.a)) {
+        error_report("NIC cannot have multicast MAC address (odd 1st byte)");
+        return -1;
+    }
     qemu_macaddr_default_if_unset(&nd->macaddr);
 
     if (nic->has_vectors) {
diff --git a/net/util.c b/net/util.c
index 7e95076..0177bb3 100644
--- a/net/util.c
+++ b/net/util.c
@@ -58,3 +58,8 @@ int net_parse_macaddr(uint8_t *macaddr, const char *p)
 
     return 0;
 }
+
+bool net_macaddr_is_multicast(uint8_t *macaddr)
+{
+    return macaddr[0] % 2;
+}
diff --git a/net/util.h b/net/util.h
index 10c7da9..4581cb7 100644
--- a/net/util.h
+++ b/net/util.h
@@ -26,7 +26,9 @@
 #define QEMU_NET_UTIL_H
 
 #include <stdint.h>
+#include <stdbool.h>
 
 int net_parse_macaddr(uint8_t *macaddr, const char *p);
+bool net_macaddr_is_multicast(uint8_t *macaddr);
 
 #endif /* QEMU_NET_UTIL_H */
-- 
1.8.3.1

Re: [Qemu-devel] [PULL 4/4] net: disallow to specify multicast MAC address

[Amos Kong]

On Fri, Oct 18, 2013 at 03:35:14PM +0200, Stefan Hajnoczi wrote:
> From: Dmitry Krivenok <krivenok.dmitry@gmail.com>
> 
> Added explicit check of MAC address specified via macaddr option.
> Multicast MAC addresses are no longer allowed.
> This fixes bug lp#495566.
> 
> Signed-off-by: Dmitry V. Krivenok <krivenok.dmitry@gmail.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  net/net.c  | 5 +++++
>  net/util.c | 5 +++++
>  net/util.h | 2 ++
>  3 files changed, 12 insertions(+)
> 
> diff --git a/net/net.c b/net/net.c
> index c330c9a..009dece 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -689,6 +689,11 @@ static int net_init_nic(const NetClientOptions *opts, const char *name,
>          error_report("invalid syntax for ethernet address");
>          return -1;
>      }
> +    if (nic->has_macaddr &&
> +        net_macaddr_is_multicast(nd->macaddr.a)) {
> +        error_report("NIC cannot have multicast MAC address (odd 1st byte)");
> +        return -1;
> +    }
>      qemu_macaddr_default_if_unset(&nd->macaddr);
>  
>      if (nic->has_vectors) {
> diff --git a/net/util.c b/net/util.c
> index 7e95076..0177bb3 100644
> --- a/net/util.c
> +++ b/net/util.c
> @@ -58,3 +58,8 @@ int net_parse_macaddr(uint8_t *macaddr, const char *p)
>  
>      return 0;
>  }
> +
> +bool net_macaddr_is_multicast(uint8_t *macaddr)
> +{
> +    return macaddr[0] % 2;
> +}

Duplicate, we already have is_multicast_ether_addr() in include/net/eth.h

> diff --git a/net/util.h b/net/util.h
> index 10c7da9..4581cb7 100644
> --- a/net/util.h
> +++ b/net/util.h
> @@ -26,7 +26,9 @@
>  #define QEMU_NET_UTIL_H
>  
>  #include <stdint.h>
> +#include <stdbool.h>
>  
>  int net_parse_macaddr(uint8_t *macaddr, const char *p);
> +bool net_macaddr_is_multicast(uint8_t *macaddr);
>  
>  #endif /* QEMU_NET_UTIL_H */
> -- 
> 1.8.3.1
> 

-- 
			Amos.

Re: [Qemu-devel] [PULL 4/4] net: disallow to specify multicast MAC address

[Stefan Hajnoczi]

On Fri, Oct 18, 2013 at 10:56:55PM +0800, Amos Kong wrote:
> On Fri, Oct 18, 2013 at 03:35:14PM +0200, Stefan Hajnoczi wrote:
> > From: Dmitry Krivenok <krivenok.dmitry@gmail.com>
> > 
> > Added explicit check of MAC address specified via macaddr option.
> > Multicast MAC addresses are no longer allowed.
> > This fixes bug lp#495566.
> > 
> > Signed-off-by: Dmitry V. Krivenok <krivenok.dmitry@gmail.com>
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  net/net.c  | 5 +++++
> >  net/util.c | 5 +++++
> >  net/util.h | 2 ++
> >  3 files changed, 12 insertions(+)
> > 
> > diff --git a/net/net.c b/net/net.c
> > index c330c9a..009dece 100644
> > --- a/net/net.c
> > +++ b/net/net.c
> > @@ -689,6 +689,11 @@ static int net_init_nic(const NetClientOptions *opts, const char *name,
> >          error_report("invalid syntax for ethernet address");
> >          return -1;
> >      }
> > +    if (nic->has_macaddr &&
> > +        net_macaddr_is_multicast(nd->macaddr.a)) {
> > +        error_report("NIC cannot have multicast MAC address (odd 1st byte)");
> > +        return -1;
> > +    }
> >      qemu_macaddr_default_if_unset(&nd->macaddr);
> >  
> >      if (nic->has_vectors) {
> > diff --git a/net/util.c b/net/util.c
> > index 7e95076..0177bb3 100644
> > --- a/net/util.c
> > +++ b/net/util.c
> > @@ -58,3 +58,8 @@ int net_parse_macaddr(uint8_t *macaddr, const char *p)
> >  
> >      return 0;
> >  }
> > +
> > +bool net_macaddr_is_multicast(uint8_t *macaddr)
> > +{
> > +    return macaddr[0] % 2;
> > +}
> 
> Duplicate, we already have is_multicast_ether_addr() in include/net/eth.h

And net_macaddr_is_multicast() is wrong:
bit 0 is the unicast/multicast bit, not bit 1.

I will drop the patch and send a new pull request.

Re: [Qemu-devel] [PULL 4/4] net: disallow to specify multicast MAC address

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 647 bytes --]

On 10/18/2013 10:34 AM, Stefan Hajnoczi wrote:

>>> +
>>> +bool net_macaddr_is_multicast(uint8_t *macaddr)
>>> +{
>>> +    return macaddr[0] % 2;
>>> +}
>>
>> Duplicate, we already have is_multicast_ether_addr() in include/net/eth.h
> 
> And net_macaddr_is_multicast() is wrong:
> bit 0 is the unicast/multicast bit, not bit 1.

Huh? 'foo % 2' is the same as 'foo & 1' - both check bit 0.  But I agree
that the '&' form is a bit easier to reason with, even if the compiler
is smart enough to use the same code for both.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PULL 4/4] net: disallow to specify multicast MAC address

[Stefan Hajnoczi]

On Fri, Oct 18, 2013 at 10:56:55PM +0800, Amos Kong wrote:
> On Fri, Oct 18, 2013 at 03:35:14PM +0200, Stefan Hajnoczi wrote:
> > From: Dmitry Krivenok <krivenok.dmitry@gmail.com>
> > 
> > Added explicit check of MAC address specified via macaddr option.
> > Multicast MAC addresses are no longer allowed.
> > This fixes bug lp#495566.
> > 
> > Signed-off-by: Dmitry V. Krivenok <krivenok.dmitry@gmail.com>
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  net/net.c  | 5 +++++
> >  net/util.c | 5 +++++
> >  net/util.h | 2 ++
> >  3 files changed, 12 insertions(+)
> > 
> > diff --git a/net/net.c b/net/net.c
> > index c330c9a..009dece 100644
> > --- a/net/net.c
> > +++ b/net/net.c
> > @@ -689,6 +689,11 @@ static int net_init_nic(const NetClientOptions *opts, const char *name,
> >          error_report("invalid syntax for ethernet address");
> >          return -1;
> >      }
> > +    if (nic->has_macaddr &&
> > +        net_macaddr_is_multicast(nd->macaddr.a)) {
> > +        error_report("NIC cannot have multicast MAC address (odd 1st byte)");
> > +        return -1;
> > +    }
> >      qemu_macaddr_default_if_unset(&nd->macaddr);
> >  
> >      if (nic->has_vectors) {
> > diff --git a/net/util.c b/net/util.c
> > index 7e95076..0177bb3 100644
> > --- a/net/util.c
> > +++ b/net/util.c
> > @@ -58,3 +58,8 @@ int net_parse_macaddr(uint8_t *macaddr, const char *p)
> >  
> >      return 0;
> >  }
> > +
> > +bool net_macaddr_is_multicast(uint8_t *macaddr)
> > +{
> > +    return macaddr[0] % 2;
> > +}

Scratch what I said about bit 0 and bit 1, this function is using '%' not
'&'.

But we should still respin to use the existing function in eth.h.

Re: [Qemu-devel] [PULL 4/4] net: disallow to specify multicast MAC address

[Dmitry Krivenok]

> Scratch what I said about bit 0 and bit 1, this function is using '%' not
> '&'.
>
> But we should still respin to use the existing function in eth.h.

This is my first qemu patch, so sorry for inconsistent style and
reinventing the wheel.
I'll re-use existing function and send new patch.
Thanks!

Re: [Qemu-devel] [PULL 4/4] net: disallow to specify multicast MAC address

[Dmitry Krivenok]

> Duplicate, we already have is_multicast_ether_addr() in include/net/eth.h

I tried to #include "net/eth.h" in net/net.c, but it didn't work:

diff --git a/net/net.c b/net/net.c
index c330c9a..870d3bb 100644
--- a/net/net.c
+++ b/net/net.c
@@ -27,6 +27,7 @@
 #include "clients.h"
 #include "hub.h"
 #include "net/slirp.h"
+#include "net/eth.h"
 #include "util.h"

 #include "monitor/monitor.h"
@@ -689,6 +690,11 @@ static int net_init_nic(const NetClientOptions
*opts, const char *name,
         error_report("invalid syntax for ethernet address");
         return -1;
     }
+    if (nic->has_macaddr &&
+        is_multicast_ether_addr(nd->macaddr.a)) {
+        error_report("NIC cannot have multicast MAC address (odd 1st byte)");
+        return -1;
+    }
     qemu_macaddr_default_if_unset(&nd->macaddr);

     if (nic->has_vectors) {

$ make
  CC    net/net.o
In file included from /home/krivenok/qemu/include/qemu/sockets.h:18,
                 from net/net.c:35:
/usr/include/netinet/in.h:199: error: redefinition of 'struct in6_addr'
make: *** [net/net.o] Error 1
$

eth.h seems to be used only in vmware specific code and likely needs
to be fixed to be used in other places.
Other option is to move is_*_ether_addr() functions to another header
and include it in both places.

Re: [Qemu-devel] [PULL 4/4] net: disallow to specify multicast MAC address

[Amos Kong]

On Sat, Oct 19, 2013 at 12:08:14AM +0400, Dmitry Krivenok wrote:
> > Duplicate, we already have is_multicast_ether_addr() in include/net/eth.h
> 
> I tried to #include "net/eth.h" in net/net.c, but it didn't work:
> 
> diff --git a/net/net.c b/net/net.c
> index c330c9a..870d3bb 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -27,6 +27,7 @@
>  #include "clients.h"
>  #include "hub.h"
>  #include "net/slirp.h"
> +#include "net/eth.h"
>  #include "util.h"
> 
>  #include "monitor/monitor.h"
> @@ -689,6 +690,11 @@ static int net_init_nic(const NetClientOptions
> *opts, const char *name,
>          error_report("invalid syntax for ethernet address");
>          return -1;
>      }
> +    if (nic->has_macaddr &&
> +        is_multicast_ether_addr(nd->macaddr.a)) {
> +        error_report("NIC cannot have multicast MAC address (odd 1st byte)");
> +        return -1;
> +    }
>      qemu_macaddr_default_if_unset(&nd->macaddr);
> 
>      if (nic->has_vectors) {
> 
> $ make
>   CC    net/net.o
> In file included from /home/krivenok/qemu/include/qemu/sockets.h:18,
>                  from net/net.c:35:
> /usr/include/netinet/in.h:199: error: redefinition of 'struct in6_addr'
> make: *** [net/net.o] Error 1
> $

Hi Dmitry,

"struct in6_addr" exists in <netinet/in.h>, you can remove the
definition in eth.h

diff --git a/include/net/eth.h b/include/net/eth.h
index 1d48e06..1cce570 100644
--- a/include/net/eth.h
+++ b/include/net/eth.h
@@ -28,6 +28,7 @@
 
 #include <sys/types.h>
 #include <string.h>
+#include <netinet/in.h>
 #include "qemu/bswap.h"
 #include "qemu/iov.h"
 
@@ -83,13 +84,6 @@ typedef struct ip_pseudo_header {
     uint16_t ip_payload;
 } ip_pseudo_header;
 
-/* IPv6 address */
-struct in6_addr {
-    union {
-        uint8_t __u6_addr8[16];
-    } __in6_u;
-};
-
 struct ip6_header {
     union {
         struct ip6_hdrctl {

> 
> eth.h seems to be used only in vmware specific code and likely needs
> to be fixed to be used in other places.
> Other option is to move is_*_ether_addr() functions to another header
> and include it in both places.

-- 
			Amos.

Re: [Qemu-devel] [PULL 4/4] net: disallow to specify multicast MAC address

[Dmitry Krivenok]

[-- Attachment #1: Type: text/plain, Size: 330 bytes --]

This was my original intention, but I noticed that structs defined
differently and standard one may be bigger depending on some #defines. So I
renamed in6_addr to in6_address in include/net/eth.h (see v2 patch I sent
yesterday). In case qemu version is not supposed to be a different struct,
then your change is definitely right.

[-- Attachment #2: Type: text/html, Size: 350 bytes --]