Re: [Qemu-devel] [RFC 12/13] dp8393x: put DMA temp buffer in the state, not in the stack

2018-06-09 Thread Thomas Huth 
On 08.06.2018 22:05, Laurent Vivier wrote:
> It's only 32 bytes, and this simplifies the dp8393x_get()/
> dp8393x_put() interface.

Maybe not worth the effort ... or do you need this in a later patch,
too? If so, please mention it in the patch description here.

> Signed-off-by: Laurent Vivier 
> ---
>  hw/net/dp8393x.c | 107 
> ++-
>  1 file changed, 51 insertions(+), 56 deletions(-)
> 
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index 5061474e6b..40e5f8257b 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -168,6 +168,7 @@ typedef struct dp8393xState {
>  
>  /* Temporaries */
>  uint8_t tx_buffer[0x1];
> +uint16_t data[16];

Why 16? The biggest array that you replaced has only 12 entries...

Also, while you're at it, maybe change the name of the variable
("dma_data"?) or add a comment with a short explanation ?

 Thomas

Re: [Qemu-devel] [RFC 12/13] dp8393x: put DMA temp buffer in the state, not in the stack

2018-06-09 Thread Hervé Poussineau 

Le 08/06/2018 à 22:05, Laurent Vivier a écrit :

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

Signed-off-by: Laurent Vivier 
---
  hw/net/dp8393x.c | 107 ++-
  1 file changed, 51 insertions(+), 56 deletions(-)



Works OK on NetBSD 5.1/arc on MIPS Magnum.

Tested-by: Hervé Poussineau

[Qemu-devel] [RFC 12/13] dp8393x: put DMA temp buffer in the state, not in the stack

2018-06-08 Thread Laurent Vivier 
It's only 32 bytes, and this simplifies the dp8393x_get()/
dp8393x_put() interface.

Signed-off-by: Laurent Vivier 
---
 hw/net/dp8393x.c | 107 ++-
 1 file changed, 51 insertions(+), 56 deletions(-)

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 5061474e6b..40e5f8257b 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -168,6 +168,7 @@ typedef struct dp8393xState {
 
 /* Temporaries */
 uint8_t tx_buffer[0x1];
+uint16_t data[16];
 int loopback_packet;
 
 /* Memory access */
@@ -227,15 +228,14 @@ 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 = base[offset * width + width - 1];
+val = s->data[offset * width + width - 1];
 } else {
-val = base[offset * width];
+val = s->data[offset * width];
 }
 if (s->big_endian != host_big_endian) {
 val = bswap16(val);
@@ -243,7 +243,7 @@ static uint16_t dp8393x_get(dp8393xState *s, int width, 
uint16_t *base,
 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 != host_big_endian) {
@@ -251,9 +251,9 @@ static void dp8393x_put(dp8393xState *s, int width, 
uint16_t *base, int offset,
 }
 
 if (s->big_endian) {
-base[offset * width + width - 1] = val;
+s->data[offset * width + width - 1] = val;
 } else {
-base[offset * width] = val;
+s->data[offset * width] = val;
 }
 }
 
@@ -277,7 +277,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;
 
@@ -287,13 +286,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]);
@@ -305,8 +304,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 */
@@ -317,20 +316,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]);
@@ -427,7 +425,6 @@ stat