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

[David Gibson <david@gibson.dropbear.id.au>]

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

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

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]