From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38868) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bxP6n-0006hO-6T for qemu-devel@nongnu.org; Thu, 20 Oct 2016 21:58:58 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bxP6m-0006kw-6g for qemu-devel@nongnu.org; Thu, 20 Oct 2016 21:58:57 -0400 Received: from mx1.redhat.com ([209.132.183.28]:36768) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1bxP6l-0006kX-V3 for qemu-devel@nongnu.org; Thu, 20 Oct 2016 21:58:56 -0400 References: <1476657307-26165-1-git-send-email-mail@kevin-wolf.de> From: Jason Wang Message-ID: Date: Fri, 21 Oct 2016 09:58:48 +0800 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] e1000e: Don't zero out buffer address in rx descriptor List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Dmitry Fleytman , Kevin Wolf Cc: kwolf@redhat.com, qemu-devel@nongnu.org On 2016=E5=B9=B410=E6=9C=8818=E6=97=A5 22:10, Dmitry Fleytman wrote: >> On 17 Oct 2016, at 01:35 AM, Kevin Wolf wrote: >> >> The e1000e emulation zeroes out any used rx descriptor and then writes= a >> completely newly constructed value there. By doing this, it doesn't on= ly >> update the write-back area of the descriptors (as it's supposed to do)= , >> but it also clears the buffer address, which real hardware doesn't do. >> >> The spec explicitly mentions in chapter 7.1.8 that it is valid for a >> driver to reuse a descriptor and only update the status field while >> doing so, i.e. reusing the old buffer address: >> >> If software statically allocates buffers, and uses memory read to >> check for completed descriptors, it simply has to zero the status >> byte in the descriptor to make it ready for reuse by hardware. >> >> This patch fixes the behaviour to leave the buffer address in >> descriptors unchanged even after the descriptor has been used. > Hi Kevin, > > Reviewed-by: Dmitry Fleytman > > Thanks for catching this! > > ~Dmitry Applied, thanks. > >> Signed-off-by: Kevin Wolf >> --- >> hw/net/e1000e_core.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c >> index 9fa4116..a5ca97d 100644 >> --- a/hw/net/e1000e_core.c >> +++ b/hw/net/e1000e_core.c >> @@ -1280,11 +1280,10 @@ e1000e_write_lgcy_rx_descr(E1000ECore *core, u= int8_t *desc, >> >> struct e1000_rx_desc *d =3D (struct e1000_rx_desc *) desc; >> >> - memset(d, 0, sizeof(*d)); >> - >> assert(!rss_info->enabled); >> >> d->length =3D cpu_to_le16(length); >> + d->csum =3D 0; >> >> e1000e_build_rx_metadata(core, pkt, pkt !=3D NULL, >> rss_info, >> @@ -1293,6 +1292,7 @@ e1000e_write_lgcy_rx_descr(E1000ECore *core, uin= t8_t *desc, >> &d->special); >> d->errors =3D (uint8_t) (le32_to_cpu(status_flags) >> 24); >> d->status =3D (uint8_t) le32_to_cpu(status_flags); >> + d->special =3D 0; >> } >> >> static inline void >> @@ -1303,7 +1303,7 @@ e1000e_write_ext_rx_descr(E1000ECore *core, uint= 8_t *desc, >> { >> union e1000_rx_desc_extended *d =3D (union e1000_rx_desc_extended= *) desc; >> >> - memset(d, 0, sizeof(*d)); >> + memset(&d->wb, 0, sizeof(d->wb)); >> >> d->wb.upper.length =3D cpu_to_le16(length); >> >> @@ -1327,7 +1327,7 @@ e1000e_write_ps_rx_descr(E1000ECore *core, uint8= _t *desc, >> union e1000_rx_desc_packet_split *d =3D >> (union e1000_rx_desc_packet_split *) desc; >> >> - memset(d, 0, sizeof(*d)); >> + memset(&d->wb, 0, sizeof(d->wb)); >> >> d->wb.middle.length0 =3D cpu_to_le16((*written)[0]); >> >> --=20 >> 2.1.4 >>