From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45687) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VgGDU-0002VM-Tr for qemu-devel@nongnu.org; Tue, 12 Nov 2013 10:49:30 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VgGDO-00074o-Tp for qemu-devel@nongnu.org; Tue, 12 Nov 2013 10:49:24 -0500 Received: from mx1.redhat.com ([209.132.183.28]:28384) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VgGDO-000722-L8 for qemu-devel@nongnu.org; Tue, 12 Nov 2013 10:49:18 -0500 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id rACFnHda026880 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 12 Nov 2013 10:49:17 -0500 Message-ID: <52824DFB.4070700@redhat.com> Date: Tue, 12 Nov 2013 10:49:15 -0500 From: Vlad Yasevich MIME-Version: 1.0 References: <1383650238-16015-1-git-send-email-akong@redhat.com> <1383939747-10158-1-git-send-email-vyasevic@redhat.com> <20131110121151.GA5908@redhat.com> In-Reply-To: <20131110121151.GA5908@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] e1000/rtl8139: update HMP NIC when every bit is written Reply-To: vyasevic@redhat.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: jasowang@redhat.com, akong@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com On 11/10/2013 07:11 AM, Michael S. Tsirkin wrote: > 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? Yes, that should be fine. I'll fix and find a better spot to put it. (may be a hole in the struct). > > 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). Good point. Didn't think of that... > >> +#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. Goofed on the bits. That should have been (1<<0) and (1<<1). > >> } 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. OK > >> 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 :) Heh. OK. Thanks -vlad > >> + 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