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