| Message ID | e5d4133abf4ecbb37d4abc45d7166cbd3cfac1d4.1579474761.git.fthain@telegraphics.com.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fixes for DP8393X SONIC device emulation | expand |
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 { >
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 { > > > >
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 {