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

Patrick Venture posted 1 patch 1 week ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20220921234646.949273-1-venture@google.com
Maintainers: Havard Skinnemoen <hskinnemoen@google.com>, Tyrone Ting <kfting@nuvoton.com>, Jason Wang <jasowang@redhat.com>
hw/net/npcm7xx_emc.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
[PATCH] hw/net: npcm7xx_emc: set MAC in register space
Posted by Patrick Venture 1 week ago
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>
---
 hw/net/npcm7xx_emc.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/hw/net/npcm7xx_emc.c b/hw/net/npcm7xx_emc.c
index 7c86bb52e5..6be1008529 100644
--- a/hw/net/npcm7xx_emc.c
+++ b/hw/net/npcm7xx_emc.c
@@ -96,6 +96,9 @@ static const char *emc_reg_name(int regno)
 #undef REG
 }
 
+static void npcm7xx_emc_write(void *opaque, hwaddr offset,
+                              uint64_t v, unsigned size);
+
 static void emc_reset(NPCM7xxEMCState *emc)
 {
     trace_npcm7xx_emc_reset(emc->emc_num);
@@ -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));
 }
 
 static void npcm7xx_emc_reset(DeviceState *dev)
-- 
2.37.3.998.g577e59143f-goog
Re: [PATCH] hw/net: npcm7xx_emc: set MAC in register space
Posted by Peter Maydell 6 days, 16 hours ago
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
Re: [PATCH] hw/net: npcm7xx_emc: set MAC in register space
Posted by Jason Wang 6 days, 2 hours ago
On Thu, Sep 22, 2022 at 8:35 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> 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 ?

I think it depends on the NIC.

E1000 has a EEPROM interface for providing the MAC address for the
ethernet controller before it can be accessed by the driver during
reset. For modern Intel NICs like E810, it has similar semantics but
using NVM instead of EEPROM. So the current e1000 behaviour seems to
be correct (treat the initiali MAC as the one stored in the EEPROM).

I guess most NIC should behave the same as having a persistent storage
for MAC for the controller during reset, but I'm not sure this is the
case for imx_fec.

(Anyhow, if we change the behaviour we need keep migration compatibility)

Thanks

>
> thanks
> -- PMM
>
Re: [PATCH] hw/net: npcm7xx_emc: set MAC in register space
Posted by Patrick Venture 5 days, 5 hours ago
On Thu, Sep 22, 2022 at 8:21 PM Jason Wang <jasowang@redhat.com> wrote:

> On Thu, Sep 22, 2022 at 8:35 PM Peter Maydell <peter.maydell@linaro.org>
> wrote:
> >
> > 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 ?
>
> I think it depends on the NIC.
>
> E1000 has a EEPROM interface for providing the MAC address for the
> ethernet controller before it can be accessed by the driver during
> reset. For modern Intel NICs like E810, it has similar semantics but
> using NVM instead of EEPROM. So the current e1000 behaviour seems to
> be correct (treat the initiali MAC as the one stored in the EEPROM).
>
> I guess most NIC should behave the same as having a persistent storage
> for MAC for the controller during reset, but I'm not sure this is the
> case for imx_fec.
>

So the first time the NIC is realized, it should take the value from the
command line.  Then later if the guest OS updates it, it should always on
reset use that provided value?


>
> (Anyhow, if we change the behaviour we need keep migration compatibility)
>
> Thanks
>
> >
> > thanks
> > -- PMM
> >
>
>
Re: [PATCH] hw/net: npcm7xx_emc: set MAC in register space
Posted by Peter Maydell 4 days, 20 hours ago
On Sat, 24 Sept 2022 at 00:42, Patrick Venture <venture@google.com> wrote:
> On Thu, Sep 22, 2022 at 8:21 PM Jason Wang <jasowang@redhat.com> wrote:
>>
>> On Thu, Sep 22, 2022 at 8:35 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>> > 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 ?
>>
>> I think it depends on the NIC.
>>
>> E1000 has a EEPROM interface for providing the MAC address for the
>> ethernet controller before it can be accessed by the driver during
>> reset. For modern Intel NICs like E810, it has similar semantics but
>> using NVM instead of EEPROM. So the current e1000 behaviour seems to
>> be correct (treat the initiali MAC as the one stored in the EEPROM).
>>
>> I guess most NIC should behave the same as having a persistent storage
>> for MAC for the controller during reset, but I'm not sure this is the
>> case for imx_fec.

> So the first time the NIC is realized, it should take the value from
> the command line.  Then later if the guest OS updates it, it should
> always on reset use that provided value?

I think what Jason is suggesting is that that should depend on what
the real hardware does. On a physical board, if the guest sets the
MAC address, and then you power-cycle the hardware, does the MAC
that it set still persist after powercycle ? Does the guest writing
to these MAC registers correspond to writing to an EEPROM ?

thanks
-- PMM
Re: [PATCH] hw/net: npcm7xx_emc: set MAC in register space
Posted by Patrick Venture 2 days, 13 hours ago
On Sat, Sep 24, 2022 at 2:10 AM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On Sat, 24 Sept 2022 at 00:42, Patrick Venture <venture@google.com> wrote:
> > On Thu, Sep 22, 2022 at 8:21 PM Jason Wang <jasowang@redhat.com> wrote:
> >>
> >> On Thu, Sep 22, 2022 at 8:35 PM Peter Maydell <peter.maydell@linaro.org>
> wrote:
> >> > 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 ?
> >>
> >> I think it depends on the NIC.
> >>
> >> E1000 has a EEPROM interface for providing the MAC address for the
> >> ethernet controller before it can be accessed by the driver during
> >> reset. For modern Intel NICs like E810, it has similar semantics but
> >> using NVM instead of EEPROM. So the current e1000 behaviour seems to
> >> be correct (treat the initiali MAC as the one stored in the EEPROM).
> >>
> >> I guess most NIC should behave the same as having a persistent storage
> >> for MAC for the controller during reset, but I'm not sure this is the
> >> case for imx_fec.
>
> > So the first time the NIC is realized, it should take the value from
> > the command line.  Then later if the guest OS updates it, it should
> > always on reset use that provided value?
>
> I think what Jason is suggesting is that that should depend on what
> the real hardware does. On a physical board, if the guest sets the
> MAC address, and then you power-cycle the hardware, does the MAC
> that it set still persist after powercycle ? Does the guest writing
> to these MAC registers correspond to writing to an EEPROM ?
>

Thanks, Peter - we've reached out to the vendor off-list to seek out some
details, I'll update this with a v2 when I get an answer.


>
> thanks
> -- PMM
>