[PATCH 0/3] dp8393x: fix problems detected with Quadra 800 machine

[PATCH 0/3] dp8393x: fix problems detected with Quadra 800 machine

[Laurent Vivier]

This series starts with a cleanup of the DMA buffer, moving
it from the stack to the state structure.

The following patch allows to negociate the IP address with
the DHCP server.

The last one fixes the buffer exhaustion case.

With this series Q800 networking card is fully functionnal.

Laurent Vivier (3):
  dp8393x: put the DMA buffer in the state structure
  dp8393x: fix dp8393x_receive()
  dp8393x: fix receiving buffer exhaustion

 hw/net/dp8393x.c | 136 +++++++++++++++++++++++------------------------
 1 file changed, 66 insertions(+), 70 deletions(-)

-- 
2.21.0

[PATCH 1/3] dp8393x: put the DMA buffer in the state structure

[Laurent Vivier]

Move it from the stack.

It's only 24 bytes, and this simplifies the dp8393x_get()/
dp8393x_put() interface.

Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
 hw/net/dp8393x.c | 105 ++++++++++++++++++++++-------------------------
 1 file changed, 50 insertions(+), 55 deletions(-)

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 693e244ce6..85d3f3788e 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -171,6 +171,7 @@ typedef struct dp8393xState {
 
     /* Temporaries */
     uint8_t tx_buffer[0x10000];
+    uint16_t data[12];
     int loopback_packet;
 
     /* Memory access */
@@ -224,26 +225,25 @@ static uint32_t dp8393x_wt(dp8393xState *s)
     return s->regs[SONIC_WT1] << 16 | s->regs[SONIC_WT0];
 }
 
-static uint16_t dp8393x_get(dp8393xState *s, int width, uint16_t *base,
-                            int offset)
+static uint16_t dp8393x_get(dp8393xState *s, int width, int offset)
 {
     uint16_t val;
 
     if (s->big_endian) {
-        val = be16_to_cpu(base[offset * width + width - 1]);
+        val = be16_to_cpu(s->data[offset * width + width - 1]);
     } else {
-        val = le16_to_cpu(base[offset * width]);
+        val = le16_to_cpu(s->data[offset * width]);
     }
     return val;
 }
 
-static void dp8393x_put(dp8393xState *s, int width, uint16_t *base, int offset,
+static void dp8393x_put(dp8393xState *s, int width, int offset,
                         uint16_t val)
 {
     if (s->big_endian) {
-        base[offset * width + width - 1] = cpu_to_be16(val);
+        s->data[offset * width + width - 1] = cpu_to_be16(val);
     } else {
-        base[offset * width] = cpu_to_le16(val);
+        s->data[offset * width] = cpu_to_le16(val);
     }
 }
 
@@ -267,7 +267,6 @@ static void dp8393x_update_irq(dp8393xState *s)
 
 static void dp8393x_do_load_cam(dp8393xState *s)
 {
-    uint16_t data[8];
     int width, size;
     uint16_t index = 0;
 
@@ -277,13 +276,13 @@ static void dp8393x_do_load_cam(dp8393xState *s)
     while (s->regs[SONIC_CDC] & 0x1f) {
         /* Fill current entry */
         address_space_rw(&s->as, dp8393x_cdp(s),
-            MEMTXATTRS_UNSPECIFIED, (uint8_t *)data, size, 0);
-        s->cam[index][0] = dp8393x_get(s, width, data, 1) & 0xff;
-        s->cam[index][1] = dp8393x_get(s, width, data, 1) >> 8;
-        s->cam[index][2] = dp8393x_get(s, width, data, 2) & 0xff;
-        s->cam[index][3] = dp8393x_get(s, width, data, 2) >> 8;
-        s->cam[index][4] = dp8393x_get(s, width, data, 3) & 0xff;
-        s->cam[index][5] = dp8393x_get(s, width, data, 3) >> 8;
+            MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
+        s->cam[index][0] = dp8393x_get(s, width, 1) & 0xff;
+        s->cam[index][1] = dp8393x_get(s, width, 1) >> 8;
+        s->cam[index][2] = dp8393x_get(s, width, 2) & 0xff;
+        s->cam[index][3] = dp8393x_get(s, width, 2) >> 8;
+        s->cam[index][4] = dp8393x_get(s, width, 3) & 0xff;
+        s->cam[index][5] = dp8393x_get(s, width, 3) >> 8;
         DPRINTF("load cam[%d] with %02x%02x%02x%02x%02x%02x\n", index,
             s->cam[index][0], s->cam[index][1], s->cam[index][2],
             s->cam[index][3], s->cam[index][4], s->cam[index][5]);
@@ -295,8 +294,8 @@ static void dp8393x_do_load_cam(dp8393xState *s)
 
     /* Read CAM enable */
     address_space_rw(&s->as, dp8393x_cdp(s),
-        MEMTXATTRS_UNSPECIFIED, (uint8_t *)data, size, 0);
-    s->regs[SONIC_CE] = dp8393x_get(s, width, data, 0);
+        MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
+    s->regs[SONIC_CE] = dp8393x_get(s, width, 0);
     DPRINTF("load cam done. cam enable mask 0x%04x\n", s->regs[SONIC_CE]);
 
     /* Done */
@@ -307,20 +306,19 @@ static void dp8393x_do_load_cam(dp8393xState *s)
 
 static void dp8393x_do_read_rra(dp8393xState *s)
 {
-    uint16_t data[8];
     int width, size;
 
     /* Read memory */
     width = (s->regs[SONIC_DCR] & SONIC_DCR_DW) ? 2 : 1;
     size = sizeof(uint16_t) * 4 * width;
     address_space_rw(&s->as, dp8393x_rrp(s),
-        MEMTXATTRS_UNSPECIFIED, (uint8_t *)data, size, 0);
+        MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
 
     /* Update SONIC registers */
-    s->regs[SONIC_CRBA0] = dp8393x_get(s, width, data, 0);
-    s->regs[SONIC_CRBA1] = dp8393x_get(s, width, data, 1);
-    s->regs[SONIC_RBWC0] = dp8393x_get(s, width, data, 2);
-    s->regs[SONIC_RBWC1] = dp8393x_get(s, width, data, 3);
+    s->regs[SONIC_CRBA0] = dp8393x_get(s, width, 0);
+    s->regs[SONIC_CRBA1] = dp8393x_get(s, width, 1);
+    s->regs[SONIC_RBWC0] = dp8393x_get(s, width, 2);
+    s->regs[SONIC_RBWC1] = dp8393x_get(s, width, 3);
     DPRINTF("CRBA0/1: 0x%04x/0x%04x, RBWC0/1: 0x%04x/0x%04x\n",
         s->regs[SONIC_CRBA0], s->regs[SONIC_CRBA1],
         s->regs[SONIC_RBWC0], s->regs[SONIC_RBWC1]);
@@ -417,7 +415,6 @@ static void dp8393x_do_receiver_disable(dp8393xState *s)
 static void dp8393x_do_transmit_packets(dp8393xState *s)
 {
     NetClientState *nc = qemu_get_queue(s->nic);
-    uint16_t data[12];
     int width, size;
     int tx_len, len;
     uint16_t i;
@@ -429,18 +426,17 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
         size = sizeof(uint16_t) * 6 * width;
         s->regs[SONIC_TTDA] = s->regs[SONIC_CTDA];
         DPRINTF("Transmit packet at %08x\n", dp8393x_ttda(s));
-        address_space_rw(&s->as,
-            dp8393x_ttda(s) + sizeof(uint16_t) * width,
-            MEMTXATTRS_UNSPECIFIED, (uint8_t *)data, size, 0);
+        address_space_rw(&s->as, dp8393x_ttda(s) + sizeof(uint16_t) * width,
+            MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
         tx_len = 0;
 
         /* Update registers */
-        s->regs[SONIC_TCR] = dp8393x_get(s, width, data, 0) & 0xf000;
-        s->regs[SONIC_TPS] = dp8393x_get(s, width, data, 1);
-        s->regs[SONIC_TFC] = dp8393x_get(s, width, data, 2);
-        s->regs[SONIC_TSA0] = dp8393x_get(s, width, data, 3);
-        s->regs[SONIC_TSA1] = dp8393x_get(s, width, data, 4);
-        s->regs[SONIC_TFS] = dp8393x_get(s, width, data, 5);
+        s->regs[SONIC_TCR] = dp8393x_get(s, width, 0) & 0xf000;
+        s->regs[SONIC_TPS] = dp8393x_get(s, width, 1);
+        s->regs[SONIC_TFC] = dp8393x_get(s, width, 2);
+        s->regs[SONIC_TSA0] = dp8393x_get(s, width, 3);
+        s->regs[SONIC_TSA1] = dp8393x_get(s, width, 4);
+        s->regs[SONIC_TFS] = dp8393x_get(s, width, 5);
 
         /* Handle programmable interrupt */
         if (s->regs[SONIC_TCR] & SONIC_TCR_PINT) {
@@ -465,10 +461,10 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
                 size = sizeof(uint16_t) * 3 * width;
                 address_space_rw(&s->as,
                     dp8393x_ttda(s) + sizeof(uint16_t) * (4 + 3 * i) * width,
-                    MEMTXATTRS_UNSPECIFIED, (uint8_t *)data, size, 0);
-                s->regs[SONIC_TSA0] = dp8393x_get(s, width, data, 0);
-                s->regs[SONIC_TSA1] = dp8393x_get(s, width, data, 1);
-                s->regs[SONIC_TFS] = dp8393x_get(s, width, data, 2);
+                    MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
+                s->regs[SONIC_TSA0] = dp8393x_get(s, width, 0);
+                s->regs[SONIC_TSA1] = dp8393x_get(s, width, 1);
+                s->regs[SONIC_TFS] = dp8393x_get(s, width, 2);
             }
         }
 
@@ -495,12 +491,12 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
         s->regs[SONIC_TCR] |= SONIC_TCR_PTX;
 
         /* Write status */
-        dp8393x_put(s, width, data, 0,
+        dp8393x_put(s, width, 0,
                     s->regs[SONIC_TCR] & 0x0fff); /* status */
         size = sizeof(uint16_t) * width;
         address_space_rw(&s->as,
             dp8393x_ttda(s),
-            MEMTXATTRS_UNSPECIFIED, (uint8_t *)data, size, 1);
+            MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 1);
 
         if (!(s->regs[SONIC_CR] & SONIC_CR_HTX)) {
             /* Read footer of packet */
@@ -509,9 +505,9 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
                 dp8393x_ttda(s) +
                              sizeof(uint16_t) *
                              (4 + 3 * s->regs[SONIC_TFC]) * width,
-                MEMTXATTRS_UNSPECIFIED, (uint8_t *)data, size, 0);
-            s->regs[SONIC_CTDA] = dp8393x_get(s, width, data, 0) & ~0x1;
-            if (dp8393x_get(s, width, data, 0) & 0x1) {
+                MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
+            s->regs[SONIC_CTDA] = dp8393x_get(s, width, 0) & ~0x1;
+            if (dp8393x_get(s, width, 0) & 0x1) {
                 /* EOL detected */
                 break;
             }
@@ -748,7 +744,6 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
                                size_t size)
 {
     dp8393xState *s = qemu_get_nic_opaque(nc);
-    uint16_t data[10];
     int packet_type;
     uint32_t available, address;
     int width, rx_len = size;
@@ -773,8 +768,8 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
         size = sizeof(uint16_t) * 1 * width;
         address = dp8393x_crda(s) + sizeof(uint16_t) * 5 * width;
         address_space_rw(&s->as, address, MEMTXATTRS_UNSPECIFIED,
-                         (uint8_t *)data, size, 0);
-        if (dp8393x_get(s, width, data, 0) & 0x1) {
+                         (uint8_t *)s->data, size, 0);
+        if (dp8393x_get(s, width, 0) & 0x1) {
             /* Still EOL ; stop reception */
             return -1;
         } else {
@@ -818,27 +813,27 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
 
     /* Write status to memory */
     DPRINTF("Write status at %08x\n", dp8393x_crda(s));
-    dp8393x_put(s, width, data, 0, s->regs[SONIC_RCR]); /* status */
-    dp8393x_put(s, width, data, 1, rx_len); /* byte count */
-    dp8393x_put(s, width, data, 2, s->regs[SONIC_TRBA0]); /* pkt_ptr0 */
-    dp8393x_put(s, width, data, 3, s->regs[SONIC_TRBA1]); /* pkt_ptr1 */
-    dp8393x_put(s, width, data, 4, s->regs[SONIC_RSC]); /* seq_no */
+    dp8393x_put(s, width, 0, s->regs[SONIC_RCR]); /* status */
+    dp8393x_put(s, width, 1, rx_len); /* byte count */
+    dp8393x_put(s, width, 2, s->regs[SONIC_TRBA0]); /* pkt_ptr0 */
+    dp8393x_put(s, width, 3, s->regs[SONIC_TRBA1]); /* pkt_ptr1 */
+    dp8393x_put(s, width, 4, s->regs[SONIC_RSC]); /* seq_no */
     size = sizeof(uint16_t) * 5 * width;
     address_space_rw(&s->as, dp8393x_crda(s),
-        MEMTXATTRS_UNSPECIFIED, (uint8_t *)data, size, 1);
+        MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 1);
 
     /* Move to next descriptor */
     size = sizeof(uint16_t) * width;
     address_space_rw(&s->as, dp8393x_crda(s) + sizeof(uint16_t) * 5 * width,
-        MEMTXATTRS_UNSPECIFIED, (uint8_t *)data, size, 0);
-    s->regs[SONIC_LLFA] = dp8393x_get(s, width, data, 0);
+        MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
+    s->regs[SONIC_LLFA] = dp8393x_get(s, width, 0);
     if (s->regs[SONIC_LLFA] & 0x1) {
         /* EOL detected */
         s->regs[SONIC_ISR] |= SONIC_ISR_RDE;
     } else {
-        dp8393x_put(s, width, data, 0, 0); /* in_use */
+        dp8393x_put(s, width, 0, 0); /* in_use */
         address_space_rw(&s->as, dp8393x_crda(s) + sizeof(uint16_t) * 6 * width,
-            MEMTXATTRS_UNSPECIFIED, (uint8_t *)data, sizeof(uint16_t), 1);
+            MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, sizeof(uint16_t), 1);
         s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
         s->regs[SONIC_ISR] |= SONIC_ISR_PKTRX;
         s->regs[SONIC_RSC] = (s->regs[SONIC_RSC] & 0xff00) | (((s->regs[SONIC_RSC] & 0x00ff) + 1) & 0x00ff);
-- 
2.21.0

[PATCH 2/3] dp8393x: fix dp8393x_receive()

[Laurent Vivier]

address_space_rw() access size must be multiplied by the width.

This fixes DHCP for Q800 guest.

Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
 hw/net/dp8393x.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 85d3f3788e..b8c4473f99 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -833,7 +833,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
     } else {
         dp8393x_put(s, width, 0, 0); /* in_use */
         address_space_rw(&s->as, dp8393x_crda(s) + sizeof(uint16_t) * 6 * width,
-            MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, sizeof(uint16_t), 1);
+            MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 1);
         s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
         s->regs[SONIC_ISR] |= SONIC_ISR_PKTRX;
         s->regs[SONIC_RSC] = (s->regs[SONIC_RSC] & 0xff00) | (((s->regs[SONIC_RSC] & 0x00ff) + 1) & 0x00ff);
-- 
2.21.0

[PATCH 3/3] dp8393x: fix receiving buffer exhaustion

[Laurent Vivier]

The card is not able to exit from exhaustion state, because
while the drive consumes the buffers, the RRP is incremented
(when the driver clears the ISR RBE bit), so it stays equal
to RWP, and while RRP == RWP, the card thinks it is always
in exhaustion state. So the driver consumes all the buffers,
but the card cannot receive new ones.

This patch fixes the problem by not incrementing RRP when
the driver clears the ISR RBE bit.

Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
 hw/net/dp8393x.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index b8c4473f99..21deb32456 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -304,7 +304,7 @@ static void dp8393x_do_load_cam(dp8393xState *s)
     dp8393x_update_irq(s);
 }
 
-static void dp8393x_do_read_rra(dp8393xState *s)
+static void dp8393x_do_read_rra(dp8393xState *s, int next)
 {
     int width, size;
 
@@ -323,19 +323,20 @@ static void dp8393x_do_read_rra(dp8393xState *s)
         s->regs[SONIC_CRBA0], s->regs[SONIC_CRBA1],
         s->regs[SONIC_RBWC0], s->regs[SONIC_RBWC1]);
 
-    /* Go to next entry */
-    s->regs[SONIC_RRP] += size;
+    if (next) {
+        /* Go to next entry */
+        s->regs[SONIC_RRP] += size;
 
-    /* Handle wrap */
-    if (s->regs[SONIC_RRP] == s->regs[SONIC_REA]) {
-        s->regs[SONIC_RRP] = s->regs[SONIC_RSA];
-    }
+        /* Handle wrap */
+        if (s->regs[SONIC_RRP] == s->regs[SONIC_REA]) {
+            s->regs[SONIC_RRP] = s->regs[SONIC_RSA];
+        }
 
-    /* Check resource exhaustion */
-    if (s->regs[SONIC_RRP] == s->regs[SONIC_RWP])
-    {
-        s->regs[SONIC_ISR] |= SONIC_ISR_RBE;
-        dp8393x_update_irq(s);
+        /* Check resource exhaustion */
+        if (s->regs[SONIC_RRP] == s->regs[SONIC_RWP]) {
+            s->regs[SONIC_ISR] |= SONIC_ISR_RBE;
+            dp8393x_update_irq(s);
+        }
     }
 
     /* Done */
@@ -549,7 +550,7 @@ static void dp8393x_do_command(dp8393xState *s, uint16_t command)
     if (command & SONIC_CR_RST)
         dp8393x_do_software_reset(s);
     if (command & SONIC_CR_RRRA)
-        dp8393x_do_read_rra(s);
+        dp8393x_do_read_rra(s, 1);
     if (command & SONIC_CR_LCAM)
         dp8393x_do_load_cam(s);
 }
@@ -640,7 +641,7 @@ static void dp8393x_write(void *opaque, hwaddr addr, uint64_t data,
             data &= s->regs[reg];
             s->regs[reg] &= ~data;
             if (data & SONIC_ISR_RBE) {
-                dp8393x_do_read_rra(s);
+                dp8393x_do_read_rra(s, 0);
             }
             dp8393x_update_irq(s);
             if (dp8393x_can_receive(s->nic->ncs)) {
@@ -840,7 +841,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
 
         if (s->regs[SONIC_RCR] & SONIC_RCR_LPKT) {
             /* Read next RRA */
-            dp8393x_do_read_rra(s);
+            dp8393x_do_read_rra(s, 1);
         }
     }
 
-- 
2.21.0

Re: [PATCH 2/3] dp8393x: fix dp8393x_receive()

[Philippe Mathieu-Daudé]

On 11/2/19 6:15 PM, Laurent Vivier wrote:
> address_space_rw() access size must be multiplied by the width.
> 
> This fixes DHCP for Q800 guest.
> 
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---
>   hw/net/dp8393x.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index 85d3f3788e..b8c4473f99 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -833,7 +833,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
>       } else {
>           dp8393x_put(s, width, 0, 0); /* in_use */
>           address_space_rw(&s->as, dp8393x_crda(s) + sizeof(uint16_t) * 6 * width,
> -            MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, sizeof(uint16_t), 1);
> +            MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 1);

Which is following:

     if (s->regs[SONIC_LLFA] & 0x1) {
         size = sizeof(uint16_t) * 1 * width;

So OK (you describe 'width' but use 'size').

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

>           s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
>           s->regs[SONIC_ISR] |= SONIC_ISR_PKTRX;
>           s->regs[SONIC_RSC] = (s->regs[SONIC_RSC] & 0xff00) | (((s->regs[SONIC_RSC] & 0x00ff) + 1) & 0x00ff);
> 

Re: [PATCH 2/3] dp8393x: fix dp8393x_receive()

[Laurent Vivier]

Le 02/11/2019 à 20:41, Philippe Mathieu-Daudé a écrit :
> On 11/2/19 6:15 PM, Laurent Vivier wrote:
>> address_space_rw() access size must be multiplied by the width.
>>
>> This fixes DHCP for Q800 guest.
>>
>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>> ---
>>   hw/net/dp8393x.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
>> index 85d3f3788e..b8c4473f99 100644
>> --- a/hw/net/dp8393x.c
>> +++ b/hw/net/dp8393x.c
>> @@ -833,7 +833,7 @@ static ssize_t dp8393x_receive(NetClientState *nc,
>> const uint8_t * buf,
>>       } else {
>>           dp8393x_put(s, width, 0, 0); /* in_use */
>>           address_space_rw(&s->as, dp8393x_crda(s) + sizeof(uint16_t)
>> * 6 * width,
>> -            MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data,
>> sizeof(uint16_t), 1);
>> +            MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 1);
> 
> Which is following:
> 
>     if (s->regs[SONIC_LLFA] & 0x1) {
>         size = sizeof(uint16_t) * 1 * width;

but we have "size = sizeof(uint16_t) * width;" later.

This file needs cleanup...

> So OK (you describe 'width' but use 'size').
>

I meant the access size "sizeof(uint16_t)" must be adjusted by the width
of the bus (defined by (s->regs[SONIC_DCR] & SONIC_DCR_DW). [1]

And this is exactly what contains "size".

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

Thanks,
Laurent

[1] DP83932C-20/25/33 MHz SONIC (tm) Systems-Oriented Network Interface
Controller

"1.3 DATA WIDTH AND BYTE ORDERING

The SONIC can be programmed to operate with either
32-bit or 16-bit wide memory. The data width is configured
during initialization by programming the DW bit in the Data
Configuration Register (DCR, Section 4.3.2). If the 16-bit
data path is selected, data is driven on pins D15– D0."

Re: [PATCH 3/3] dp8393x: fix receiving buffer exhaustion

[Laurent Vivier]

Le 02/11/2019 à 18:15, Laurent Vivier a écrit :
> The card is not able to exit from exhaustion state, because
> while the drive consumes the buffers, the RRP is incremented
> (when the driver clears the ISR RBE bit), so it stays equal
> to RWP, and while RRP == RWP, the card thinks it is always
> in exhaustion state. So the driver consumes all the buffers,
> but the card cannot receive new ones.
> 
> This patch fixes the problem by not incrementing RRP when
> the driver clears the ISR RBE bit.
> 
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---
>  hw/net/dp8393x.c | 31 ++++++++++++++++---------------
>  1 file changed, 16 insertions(+), 15 deletions(-)

For reviewers, this patch main changes (without indentation changes) can
be simplified to:

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index b8c4473f99..123d110f16 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -304,7 +304,7 @@ static void dp8393x_do_load_cam(dp8393xState *s)
     dp8393x_update_irq(s);
 }

-static void dp8393x_do_read_rra(dp8393xState *s)
+static void dp8393x_do_read_rra(dp8393xState *s, int next)
 {
     int width, size;

@@ -323,6 +323,7 @@ static void dp8393x_do_read_rra(dp8393xState *s)
         s->regs[SONIC_CRBA0], s->regs[SONIC_CRBA1],
         s->regs[SONIC_RBWC0], s->regs[SONIC_RBWC1]);

+    if (next) {
         /* Go to next entry */
         s->regs[SONIC_RRP] += size;

@@ -337,6 +338,7 @@ static void dp8393x_do_read_rra(dp8393xState *s)
             s->regs[SONIC_ISR] |= SONIC_ISR_RBE;
             dp8393x_update_irq(s);
         }
+    }

     /* Done */
     s->regs[SONIC_CR] &= ~SONIC_CR_RRRA;
@@ -549,7 +551,7 @@ static void dp8393x_do_command(dp8393xState *s,
uint16_t command)
     if (command & SONIC_CR_RST)
         dp8393x_do_software_reset(s);
     if (command & SONIC_CR_RRRA)
-        dp8393x_do_read_rra(s);
+        dp8393x_do_read_rra(s, 1);
     if (command & SONIC_CR_LCAM)
         dp8393x_do_load_cam(s);
 }
@@ -640,7 +642,7 @@ static void dp8393x_write(void *opaque, hwaddr addr,
uint64_t data,
             data &= s->regs[reg];
             s->regs[reg] &= ~data;
             if (data & SONIC_ISR_RBE) {
-                dp8393x_do_read_rra(s);
+                dp8393x_do_read_rra(s, 0);
             }
             dp8393x_update_irq(s);
             if (dp8393x_can_receive(s->nic->ncs)) {
@@ -840,7 +842,7 @@ static ssize_t dp8393x_receive(NetClientState *nc,
const uint8_t * buf,

         if (s->regs[SONIC_RCR] & SONIC_RCR_LPKT) {
             /* Read next RRA */
-            dp8393x_do_read_rra(s);
+            dp8393x_do_read_rra(s, 1);
         }
     }

Re: [PATCH 0/3] dp8393x: fix problems detected with Quadra 800 machine

[Laurent Vivier]

Le 02/11/2019 à 18:15, Laurent Vivier a écrit :
> This series starts with a cleanup of the DMA buffer, moving
> it from the stack to the state structure.
> 
> The following patch allows to negociate the IP address with
> the DHCP server.
> 
> The last one fixes the buffer exhaustion case.
> 
> With this series Q800 networking card is fully functionnal.
> 
> Laurent Vivier (3):
>   dp8393x: put the DMA buffer in the state structure
>   dp8393x: fix dp8393x_receive()
>   dp8393x: fix receiving buffer exhaustion
> 
>  hw/net/dp8393x.c | 136 +++++++++++++++++++++++------------------------
>  1 file changed, 66 insertions(+), 70 deletions(-)
> 

Hervé,

I tried to test this with Magnum machine type and Windows NT 4.0 SP1
installation: the installation works well but the machine crashes when I
tries to boot from HD disk (with and without my patches).

I'd like to have my patches in this QEMU release because without them
the Macintosh Q800 cannot receive an IP address from DHCP.

Could you help me?

Thanks,
Laurent

Re: [PATCH 1/3] dp8393x: put the DMA buffer in the state structure

[Hervé Poussineau]

Le 02/11/2019 à 18:15, Laurent Vivier a écrit :
> Move it from the stack.
> 
> It's only 24 bytes, and this simplifies the dp8393x_get()/
> dp8393x_put() interface.
> 
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---
>   hw/net/dp8393x.c | 105 ++++++++++++++++++++++-------------------------
>   1 file changed, 50 insertions(+), 55 deletions(-)

Reviewed-by: Hervé Poussineau <hpoussin@reactos.org>

Re: [PATCH 3/3] dp8393x: fix receiving buffer exhaustion

[Hervé Poussineau]

Le 02/11/2019 à 18:15, Laurent Vivier a écrit :
> The card is not able to exit from exhaustion state, because
> while the drive consumes the buffers, the RRP is incremented
> (when the driver clears the ISR RBE bit), so it stays equal
> to RWP, and while RRP == RWP, the card thinks it is always
> in exhaustion state. So the driver consumes all the buffers,
> but the card cannot receive new ones.
> 
> This patch fixes the problem by not incrementing RRP when
> the driver clears the ISR RBE bit.
> 
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---
>   hw/net/dp8393x.c | 31 ++++++++++++++++---------------
>   1 file changed, 16 insertions(+), 15 deletions(-)

I checked the DP83932C specification, available at
https://www.eit.lth.se/fileadmin/eit/courses/datablad/Periphery/Communication/DP83932C.pdf

In the Buffer Resources Exhausted section (page 20):
"To continue reception after the last RBA is used, the system must supply
additional RRA descriptor(s), update the RWP register, and clear the RBE
bit in the ISR. The SONIC rereads the RRA after this bit is cleared."

If I understand correctly, if the OS updates first the RWP and then clear the RBE bit,
then RRP should be different of RWP in dp8393x_do_read_rra() ? Or did I miss something?

> 
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index b8c4473f99..21deb32456 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -304,7 +304,7 @@ static void dp8393x_do_load_cam(dp8393xState *s)
>       dp8393x_update_irq(s);
>   }
>   
> -static void dp8393x_do_read_rra(dp8393xState *s)
> +static void dp8393x_do_read_rra(dp8393xState *s, int next)
>   {
>       int width, size;
>   
> @@ -323,19 +323,20 @@ static void dp8393x_do_read_rra(dp8393xState *s)
>           s->regs[SONIC_CRBA0], s->regs[SONIC_CRBA1],
>           s->regs[SONIC_RBWC0], s->regs[SONIC_RBWC1]);
>   
> -    /* Go to next entry */
> -    s->regs[SONIC_RRP] += size;
> +    if (next) {
> +        /* Go to next entry */
> +        s->regs[SONIC_RRP] += size;
>   
> -    /* Handle wrap */
> -    if (s->regs[SONIC_RRP] == s->regs[SONIC_REA]) {
> -        s->regs[SONIC_RRP] = s->regs[SONIC_RSA];
> -    }
> +        /* Handle wrap */
> +        if (s->regs[SONIC_RRP] == s->regs[SONIC_REA]) {
> +            s->regs[SONIC_RRP] = s->regs[SONIC_RSA];
> +        }
>   
> -    /* Check resource exhaustion */
> -    if (s->regs[SONIC_RRP] == s->regs[SONIC_RWP])
> -    {
> -        s->regs[SONIC_ISR] |= SONIC_ISR_RBE;
> -        dp8393x_update_irq(s);
> +        /* Check resource exhaustion */
> +        if (s->regs[SONIC_RRP] == s->regs[SONIC_RWP]) {
> +            s->regs[SONIC_ISR] |= SONIC_ISR_RBE;
> +            dp8393x_update_irq(s);
> +        }
>       }
>   
>       /* Done */
> @@ -549,7 +550,7 @@ static void dp8393x_do_command(dp8393xState *s, uint16_t command)
>       if (command & SONIC_CR_RST)
>           dp8393x_do_software_reset(s);
>       if (command & SONIC_CR_RRRA)
> -        dp8393x_do_read_rra(s);
> +        dp8393x_do_read_rra(s, 1);
>       if (command & SONIC_CR_LCAM)
>           dp8393x_do_load_cam(s);
>   }
> @@ -640,7 +641,7 @@ static void dp8393x_write(void *opaque, hwaddr addr, uint64_t data,
>               data &= s->regs[reg];
>               s->regs[reg] &= ~data;
>               if (data & SONIC_ISR_RBE) {
> -                dp8393x_do_read_rra(s);
> +                dp8393x_do_read_rra(s, 0);
>               }
>               dp8393x_update_irq(s);
>               if (dp8393x_can_receive(s->nic->ncs)) {
> @@ -840,7 +841,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
>   
>           if (s->regs[SONIC_RCR] & SONIC_RCR_LPKT) {
>               /* Read next RRA */
> -            dp8393x_do_read_rra(s);
> +            dp8393x_do_read_rra(s, 1);
>           }
>       }
>   
> 

Re: [PATCH 3/3] dp8393x: fix receiving buffer exhaustion

[Laurent Vivier]

Le 05/11/2019 à 21:45, Hervé Poussineau a écrit :
> Le 02/11/2019 à 18:15, Laurent Vivier a écrit :
>> The card is not able to exit from exhaustion state, because
>> while the drive consumes the buffers, the RRP is incremented
>> (when the driver clears the ISR RBE bit), so it stays equal
>> to RWP, and while RRP == RWP, the card thinks it is always
>> in exhaustion state. So the driver consumes all the buffers,
>> but the card cannot receive new ones.
>>
>> This patch fixes the problem by not incrementing RRP when
>> the driver clears the ISR RBE bit.
>>
>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>> ---
>>   hw/net/dp8393x.c | 31 ++++++++++++++++---------------
>>   1 file changed, 16 insertions(+), 15 deletions(-)
> 
> I checked the DP83932C specification, available at
> https://www.eit.lth.se/fileadmin/eit/courses/datablad/Periphery/Communication/DP83932C.pdf
> 
> 
> In the Buffer Resources Exhausted section (page 20):
> "To continue reception after the last RBA is used, the system must supply
> additional RRA descriptor(s), update the RWP register, and clear the RBE
> bit in the ISR. The SONIC rereads the RRA after this bit is cleared."
> 
> If I understand correctly, if the OS updates first the RWP and then
> clear the RBE bit,
> then RRP should be different of RWP in dp8393x_do_read_rra() ? Or did I
> miss something?

No, I found that by debugging the problem, I didn't have the
documentation. I'll rework this patch, relying on the doc to better
understand the problem.

Thanks,
Laurent

Re: [PATCH 2/3] dp8393x: fix dp8393x_receive()

[Hervé Poussineau]

Le 02/11/2019 à 18:15, Laurent Vivier a écrit :
> address_space_rw() access size must be multiplied by the width.
> 
> This fixes DHCP for Q800 guest.
> 
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---
>   hw/net/dp8393x.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index 85d3f3788e..b8c4473f99 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -833,7 +833,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
>       } else {
>           dp8393x_put(s, width, 0, 0); /* in_use */
>           address_space_rw(&s->as, dp8393x_crda(s) + sizeof(uint16_t) * 6 * width,
> -            MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, sizeof(uint16_t), 1);
> +            MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 1);
>           s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
>           s->regs[SONIC_ISR] |= SONIC_ISR_PKTRX;
>           s->regs[SONIC_RSC] = (s->regs[SONIC_RSC] & 0xff00) | (((s->regs[SONIC_RSC] & 0x00ff) + 1) & 0x00ff);
> 

This patch is problematic.
The code was initially created with "size".
It was changed in 409b52bfe199d8106dadf7c5ff3d88d2228e89b5 to fix networking in NetBSD 5.1.

To test with NetBSD 5.1
- boot the installer (arccd-5.1.iso)
- choose (S)hell option
- "ifconfig sn0 10.0.2.15 netmask 255.255.255.0"
- "route add default 10.0.2.2"
- networking should work (I test with "ftp 212.27.63.3")

Without this patch, I get the FTP banner.
With this patch, connection can't be established.

In datasheet page 17, you can see the "Receive Descriptor Format", which contains the in_use field.
It is clearly said that RXpkt.in_use is 16 bit wide, and that the bits 16-31 are not used in 32-bit mode.

So, I don't see why you need to clear 32 bits in 32-bit mode. Maybe you need to clear only the other
16 bits ? Maybe it depends of endianness ?

Regards,

Hervé

Re: [PATCH 2/3] dp8393x: fix dp8393x_receive()

[Laurent Vivier]

Le 05/11/2019 à 22:06, Hervé Poussineau a écrit :
> Le 02/11/2019 à 18:15, Laurent Vivier a écrit :
>> address_space_rw() access size must be multiplied by the width.
>>
>> This fixes DHCP for Q800 guest.
>>
>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>> ---
>>   hw/net/dp8393x.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
>> index 85d3f3788e..b8c4473f99 100644
>> --- a/hw/net/dp8393x.c
>> +++ b/hw/net/dp8393x.c
>> @@ -833,7 +833,7 @@ static ssize_t dp8393x_receive(NetClientState *nc,
>> const uint8_t * buf,
>>       } else {
>>           dp8393x_put(s, width, 0, 0); /* in_use */
>>           address_space_rw(&s->as, dp8393x_crda(s) + sizeof(uint16_t)
>> * 6 * width,
>> -            MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data,
>> sizeof(uint16_t), 1);
>> +            MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 1);
>>           s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
>>           s->regs[SONIC_ISR] |= SONIC_ISR_PKTRX;
>>           s->regs[SONIC_RSC] = (s->regs[SONIC_RSC] & 0xff00) |
>> (((s->regs[SONIC_RSC] & 0x00ff) + 1) & 0x00ff);
>>
> 
> This patch is problematic.
> The code was initially created with "size".
> It was changed in 409b52bfe199d8106dadf7c5ff3d88d2228e89b5 to fix
> networking in NetBSD 5.1.
> 
> To test with NetBSD 5.1
> - boot the installer (arccd-5.1.iso)
> - choose (S)hell option
> - "ifconfig sn0 10.0.2.15 netmask 255.255.255.0"
> - "route add default 10.0.2.2"
> - networking should work (I test with "ftp 212.27.63.3")

I've the firmware from
http://hpoussineau.free.fr/qemu/firmware/magnum-4000/setup.zip
Which file to use? NTPROM.RAW?

> Without this patch, I get the FTP banner.
> With this patch, connection can't be established.
> 
> In datasheet page 17, you can see the "Receive Descriptor Format", which
> contains the in_use field.
> It is clearly said that RXpkt.in_use is 16 bit wide, and that the bits
> 16-31 are not used in 32-bit mode.
> 
> So, I don't see why you need to clear 32 bits in 32-bit mode. Maybe you
> need to clear only the other
> 16 bits ? Maybe it depends of endianness ?

Thank you for the details. I think the problem should likely come from
the endianness.

The offset must be adjusted according to the access mode (endianness and
size).

The following patch fixes the problem for me, and should not break other
targets:

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 85d3f3788e..3d991af163 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -831,9 +831,15 @@ static ssize_t dp8393x_receive(NetClientState *nc,
const uint8_t * buf,
         /* EOL detected */
         s->regs[SONIC_ISR] |= SONIC_ISR_RDE;
     } else {
-        dp8393x_put(s, width, 0, 0); /* in_use */
-        address_space_rw(&s->as, dp8393x_crda(s) + sizeof(uint16_t) * 6
* width,
-            MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data,
sizeof(uint16_t), 1);
+        /* Clear in_use, but it is always 16bit wide */
+        int offset = dp8393x_crda(s) + sizeof(uint16_t) * 6 * width;
+        if (s->big_endian && width == 2) {
+            /* we need to adjust the offset of the 16bit field */
+            offset += sizeof(uint16_t);
+        }
+        s->data[0] = 0;
+        address_space_rw(&s->as, offset, MEMTXATTRS_UNSPECIFIED,
+                         (uint8_t *)s->data, sizeof(uint16_t), 1);
         s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
         s->regs[SONIC_ISR] |= SONIC_ISR_PKTRX;
         s->regs[SONIC_RSC] = (s->regs[SONIC_RSC] & 0xff00) |
(((s->regs[SONIC_RSC] & 0x00ff) + 1) & 0x00ff);

What is your opinion?

Thanks,
Laurent

Re: [PATCH 2/3] dp8393x: fix dp8393x_receive()

[Hervé Poussineau]

Le 05/11/2019 à 22:53, Laurent Vivier a écrit :
> Le 05/11/2019 à 22:06, Hervé Poussineau a écrit :
>> Le 02/11/2019 à 18:15, Laurent Vivier a écrit :
>>> address_space_rw() access size must be multiplied by the width.
>>>
>>> This fixes DHCP for Q800 guest.
>>>
>>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>>> ---
>>>    hw/net/dp8393x.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
>>> index 85d3f3788e..b8c4473f99 100644
>>> --- a/hw/net/dp8393x.c
>>> +++ b/hw/net/dp8393x.c
>>> @@ -833,7 +833,7 @@ static ssize_t dp8393x_receive(NetClientState *nc,
>>> const uint8_t * buf,
>>>        } else {
>>>            dp8393x_put(s, width, 0, 0); /* in_use */
>>>            address_space_rw(&s->as, dp8393x_crda(s) + sizeof(uint16_t)
>>> * 6 * width,
>>> -            MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data,
>>> sizeof(uint16_t), 1);
>>> +            MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 1);
>>>            s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
>>>            s->regs[SONIC_ISR] |= SONIC_ISR_PKTRX;
>>>            s->regs[SONIC_RSC] = (s->regs[SONIC_RSC] & 0xff00) |
>>> (((s->regs[SONIC_RSC] & 0x00ff) + 1) & 0x00ff);
>>>
>>
>> This patch is problematic.
>> The code was initially created with "size".
>> It was changed in 409b52bfe199d8106dadf7c5ff3d88d2228e89b5 to fix
>> networking in NetBSD 5.1.
>>
>> To test with NetBSD 5.1
>> - boot the installer (arccd-5.1.iso)
>> - choose (S)hell option
>> - "ifconfig sn0 10.0.2.15 netmask 255.255.255.0"
>> - "route add default 10.0.2.2"
>> - networking should work (I test with "ftp 212.27.63.3")
> 
> I've the firmware from
> http://hpoussineau.free.fr/qemu/firmware/magnum-4000/setup.zip
> Which file to use? NTPROM.RAW?
> 
>> Without this patch, I get the FTP banner.
>> With this patch, connection can't be established.
>>
>> In datasheet page 17, you can see the "Receive Descriptor Format", which
>> contains the in_use field.
>> It is clearly said that RXpkt.in_use is 16 bit wide, and that the bits
>> 16-31 are not used in 32-bit mode.
>>
>> So, I don't see why you need to clear 32 bits in 32-bit mode. Maybe you
>> need to clear only the other
>> 16 bits ? Maybe it depends of endianness ?
> 
> Thank you for the details. I think the problem should likely come from
> the endianness.
> 
> The offset must be adjusted according to the access mode (endianness and
> size).
> 
> The following patch fixes the problem for me, and should not break other
> targets:
> 
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index 85d3f3788e..3d991af163 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -831,9 +831,15 @@ static ssize_t dp8393x_receive(NetClientState *nc,
> const uint8_t * buf,
>           /* EOL detected */
>           s->regs[SONIC_ISR] |= SONIC_ISR_RDE;
>       } else {
> -        dp8393x_put(s, width, 0, 0); /* in_use */
> -        address_space_rw(&s->as, dp8393x_crda(s) + sizeof(uint16_t) * 6
> * width,
> -            MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data,
> sizeof(uint16_t), 1);
> +        /* Clear in_use, but it is always 16bit wide */
> +        int offset = dp8393x_crda(s) + sizeof(uint16_t) * 6 * width;
> +        if (s->big_endian && width == 2) {
> +            /* we need to adjust the offset of the 16bit field */
> +            offset += sizeof(uint16_t);
> +        }
> +        s->data[0] = 0;
> +        address_space_rw(&s->as, offset, MEMTXATTRS_UNSPECIFIED,
> +                         (uint8_t *)s->data, sizeof(uint16_t), 1);
>           s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
>           s->regs[SONIC_ISR] |= SONIC_ISR_PKTRX;
>           s->regs[SONIC_RSC] = (s->regs[SONIC_RSC] & 0xff00) |
> (((s->regs[SONIC_RSC] & 0x00ff) + 1) & 0x00ff);
> 
> What is your opinion?

This one works for NetBSD.
Tested-by: Hervé Poussineau <hpoussin@reactos.org>