Re: [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 <address@hidden>
> > ---
> > 
> > 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.