From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41902) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VfTp6-00030m-GR for qemu-devel@nongnu.org; Sun, 10 Nov 2013 07:09:06 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VfTp0-0000FW-Gk for qemu-devel@nongnu.org; Sun, 10 Nov 2013 07:09:00 -0500 Received: from mx1.redhat.com ([209.132.183.28]:37975) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VfTp0-0000FH-8V for qemu-devel@nongnu.org; Sun, 10 Nov 2013 07:08:54 -0500 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id rAAC8rSb002855 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Sun, 10 Nov 2013 07:08:53 -0500 Date: Sun, 10 Nov 2013 14:11:51 +0200 From: "Michael S. Tsirkin" Message-ID: <20131110121151.GA5908@redhat.com> References: <1383650238-16015-1-git-send-email-akong@redhat.com> <1383939747-10158-1-git-send-email-vyasevic@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1383939747-10158-1-git-send-email-vyasevic@redhat.com> Subject: Re: [Qemu-devel] [PATCH] e1000/rtl8139: update HMP NIC when every bit is written List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vlad Yasevich Cc: jasowang@redhat.com, akong@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com 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 > --- > 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