e1000: add the ability to select among several specific types of e1000, plus some pointers to documentations and details.

Hello,

This patch adds this ability to select a specific variant of
the e1000 virtual ethernet from the command-line. Previously
only a single device was available, selected at compile time.
This helps for guests with limited hardware support, as with
this patch not all guests are required to use the same device.

So for instance to use the emulated 82545EM in OSX[1], a user
would add e.g. the options:

"-netdev user,id=mynet0 -device 82545EM,netdev=mynet0"

Note that the value 'e1000' is retained to mean "whatever
is the compiled-in default", as previously.

There's also some comments to help future developers to
identify relevant informations in the documentations.
There's also a couple of fixed values for constants
(didn't match the current driver).

Known cosmetic issues:
1) Some lines are long than 80 characters to avoid breaking
down URLs and titles.
2) One of the array initializer generates the error:
####
ERROR: space prohibited before open square bracket '['
#107: FILE: hw/net/e1000.c:256:
+    [PHY_ID1] = 0x141,                          [PHY_ID2] = 0xc20,
####
Despite being formatted exactly like the surrounding lines
(minus the tabs, another source of error in checkpatch.pl).

No maintainer CC:, because I couldn't find a maintainer for
hw/net/e1000 or hw/net/*.

Cordially,

Romain Dolbeau

[1] See <http://www.contrib.andrew.cmu.edu/~somlo/OSXKVM/>
    for details

Signed-off-by: Romain Dolbeau <romain@dolbeau.org>
---
 hw/net/e1000.c      |  195 +++++++++++++++++++++++++++++++++++++++++++--------
 hw/net/e1000_regs.h |   65 +++++++++++------
 hw/pci/pci.c        |    6 ++
 3 files changed, 215 insertions(+), 51 deletions(-)

On Mon, 24 Feb 2014 15:08:17 -0500 Romain Dolbeau <romain@dolbeau.org> wrote:
> This patch adds this ability to select a specific variant of
> the e1000 virtual ethernet from the command-line. Previously
> only a single device was available, selected at compile time.
> This helps for guests with limited hardware support, as with
> this patch not all guests are required to use the same device.
> 
> So for instance to use the emulated 82545EM in OSX[1], a user
> would add e.g. the options:
> 
> "-netdev user,id=mynet0 -device 82545EM,netdev=mynet0"

Apparently, starting with 10.9 (Mavericks), the built-in Intel e1000
driver (AppleIntel8254XEthernet.kext) no longer works with the default
e1000 "flavor" (DEVICE_ID) emulated by QEMU (E1000_DEV_ID_82540EM). It
seems to work great with E1000_DEV_ID_82545EM_COPPER, though.

E1000_DEV_ID_82545EM_COPPER, however, does not work with Windows (I
tried XP and 7). So there isn't currently one e1000 DEVIE_ID that
could "rule them all", so to speak... Linux works fine with whatever,
but then we already expected that :)

So, I think it would be nice to be able to select the specific
"flavor" of e1000 at run time, along the lines of what this patch
does.

Thanks,
--Gabriel

Hi,

Am 24.02.2014 20:08, schrieb Romain Dolbeau:
> Hello,
> 
> This patch adds this ability to select a specific variant of
> the e1000 virtual ethernet from the command-line. Previously
> only a single device was available, selected at compile time.
> This helps for guests with limited hardware support, as with
> this patch not all guests are required to use the same device.
> 
> So for instance to use the emulated 82545EM in OSX[1], a user
> would add e.g. the options:
> 
> "-netdev user,id=mynet0 -device 82545EM,netdev=mynet0"
> 
> Note that the value 'e1000' is retained to mean "whatever
> is the compiled-in default", as previously.
> 
> There's also some comments to help future developers to
> identify relevant informations in the documentations.
> There's also a couple of fixed values for constants
> (didn't match the current driver).
> 
> Known cosmetic issues:
> 1) Some lines are long than 80 characters to avoid breaking
> down URLs and titles.
> 2) One of the array initializer generates the error:
> ####
> ERROR: space prohibited before open square bracket '['
> #107: FILE: hw/net/e1000.c:256:
> +    [PHY_ID1] = 0x141,                          [PHY_ID2] = 0xc20,
> ####
> Despite being formatted exactly like the surrounding lines
> (minus the tabs, another source of error in checkpatch.pl).
> 
> No maintainer CC:, because I couldn't find a maintainer for
> hw/net/e1000 or hw/net/*.

CC'ing Stefan as maintainer.

> 
> Cordially,
> 
> Romain Dolbeau

When you send a "PATCH" please send it in a form we can actually commit,
without letter-style text. That can go into a cover letter 0/1 or below
--- line.

> 
> [1] See <http://www.contrib.andrew.cmu.edu/~somlo/OSXKVM/>
>     for details
> 
> Signed-off-by: Romain Dolbeau <romain@dolbeau.org>
> ---
>  hw/net/e1000.c      |  195 +++++++++++++++++++++++++++++++++++++++++++--------
>  hw/net/e1000_regs.h |   65 +++++++++++------
>  hw/pci/pci.c        |    6 ++
>  3 files changed, 215 insertions(+), 51 deletions(-)
> 
> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
> index 8387443..9073c1a 100644
> --- a/hw/net/e1000.c
> +++ b/hw/net/e1000.c
> @@ -1,8 +1,10 @@
>  /*
>   * QEMU e1000 emulation
>   *
> - * Software developer's manual:
> - * http://download.intel.com/design/network/manuals/8254x_GBe_SDM.pdf
> + * Software developer's manual (PCI, PCI-X):
> + * <http://download.intel.com/design/network/manuals/8254x_GBe_SDM.pdf>
> + * Software developer's manual (PCIe):
> + * <http://www.intel.com/content/dam/www/public/us/en/documents/manuals/pcie-gbe-controllers-open-source-manual.pdf>
>   *
>   * Nir Peleg, Tutis Systems Ltd. for Qumranet Inc.
>   * Copyright (c) 2008 Qumranet
> @@ -10,6 +12,8 @@
>   * Copyright (c) 2007 Dan Aloni
>   * Copyright (c) 2004 Antony T Curtis
>   *
> + * Additional modifications (c) 2014 Romain Dolbeau
> + *
>   * This library is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU Lesser General Public
>   * License as published by the Free Software Foundation; either
> @@ -69,10 +73,11 @@ static int debugflags = DBGBIT(TXERR) | DBGBIT(GENERAL);
>  
>  /*
>   * HW models:
> - *  E1000_DEV_ID_82540EM works with Windows and Linux
> - *  E1000_DEV_ID_82573L OK with windoze and Linux 2.6.22,
> - *	appears to perform better than 82540EM, but breaks with Linux 2.6.18
> + *  E1000_DEV_ID_82540EM works with Windows and Linux, and is the default
>   *  E1000_DEV_ID_82544GC_COPPER appears to work; not well tested
> + *  E1000_DEV_ID_82545EM_COPPER appears to work with OSX 10.9[.1]
> + *  It seems those 3 are mostly identical anyway, so picking one
> + *  over the others is a matter of guest support.
>   *  Others never tested
>   */
>  enum { E1000_DEVID = E1000_DEV_ID_82540EM };
> @@ -81,10 +86,22 @@ enum { E1000_DEVID = E1000_DEV_ID_82540EM };
>   * May need to specify additional MAC-to-PHY entries --
>   * Intel's Windows driver refuses to initialize unless they match
>   */
> -enum {
> -    PHY_ID2_INIT = E1000_DEVID == E1000_DEV_ID_82573L ?		0xcc2 :
> -                   E1000_DEVID == E1000_DEV_ID_82544GC_COPPER ?	0xc30 :
> -                   /* default to E1000_DEV_ID_82540EM */	0xc20
> +/* PHY_ID1:
> + * Most 8254x uses 0x141, but 82541xx and 82547GI/EI uses 0x2a8,
> + * and so do the 631xESB/632xESB, 82571EB/82572EI.
> + * The 82573E/82573V/82573L and 82563EB/82564EB also uses 0x141.
> + * <http://download.intel.com/design/network/manuals/8254x_GBe_SDM.pdf> page 250
> + * <http://www.intel.com/content/dam/www/public/us/en/documents/manuals/pcie-gbe-controllers-open-source-manual.pdf> page 305
> + */
> +const uint16_t PHY_ID1_INIT[][2] = {
> +  { E1000_DEV_ID_80003ES2LAN_COPPER_DPT, 0x2a8 },
> +  { -1, 0x141 } /* default for 82540EM and many others */
> +};
> +const uint16_t PHY_ID2_INIT[][2] = {
> +  { E1000_DEV_ID_82573L, 0xcc2 },
> +  { E1000_DEV_ID_82544GC_COPPER, 0xc30 },
> +  { -1, 0xc20 } /* default for 82540EM and many others ; this is one
> +                   is a lot more device-specific */
>  };
>  
>  typedef struct E1000State_st {
> @@ -153,8 +170,8 @@ typedef struct E1000State_st {
>  
>  #define TYPE_E1000 "e1000"
>  
> -#define E1000(obj) \
> -    OBJECT_CHECK(E1000State, (obj), TYPE_E1000)
> +#define E1000(obj)                              \
> +  DO_UPCAST(E1000State, parent_obj, obj)

No, don't go backwards in time please. Take a look at e.g. eepro100,
which already registers multiple data-driven subtypes for instance. The
key to making it work is to introduce an abstract base type matching
E1000() macro, with original "e1000" type becoming a subtype thereof.

>  
>  #define	defreg(x)	x = (E1000_##x>>2)
>  enum {
> @@ -235,7 +252,8 @@ static const char phy_regcap[0x20] = {
>  static const uint16_t phy_reg_init[] = {
>      [PHY_CTRL] = 0x1140,
>      [PHY_STATUS] = 0x794d, /* link initially up with not completed autoneg */
> -    [PHY_ID1] = 0x141,				[PHY_ID2] = PHY_ID2_INIT,
> +     /* both phy_id will be replaced */
> +    [PHY_ID1] = 0x141,                          [PHY_ID2] = 0xc20,
>      [PHY_1000T_CTRL] = 0x0e00,			[M88E1000_PHY_SPEC_CTRL] = 0x360,
>      [M88E1000_EXT_PHY_SPEC_CTRL] = 0x0d60,	[PHY_AUTONEG_ADV] = 0xde1,
>      [PHY_LP_ABILITY] = 0x1e0,			[PHY_1000T_STATUS] = 0x3c00,
> @@ -269,10 +287,11 @@ static void
>  set_interrupt_cause(E1000State *s, int index, uint32_t val)
>  {
>      PCIDevice *d = PCI_DEVICE(s);
> +    PCIDeviceClass *pdc = PCI_DEVICE_GET_CLASS(d);
>      uint32_t pending_ints;
>      uint32_t mit_delay;
>  
> -    if (val && (E1000_DEVID >= E1000_DEV_ID_82547EI_MOBILE)) {
> +    if (val && (pdc->device_id >= E1000_DEV_ID_82547EI_MOBILE)) {
>          /* Only for 8257x */
>          val |= E1000_ICR_INT_ASSERTED;
>      }
> @@ -375,6 +394,8 @@ rxbufsize(uint32_t v)
>  static void e1000_reset(void *opaque)
>  {
>      E1000State *d = opaque;
> +    PCIDevice *dev = PCI_DEVICE(d);
> +    PCIDeviceClass *pdc = PCI_DEVICE_GET_CLASS(dev);
>      uint8_t *macaddr = d->conf.macaddr.a;
>      int i;
>  
> @@ -385,6 +406,26 @@ 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);
> +    uint16_t phy_id1 = -1;
> +    uint16_t phy_id2 = -1;

Please declare variables at the top of the block alongside others.

> +    for (i = 0 ; PHY_ID1_INIT[i][0] != (uint16_t)-1 ; i++) {
> +        if (PHY_ID1_INIT[i][0] == pdc->device_id) {
> +            phy_id1 = PHY_ID1_INIT[i][1];
> +        }
> +    }
> +    if (phy_id1 == (uint16_t)-1) {
> +        phy_id1 = PHY_ID1_INIT[i][1];
> +    }
> +    for (i = 0 ; PHY_ID2_INIT[i][0] != (uint16_t)-1 ; i++) {
> +        if (PHY_ID2_INIT[i][0] == pdc->device_id) {
> +            phy_id2 = PHY_ID2_INIT[i][1];
> +        }
> +    }
> +    if (phy_id2 == (uint16_t)-1) {
> +        phy_id2 = PHY_ID2_INIT[i][1];
> +    }
> +    d->phy_reg[PHY_ID1] = phy_id1;
> +    d->phy_reg[PHY_ID2] = 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,15 +1481,62 @@ static const VMStateDescription vmstate_e1000 = {
>      }
>  };
>  
> +/*
> + * The content of EEPROM is documented in the documentation
> + * PCI/X: "Table 5-2. Ethernet Controller Address Map" page 98 (except 82544GC/EI and 82541ER)
> + * PCI/X: "Table 5-3. 82544GC/EI and 82541ER EEPROM Address Map" page 102
> + * PCIe: "Table 5-2. Ethernet Controller EEPROM Map" page 134
> + */
>  static const uint16_t e1000_eeprom_template[64] = {
> -    0x0000, 0x0000, 0x0000, 0x0000,      0xffff, 0x0000,      0x0000, 0x0000,
> -    0x3000, 0x1000, 0x6403, E1000_DEVID, 0x8086, E1000_DEVID, 0x8086, 0x3040,
> -    0x0008, 0x2000, 0x7e14, 0x0048,      0x1000, 0x00d8,      0x0000, 0x2700,
> -    0x6cc9, 0x3150, 0x0722, 0x040b,      0x0984, 0x0000,      0xc000, 0x0706,
> -    0x1008, 0x0000, 0x0f04, 0x7fff,      0x4d01, 0xffff,      0xffff, 0xffff,
> -    0xffff, 0xffff, 0xffff, 0xffff,      0xffff, 0xffff,      0xffff, 0xffff,
> -    0x0100, 0x4000, 0x121c, 0xffff,      0xffff, 0xffff,      0xffff, 0xffff,
> -    0xffff, 0xffff, 0xffff, 0xffff,      0xffff, 0xffff,      0xffff, 0x0000,
> +    /* 00h - 02h: Ethernet address, will be overridden */
> +    0x0000, 0x0000, 0x0000,
> +    /* 03h: compatibility, this seem a bit device-specific

"this seems"

> +            and probably should be overridden */
> +    0x0000,
> +    /* 04h: compatibility (PCIe) or SerDes config (most PCI/X) or LED */
> +    0xffff,
> +    /* 05h - 07h: EEprom version & OEM (PCIe other than 82573),
> +                  compatibility (most PCI/X, 82573) */
> +    0x0000, 0x0000, 0x0000,
> +    /* 08h - 09h : PBA (irrelevant) */
> +    0x3000, 0x1000,
> +    /* 0ah: init control 1 */
> +    0x6403,
> +    /* 0bh - 0eh: PCI ID, will be overridden */
> +    E1000_DEVID, 0x8086, E1000_DEVID, 0x8086,
> +    /* 0fh : init control 2 */
> +    0x3040,
> +    /* three next (word 10h, 11h, 12h) seem quite device-specific
> +            with several variants */
> +    0x0008, 0x2000, 0x7e14,
> +    /* management */
> +    0x0048,
> +    /* init control 3 (2nd LAN?), not 82573 */
> +    0x1000,
> +    /* IPv4 (PCI/X) or  reserved (PCIe), not 82573 */
> +    0x00d8, 0x0000,
> +    /* another batch of variants ; 17h - 1Eh is IPv6 LAN in PCI/X
> +     but is FW Config Start Address (17h, most PCIe) followed
> +    by PCI init configuration and stuff */

This comment looks off. Please use *-aligned multi-line comments.

While at it, please avoid the French punctuation here and elsewhere. ;)

> +    0x2700, 0x6cc9, 0x3150, 0x0722, 0x040b, 0x0984, 0x0000, 0xc000,
> +    /* 1fh : mostly reserved (PCI/X), LED conf (PCIe) */
> +    0x0706,
> +    /* 20h: software defined pin controls, 21h: mostly control */
> +    0x1008, 0x0000,
> +    /* LAN power, management control (not 82573) */
> +    0x0f04, 0x7fff,
> +    /* init control 3 (again?) */
> +    0x4d01,
> +    /* 25h-2eh: either IP4/6 (PCI/X) or mostly reserved (PCIe) */
> +    0xffff, 0xffff, 0xffff, 0xffff, 0xffff,
> +    0xffff, 0xffff, 0xffff, 0xffff, 0xffff,
> +    /* 2fh: LEDCTL Default (PCI/X) or Vital Product Data (VPD) Pointer (PCIe) */
> +    0xffff,
> +    /* 30h-3eh: mostly PXE/boot stuff */
> +    0x0100, 0x4000, 0x121c, 0xffff, 0xffff, 0xffff, 0xffff, 0xffff,
> +    0xffff, 0xffff, 0xffff, 0xffff, 0xffff, 0xffff, 0xffff,
> +    /* checksum to be computed */
> +    0x0000
>  };
>  
>  /* PCI interface */
> @@ -1505,6 +1593,7 @@ static NetClientInfo net_e1000_info = {
>  
>  static int pci_e1000_init(PCIDevice *pci_dev)
>  {
> +    PCIDeviceClass *pdc = PCI_DEVICE_GET_CLASS(pci_dev);
>      DeviceState *dev = DEVICE(pci_dev);
>      E1000State *d = E1000(pci_dev);
>      uint8_t *pci_conf;

I'd prefer to see this either below or directly above E1000State since
the signature of the function is going to change to DeviceState *dev as
part of the QOM realize conversion.

> @@ -1531,6 +1620,9 @@ 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];
> +    /* update eeprom with the proper device_id */
> +    d->eeprom_data[11] = pdc->device_id;
> +    d->eeprom_data[13] = pdc->device_id;
>      for (i = 0; i < EEPROM_CHECKSUM_REG; i++)
>          checksum += d->eeprom_data[i];
>      checksum = (uint16_t) EEPROM_SUM - checksum;
> @@ -1551,7 +1643,8 @@ static int pci_e1000_init(PCIDevice *pci_dev)
>  
>  static void qdev_e1000_reset(DeviceState *dev)
>  {
> -    E1000State *d = E1000(dev);
> +    PCIDevice *pci_dev = DO_UPCAST(PCIDevice, qdev, dev);

PCI_DEVICE(dev) please! DO_UPCAST() is deprecated for QOM types.

> +    E1000State *d = E1000(pci_dev);
>      e1000_reset(d);
>  }
>  
> @@ -1564,17 +1657,26 @@ static Property e1000_properties[] = {
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> +typedef struct E1000Info E1000Info;
> +struct E1000Info {
> +    const char *name;
> +    uint16_t   vendor_id;
> +    uint16_t   device_id;
> +    uint8_t    revision;
> +};
> +
>  static void e1000_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> +    E1000Info *info = data;
>  
>      k->init = pci_e1000_init;
>      k->exit = pci_e1000_uninit;
>      k->romfile = "efi-e1000.rom";
> -    k->vendor_id = PCI_VENDOR_ID_INTEL;
> -    k->device_id = E1000_DEVID;
> -    k->revision = 0x03;
> +    k->vendor_id = info->vendor_id;
> +    k->device_id = info->device_id;
> +    k->revision = info->revision;
>      k->class_id = PCI_CLASS_NETWORK_ETHERNET;
>      set_bit(DEVICE_CATEGORY_NETWORK, dc->categories);
>      dc->desc = "Intel Gigabit Ethernet";
> @@ -1583,16 +1685,47 @@ static void e1000_class_init(ObjectClass *klass, void *data)
>      dc->props = e1000_properties;
>  }
>  
> -static const TypeInfo e1000_info = {
> -    .name          = TYPE_E1000,
> -    .parent        = TYPE_PCI_DEVICE,
> -    .instance_size = sizeof(E1000State),
> -    .class_init    = e1000_class_init,
> +static E1000Info e1000_info_array[] = {

static const possibly?

> +    {
> +        .name       = "e1000",
> +        .vendor_id = PCI_VENDOR_ID_INTEL,
> +        .device_id = E1000_DEVID,
> +        .revision  = 0x03,
> +    },
> +    {
> +        .name       = "82540EM",
> +        .vendor_id = PCI_VENDOR_ID_INTEL,
> +        .device_id = E1000_DEV_ID_82540EM,
> +        .revision  = 0x03,
> +    },
> +    {
> +        .name       = "82544GC",
> +        .vendor_id = PCI_VENDOR_ID_INTEL,
> +        .device_id = E1000_DEV_ID_82544GC_COPPER,
> +        .revision  = 0x03,
> +    },
> +    {
> +        .name       = "82545EM",
> +        .vendor_id = PCI_VENDOR_ID_INTEL,
> +        .device_id = E1000_DEV_ID_82545EM_COPPER,
> +        .revision  = 0x03,
> +    }

Each .name looks off by one?

>  };
>  
>  static void e1000_register_types(void)
>  {
> -    type_register_static(&e1000_info);
> +    TypeInfo e1000_info = {
> +      .name          = TYPE_E1000,

Since you're overriding this below, better drop it here to avoid confusion.

> +      .parent        = TYPE_PCI_DEVICE,
> +      .instance_size = sizeof(E1000State),

This you will only need for the base type, not for each derived type.

> +      .class_init    = e1000_class_init,
> +    };
> +    int i;
> +    for (i = 0; i < ARRAY_SIZE(e1000_info_array); i++) {
> +        e1000_info.name = e1000_info_array[i].name;
> +        e1000_info.class_data = e1000_info_array + i;
> +        type_register(&e1000_info);
> +    }
>  }
>  
>  type_init(e1000_register_types)
> diff --git a/hw/net/e1000_regs.h b/hw/net/e1000_regs.h
> index c9cb79e..93160c0 100644
> --- a/hw/net/e1000_regs.h
> +++ b/hw/net/e1000_regs.h
> @@ -34,46 +34,71 @@
>  
>  
>  /* PCI Device IDs */
> +/* Where is the documentation for those 3 ?
> +   (they are nonetheless in e1000.ko) */
>  #define E1000_DEV_ID_82542               0x1000
>  #define E1000_DEV_ID_82543GC_FIBER       0x1001
>  #define E1000_DEV_ID_82543GC_COPPER      0x1004
> -#define E1000_DEV_ID_82544EI_COPPER      0x1008
> -#define E1000_DEV_ID_82544EI_FIBER       0x1009
> -#define E1000_DEV_ID_82544GC_COPPER      0x100C
> -#define E1000_DEV_ID_82544GC_LOM         0x100D
> +/* <http://www.intel.com/content/dam/doc/manual/pci-pci-x-family-gbe-controllers-software-dev-manual.pdf>
> + * "PCI/PCI-X Family of Gigabit Ethernet Controllers Software Developer's Manual"
> + * documents
> + * 82540EP/EM, 82541xx, 82544GC/EI, 82545GM/EM, 82546GB/EB, and 82547xx
> + * Those are handled by driver 'e1000'
> + */
>  #define E1000_DEV_ID_82540EM             0x100E
>  #define E1000_DEV_ID_82540EM_LOM         0x1015
>  #define E1000_DEV_ID_82540EP_LOM         0x1016
>  #define E1000_DEV_ID_82540EP             0x1017
>  #define E1000_DEV_ID_82540EP_LP          0x101E
> +
> +#define E1000_DEV_ID_82541EI             0x1013
> +#define E1000_DEV_ID_82541EI_MOBILE      0x1018
> +#define E1000_DEV_ID_82541ER_LOM         0x1014
> +#define E1000_DEV_ID_82541ER             0x1078
> +#define E1000_DEV_ID_82541GI             0x1076
> +#define E1000_DEV_ID_82541GI_MOBILE      0x1077
> +#define E1000_DEV_ID_82541GI_LF          0x107C
> +
> +#define E1000_DEV_ID_82544EI_COPPER      0x1008
> +#define E1000_DEV_ID_82544EI_FIBER       0x1009
> +#define E1000_DEV_ID_82544GC_COPPER      0x100C
> +#define E1000_DEV_ID_82544GC_LOM         0x100D
> +
>  #define E1000_DEV_ID_82545EM_COPPER      0x100F
>  #define E1000_DEV_ID_82545EM_FIBER       0x1011
>  #define E1000_DEV_ID_82545GM_COPPER      0x1026
>  #define E1000_DEV_ID_82545GM_FIBER       0x1027
>  #define E1000_DEV_ID_82545GM_SERDES      0x1028
> +
>  #define E1000_DEV_ID_82546EB_COPPER      0x1010
>  #define E1000_DEV_ID_82546EB_FIBER       0x1012
>  #define E1000_DEV_ID_82546EB_QUAD_COPPER 0x101D
> -#define E1000_DEV_ID_82541EI             0x1013
> -#define E1000_DEV_ID_82541EI_MOBILE      0x1018
> -#define E1000_DEV_ID_82541ER_LOM         0x1014
> -#define E1000_DEV_ID_82541ER             0x1078
> -#define E1000_DEV_ID_82547GI             0x1075
> -#define E1000_DEV_ID_82541GI             0x1076
> -#define E1000_DEV_ID_82541GI_MOBILE      0x1077
> -#define E1000_DEV_ID_82541GI_LF          0x107C
>  #define E1000_DEV_ID_82546GB_COPPER      0x1079
>  #define E1000_DEV_ID_82546GB_FIBER       0x107A
>  #define E1000_DEV_ID_82546GB_SERDES      0x107B
>  #define E1000_DEV_ID_82546GB_PCIE        0x108A
>  #define E1000_DEV_ID_82546GB_QUAD_COPPER 0x1099
> +#define E1000_DEV_ID_82546GB_QUAD_COPPER_KSP3 0x10B5
> +
> +#define E1000_DEV_ID_82547GI             0x1075
>  #define E1000_DEV_ID_82547EI             0x1019
>  #define E1000_DEV_ID_82547EI_MOBILE      0x101A
> +/* <http://www.intel.com/content/dam/www/public/us/en/documents/manuals/pcie-gbe-controllers-open-source-manual.pdf>
> + * "PCIe* GbE Controllers Open Source Software Developer's Manual"
> + * documents:
> + * 631xESB/632xESB, 82563EB/82564EB, 82571EB/82572EI & 82573E/82573V/82573L
> + * Those are actually handled by driver 'e1000e', not 'e1000'
> + */
> +/* it seems the next four are alternative names for 631xESB/632xESB */
> +#define E1000_DEV_ID_80003ES2LAN_COPPER_DPT     0x1096
> +#define E1000_DEV_ID_80003ES2LAN_SERDES_DPT     0x1098
> +#define E1000_DEV_ID_80003ES2LAN_COPPER_SPT     0x10BA
> +#define E1000_DEV_ID_80003ES2LAN_SERDES_SPT     0x10BB
> +
>  #define E1000_DEV_ID_82571EB_COPPER      0x105E
>  #define E1000_DEV_ID_82571EB_FIBER       0x105F
>  #define E1000_DEV_ID_82571EB_SERDES      0x1060
>  #define E1000_DEV_ID_82571EB_QUAD_COPPER 0x10A4
> -#define E1000_DEV_ID_82571PT_QUAD_COPPER 0x10D5
>  #define E1000_DEV_ID_82571EB_QUAD_FIBER  0x10A5
>  #define E1000_DEV_ID_82571EB_QUAD_COPPER_LOWPROFILE  0x10BC
>  #define E1000_DEV_ID_82571EB_SERDES_DUAL 0x10D9
> @@ -82,15 +107,15 @@
>  #define E1000_DEV_ID_82572EI_FIBER       0x107E
>  #define E1000_DEV_ID_82572EI_SERDES      0x107F
>  #define E1000_DEV_ID_82572EI             0x10B9
> +/* is the next one from the same document ? */
> +#define E1000_DEV_ID_82571PT_QUAD_COPPER 0x10D5
> +
>  #define E1000_DEV_ID_82573E              0x108B
>  #define E1000_DEV_ID_82573E_IAMT         0x108C
>  #define E1000_DEV_ID_82573L              0x109A
> -#define E1000_DEV_ID_82546GB_QUAD_COPPER_KSP3 0x10B5
> -#define E1000_DEV_ID_80003ES2LAN_COPPER_DPT     0x1096
> -#define E1000_DEV_ID_80003ES2LAN_SERDES_DPT     0x1098
> -#define E1000_DEV_ID_80003ES2LAN_COPPER_SPT     0x10BA
> -#define E1000_DEV_ID_80003ES2LAN_SERDES_SPT     0x10BB
>  
> +/* Where is the documentation for those 7 ? */
> +/* also, plenty more ID in e1000e for integrated devices in ICH8/9... */
>  #define E1000_DEV_ID_ICH8_IGP_M_AMT      0x1049
>  #define E1000_DEV_ID_ICH8_IGP_AMT        0x104A
>  #define E1000_DEV_ID_ICH8_IGP_C          0x104B
> @@ -542,9 +567,9 @@
>  #define E1000_EEPROM_SWDPIN0   0x0001   /* SWDPIN 0 EEPROM Value */
>  #define E1000_EEPROM_LED_LOGIC 0x0020   /* Led Logic Word */
>  #define E1000_EEPROM_RW_REG_DATA   16   /* Offset to data in EEPROM read/write registers */
> -#define E1000_EEPROM_RW_REG_DONE   0x10 /* Offset to READ/WRITE done bit */
> +#define E1000_EEPROM_RW_REG_DONE   0x2  /* Offset to READ/WRITE done bit */
>  #define E1000_EEPROM_RW_REG_START  1    /* First bit for telling part to start operation */
> -#define E1000_EEPROM_RW_ADDR_SHIFT 8    /* Shift to the address bits */
> +#define E1000_EEPROM_RW_ADDR_SHIFT 2    /* Shift to the address bits */
>  #define E1000_EEPROM_POLL_WRITE    1    /* Flag for polling for write complete */
>  #define E1000_EEPROM_POLL_READ     0    /* Flag for polling for read complete */
>  /* Register Bit Masks */
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 4e0701d..07c8cdd 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1572,6 +1572,9 @@ static const char * const pci_nic_models[] = {
>      "i82559er",
>      "rtl8139",
>      "e1000",
> +    "82540EM",
> +    "82544GC",
> +    "82545EM",
>      "pcnet",
>      "virtio",
>      NULL
> @@ -1584,6 +1587,9 @@ static const char * const pci_nic_names[] = {
>      "i82559er",
>      "rtl8139",
>      "e1000",
> +    "82540EM",
> +    "82544GC",
> +    "82545EM",
>      "pcnet",
>      "virtio-net-pci",
>      NULL

I would hope that adding to these two legacy lists is not necessary for
new types. They should be created using -device, not -net nic,model=.

Regards,
Andreas

2014-02-25 9:01 GMT+01:00 Andreas Färber <afaerber@suse.de>:

> > -#define E1000(obj) \
> > -    OBJECT_CHECK(E1000State, (obj), TYPE_E1000)
> > +#define E1000(obj)                              \
> > +  DO_UPCAST(E1000State, parent_obj, obj)
>
> No, don't go backwards in time please. Take a look at e.g. eepro100,
> which already registers multiple data-driven subtypes for instance. The
> key to making it work is to introduce an abstract base type matching
> E1000() macro, with original "e1000" type becoming a subtype thereof.
>

I don't understand. I did look at "eepro100.c", it does exactly the same
thing
in the init() and uninit() (where E1000() is used):
#####
static int e100_nic_init(PCIDevice *pci_dev)
{
    EEPRO100State *s = DO_UPCAST(EEPRO100State, dev, pci_dev);
#####

> @@ -1572,6 +1572,9 @@ static const char * const pci_nic_models[] = {
> > @@ -1584,6 +1587,9 @@ static const char * const pci_nic_names[] = {
>
> I would hope that adding to these two legacy lists is not necessary for
> new types. They should be created using -device, not -net nic,model=.
>

Again I don't understand - I took inspiration from eepro100.c, and it has 3
devices in there
(i82551, i82557b, i82559er). And the model is handled by -device, as my
example
shows:
#####
"-netdev user,id=mynet0 -device 82545EM,netdev=mynet0"
#####

Cordially,

2014-02-25 9:58 GMT+01:00 Romain Dolbeau <romain@dolbeau.org>:

> 2014-02-25 9:01 GMT+01:00 Andreas Färber <afaerber@suse.de>:
>
> > @@ -1572,6 +1572,9 @@ static const char * const pci_nic_models[] = {
>> > @@ -1584,6 +1587,9 @@ static const char * const pci_nic_names[] = {
>>
>> I would hope that adding to these two legacy lists is not necessary for
>> new types. They should be created using -device, not -net nic,model=.
>>
>
> Again I don't understand - I took inspiration from eepro100.c, and it has
> 3 devices in there
> (i82551, i82557b, i82559er). And the model is handled by -device, as my
> example
> shows:
> #####
> "-netdev user,id=mynet0 -device 82545EM,netdev=mynet0"
> #####
>
> I was misled by the presence of "e1000" and several eepro100 variants in
this list.
Modifying it is not necessary, it works fine without it. I suppose that's
what you
meant - that it works both ways (-device or -net nic), but the second
option is not
required for new device as it's a deprecated way of doing things?

Cordially,

Am 25.02.2014 09:01, schrieb Andreas Färber:
> Hi,
>
> Am 24.02.2014 20:08, schrieb Romain Dolbeau:
[...]
>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> index 4e0701d..07c8cdd 100644
>> --- a/hw/pci/pci.c
>> +++ b/hw/pci/pci.c
>> @@ -1572,6 +1572,9 @@ static const char * const pci_nic_models[] = {
>>      "i82559er",
>>      "rtl8139",
>>      "e1000",
>> +    "82540EM",
>> +    "82544GC",
>> +    "82545EM",
>>      "pcnet",
>>      "virtio",
>>      NULL
>> @@ -1584,6 +1587,9 @@ static const char * const pci_nic_names[] = {
>>      "i82559er",
>>      "rtl8139",
>>      "e1000",
>> +    "82540EM",
>> +    "82544GC",
>> +    "82545EM",
>>      "pcnet",
>>      "virtio-net-pci",
>>      NULL
> I would hope that adding to these two legacy lists is not necessary for
> new types. They should be created using -device, not -net nic,model=.
>
> Regards,
> Andreas

What about removing those lists for QEMU 2.0? We should use the version
update to get rid of some legacy interfaces.

Stefan

On Tue, Feb 25, 2014 at 10:24:30AM +0100, Romain Dolbeau wrote:
> 2014-02-25 9:58 GMT+01:00 Romain Dolbeau <romain@dolbeau.org>:
> 
> > 2014-02-25 9:01 GMT+01:00 Andreas Färber <afaerber@suse.de>:
> >
> > > @@ -1572,6 +1572,9 @@ static const char * const pci_nic_models[] = {
> >> > @@ -1584,6 +1587,9 @@ static const char * const pci_nic_names[] = {
> >>
> >> I would hope that adding to these two legacy lists is not necessary for
> >> new types. They should be created using -device, not -net nic,model=.
> >>
> >
> > Again I don't understand - I took inspiration from eepro100.c, and it has
> > 3 devices in there
> > (i82551, i82557b, i82559er). And the model is handled by -device, as my
> > example
> > shows:
> > #####
> > "-netdev user,id=mynet0 -device 82545EM,netdev=mynet0"
> > #####
> >
> > I was misled by the presence of "e1000" and several eepro100 variants in
> this list.
> Modifying it is not necessary, it works fine without it. I suppose that's
> what you
> meant - that it works both ways (-device or -net nic), but the second
> option is not
> required for new device as it's a deprecated way of doing things?

Hi Romain,
First of all, thanks for doing this work.  There have been requests to
expose additional e1000-family NIC models from users before.  They were
running guests with drivers that didn't recognize the default e1000.

About Andreas' comments: QEMU Object Model (QOM) is the newer
infrastructure for defining device classes in QEMU.  Previously qdev was
used (and that typically involved DO_UPCAST()).

A lot of the qdev code is actually still around.  Some of it has been
rewritten to use QOM.  This can sometimes make it hard to understand
what is the "modern" way of doing things.

QOM is an object-oriented model where you can define class hierarchies.
By defining a base class, you can make all the e1000 NIC types support
the same parent e1000 type.

So in C++/Java/C# this would be something like:
class E1000Base { ... }
class 82540EM : public E1000Base { ... };

That way you can refer to all e1000-based NICs by their E1000Base type
without DO_UPCAST() or explicit C type casting.

I think the example that Andreas was referring to is closer to
hw/block/m25p80.c:

static const TypeInfo m25p80_info = {
    .name           = TYPE_M25P80,
    .parent         = TYPE_SSI_SLAVE,
    .instance_size  = sizeof(Flash),
    .class_size     = sizeof(M25P80Class),
    .abstract       = true,
};

static void m25p80_register_types(void)
{
    int i;

    type_register_static(&m25p80_info);
    for (i = 0; i < ARRAY_SIZE(known_devices); ++i) {
        TypeInfo ti = {
            .name       = known_devices[i].part_name,
            .parent     = TYPE_M25P80,
            .class_init = m25p80_class_init,
            .class_data = (void *)&known_devices[i],
        };
        type_register(&ti);
    }
}

m25p80_info defines an abstract base class.  It cannot be instantiated
but it serves as the parent type for all M25P80 devices.

Does this help?

Stefan