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.bobr...@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.
