[Qemu-devel] [PATCH] e1000: clear EOP for multi-buffer descriptors

[Qemu-devel] [PATCH] e1000: clear EOP for multi-buffer descriptors

[Michael S. Tsirkin]

The e1000 spec says: 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 is not a hardware requirement (moving the
hardware tail pointer is), but is necessary for performing an in–memory
scan.

Thus the guest does not have to clear the status byte.  In case it
doesn't we need to clear EOP for all descriptors
except the last.  While I don't know of any such guests,
it's probably a good idea to stick to the spec.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reported-by: Juan Quintela <quintela@redhat.com>

---
 hw/e1000.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/hw/e1000.c b/hw/e1000.c
index 3427ff3..7853c12 100644
--- a/hw/e1000.c
+++ b/hw/e1000.c
@@ -694,6 +694,9 @@ e1000_receive(VLANClientState *nc, const uint8_t *buf, size_t size)
                 desc.length = cpu_to_le16(desc_size + fcs_len(s));
                 desc.status |= E1000_RXD_STAT_EOP | E1000_RXD_STAT_IXSM;
             } else {
+                /* Guest zeroing out status is not a hardware requirement.
+                   Clear EOP in case guest didn't do it. */
+                desc.status &= ~E1000_RXD_STAT_EOP;
                 desc.length = cpu_to_le16(desc_size);
             }
         } else { // as per intel docs; skip descriptors with null buf addr
-- 
1.7.3.2.91.g446ac

[Qemu-devel] Re: [PATCH] e1000: clear EOP for multi-buffer descriptors

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> The e1000 spec says: 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 is not a hardware requirement (moving the
> hardware tail pointer is), but is necessary for performing an in–memory
> scan.
>
> Thus the guest does not have to clear the status byte.  In case it
> doesn't we need to clear EOP for all descriptors
> except the last.  While I don't know of any such guests,
> it's probably a good idea to stick to the spec.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Reported-by: Juan Quintela <quintela@redhat.com>

reviewed-by: Juan Quintela <quintela@redhat.com>

[Qemu-devel] Re: [PATCH] e1000: clear EOP for multi-buffer descriptors

[Stefan Hajnoczi]

On Mon, Feb 07, 2011 at 02:30:26PM +0200, Michael S. Tsirkin wrote:
> The e1000 spec says: 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 is not a hardware requirement (moving the
> hardware tail pointer is), but is necessary for performing an in–memory
> scan.
> 
> Thus the guest does not have to clear the status byte.  In case it
> doesn't we need to clear EOP for all descriptors
> except the last.  While I don't know of any such guests,
> it's probably a good idea to stick to the spec.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Reported-by: Juan Quintela <quintela@redhat.com>
> 
> ---
>  hw/e1000.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)

This makes sense: if the guest didn't clear the end-of-packet bit but
we're receiving a multi-buffer packet, clear EOP for all but the last
descriptor.

Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

[Qemu-devel] Re: [PATCH] e1000: clear EOP for multi-buffer descriptors

[Alex Williamson]

On Mon, 2011-02-07 at 14:30 +0200, Michael S. Tsirkin wrote:
> The e1000 spec says: 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 is not a hardware requirement (moving the
> hardware tail pointer is), but is necessary for performing an in–memory
> scan.
> 
> Thus the guest does not have to clear the status byte.  In case it
> doesn't we need to clear EOP for all descriptors
> except the last.  While I don't know of any such guests,
> it's probably a good idea to stick to the spec.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Reported-by: Juan Quintela <quintela@redhat.com>
> 
> ---
>  hw/e1000.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/e1000.c b/hw/e1000.c
> index 3427ff3..7853c12 100644
> --- a/hw/e1000.c
> +++ b/hw/e1000.c
> @@ -694,6 +694,9 @@ e1000_receive(VLANClientState *nc, const uint8_t *buf, size_t size)
>                  desc.length = cpu_to_le16(desc_size + fcs_len(s));
>                  desc.status |= E1000_RXD_STAT_EOP | E1000_RXD_STAT_IXSM;
>              } else {
> +                /* Guest zeroing out status is not a hardware requirement.
> +                   Clear EOP in case guest didn't do it. */
> +                desc.status &= ~E1000_RXD_STAT_EOP;
>                  desc.length = cpu_to_le16(desc_size);
>              }
>          } else { // as per intel docs; skip descriptors with null buf addr

Acked-by: Alex Williamson <alex.williamson@redhat.com>

[Qemu-devel] Re: [PATCH] e1000: clear EOP for multi-buffer descriptors

[Alex Williamson]

On Mon, 2011-02-07 at 14:30 +0200, Michael S. Tsirkin wrote:
> The e1000 spec says: 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 is not a hardware requirement (moving the
> hardware tail pointer is), but is necessary for performing an in–memory
> scan.
> 
> Thus the guest does not have to clear the status byte.  In case it
> doesn't we need to clear EOP for all descriptors
> except the last.  While I don't know of any such guests,
> it's probably a good idea to stick to the spec.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Reported-by: Juan Quintela <quintela@redhat.com>
> 
> ---
>  hw/e1000.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/e1000.c b/hw/e1000.c
> index 3427ff3..7853c12 100644
> --- a/hw/e1000.c
> +++ b/hw/e1000.c
> @@ -694,6 +694,9 @@ e1000_receive(VLANClientState *nc, const uint8_t *buf, size_t size)
>                  desc.length = cpu_to_le16(desc_size + fcs_len(s));
>                  desc.status |= E1000_RXD_STAT_EOP | E1000_RXD_STAT_IXSM;
>              } else {
> +                /* Guest zeroing out status is not a hardware requirement.
> +                   Clear EOP in case guest didn't do it. */
> +                desc.status &= ~E1000_RXD_STAT_EOP;
>                  desc.length = cpu_to_le16(desc_size);
>              }
>          } else { // as per intel docs; skip descriptors with null buf addr

Acked-by: Alex Williamson <alex.williamson@redhat.com>