From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52481) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VgK5l-0004U3-Kr for qemu-devel@nongnu.org; Tue, 12 Nov 2013 14:57:47 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VgK5f-0001Uu-KX for qemu-devel@nongnu.org; Tue, 12 Nov 2013 14:57:41 -0500 Received: from mx1.redhat.com ([209.132.183.28]:62688) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VgK5f-0001T7-Cv for qemu-devel@nongnu.org; Tue, 12 Nov 2013 14:57:35 -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 rACJvYQk016229 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 12 Nov 2013 14:57:34 -0500 Message-ID: <5282882C.7030107@redhat.com> Date: Tue, 12 Nov 2013 14:57:32 -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> <20131109034351.GB2450@amosk.info> In-Reply-To: <20131109034351.GB2450@amosk.info> 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: Amos Kong Cc: jasowang@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, mst@redhat.com On 11/08/2013 10:43 PM, Amos Kong 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. > > Hi Vlad, > > Looks good to me. > > Using this patch, we don't need to care the writing order. > If we add event notify in future, it can reduce the noise > event. > > BTW, can we use a tmp buffer to record the new mac (changing is unfinished), > and write the new mac to registers until all bits is written? I've thought about this as well, but I am not sure what it would buy us and what's the best way to do it. At first I thought that we could have a function local static that we could accumulate into. But then Michael mentioned migration and what happens if we migrate in middle of the mac change? So we put into the device, but then it isn't much different then what we have now with the possible exception that the mac change happens slightly "more atomically". :) For this, it might make more sense to temporarily stop the link when the mac address change is happening. -vlad > > Amos > >> -vlad >> >> -- >8 -- >> Subject: [PATCH] e1000/rtl8139: update HMP NIC when every bit is written > > Subject: [PATCH] e1000/rtl8139: update HMP NIC when all bits are 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. >> >> 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; >> +#define E1000_RA0_CHANGED 0 >> +#define E1000_RA1_CHANGED 1 >> +#define E1000_RA_ALL_CHANGED (E1000_RA0_CHANGED|E1000_RA1_CHANGED) >> } 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) { >> 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; >> } >> } >> >> diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c >> 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; >> +#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)); >> + 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