On 7/7/21 11:08 AM, Mark Cave-Ayland wrote: > On 07/07/2021 00:48, Finn Thain wrote: > >> On Mon, 5 Jul 2021, Mark Cave-Ayland wrote: >> >>> From: Philippe Mathieu-Daudé <[email protected]> >>> >>> Per the DP83932C datasheet from July 1995: >>> >>> 4.0 SONIC Registers >>> 4.1 THE CAM UNIT >>> >>> The Content Addressable Memory (CAM) consists of sixteen >>> 48-bit entries for complete address filtering of network >>> packets. Each entry corresponds to a 48-bit destination >>> address that is user programmable and can contain any >>> combination of Multicast or Physical addresses. Each entry >>> is partitioned into three 16-bit CAM cells accessible >>> through CAM Address Ports (CAP 2, CAP 1 and CAP 0) with >>> CAP0 corresponding to the least significant 16 bits of >>> the Destination Address and CAP2 corresponding to the >>> most significant bits. >>> >>> Store the CAM registers as 16-bit as it simplifies the code. >>> There is no change in the migration stream. >>> >>> Signed-off-by: Philippe Mathieu-Daudé <[email protected]> >>> --- >>> hw/net/dp8393x.c | 23 ++++++++++------------- >>> 1 file changed, 10 insertions(+), 13 deletions(-) >>> >>> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c >>> index cc7c001edb..22ceea338c 100644 >>> --- a/hw/net/dp8393x.c >>> +++ b/hw/net/dp8393x.c >>> @@ -157,7 +157,7 @@ struct dp8393xState { >>> MemoryRegion mmio; >>> /* Registers */ >>> - uint8_t cam[16][6]; >>> + uint16_t cam[16][3]; >>> uint16_t regs[0x40]; >>> /* Temporaries */ >>> @@ -280,15 +280,13 @@ static void dp8393x_do_load_cam(dp8393xState *s) >>> address_space_read(&s->as, dp8393x_cdp(s), >>> MEMTXATTRS_UNSPECIFIED, s->data, size); >>> index = dp8393x_get(s, width, 0) & 0xf; >>> - 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; >>> - trace_dp8393x_load_cam(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]); >>> + s->cam[index][0] = dp8393x_get(s, width, 1); >>> + s->cam[index][1] = dp8393x_get(s, width, 2); >>> + s->cam[index][2] = dp8393x_get(s, width, 3); >>> + trace_dp8393x_load_cam(index, >>> + s->cam[index][0] >> 8, >>> s->cam[index][0] & 0xff, >>> + s->cam[index][1] >> 8, >>> s->cam[index][1] & 0xff, >>> + s->cam[index][2] >> 8, >>> s->cam[index][2] & 0xff); >>> /* Move to next entry */ >>> s->regs[SONIC_CDC]--; >>> s->regs[SONIC_CDP] += size; >>> @@ -591,8 +589,7 @@ static uint64_t dp8393x_read(void *opaque, hwaddr >>> addr, unsigned int size) >>> case SONIC_CAP1: >>> case SONIC_CAP0: >>> if (s->regs[SONIC_CR] & SONIC_CR_RST) { >>> - val = s->cam[s->regs[SONIC_CEP] & 0xf][2 * (SONIC_CAP0 - >>> reg) + 1] << 8; >>> - val |= s->cam[s->regs[SONIC_CEP] & 0xf][2 * (SONIC_CAP0 >>> - reg)]; >>> + val = s->cam[s->regs[SONIC_CEP] & 0xf][2 * (SONIC_CAP0 - >>> reg)]; >>> } >>> break; >>> /* All other registers have no special contraints */ >> >> This patch incorrectly alters the behaviour of the jazzsonic.c driver >> which reads the MAC address from the CAP registers in sonic_probe1(). >> >> With mainline QEMU, the driver reports: >> SONIC ethernet @e0001000, MAC 00:00:00:44:33:22, IRQ 28 >> >> With this patch: >> SONIC ethernet @e0001000, MAC 00:00:33:22:00:00, IRQ 28 >> >>> @@ -990,7 +987,7 @@ static const VMStateDescription vmstate_dp8393x = { >>> .version_id = 0, >>> .minimum_version_id = 0, >>> .fields = (VMStateField []) { >>> - VMSTATE_BUFFER_UNSAFE(cam, dp8393xState, 0, 16 * 6), >>> + VMSTATE_BUFFER_UNSAFE(cam, dp8393xState, 0, 16 * 3 * 2), >>> VMSTATE_UINT16_ARRAY(regs, dp8393xState, 0x40), >>> VMSTATE_END_OF_LIST() >>> } > > I was staring at the code in dp8393x_do_load_cam() trying to figure out > how on earth this could be reading the wrong values from the CAM > descriptors, when I realised the problem is actually in the read back > from the CAP registers (it doesn't need the x2 multiplier since the > conversion from uint8_t to uint16_t). > > This should be fixed by the following: > > diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c > index 22ceea338c..09c34c3584 100644 > --- a/hw/net/dp8393x.c > +++ b/hw/net/dp8393x.c > @@ -589,7 +589,7 @@ static uint64_t dp8393x_read(void *opaque, hwaddr > addr, unsigned int size) > case SONIC_CAP1: > case SONIC_CAP0: > if (s->regs[SONIC_CR] & SONIC_CR_RST) { > - val = s->cam[s->regs[SONIC_CEP] & 0xf][2 * (SONIC_CAP0 - > reg)]; > + val = s->cam[s->regs[SONIC_CEP] & 0xf][SONIC_CAP0 - reg];
Oops, thanks!
