From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40283) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c8lcO-0006zT-Sc for qemu-devel@nongnu.org; Mon, 21 Nov 2016 05:14:34 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c8lcL-00037c-K6 for qemu-devel@nongnu.org; Mon, 21 Nov 2016 05:14:32 -0500 References: From: Thomas Huth Message-ID: <29d82af9-1695-f277-ff00-734e6952a1fc@redhat.com> Date: Mon, 21 Nov 2016 11:14:26 +0100 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 1/1] hw/net/spapr_llan: 6 byte mac address device tree entry List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Sam Bobroff , qemu-devel@nongnu.org Cc: qemu-ppc@nongnu.org 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 > --- > > 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