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é <f4bug@amsat.org>
---
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 c9b478c127c..e0055b178b1 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 */
@@ -987,7 +984,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()
}
--
2.31.1
On 03/07/2021 15:19, Philippe Mathieu-Daudé wrote:
> 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é <f4bug@amsat.org>
> ---
> 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 c9b478c127c..e0055b178b1 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 */
> @@ -987,7 +984,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 need to do some testing first to make sure dp8393x_get() and dp8393x_put() are
doing the right thing here, but it looks correct.
Also given that the migration stream for MIPS machines has been broken for many years
until your latest PR, I would have no objection if you wanted to change
VMSTATE_BUFFER_UNSAFE to VMSTATE_UINT16_ARRAY.
ATB,
Mark.
On 03/07/2021 15:19, Philippe Mathieu-Daudé wrote:
> 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é <f4bug@amsat.org>
> ---
> 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 c9b478c127c..e0055b178b1 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 */
> @@ -987,7 +984,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'd still be inclined to change VMSTATE_BUFFER_UNSAFE for VMSTATE_UINT16_ARRAY whilst
you can do it without having to worry about the migration stream being already
broken, but anyhow:
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
ATB,
Mark.
On 7/4/21 4:48 PM, Mark Cave-Ayland wrote:
> On 03/07/2021 15:19, Philippe Mathieu-Daudé wrote:
>
>> 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é <f4bug@amsat.org>
>> ---
>> hw/net/dp8393x.c | 23 ++++++++++-------------
>> 1 file changed, 10 insertions(+), 13 deletions(-)
>> @@ -987,7 +984,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'd still be inclined to change VMSTATE_BUFFER_UNSAFE for
> VMSTATE_UINT16_ARRAY whilst you can do it without having to worry about
> the migration stream being already broken, but anyhow:
>
> Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Do you want me to squash:
-- >8 --
diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 1d1837dbd38..4c2fa0aabbd 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -951,10 +951,10 @@ static void dp8393x_realize(DeviceState *dev,
Error **errp)
static const VMStateDescription vmstate_dp8393x = {
.name = "dp8393x",
- .version_id = 0,
- .minimum_version_id = 0,
+ .version_id = 1,
+ .minimum_version_id = 1,
.fields = (VMStateField []) {
- VMSTATE_BUFFER_UNSAFE(cam, dp8393xState, 0, 16 * 3 * 2),
+ VMSTATE_UINT16_ARRAY(cam, dp8393xState, 0, 16 * 3),
VMSTATE_UINT16_ARRAY(regs, dp8393xState, 0x40),
VMSTATE_END_OF_LIST()
}
---
Or send it as a new patch?
On 06/07/2021 18:29, Philippe Mathieu-Daudé wrote:
> On 7/4/21 4:48 PM, Mark Cave-Ayland wrote:
>> On 03/07/2021 15:19, Philippe Mathieu-Daudé wrote:
>>
>>> 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é <f4bug@amsat.org>
>>> ---
>>> hw/net/dp8393x.c | 23 ++++++++++-------------
>>> 1 file changed, 10 insertions(+), 13 deletions(-)
>
>>> @@ -987,7 +984,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'd still be inclined to change VMSTATE_BUFFER_UNSAFE for
>> VMSTATE_UINT16_ARRAY whilst you can do it without having to worry about
>> the migration stream being already broken, but anyhow:
>>
>> Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>
> Do you want me to squash:
>
> -- >8 --
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index 1d1837dbd38..4c2fa0aabbd 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -951,10 +951,10 @@ static void dp8393x_realize(DeviceState *dev,
> Error **errp)
>
> static const VMStateDescription vmstate_dp8393x = {
> .name = "dp8393x",
> - .version_id = 0,
> - .minimum_version_id = 0,
> + .version_id = 1,
> + .minimum_version_id = 1,
> .fields = (VMStateField []) {
> - VMSTATE_BUFFER_UNSAFE(cam, dp8393xState, 0, 16 * 3 * 2),
> + VMSTATE_UINT16_ARRAY(cam, dp8393xState, 0, 16 * 3),
> VMSTATE_UINT16_ARRAY(regs, dp8393xState, 0x40),
> VMSTATE_END_OF_LIST()
> }
> ---
>
> Or send it as a new patch?
I don't mind either way. I think VMSTATE_UINT16_ARRAY is nicer, and it's very rare
that you get the freedom to make a migration change like this without having to worry
about compatibility. It's also really quick and easy to test.
If it passes your local tests and you would prefer to squash rather than re-post then
you can also add a:
Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
to the patchset. I included the list of guests I tested in the cover note, but forgot
to explicitly add the T-b tag.
ATB,
Mark.
© 2016 - 2026 Red Hat, Inc.