[v3,1/1] hw/net/spapr_llan: 6 byte mac address device tree entry

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

v3:

Made code comments more accurate.

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(-)

On 22.11.2016 00:19, 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>
> ---
> 
> v3:
> 
> Made code comments more accurate.
> 
> 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..74910e8 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 kernel driver (before 3.10) 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 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;
>      }
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

On Tue, Nov 22, 2016 at 10:19:38AM +1100, 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>

Sorry, I didn't see this one until now.

Since we need a workaround for the workaround, is it actually worth
going to the 6-byte property?

> ---
> 
> v3:
> 
> Made code comments more accurate.
> 
> 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..74910e8 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 kernel driver (before 3.10) 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 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;
>      }

On 13.02.2017 06:33, David Gibson wrote:
> On Tue, Nov 22, 2016 at 10:19:38AM +1100, 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>
> 
> Sorry, I didn't see this one until now.
> 
> Since we need a workaround for the workaround, is it actually worth
> going to the 6-byte property?

8-byte addresses are just wrong, all other network devices and the
standard use 6-byte MAC addresses instead. So we should use 6-byte
addresses in QEMU whenever it is possible, too. Unfortunately there are
still guests in the field that use this bad assumption with 8 byte
addresses, so I think this work-around is the best we can and should do
right now.

 Thomas

On Mon, Feb 13, 2017 at 08:36:45AM +0100, Thomas Huth wrote:
> On 13.02.2017 06:33, David Gibson wrote:
> > On Tue, Nov 22, 2016 at 10:19:38AM +1100, 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>
> > 
> > Sorry, I didn't see this one until now.
> > 
> > Since we need a workaround for the workaround, is it actually worth
> > going to the 6-byte property?
> 
> 8-byte addresses are just wrong, all other network devices and the
> standard use 6-byte MAC addresses instead. So we should use 6-byte
> addresses in QEMU whenever it is possible, too. Unfortunately there are
> still guests in the field that use this bad assumption with 8 byte
> addresses, so I think this work-around is the best we can and should do
> right now.

Hm, yes, good point.  Applied to ppc-for-2.9.