Re: [Qemu-devel] [PATCH v4 1/3] e1000: allow command-line selection of card model

[Stefan Hajnoczi]

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 <address@hidden>
> > Signed-off-by: Gabriel Somlo <address@hidden>
> > Reviewed-by: Michael S. Tsirkin <address@hidden>
> > Reviewed-by: Peter Crosthwaite <address@hidden>
> > ---
> >  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