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

[Jason Wang <jasowang@redhat.com>]

On 2016年10月18日 22:10, Dmitry Fleytman wrote:
>> On 17 Oct 2016, at 01:35 AM, Kevin Wolf <mail@kevin-wolf.de> 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 <dmitry@daynix.com>
>
> Thanks for catching this!
>
> ~Dmitry

Applied, thanks.

>
>> Signed-off-by: Kevin Wolf <mail@kevin-wolf.de>
>> ---
>> 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
>>