From patchwork Thu Nov 7 15:43:33 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Vlad Yasevich X-Patchwork-Id: 289486 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 096BE2C00BD for ; Fri, 8 Nov 2013 09:30:37 +1100 (EST) Received: from localhost ([::1]:42783 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VeXJC-0003Ys-Oz for incoming@patchwork.ozlabs.org; Thu, 07 Nov 2013 16:40:10 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33106) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VeRkE-0007Fp-1S for qemu-devel@nongnu.org; Thu, 07 Nov 2013 10:43:48 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VeRk8-0006LB-0T for qemu-devel@nongnu.org; Thu, 07 Nov 2013 10:43:41 -0500 Received: from mx1.redhat.com ([209.132.183.28]:36324) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VeRk7-0006Kt-Pn for qemu-devel@nongnu.org; Thu, 07 Nov 2013 10:43:35 -0500 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id rA7FhYmo008118 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 7 Nov 2013 10:43:34 -0500 Received: from [10.3.113.153] (ovpn-113-153.phx2.redhat.com [10.3.113.153]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id rA7FhXEO024532; Thu, 7 Nov 2013 10:43:33 -0500 Message-ID: <527BB525.4050900@redhat.com> Date: Thu, 07 Nov 2013 10:43:33 -0500 From: Vlad Yasevich Organization: Red Hat User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.0 MIME-Version: 1.0 To: qemu-devel@nongnu.org References: <1383650238-16015-1-git-send-email-akong@redhat.com> <20131107065922.GA29567@redhat.com> <20131107073229.GA10114@amosk.info> <20131107102615.GB31075@redhat.com> <1383834837.21055.240.camel@ul30vt.home> <20131107152728.GA3739@redhat.com> In-Reply-To: <20131107152728.GA3739@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.22 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 209.132.183.28 X-Mailman-Approved-At: Thu, 07 Nov 2013 16:39:12 -0500 Subject: Re: [Qemu-devel] [PATCH] e1000/rtl8139: update HMP NIC when every bit is written X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list Reply-To: vyasevic@redhat.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org On 11/07/2013 10:27 AM, Michael S. Tsirkin wrote: > On Thu, Nov 07, 2013 at 07:33:57AM -0700, Alex Williamson wrote: >> On Thu, 2013-11-07 at 12:26 +0200, Michael S. Tsirkin wrote: >>> On Thu, Nov 07, 2013 at 03:32:29PM +0800, Amos Kong wrote: >>>> On Thu, Nov 07, 2013 at 08:59:22AM +0200, Michael S. Tsirkin wrote: >>>>> On Tue, Nov 05, 2013 at 07:17:18PM +0800, Amos Kong wrote: >>>>>> 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 is changed. It will be same as virtio-net. >>>>>> >>>>>> Signed-off-by: Amos Kong >>>>> >>>>> I'm not sure I buy this. >>>>> >>>>> If we actually implement e.g. mac change notifications, >>>>> sending them on writes of random bytes will confuse >>>>> the host. >>>> >>>> This patch just effects the monitor display of macaddr. >>>> During each writing, the macaddr is used for rx-filter is really >>>> changed. >>>> >>>> In the real hardware, it supports to just write part of bits, >>>> the rx-filtering is effected by every bit writing. >>> >>> Yes but again, the window can just be too small to matter >>> on real hardware. >>> >>> Our emulation is not perfect, fixing this to be just like real >>> hardware just might expose other bugs we can't fix >>> that easily. >> >> If we were to implement mac change notification, then every partial >> update would send a notify and the host. Is that a problem? It seems >> no different than if the device had an atomic mac update procedure and >> the guest admin changed the mac several times. > > I think modern nics make address updates atomic. > Problem is, we are emulating an ancient one, > so to make host configuration of a modern one > reasonable we need to resort to tricks. > >> The problem with assuming that a given byte is always written last is >> that unless the hardware spec identifies an order, we're just basing our >> code on examples where we know what the guest driver does, either by >> code inspection or access tracing. If there are examples of guests that >> write the last byte first, then the host will never know the correct mac >> address. Maybe there are no guests that use the wrong order, but that's >> a pretty exhaustive search. > > I agree what we have is a hack. Maybe we need some better hacks. > For example, maybe we should update mac at link state change > (I think link is always down when mac is updated?). > Needs some thought. I thought this too, but checking recent linux kernel, e1000 and rtl8139 seem to allow live mac change so link is up. So here is a stupid, untested patch for e1000 to notify only once: Any thoughts? -vlad diff --git a/hw/net/e1000.c b/hw/net/e1000.c index 8387443..b99eba4 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_change_flags; +#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_change_flags = 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_change_flags |= E1000_RA0_CHANGED; + break; + case (RA + 1): + s->mac_change_flags |= E1000_RA1_CHANGED; + break; + } + + if (!(s->mac_change_flags ^ 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_change_flags = 0; } }