e1000: clear EOP for multi-buffer descriptors
PATCH 1/1 e1000: clear EOP for multi-buffer descriptors
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.
--- hw/e1000.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
hw/e1000.c +3 -0
@@ -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
Signature · 1 line
1.7.3.2.91.g446ac
Quoted 7 lines · 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.
Quoted 7 lines
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 <[email protected]>Reported-by Juan Quintela <[email protected]>
Quoted 12 lines · 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.
Quoted 6 lines
Signed-off-by Michael S. Tsirkin <[email protected]>Reported-by Juan Quintela <[email protected]>--- 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.
Quoted 24 lines · 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 <[email protected]>Reported-by Juan Quintela <[email protected]>--- hw/e1000.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)hw/e1000.c +0
-0
hw/e1000.c e1000_receive(VLANClientState *nc, const uint8_t *buf, size_t size) · line 702
Quoted 9 lines
hw/e1000.c +3
-0@@ -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
Quoted 24 lines · 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 <[email protected]>Reported-by Juan Quintela <[email protected]>--- hw/e1000.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)hw/e1000.c +0
-0
hw/e1000.c e1000_receive(VLANClientState *nc, const uint8_t *buf, size_t size) · line 702
Quoted 9 lines
hw/e1000.c +3
-0@@ -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