From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39746) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Uzux0-0002h0-TP for qemu-devel@nongnu.org; Thu, 18 Jul 2013 16:37:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Uzuwx-0006Mr-Qr for qemu-devel@nongnu.org; Thu, 18 Jul 2013 16:37:22 -0400 Date: Thu, 18 Jul 2013 15:37:05 -0500 From: Scott Wood In-Reply-To: <51E7B516.9060709@adacore.com> (from chouteau@adacore.com on Thu Jul 18 04:27:50 2013) Message-ID: <1374179825.5357.13@snotra> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; delsp=Yes; format=Flowed Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/2] Add Enhanced Three-Speed Ethernet Controller (eTSEC) List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fabien Chouteau Cc: qemu-ppc@nongnu.org, qemu-devel@nongnu.org On 07/18/2013 04:27:50 AM, Fabien Chouteau wrote: > On 07/17/2013 11:02 PM, Scott Wood wrote: > > On 07/17/2013 05:17:06 AM, Fabien Chouteau wrote: > >> On 07/16/2013 07:50 PM, Scott Wood wrote: > >> > On 07/16/2013 10:28:28 AM, Fabien Chouteau wrote: > >> >> On 07/16/2013 04:06 AM, Scott Wood wrote: > >> >> > On 07/10/2013 12:10:02 PM, Fabien Chouteau wrote: > >> >> >> + if (*size =3D=3D etsec->rx_padding) { > >> >> >> + /* The remaining bytes are for padding which is not =20 > actually allocated > >> >> >> + in the buffer */ > >> >> >> + > >> >> >> + rem =3D MIN(etsec->regs[MRBLR].value - bd->length, =20 > etsec->rx_padding); > >> >> >> + > >> >> >> + if (rem > 0) { > >> >> >> + memset(padd, 0x0, sizeof(padd)); > >> >> >> + etsec->rx_padding -=3D rem; > >> >> >> + *size -=3D rem; > >> >> >> + bd->length +=3D rem; > >> >> >> + cpu_physical_memory_write(bufptr, padd, rem); > >> >> >> + } > >> >> >> + } > >> >> > > >> >> > What if *size > 0 && *size < etsec->rx_padding? > >> >> > >> >> I don't think it's possible... > >> > > >> > Maybe throw in an assertion, then? > >> > > >> > I can see how it might not be possible if rx_padding is being =20 > used for padding a short frame, since MRBLR must be a multiple of 64, =20 > but what if it's 4 bytes for CRC? > >> > > >> > >> Can you explain a possible error scenario? > > > > 126 byte packet, no fcb. rx_padding is 4 for CRC. Suppose MRBLR =20 > is 128. Wouldn't *size be 2 here? > > >=20 > Yes, at the end of the function, but then rx_padding is 2 as well. How is rx_padding 2? rx_init_frame() will set it to 4 (since the =20 packet size is not less than 60). The only other place I see that =20 modifies rx_padding is "etsec->rx_padding -=3D rem", but that doesn't =20 happen because the "*size =3D=3D etsec->rx_padding" check happens first. > value of "to_write" will be 126: >=20 > *size =3D etsec->rx_remaining_data + etsec->rx_padding; > =3D 126 + 4; > =3D 130; >=20 > to_write =3D MIN(etsec->rx_fcb_size + *size - etsec->rx_padding, =20 > etsec->regs[MRBLR].value); > =3D MIN(0 + 130 - 4, 128); > =3D MIN(126, 128); > =3D 126; >=20 > So we write the packet in the first part of the BD, and there's 2 =20 > bytes > left in the BD. >=20 > *size -=3D to_write; > =3D 4; > bd->length =3D to_write; > =3D 126; >=20 > So *size =3D=3D etsec->rx_padding (This is expected as the first write > operation can only write data and no padding, I will comment this =20 > fact) >=20 > rem =3D MIN(etsec->regs[MRBLR].value - bd->length, etsec->rx_padding); > =3D MIN(128 - 126, 4); > =3D MIN(2, 4); > =3D 2; >=20 > We write 2 bytes of padding. >=20 > etsec->rx_padding -=3D rem; > =3D 2; > *size -=3D rem; > =3D 2; > bd->length +=3D rem; > =3D 128; >=20 > The BD is full, we will have to put the rest of padding in the next =20 > one. What rest of padding? I thought you said rx_padding was 2 somehow? If =20 that were true, then it would be zero at the end. > >> > Could you at least have a way to diagnose when the guest OS =20 > tries to > >> > use some functionality that you don't support, rather than =20 > silently > >> > doing the wrong thing? > >> > > >> > >> This device is so complex, detecting unsupported features would =20 > take too > >> much work. > > > > I was thinking along the lines of marking registers and bits within =20 > registers as supported (or which are properly no-ops in QEMU) -- and =20 > warning the first time you see a non-default-value write to an =20 > unsupported field or register. It could save a lot of debugging. > > >=20 > I think we'll spend more time implementing this than debugging. =20 > Another > solution is to enable debug output and see which registers are read or > write. I don't think it'd be that hard -- you already have an array of =20 registers. The information could easily go in there, and be checked by =20 common infrastructure. -Scott=