Re: [PATCH] hw/net: npcm7xx_emc: set MAC in register space

[Peter Maydell]

On Thu, 22 Sept 2022 at 00:47, Patrick Venture <venture@google.com> wrote:
>
> The MAC address set from Qemu wasn't being saved into the register space.
>
> Reviewed-by: Hao Wu <wuhaotsh@google.com>
> Signed-off-by: Patrick Venture <venture@google.com>

> @@ -112,6 +115,18 @@ static void emc_reset(NPCM7xxEMCState *emc)
>
>      emc->tx_active = false;
>      emc->rx_active = false;
> +
> +    /* Set the MAC address in the register space. */
> +    uint32_t value = (emc->conf.macaddr.a[0] << 24) |
> +        (emc->conf.macaddr.a[1] << 16) |
> +        (emc->conf.macaddr.a[2] << 8) |
> +        emc->conf.macaddr.a[3];
> +    npcm7xx_emc_write(emc, REG_CAMM_BASE * sizeof(uint32_t), value,
> +                      sizeof(uint32_t));
> +
> +    value = (emc->conf.macaddr.a[4] << 24) | (emc->conf.macaddr.a[5] << 16);
> +    npcm7xx_emc_write(emc, REG_CAML_BASE * sizeof(uint32_t), value,
> +                      sizeof(uint32_t));

If I understand correctly, the issue here is that emc->regs[REG_CAMM_BASE]
and emc->regs[REG_CAML_BASE] aren't being reset correctly. If so,
I think the better approach is to simply reset them here, without
going through the register-write function, the same way we already
do for the handful of other registers which have non-zero reset values.
That's the way other devices seem to do it.

A question to which I don't know the answer: if the guest writes to
the device to change the MAC address, should that persist across
reset, or should reset revert the device to the original MAC address
as specified by the user on the command line or whatever ? At the
moment you have the former behaviour (and end up storing the MAC
address in two places as a result -- it would be neater to either
keep it in only one place, or else have emc->regs[] be the current
programmed MAC address and emc->conf.macaddr the value to reset to).

I'm not sure we're consistent between device models about that,
eg the e1000 seems to reset to the initial MAC addr, but the
imx_fec keeps the guest-set value over resets. Jason, is there
a recommended "right way" to handle guest-programmable MAC addresses
over device reset ?

thanks
-- PMM