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 <rom...@dolbeau.org> > > Signed-off-by: Gabriel Somlo <so...@cmu.edu> > > Reviewed-by: Michael S. Tsirkin <m...@redhat.com> > > Reviewed-by: Peter Crosthwaite <peter.crosthwa...@xilinx.com> > > --- > > 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