From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35626) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ViV8D-0004cG-4B for qemu-devel@nongnu.org; Mon, 18 Nov 2013 15:09:19 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ViV87-0000Ip-1c for qemu-devel@nongnu.org; Mon, 18 Nov 2013 15:09:12 -0500 Received: from mx1.redhat.com ([209.132.183.28]:17805) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ViV86-0000Gp-Ji for qemu-devel@nongnu.org; Mon, 18 Nov 2013 15:09:06 -0500 Message-ID: <528A73DF.40807@redhat.com> Date: Mon, 18 Nov 2013 15:09:03 -0500 From: Vlad Yasevich MIME-Version: 1.0 References: <20131118194741.GA32700@redhat.com> <1384804699.2879.195.camel@ul30vt.home> In-Reply-To: <1384804699.2879.195.camel@ul30vt.home> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH for-1.7] Revert "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: Alex Williamson , "Michael S. Tsirkin" Cc: Paolo Bonzini , Amos Kong , Stefan Hajnoczi , qemu-devel@nongnu.org, Anthony Liguori On 11/18/2013 02:58 PM, Alex Williamson wrote: > On Mon, 2013-11-18 at 21:47 +0200, Michael S. Tsirkin wrote: >> This reverts commit cd5be5829c1ce87aa6b3a7806524fac07ac9a757. >> Digging into hardware specs shows this does not >> actually make QEMU behave more like hardware. >> Let's stick to the tried heuristic for 1.7 and >> possibly revisit for 1.8. > > If this is broken, then so are these: > > 23c37c37f0280761072c23bf67d3a4f3c0ff25aa > 7c36507c2b8776266f50c5e2739bd18279953b93 These aren't really broken. They just assume that the high order writes will happen after the low order writes. In the case of e1000, this is a little more then an assumption (particularly in the case of nic initilization). In the case of RTL nic, it is just an assumption, but it hasn't been shown faulty yet. We do plan to address this a bit more thoroughly. The patch that was applied was controversial and more then 1 person expressed reservations. -vlad > > None of these change the behavior of hardware, they only change when the > monitor gets told about mac address changes. I'd suggest either add the > emulation described in each spec or revert all of them. A partial > revert is just noise. Thanks, > > Alex > >> >> Reported-by: Vlad Yasevich >> Cc: Amos Kong >> Cc: Alex Williamson >> --- >> hw/net/e1000.c | 2 +- >> hw/net/rtl8139.c | 5 ++++- >> 2 files changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/hw/net/e1000.c b/hw/net/e1000.c >> index ae63591..8387443 100644 >> --- a/hw/net/e1000.c >> +++ b/hw/net/e1000.c >> @@ -1106,7 +1106,7 @@ mac_writereg(E1000State *s, int index, uint32_t val) >> >> s->mac_reg[index] = val; >> >> - if (index == RA || index == RA + 1) { >> + if (index == RA + 1) { >> 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); >> diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c >> index 7f2b4db..5329f44 100644 >> --- a/hw/net/rtl8139.c >> +++ b/hw/net/rtl8139.c >> @@ -2741,7 +2741,10 @@ static void rtl8139_io_writeb(void *opaque, uint8_t addr, uint32_t val) >> >> switch (addr) >> { >> - case MAC0 ... MAC0+5: >> + case MAC0 ... MAC0+4: >> + s->phys[addr - MAC0] = val; >> + break; >> + case MAC0+5: >> s->phys[addr - MAC0] = val; >> qemu_format_nic_info_str(qemu_get_queue(s->nic), s->phys); >> break; > > >