Re: [Qemu-devel] [PATCH] e1000e: Don't zero out buffer address in rx descriptor

[Dmitry Fleytman]

> On 17 Oct 2016, at 01:35 AM, Kevin Wolf <address@hidden> 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 only
> 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 <address@hidden>

Thanks for catching this!

~Dmitry

> 
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
> 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, uint8_t 
> *desc,
> 
>     struct e1000_rx_desc *d = (struct e1000_rx_desc *) desc;
> 
> -    memset(d, 0, sizeof(*d));
> -
>     assert(!rss_info->enabled);
> 
>     d->length = cpu_to_le16(length);
> +    d->csum = 0;
> 
>     e1000e_build_rx_metadata(core, pkt, pkt != NULL,
>                              rss_info,
> @@ -1293,6 +1292,7 @@ e1000e_write_lgcy_rx_descr(E1000ECore *core, uint8_t 
> *desc,
>                              &d->special);
>     d->errors = (uint8_t) (le32_to_cpu(status_flags) >> 24);
>     d->status = (uint8_t) le32_to_cpu(status_flags);
> +    d->special = 0;
> }
> 
> static inline void
> @@ -1303,7 +1303,7 @@ e1000e_write_ext_rx_descr(E1000ECore *core, uint8_t 
> *desc,
> {
>     union e1000_rx_desc_extended *d = (union e1000_rx_desc_extended *) desc;
> 
> -    memset(d, 0, sizeof(*d));
> +    memset(&d->wb, 0, sizeof(d->wb));
> 
>     d->wb.upper.length = 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 =
>         (union e1000_rx_desc_packet_split *) desc;
> 
> -    memset(d, 0, sizeof(*d));
> +    memset(&d->wb, 0, sizeof(d->wb));
> 
>     d->wb.middle.length0 = cpu_to_le16((*written)[0]);
> 
> -- 
> 2.1.4
>