[Qemu-devel] [PATCH v2 1/1] hw/net/spapr_llan: 6 byte mac address device tree entry

[Qemu-devel] [PATCH v2 1/1] hw/net/spapr_llan: 6 byte mac address device tree entry

[Sam Bobroff]

The spapr-vlan device in QEMU has always presented it's MAC address in
the device tree as an 8 byte value, even though PAPR requires it to be
6 bytes.  This is because, at the time, AIX required the value to be 8
bytes.  However, modern versions of AIX support the (correct) 6
byte value so they no longer require the workaround.

It would be neatest to always provide a 6 byte value but that would
cause a problem with old Linux kernel ibmveth drivers, so the old 8
byte value is still presented when necessary.

Since commit 13f85203e (3.10, May 2013) the driver has been able to
handle 6 or 8 byte addresses so versions after that don't need to be
considered specially.

Drivers from kernels before that can also handle either type of
address, but not always:
* If the first byte's lowest bits are 10, the address must be 6 bytes.
* Otherwise, the address must be 8 bytes.
(The two bits in question are significant in a MAC address: they
indicate a locally-administered unicast address.)

So to maintain compatibility the old 8 byte value is presented when
the lowest two bits of the first byte are not 10.

Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>
---

v2:

Re-introduced the old workaround so that old Linux kernel drivers aren't
broken, at the cost of AIX seeing the old value for for non-locally generated
or broadcast addresses (this shouldn't matter because those addresses probably
aren't used on virtual adapters).

Reworded first paragraph of commit message because AIX seems to still support
the old 8 byte value.

 hw/net/spapr_llan.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/hw/net/spapr_llan.c b/hw/net/spapr_llan.c
index 01ecb02..b73be87 100644
--- a/hw/net/spapr_llan.c
+++ b/hw/net/spapr_llan.c
@@ -385,18 +385,24 @@ static int spapr_vlan_devnode(VIOsPAPRDevice *dev, void *fdt, int node_off)
     int ret;
 
     /* Some old phyp versions give the mac address in an 8-byte
-     * property.  The kernel driver has an insane workaround for this;
+     * property.  The 3.10 kernel driver has an insane workaround;
      * rather than doing the obvious thing and checking the property
      * length, it checks whether the first byte has 0b10 in the low
      * bits.  If a correct 6-byte property has a different first byte
      * the kernel will get the wrong mac address, overrunning its
      * buffer in the process (read only, thank goodness).
      *
-     * Here we workaround the kernel workaround by always supplying an
-     * 8-byte property, with the mac address in the last six bytes */
-    memcpy(&padded_mac[2], &vdev->nicconf.macaddr, ETH_ALEN);
-    ret = fdt_setprop(fdt, node_off, "local-mac-address",
-                      padded_mac, sizeof(padded_mac));
+     * Here we return a 6-byte address unless that would break a 3.10 driver.
+     * In that case we return a padded 8-byte address to allow the old
+     * workaround to succeed. */
+    if ((vdev->nicconf.macaddr.a[0] & 0x3) == 0x2) {
+        ret = fdt_setprop(fdt, node_off, "local-mac-address",
+                          &vdev->nicconf.macaddr, ETH_ALEN);
+    } else {
+        memcpy(&padded_mac[2], &vdev->nicconf.macaddr, ETH_ALEN);
+        ret = fdt_setprop(fdt, node_off, "local-mac-address",
+                          padded_mac, sizeof(padded_mac));
+    }
     if (ret < 0) {
         return ret;
     }
-- 
2.10.0.297.gf6727b0

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 1/1] hw/net/spapr_llan: 6 byte mac address device tree entry

[Thomas Huth]

On 21.11.2016 06:04, Sam Bobroff wrote:
> The spapr-vlan device in QEMU has always presented it's MAC address in
> the device tree as an 8 byte value, even though PAPR requires it to be
> 6 bytes.  This is because, at the time, AIX required the value to be 8
> bytes.  However, modern versions of AIX support the (correct) 6
> byte value so they no longer require the workaround.
> 
> It would be neatest to always provide a 6 byte value but that would
> cause a problem with old Linux kernel ibmveth drivers, so the old 8
> byte value is still presented when necessary.
> 
> Since commit 13f85203e (3.10, May 2013) the driver has been able to
> handle 6 or 8 byte addresses so versions after that don't need to be
> considered specially.
> 
> Drivers from kernels before that can also handle either type of
> address, but not always:
> * If the first byte's lowest bits are 10, the address must be 6 bytes.
> * Otherwise, the address must be 8 bytes.
> (The two bits in question are significant in a MAC address: they
> indicate a locally-administered unicast address.)
> 
> So to maintain compatibility the old 8 byte value is presented when
> the lowest two bits of the first byte are not 10.
> 
> Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>
> ---
> 
> v2:
> 
> Re-introduced the old workaround so that old Linux kernel drivers aren't
> broken, at the cost of AIX seeing the old value for for non-locally generated
> or broadcast addresses (this shouldn't matter because those addresses probably
> aren't used on virtual adapters).
> 
> Reworded first paragraph of commit message because AIX seems to still support
> the old 8 byte value.
> 
>  hw/net/spapr_llan.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/net/spapr_llan.c b/hw/net/spapr_llan.c
> index 01ecb02..b73be87 100644
> --- a/hw/net/spapr_llan.c
> +++ b/hw/net/spapr_llan.c
> @@ -385,18 +385,24 @@ static int spapr_vlan_devnode(VIOsPAPRDevice *dev, void *fdt, int node_off)
>      int ret;
>  
>      /* Some old phyp versions give the mac address in an 8-byte
> -     * property.  The kernel driver has an insane workaround for this;
> +     * property.  The 3.10 kernel driver has an insane workaround;

Kernel 3.10 has already the fix, so I think it would be better to write
something like "The kernel driver (before version 3.10) has ...".

>       * rather than doing the obvious thing and checking the property
>       * length, it checks whether the first byte has 0b10 in the low
>       * bits.  If a correct 6-byte property has a different first byte
>       * the kernel will get the wrong mac address, overrunning its
>       * buffer in the process (read only, thank goodness).
>       *
> -     * Here we workaround the kernel workaround by always supplying an
> -     * 8-byte property, with the mac address in the last six bytes */
> -    memcpy(&padded_mac[2], &vdev->nicconf.macaddr, ETH_ALEN);
> -    ret = fdt_setprop(fdt, node_off, "local-mac-address",
> -                      padded_mac, sizeof(padded_mac));
> +     * Here we return a 6-byte address unless that would break a 3.10 driver.

" ... break a pre-3.10 driver." ?

> +     * In that case we return a padded 8-byte address to allow the old
> +     * workaround to succeed. */
> +    if ((vdev->nicconf.macaddr.a[0] & 0x3) == 0x2) {
> +        ret = fdt_setprop(fdt, node_off, "local-mac-address",
> +                          &vdev->nicconf.macaddr, ETH_ALEN);
> +    } else {
> +        memcpy(&padded_mac[2], &vdev->nicconf.macaddr, ETH_ALEN);
> +        ret = fdt_setprop(fdt, node_off, "local-mac-address",
> +                          padded_mac, sizeof(padded_mac));
> +    }
>      if (ret < 0) {
>          return ret;
>      }
> 

 Thomas

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 1/1] hw/net/spapr_llan: 6 byte mac address device tree entry

[Sam Bobroff]

On Mon, Nov 21, 2016 at 11:14:26AM +0100, Thomas Huth wrote:
> On 21.11.2016 06:04, Sam Bobroff wrote:
> > The spapr-vlan device in QEMU has always presented it's MAC address in
> > the device tree as an 8 byte value, even though PAPR requires it to be
> > 6 bytes.  This is because, at the time, AIX required the value to be 8
> > bytes.  However, modern versions of AIX support the (correct) 6
> > byte value so they no longer require the workaround.
> > 
> > It would be neatest to always provide a 6 byte value but that would
> > cause a problem with old Linux kernel ibmveth drivers, so the old 8
> > byte value is still presented when necessary.
> > 
> > Since commit 13f85203e (3.10, May 2013) the driver has been able to
> > handle 6 or 8 byte addresses so versions after that don't need to be
> > considered specially.
> > 
> > Drivers from kernels before that can also handle either type of
> > address, but not always:
> > * If the first byte's lowest bits are 10, the address must be 6 bytes.
> > * Otherwise, the address must be 8 bytes.
> > (The two bits in question are significant in a MAC address: they
> > indicate a locally-administered unicast address.)
> > 
> > So to maintain compatibility the old 8 byte value is presented when
> > the lowest two bits of the first byte are not 10.
> > 
> > Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>
> > ---
> > 
> > v2:
> > 
> > Re-introduced the old workaround so that old Linux kernel drivers aren't
> > broken, at the cost of AIX seeing the old value for for non-locally generated
> > or broadcast addresses (this shouldn't matter because those addresses probably
> > aren't used on virtual adapters).
> > 
> > Reworded first paragraph of commit message because AIX seems to still support
> > the old 8 byte value.
> > 
> >  hw/net/spapr_llan.c | 18 ++++++++++++------
> >  1 file changed, 12 insertions(+), 6 deletions(-)
> > 
> > diff --git a/hw/net/spapr_llan.c b/hw/net/spapr_llan.c
> > index 01ecb02..b73be87 100644
> > --- a/hw/net/spapr_llan.c
> > +++ b/hw/net/spapr_llan.c
> > @@ -385,18 +385,24 @@ static int spapr_vlan_devnode(VIOsPAPRDevice *dev, void *fdt, int node_off)
> >      int ret;
> >  
> >      /* Some old phyp versions give the mac address in an 8-byte
> > -     * property.  The kernel driver has an insane workaround for this;
> > +     * property.  The 3.10 kernel driver has an insane workaround;
> 
> Kernel 3.10 has already the fix, so I think it would be better to write
> something like "The kernel driver (before version 3.10) has ...".

Right, will do.

> >       * rather than doing the obvious thing and checking the property
> >       * length, it checks whether the first byte has 0b10 in the low
> >       * bits.  If a correct 6-byte property has a different first byte
> >       * the kernel will get the wrong mac address, overrunning its
> >       * buffer in the process (read only, thank goodness).
> >       *
> > -     * Here we workaround the kernel workaround by always supplying an
> > -     * 8-byte property, with the mac address in the last six bytes */
> > -    memcpy(&padded_mac[2], &vdev->nicconf.macaddr, ETH_ALEN);
> > -    ret = fdt_setprop(fdt, node_off, "local-mac-address",
> > -                      padded_mac, sizeof(padded_mac));
> > +     * Here we return a 6-byte address unless that would break a 3.10 driver.
> 
> " ... break a pre-3.10 driver." ?

OK.

> > +     * In that case we return a padded 8-byte address to allow the old
> > +     * workaround to succeed. */
> > +    if ((vdev->nicconf.macaddr.a[0] & 0x3) == 0x2) {
> > +        ret = fdt_setprop(fdt, node_off, "local-mac-address",
> > +                          &vdev->nicconf.macaddr, ETH_ALEN);
> > +    } else {
> > +        memcpy(&padded_mac[2], &vdev->nicconf.macaddr, ETH_ALEN);
> > +        ret = fdt_setprop(fdt, node_off, "local-mac-address",
> > +                          padded_mac, sizeof(padded_mac));
> > +    }
> >      if (ret < 0) {
> >          return ret;
> >      }
> > 
> 
>  Thomas

Thanks,
Sam.