On Fri, Nov 08, 2013 at 02:42:27PM -0500, Vlad Yasevich wrote:
> What about this approach? This only updates the monitory when all the
> bits have been written to.
>
> -vlad
Thanks!
Some comments below.
> -- >8 --
> Subject: [PATCH] e1000/rtl8139: update HMP NIC when every bit is written
>
> We currently just update the HMP NIC info when the last bit of macaddr
> is written. This assumes that guest driver will write all the macaddr
> from bit 0 to bit 5 when it changes the macaddr, this is the current
> behavior of linux driver (e1000/rtl8139cp), but we can't do this
> assumption.
I would rather say "it seems better not to make this assumption".
This does look somewhat safer than what Amos proposed.
>
> The macaddr that is used for rx-filter will be updated when every bit
> is changed. This patch updates the e1000/rtl8139 nic to update HMP NIC
> info when every bit has been changed. It will be same as virtio-net.
>
> Signed-off-by: Vlad Yasevich <vyase...@redhat.com>
> ---
> hw/net/e1000.c | 17 ++++++++++++++++-
> hw/net/rtl8139.c | 14 +++++++++-----
> 2 files changed, 25 insertions(+), 6 deletions(-)
>
> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
> index 8387443..a5967ed 100644
> --- a/hw/net/e1000.c
> +++ b/hw/net/e1000.c
> @@ -149,6 +149,10 @@ typedef struct E1000State_st {
> #define E1000_FLAG_AUTONEG (1 << E1000_FLAG_AUTONEG_BIT)
> #define E1000_FLAG_MIT (1 << E1000_FLAG_MIT_BIT)
> uint32_t compat_flags;
> + uint32_t mac_changed;
Hmm why uint32_t? uint8_t is enough here isn't?
This new state has to be migrated then, and
we need to fallback to old behaviour if migrating to/from
an old version (see compat_flags for one way to
detect this compatibility mode).
> +#define E1000_RA0_CHANGED 0
> +#define E1000_RA1_CHANGED 1
> +#define E1000_RA_ALL_CHANGED (E1000_RA0_CHANGED|E1000_RA1_CHANGED)
I don't get it. So E1000_RA_ALL_CHANGED is 0 | 1 == 1.
it looks like the trigger is when E1000_RA1_CHANGED
so that's more or less equivalent.
> } E1000State;
>
> #define TYPE_E1000 "e1000"
> @@ -402,6 +406,7 @@ static void e1000_reset(void *opaque)
> d->mac_reg[RA + 1] |= (i < 2) ? macaddr[i + 4] << (8 * i) : 0;
> }
> qemu_format_nic_info_str(qemu_get_queue(d->nic), macaddr);
> + d->mac_changed = 0;
> }
>
> static void
> @@ -1106,10 +1111,20 @@ mac_writereg(E1000State *s, int index, uint32_t val)
>
> s->mac_reg[index] = val;
>
> - if (index == RA + 1) {
> + switch (index) {
> + case RA:
> + s->mac_changed |= E1000_RA0_CHANGED;
> + break;
> + case (RA + 1):
> + s->mac_changed |= E1000_RA1_CHANGED;
> + break;
> + }
> +
> + if (s->mac_changed == E1000_RA_ALL_CHANGED) {
Some whitespace damage here.
> macaddr[0] = cpu_to_le32(s->mac_reg[RA]);
> macaddr[1] = cpu_to_le32(s->mac_reg[RA + 1]);
> qemu_format_nic_info_str(qemu_get_queue(s->nic), (uint8_t *)macaddr);
> + s->mac_changed = 0;
Need to use spaces for indent in qemu.
> }
> }
>
> diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
Best to split out in a separate commit.
> index 5329f44..6dac10c 100644
> --- a/hw/net/rtl8139.c
> +++ b/hw/net/rtl8139.c
> @@ -476,6 +476,8 @@ typedef struct RTL8139State {
>
> uint16_t CpCmd;
> uint8_t TxThresh;
> + uint8_t mac_changed;
This new state has to be migrated then, and
we need to fallback to old behaviour if migrating to/from
an old version.
> +#define RTL8139_MAC_CHANGED_ALL 0x3F
>
> NICState *nic;
> NICConf conf;
> @@ -1215,6 +1217,7 @@ static void rtl8139_reset(DeviceState *d)
> /* restore MAC address */
> memcpy(s->phys, s->conf.macaddr.a, 6);
> qemu_format_nic_info_str(qemu_get_queue(s->nic), s->phys);
> + s->mac_changed = 0;
>
> /* reset interrupt mask */
> s->IntrStatus = 0;
> @@ -2741,12 +2744,13 @@ static void rtl8139_io_writeb(void *opaque, uint8_t
> addr, uint32_t val)
>
> switch (addr)
> {
> - case MAC0 ... MAC0+4:
> - s->phys[addr - MAC0] = val;
> - break;
> - case MAC0+5:
> + case MAC0 ... MAC0+5:
> s->phys[addr - MAC0] = val;
> - qemu_format_nic_info_str(qemu_get_queue(s->nic), s->phys);
> + s->mac_changed |= (1 << (addr - MAC0));
Better drop the external () here otherwise it starts looking like lisp :)
> + if (s->mac_changed == RTL8139_MAC_CHANGED_ALL) {
> + qemu_format_nic_info_str(qemu_get_queue(s->nic), s->phys);
> + s->mac_changed = 0;
> + }
> break;
> case MAC0+6 ... MAC0+7:
> /* reserved */
> --
> 1.8.4.2
