1. Patchew
  2. QEMU
  3. [PATCH v3 00/14] Fixes for DP8393X SONIC device emulation
  4. View patch

[PATCH v3 01/14] dp8393x: Mask EOL bit from descriptor addresses

Finn Thain posted 14 patches 6 years ago
Maintainers: Jason Wang <jasowang@redhat.com>
There is a newer version of this series
[PATCH v3 01/14] dp8393x: Mask EOL bit from descriptor addresses
Posted by Finn Thain 6 years ago 
The Least Significant bit of a descriptor address register is used as
an EOL flag. It has to be masked when the register value is to be used
as an actual address for copying memory around. But when the registers
are to be updated the EOL bit should not be masked.

Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
Tested-by: Laurent Vivier <laurent@vivier.eu>
---
Changed since v1:
 - Added macros to name constants as requested by Philippe Mathieu-Daudé.
---
 hw/net/dp8393x.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index cdc2631c0c..14901c1445 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -145,6 +145,9 @@ do { printf("sonic ERROR: %s: " fmt, __func__ , ## __VA_ARGS__); } while (0)
 #define SONIC_ISR_PINT   0x0800
 #define SONIC_ISR_LCD    0x1000
 
+#define SONIC_DESC_EOL   0x0001
+#define SONIC_DESC_ADDR  0xFFFE
+
 #define TYPE_DP8393X "dp8393x"
 #define DP8393X(obj) OBJECT_CHECK(dp8393xState, (obj), TYPE_DP8393X)
 
@@ -197,7 +200,8 @@ static uint32_t dp8393x_crba(dp8393xState *s)
 
 static uint32_t dp8393x_crda(dp8393xState *s)
 {
-    return (s->regs[SONIC_URDA] << 16) | s->regs[SONIC_CRDA];
+    return (s->regs[SONIC_URDA] << 16) |
+           (s->regs[SONIC_CRDA] & SONIC_DESC_ADDR);
 }
 
 static uint32_t dp8393x_rbwc(dp8393xState *s)
@@ -217,7 +221,8 @@ static uint32_t dp8393x_tsa(dp8393xState *s)
 
 static uint32_t dp8393x_ttda(dp8393xState *s)
 {
-    return (s->regs[SONIC_UTDA] << 16) | s->regs[SONIC_TTDA];
+    return (s->regs[SONIC_UTDA] << 16) |
+           (s->regs[SONIC_TTDA] & SONIC_DESC_ADDR);
 }
 
 static uint32_t dp8393x_wt(dp8393xState *s)
@@ -506,8 +511,8 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
                              sizeof(uint16_t) *
                              (4 + 3 * s->regs[SONIC_TFC]) * width,
                 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) {
+            s->regs[SONIC_CTDA] = dp8393x_get(s, width, 0);
+            if (s->regs[SONIC_CTDA] & SONIC_DESC_EOL) {
                 /* EOL detected */
                 break;
             }
@@ -763,13 +768,13 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
     /* XXX: Check byte ordering */
 
     /* Check for EOL */
-    if (s->regs[SONIC_LLFA] & 0x1) {
+    if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) {
         /* Are we still in resource exhaustion? */
         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 *)s->data, size, 0);
-        if (dp8393x_get(s, width, 0) & 0x1) {
+        if (dp8393x_get(s, width, 0) & SONIC_DESC_EOL) {
             /* Still EOL ; stop reception */
             return -1;
         } else {
@@ -827,7 +832,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
     address_space_rw(&s->as, dp8393x_crda(s) + sizeof(uint16_t) * 5 * width,
         MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
     s->regs[SONIC_LLFA] = dp8393x_get(s, width, 0);
-    if (s->regs[SONIC_LLFA] & 0x1) {
+    if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) {
         /* EOL detected */
         s->regs[SONIC_ISR] |= SONIC_ISR_RDE;
     } else {
-- 
2.24.1
Re: [PATCH v3 01/14] dp8393x: Mask EOL bit from descriptor addresses
Posted by Philippe Mathieu-Daudé 6 years ago 
Hi Finn,

On 1/19/20 11:59 PM, Finn Thain wrote:
> The Least Significant bit of a descriptor address register is used as
> an EOL flag. It has to be masked when the register value is to be used
> as an actual address for copying memory around. But when the registers
> are to be updated the EOL bit should not be masked.
> 
> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
> Tested-by: Laurent Vivier <laurent@vivier.eu>
> ---
> Changed since v1:
>   - Added macros to name constants as requested by Philippe Mathieu-Daudé.
> ---
>   hw/net/dp8393x.c | 19 ++++++++++++-------
>   1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index cdc2631c0c..14901c1445 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -145,6 +145,9 @@ do { printf("sonic ERROR: %s: " fmt, __func__ , ## __VA_ARGS__); } while (0)
>   #define SONIC_ISR_PINT   0x0800
>   #define SONIC_ISR_LCD    0x1000
>   
> +#define SONIC_DESC_EOL   0x0001
> +#define SONIC_DESC_ADDR  0xFFFE

I'd rather not add SONIC_DESC_ADDR and use ~SONIC_DESC_EOL instead.

Please consider it if you respin the series.

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

> +
>   #define TYPE_DP8393X "dp8393x"
>   #define DP8393X(obj) OBJECT_CHECK(dp8393xState, (obj), TYPE_DP8393X)
>   
> @@ -197,7 +200,8 @@ static uint32_t dp8393x_crba(dp8393xState *s)
>   
>   static uint32_t dp8393x_crda(dp8393xState *s)
>   {
> -    return (s->regs[SONIC_URDA] << 16) | s->regs[SONIC_CRDA];
> +    return (s->regs[SONIC_URDA] << 16) |
> +           (s->regs[SONIC_CRDA] & SONIC_DESC_ADDR);
>   }
>   
>   static uint32_t dp8393x_rbwc(dp8393xState *s)
> @@ -217,7 +221,8 @@ static uint32_t dp8393x_tsa(dp8393xState *s)
>   
>   static uint32_t dp8393x_ttda(dp8393xState *s)
>   {
> -    return (s->regs[SONIC_UTDA] << 16) | s->regs[SONIC_TTDA];
> +    return (s->regs[SONIC_UTDA] << 16) |
> +           (s->regs[SONIC_TTDA] & SONIC_DESC_ADDR);
>   }
>   
>   static uint32_t dp8393x_wt(dp8393xState *s)
> @@ -506,8 +511,8 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
>                                sizeof(uint16_t) *
>                                (4 + 3 * s->regs[SONIC_TFC]) * width,
>                   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) {
> +            s->regs[SONIC_CTDA] = dp8393x_get(s, width, 0);
> +            if (s->regs[SONIC_CTDA] & SONIC_DESC_EOL) {
>                   /* EOL detected */
>                   break;
>               }
> @@ -763,13 +768,13 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
>       /* XXX: Check byte ordering */
>   
>       /* Check for EOL */
> -    if (s->regs[SONIC_LLFA] & 0x1) {
> +    if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) {
>           /* Are we still in resource exhaustion? */
>           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 *)s->data, size, 0);
> -        if (dp8393x_get(s, width, 0) & 0x1) {
> +        if (dp8393x_get(s, width, 0) & SONIC_DESC_EOL) {
>               /* Still EOL ; stop reception */
>               return -1;
>           } else {
> @@ -827,7 +832,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
>       address_space_rw(&s->as, dp8393x_crda(s) + sizeof(uint16_t) * 5 * width,
>           MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
>       s->regs[SONIC_LLFA] = dp8393x_get(s, width, 0);
> -    if (s->regs[SONIC_LLFA] & 0x1) {
> +    if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) {
>           /* EOL detected */
>           s->regs[SONIC_ISR] |= SONIC_ISR_RDE;
>       } else {
>
Re: [PATCH v3 01/14] dp8393x: Mask EOL bit from descriptor addresses
Posted by Finn Thain 6 years ago 
On Tue, 28 Jan 2020, Philippe Mathieu-Daudé wrote:

> Hi Finn,
> 
> On 1/19/20 11:59 PM, Finn Thain wrote:
> > The Least Significant bit of a descriptor address register is used as
> > an EOL flag. It has to be masked when the register value is to be used
> > as an actual address for copying memory around. But when the registers
> > are to be updated the EOL bit should not be masked.
> > 
> > Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
> > Tested-by: Laurent Vivier <laurent@vivier.eu>
> > ---
> > Changed since v1:
> >   - Added macros to name constants as requested by Philippe Mathieu-Daudé.
> > ---
> >   hw/net/dp8393x.c | 19 ++++++++++++-------
> >   1 file changed, 12 insertions(+), 7 deletions(-)
> > 
> > diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> > index cdc2631c0c..14901c1445 100644
> > --- a/hw/net/dp8393x.c
> > +++ b/hw/net/dp8393x.c
> > @@ -145,6 +145,9 @@ do { printf("sonic ERROR: %s: " fmt, __func__ , ##
> > __VA_ARGS__); } while (0)
> >   #define SONIC_ISR_PINT   0x0800
> >   #define SONIC_ISR_LCD    0x1000
> >   +#define SONIC_DESC_EOL   0x0001
> > +#define SONIC_DESC_ADDR  0xFFFE
> 
> I'd rather not add SONIC_DESC_ADDR and use ~SONIC_DESC_EOL instead.
> 
> Please consider it if you respin the series.
> 

I chose to use 0xFFFE instead of ~SONIC_DESC_EOL because the former 
correctly implies an unsigned short word, while the latter mask may 
suggest some need for sign extension or longer words.

I agree that ~SONIC_DESC_EOL is easily understood as "all the other bits". 
But the bits in SONIC_DESC_EOL will never change, since this value is 
dictated by the hardware.

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

Thanks for reviewing this series.

> > +
> >   #define TYPE_DP8393X "dp8393x"
> >   #define DP8393X(obj) OBJECT_CHECK(dp8393xState, (obj), TYPE_DP8393X)
> >   @@ -197,7 +200,8 @@ static uint32_t dp8393x_crba(dp8393xState *s)
> >     static uint32_t dp8393x_crda(dp8393xState *s)
> >   {
> > -    return (s->regs[SONIC_URDA] << 16) | s->regs[SONIC_CRDA];
> > +    return (s->regs[SONIC_URDA] << 16) |
> > +           (s->regs[SONIC_CRDA] & SONIC_DESC_ADDR);
> >   }
> >     static uint32_t dp8393x_rbwc(dp8393xState *s)
> > @@ -217,7 +221,8 @@ static uint32_t dp8393x_tsa(dp8393xState *s)
> >     static uint32_t dp8393x_ttda(dp8393xState *s)
> >   {
> > -    return (s->regs[SONIC_UTDA] << 16) | s->regs[SONIC_TTDA];
> > +    return (s->regs[SONIC_UTDA] << 16) |
> > +           (s->regs[SONIC_TTDA] & SONIC_DESC_ADDR);
> >   }
> >     static uint32_t dp8393x_wt(dp8393xState *s)
> > @@ -506,8 +511,8 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
> >                                sizeof(uint16_t) *
> >                                (4 + 3 * s->regs[SONIC_TFC]) * width,
> >                   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) {
> > +            s->regs[SONIC_CTDA] = dp8393x_get(s, width, 0);
> > +            if (s->regs[SONIC_CTDA] & SONIC_DESC_EOL) {
> >                   /* EOL detected */
> >                   break;
> >               }
> > @@ -763,13 +768,13 @@ static ssize_t dp8393x_receive(NetClientState *nc,
> > const uint8_t * buf,
> >       /* XXX: Check byte ordering */
> >         /* Check for EOL */
> > -    if (s->regs[SONIC_LLFA] & 0x1) {
> > +    if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) {
> >           /* Are we still in resource exhaustion? */
> >           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 *)s->data, size, 0);
> > -        if (dp8393x_get(s, width, 0) & 0x1) {
> > +        if (dp8393x_get(s, width, 0) & SONIC_DESC_EOL) {
> >               /* Still EOL ; stop reception */
> >               return -1;
> >           } else {
> > @@ -827,7 +832,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const
> > uint8_t * buf,
> >       address_space_rw(&s->as, dp8393x_crda(s) + sizeof(uint16_t) * 5 *
> > width,
> >           MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
> >       s->regs[SONIC_LLFA] = dp8393x_get(s, width, 0);
> > -    if (s->regs[SONIC_LLFA] & 0x1) {
> > +    if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) {
> >           /* EOL detected */
> >           s->regs[SONIC_ISR] |= SONIC_ISR_RDE;
> >       } else {
> > 
> 
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.