From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34809) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c8xnI-0007hU-RU for qemu-devel@nongnu.org; Mon, 21 Nov 2016 18:14:38 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c8xnF-00011i-Kv for qemu-devel@nongnu.org; Mon, 21 Nov 2016 18:14:36 -0500 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:36175) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1c8xnF-00011O-Bv for qemu-devel@nongnu.org; Mon, 21 Nov 2016 18:14:33 -0500 Received: from pps.filterd (m0098409.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.17/8.16.0.17) with SMTP id uALNDgCq065876 for ; Mon, 21 Nov 2016 18:14:31 -0500 Received: from e23smtp04.au.ibm.com (e23smtp04.au.ibm.com [202.81.31.146]) by mx0a-001b2d01.pphosted.com with ESMTP id 26v5j4w51w-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Mon, 21 Nov 2016 18:14:31 -0500 Received: from localhost by e23smtp04.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 22 Nov 2016 09:14:29 +1000 Date: Tue, 22 Nov 2016 10:14:25 +1100 From: Sam Bobroff References: <29d82af9-1695-f277-ff00-734e6952a1fc@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <29d82af9-1695-f277-ff00-734e6952a1fc@redhat.com> Message-Id: <20161121231425.GA3426@tungsten.ozlabs.ibm.com> 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: Thomas Huth Cc: qemu-devel@nongnu.org, qemu-ppc@nongnu.org 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 > > --- > > > > 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.