diff mbox series

[v3,10/14] dp8393x: Pad frames to word or long word boundary

Message ID 49ee52289db03344afc627dd49706ae81eb7d165.1579474761.git.fthain@telegraphics.com.au (mailing list archive)
State New, archived
Headers show
Series Fixes for DP8393X SONIC device emulation | expand

Commit Message

Finn Thain Jan. 19, 2020, 10:59 p.m. UTC 
The existing code has a bug where the Remaining Buffer Word Count (RBWC)
is calculated with a truncating division, which gives the wrong result
for odd-sized packets.

Section 1.4.1 of the datasheet says,

    Once the end of the packet has been reached, the serializer will
    fill out the last word (16-bit mode) or long word (32-bit mode)
    if the last byte did not end on a word or long word boundary
    respectively. The fill byte will be 0FFh.

Implement buffer padding so that buffer limits are correctly enforced.

Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
Tested-by: Laurent Vivier <laurent@vivier.eu>
---
 hw/net/dp8393x.c | 39 ++++++++++++++++++++++++++++-----------
 1 file changed, 28 insertions(+), 11 deletions(-)

Comments

Philippe Mathieu-Daudé Jan. 28, 2020, 11:02 a.m. UTC | #1 
On 1/19/20 11:59 PM, Finn Thain wrote:
> The existing code has a bug where the Remaining Buffer Word Count (RBWC)
> is calculated with a truncating division, which gives the wrong result
> for odd-sized packets.
> 
> Section 1.4.1 of the datasheet says,
> 
>      Once the end of the packet has been reached, the serializer will
>      fill out the last word (16-bit mode) or long word (32-bit mode)
>      if the last byte did not end on a word or long word boundary
>      respectively. The fill byte will be 0FFh.
> 
> Implement buffer padding so that buffer limits are correctly enforced.
> 
> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
> Tested-by: Laurent Vivier <laurent@vivier.eu>
> ---
>   hw/net/dp8393x.c | 39 ++++++++++++++++++++++++++++-----------
>   1 file changed, 28 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index b052e2c854..13513986f0 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -766,16 +766,23 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
>       dp8393xState *s = qemu_get_nic_opaque(nc);
>       int packet_type;
>       uint32_t available, address;
> -    int width, rx_len = pkt_size;
> +    int width, rx_len, padded_len;
>       uint32_t checksum;
>       int size;
>   
> -    width = (s->regs[SONIC_DCR] & SONIC_DCR_DW) ? 2 : 1;
> -
>       s->regs[SONIC_RCR] &= ~(SONIC_RCR_PRX | SONIC_RCR_LBK | SONIC_RCR_FAER |
>           SONIC_RCR_CRCR | SONIC_RCR_LPKT | SONIC_RCR_BC | SONIC_RCR_MC);
>   
> -    if (pkt_size + 4 > dp8393x_rbwc(s) * 2) {
> +    rx_len = pkt_size + sizeof(checksum);
> +    if (s->regs[SONIC_DCR] & SONIC_DCR_DW) {
> +        width = 2;
> +        padded_len = ((rx_len - 1) | 3) + 1;
> +    } else {
> +        width = 1;
> +        padded_len = ((rx_len - 1) | 1) + 1;
> +    }
> +
> +    if (padded_len > dp8393x_rbwc(s) * 2) {
>           DPRINTF("oversize packet, pkt_size is %d\n", pkt_size);
>           s->regs[SONIC_ISR] |= SONIC_ISR_RBAE;
>           dp8393x_update_irq(s);
> @@ -810,22 +817,32 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
>       s->regs[SONIC_TRBA0] = s->regs[SONIC_CRBA0];
>   
>       /* Calculate the ethernet checksum */
> -    checksum = cpu_to_le32(crc32(0, buf, rx_len));
> +    checksum = cpu_to_le32(crc32(0, buf, pkt_size));
>   
>       /* Put packet into RBA */
>       DPRINTF("Receive packet at %08x\n", dp8393x_crba(s));
>       address = dp8393x_crba(s);
>       address_space_rw(&s->as, address,
> -        MEMTXATTRS_UNSPECIFIED, (uint8_t *)buf, rx_len, 1);
> -    address += rx_len;
> +        MEMTXATTRS_UNSPECIFIED, (uint8_t *)buf, pkt_size, 1);
> +    address += pkt_size;
> +
> +    /* Put frame checksum into RBA */
>       address_space_rw(&s->as, address,
> -        MEMTXATTRS_UNSPECIFIED, (uint8_t *)&checksum, 4, 1);
> -    address += 4;
> -    rx_len += 4;
> +        MEMTXATTRS_UNSPECIFIED, (uint8_t *)&checksum, sizeof(checksum), 1);
> +    address += sizeof(checksum);
> +
> +    /* Pad short packets to keep pointers aligned */
> +    if (rx_len < padded_len) {
> +        size = padded_len - rx_len;
> +        address_space_rw(&s->as, address, MEMTXATTRS_UNSPECIFIED,
> +            (uint8_t *)"\xFF\xFF\xFF", size, 1);
> +        address += size;
> +    }
> +
>       s->regs[SONIC_CRBA1] = address >> 16;
>       s->regs[SONIC_CRBA0] = address & 0xffff;
>       available = dp8393x_rbwc(s);
> -    available -= rx_len / 2;
> +    available -= padded_len >> 1;
>       s->regs[SONIC_RBWC1] = available >> 16;
>       s->regs[SONIC_RBWC0] = available & 0xffff;
>   
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
diff mbox series

Patch

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index b052e2c854..13513986f0 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -766,16 +766,23 @@  static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
     dp8393xState *s = qemu_get_nic_opaque(nc);
     int packet_type;
     uint32_t available, address;
-    int width, rx_len = pkt_size;
+    int width, rx_len, padded_len;
     uint32_t checksum;
     int size;
 
-    width = (s->regs[SONIC_DCR] & SONIC_DCR_DW) ? 2 : 1;
-
     s->regs[SONIC_RCR] &= ~(SONIC_RCR_PRX | SONIC_RCR_LBK | SONIC_RCR_FAER |
         SONIC_RCR_CRCR | SONIC_RCR_LPKT | SONIC_RCR_BC | SONIC_RCR_MC);
 
-    if (pkt_size + 4 > dp8393x_rbwc(s) * 2) {
+    rx_len = pkt_size + sizeof(checksum);
+    if (s->regs[SONIC_DCR] & SONIC_DCR_DW) {
+        width = 2;
+        padded_len = ((rx_len - 1) | 3) + 1;
+    } else {
+        width = 1;
+        padded_len = ((rx_len - 1) | 1) + 1;
+    }
+
+    if (padded_len > dp8393x_rbwc(s) * 2) {
         DPRINTF("oversize packet, pkt_size is %d\n", pkt_size);
         s->regs[SONIC_ISR] |= SONIC_ISR_RBAE;
         dp8393x_update_irq(s);
@@ -810,22 +817,32 @@  static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
     s->regs[SONIC_TRBA0] = s->regs[SONIC_CRBA0];
 
     /* Calculate the ethernet checksum */
-    checksum = cpu_to_le32(crc32(0, buf, rx_len));
+    checksum = cpu_to_le32(crc32(0, buf, pkt_size));
 
     /* Put packet into RBA */
     DPRINTF("Receive packet at %08x\n", dp8393x_crba(s));
     address = dp8393x_crba(s);
     address_space_rw(&s->as, address,
-        MEMTXATTRS_UNSPECIFIED, (uint8_t *)buf, rx_len, 1);
-    address += rx_len;
+        MEMTXATTRS_UNSPECIFIED, (uint8_t *)buf, pkt_size, 1);
+    address += pkt_size;
+
+    /* Put frame checksum into RBA */
     address_space_rw(&s->as, address,
-        MEMTXATTRS_UNSPECIFIED, (uint8_t *)&checksum, 4, 1);
-    address += 4;
-    rx_len += 4;
+        MEMTXATTRS_UNSPECIFIED, (uint8_t *)&checksum, sizeof(checksum), 1);
+    address += sizeof(checksum);
+
+    /* Pad short packets to keep pointers aligned */
+    if (rx_len < padded_len) {
+        size = padded_len - rx_len;
+        address_space_rw(&s->as, address, MEMTXATTRS_UNSPECIFIED,
+            (uint8_t *)"\xFF\xFF\xFF", size, 1);
+        address += size;
+    }
+
     s->regs[SONIC_CRBA1] = address >> 16;
     s->regs[SONIC_CRBA0] = address & 0xffff;
     available = dp8393x_rbwc(s);
-    available -= rx_len / 2;
+    available -= padded_len >> 1;
     s->regs[SONIC_RBWC1] = available >> 16;
     s->regs[SONIC_RBWC0] = available & 0xffff;