From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40015) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WrkD6-0001QL-Tn for qemu-devel@nongnu.org; Tue, 03 Jun 2014 04:36:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WrkD1-0006tX-Jm for qemu-devel@nongnu.org; Tue, 03 Jun 2014 04:36:44 -0400 Received: from mx1.redhat.com ([209.132.183.28]:58948) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WrkD1-0006rG-BL for qemu-devel@nongnu.org; Tue, 03 Jun 2014 04:36:39 -0400 Date: Tue, 3 Jun 2014 10:36:31 +0200 From: Stefan Hajnoczi Message-ID: <20140603083631.GB19366@stefanha-thinkpad.muc.redhat.com> References: <1401716009-7005-1-git-send-email-somlo@cmu.edu> <1401716009-7005-2-git-send-email-somlo@cmu.edu> <20140602161748.GA1653@ERROL.INI.CMU.EDU> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20140602161748.GA1653@ERROL.INI.CMU.EDU> Subject: Re: [Qemu-devel] [PATCH v4 1/3] e1000: allow command-line selection of card model List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Gabriel L. Somlo" Cc: peter.crosthwaite@xilinx.com, romain@dolbeau.org, mst@redhat.com, qemu-devel@nongnu.org, agraf@suse.de, pbonzini@redhat.com, afaerber@suse.de On Mon, Jun 02, 2014 at 12:17:48PM -0400, Gabriel L. Somlo wrote: > I was looking over my submission, and have one remaining question: > > On Mon, Jun 02, 2014 at 09:33:27AM -0400, Gabriel L. Somlo wrote: > > Allow selection of different card models from the qemu > > command line, to better accomodate a wider range of guests. > > > > Signed-off-by: Romain Dolbeau > > Signed-off-by: Gabriel Somlo > > Reviewed-by: Michael S. Tsirkin > > Reviewed-by: Peter Crosthwaite > > --- > > hw/net/e1000.c | 120 +++++++++++++++++++++++++++++++++++++++++----------- > > hw/net/e1000_regs.h | 6 +++ > > 2 files changed, 102 insertions(+), 24 deletions(-) > > > > diff --git a/hw/net/e1000.c b/hw/net/e1000.c > > index 8387443..1c51be8 100644 > > --- a/hw/net/e1000.c > > +++ b/hw/net/e1000.c > > > > [...] > > > > @@ -385,6 +390,7 @@ static void e1000_reset(void *opaque) > > d->mit_ide = 0; > > memset(d->phy_reg, 0, sizeof d->phy_reg); > > memmove(d->phy_reg, phy_reg_init, sizeof phy_reg_init); > > + d->phy_reg[PHY_ID2] = edc->phy_id2; > > Should this instead be "cpu_to_le16(edc->phy_id2)" ? No, ->phy_reg[] is in host endian. > > memset(d->mac_reg, 0, sizeof d->mac_reg); > > memmove(d->mac_reg, mac_reg_init, sizeof mac_reg_init); > > d->rxbuf_min_shift = 1; > > @@ -1440,9 +1446,13 @@ static const VMStateDescription vmstate_e1000 = { > > > >[...] > > > > @@ -1531,6 +1542,7 @@ static int pci_e1000_init(PCIDevice *pci_dev) > > macaddr = d->conf.macaddr.a; > > for (i = 0; i < 3; i++) > > d->eeprom_data[i] = (macaddr[2*i+1]<<8) | macaddr[2*i]; > > + d->eeprom_data[11] = d->eeprom_data[13] = pdc->device_id; > > Same here, use "cpu_to_le16()" ? No, ->eeprom_data[] is in host endian. > I'm primarily concerned about the correctness of my own patch set here, > but wondering if e1000.c might need additional work (e.g. phy_reg_init[] > and friends) ? :) The hardware register accesses go through QEMU MemoryRegion. The memory regions are marked DEVICE_LITTLE_ENDIAN so the QEMU core device emulation code handles the byteswapping for us (if necessary). The e1000 code itself operates in host endian. The exception to this are the guest-memory DMA structures like the descriptor rings. Here we need to byteswap since we're reading/writing guest memory ourselves. Since your patch doesn't touch this you shouldn't need to worry. Stefan