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

[Philippe Mathieu-Daudé]

Hi,

On 3/10/22 19:38, Patrick Venture 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>
> ---
> v2: only set the registers from qemu on reset
>      once registers set, only read and write to them
> ---
>   hw/net/npcm7xx_emc.c | 30 +++++++++++++++++++++++-------
>   1 file changed, 23 insertions(+), 7 deletions(-)
>
> diff --git a/hw/net/npcm7xx_emc.c b/hw/net/npcm7xx_emc.c
> index 7c86bb52e5..a33f8c7b23 100644
> --- a/hw/net/npcm7xx_emc.c
> +++ b/hw/net/npcm7xx_emc.c
> @@ -112,6 +112,16 @@ 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];

Per QEMU Coding style:

  Mixed declarations (interleaving statements and declarations
  within blocks) are generally not allowed; declarations should
  be at the beginning of blocks.

https://qemu-project.gitlab.io/qemu/devel/style.html#declarations

> +    emc->regs[REG_CAMM_BASE] = value;

Alternatively:

  emc->regs[REG_CAMM_BASE] = ldl_be_p(s->conf.macaddr.a);
  emc->regs[REG_CAML_BASE] = lduw_be_p(s->conf.macaddr.a + 4);

> +
> +    value = (emc->conf.macaddr.a[4] << 24) | (emc->conf.macaddr.a[5] << 16);
> +    emc->regs[REG_CAML_BASE] = value;
>   }
>   
>   static void npcm7xx_emc_reset(DeviceState *dev)
> @@ -432,13 +442,25 @@ static bool emc_receive_filter1(NPCM7xxEMCState *emc, 
> const uint8_t *buf,
>           }
>       case ETH_PKT_UCAST: {
>           bool matches;
> +        uint32_t value;
> +        struct MACAddr mac;
>           if (emc->regs[REG_CAMCMR] & REG_CAMCMR_AUP) {
>               return true;
>           }
> +
> +        value = emc->regs[REG_CAMM_BASE];
> +        mac.a[0] = value >> 24;
> +        mac.a[1] = value >> 16;
> +        mac.a[2] = value >> 8;
> +        mac.a[3] = value >> 0;
> +        value = emc->regs[REG_CAML_BASE];
> +        mac.a[4] = value >> 24;
> +        mac.a[5] = value >> 16;

Similarly:

  stl_be_p(mac.a, emc->regs[REG_CAMM_BASE]);
  stw_be_p(mac.a + 4, emc->regs[REG_CAML_BASE]);

>           matches = ((emc->regs[REG_CAMCMR] & REG_CAMCMR_ECMP) &&
>                      /* We only support one CAM register, CAM0. */
>                      (emc->regs[REG_CAMEN] & (1 << 0)) &&
> -                   memcmp(buf, emc->conf.macaddr.a, ETH_ALEN) == 0);
> +                   memcmp(buf, mac.a, ETH_ALEN) == 0);
>           if (emc->regs[REG_CAMCMR] & REG_CAMCMR_CCAM) {
>               *fail_reason = "MACADDR matched, comparison complemented";
>               return !matches;
> @@ -661,15 +683,9 @@ static void npcm7xx_emc_write(void *opaque, hwaddr offset,
>           break;
>       case REG_CAMM_BASE + 0:
>           emc->regs[reg] = value;
> -        emc->conf.macaddr.a[0] = value >> 24;
> -        emc->conf.macaddr.a[1] = value >> 16;
> -        emc->conf.macaddr.a[2] = value >> 8;
> -        emc->conf.macaddr.a[3] = value >> 0;
>           break;
>       case REG_CAML_BASE + 0:
>           emc->regs[reg] = value;
> -        emc->conf.macaddr.a[4] = value >> 24;
> -        emc->conf.macaddr.a[5] = value >> 16;
>           break;
>       case REG_MCMDR: {
>           uint32_t prev;

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

(original patch: 
https://lore.kernel.org/qemu-devel/20221003173810.1933849-1-venture@google.com/)