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

[Gabriel L. Somlo]

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)" ?

>      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()" ?

If "Yes", then how about the "phy_reg_init[]" table ? It's an array
of uint16_t's with a bunch of statically defined elements, but gets
memove()'d in place during e1000_reset in cpu format, which may not be
appropriate if the host is a big-endian machine.

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) ? :)

Thanks much,
--Gabriel