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

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

[Michael S. Tsirkin]

On Sat, May 31, 2014 at 11:33:14PM -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 <romain@dolbeau.org>
> Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> ---
>  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..4e208e3 100644
> --- a/hw/net/e1000.c
> +++ b/hw/net/e1000.c
> @@ -69,23 +69,13 @@ static int debugflags = DBGBIT(TXERR) | DBGBIT(GENERAL);
>  
>  /*
>   * HW models:
> - *  E1000_DEV_ID_82540EM works with Windows and Linux
> + *  E1000_DEV_ID_82540EM works with Windows, Linux, and OS X <= 10.8
>   *  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_82544GC_COPPER appears to work; not well tested
> + *  E1000_DEV_ID_82545EM_COPPER works with Linux and OS X >= 10.6
>   *  Others never tested
>   */
> -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
> -};
>  
>  typedef struct E1000State_st {
>      /*< private >*/
> @@ -151,10 +141,21 @@ typedef struct E1000State_st {
>      uint32_t compat_flags;
>  } E1000State;
>  
> -#define TYPE_E1000 "e1000"
> +typedef struct E1000DeviceClass {
> +    PCIDeviceClass parent_class;
> +    uint16_t phy_id2;
> +    bool is_8257xx;
> +} E1000DeviceClass;
> +
> +#define TYPE_E1000_BASE "e1000-base"
>  
>  #define E1000(obj) \
> -    OBJECT_CHECK(E1000State, (obj), TYPE_E1000)
> +    OBJECT_CHECK(E1000State, (obj), TYPE_E1000_BASE)
> +
> +#define E1000_DEVICE_CLASS(klass) \
> +     OBJECT_CLASS_CHECK(E1000DeviceClass, (klass), TYPE_E1000_BASE)
> +#define E1000_DEVICE_GET_CLASS(obj) \
> +    OBJECT_GET_CLASS(E1000DeviceClass, (obj), TYPE_E1000_BASE)
>  
>  #define	defreg(x)	x = (E1000_##x>>2)
>  enum {
> @@ -232,10 +233,11 @@ static const char phy_regcap[0x20] = {
>      [PHY_ID2] = PHY_R,		[M88E1000_PHY_SPEC_STATUS] = PHY_R
>  };
>  
> +/* PHY_ID2 documented in 8254x_GBe_SDM.pdf, pp. 250 */
>  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,
> +    [PHY_ID1] = 0x141, /* [PHY_ID2] configured per DevId, from e1000_reset() */
>      [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,13 +271,15 @@ static void
>  set_interrupt_cause(E1000State *s, int index, uint32_t val)
>  {
>      PCIDevice *d = PCI_DEVICE(s);
> +    E1000DeviceClass *edc = E1000_DEVICE_GET_CLASS(d);
>      uint32_t pending_ints;
>      uint32_t mit_delay;
>  
> -    if (val && (E1000_DEVID >= E1000_DEV_ID_82547EI_MOBILE)) {
> -        /* Only for 8257x */
> +    if (val && edc->is_8257xx) {
> +        /* hack only for 8257xx models */
>          val |= E1000_ICR_INT_ASSERTED;
>      }
> +
>      s->mac_reg[ICR] = val;
>  
>      /*
> @@ -375,6 +379,7 @@ rxbufsize(uint32_t v)
>  static void e1000_reset(void *opaque)
>  {
>      E1000State *d = opaque;
> +    E1000DeviceClass *edc = E1000_DEVICE_GET_CLASS(d);
>      uint8_t *macaddr = d->conf.macaddr.a;
>      int i;
>  
> @@ -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;
>      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 = {
>      }
>  };
>  
> +/*
> + * EEPROM contents documented in Tables 5-2 and 5-3, pp. 98-102.
> + * Note: A valid DevId will be inserted during pci_e1000_init().
> + */
>  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,
> +    0x3000, 0x1000, 0x6403, 0 /*DevId*/, 0x8086, 0 /*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,
> @@ -1507,6 +1517,7 @@ static int pci_e1000_init(PCIDevice *pci_dev)
>  {
>      DeviceState *dev = DEVICE(pci_dev);
>      E1000State *d = E1000(pci_dev);
> +    PCIDeviceClass *pdc = PCI_DEVICE_GET_CLASS(pci_dev);
>      uint8_t *pci_conf;
>      uint16_t checksum = 0;
>      int i;
> @@ -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;
>      for (i = 0; i < EEPROM_CHECKSUM_REG; i++)
>          checksum += d->eeprom_data[i];
>      checksum = (uint16_t) EEPROM_SUM - checksum;
> @@ -1564,17 +1576,29 @@ static Property e1000_properties[] = {
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> +typedef struct E1000Info_st {

Sorry to nitpick, please rename this E1000Info.
We have two styles in QEMU:
- CamelCase
- with_underscores
mixing these is not a good idea.


> +    const char *name;
> +    uint16_t   device_id;
> +    uint8_t    revision;
> +    uint16_t   phy_id2;
> +    bool       is_8257xx;
> +} E1000Info;
> +
>  static void e1000_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> +    E1000DeviceClass *e = E1000_DEVICE_CLASS(klass);
> +    const 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->device_id = info->device_id;
> +    k->revision = info->revision;
> +    e->phy_id2 = info->phy_id2;
> +    e->is_8257xx = info->is_8257xx;
>      k->class_id = PCI_CLASS_NETWORK_ETHERNET;
>      set_bit(DEVICE_CATEGORY_NETWORK, dc->categories);
>      dc->desc = "Intel Gigabit Ethernet";
> @@ -1583,16 +1607,64 @@ static void e1000_class_init(ObjectClass *klass, void *data)
>      dc->props = e1000_properties;
>  }
>  
> -static const TypeInfo e1000_info = {
> -    .name          = TYPE_E1000,
> +static const TypeInfo e1000_base_info = {
> +    .name          = TYPE_E1000_BASE,
>      .parent        = TYPE_PCI_DEVICE,
>      .instance_size = sizeof(E1000State),
> -    .class_init    = e1000_class_init,
> +    .class_size    = sizeof(E1000DeviceClass),
> +    .abstract      = true,
> +};
> +
> +static const E1000Info e1000_devices[] = {
> +    {
> +        .name      = "e1000-82540em",
> +        .device_id = E1000_DEV_ID_82540EM,
> +        .revision  = 0x03,
> +        .phy_id2   = E1000_PHY_ID2_8254xx_DEFAULT,
> +    },
> +    {
> +        .name      = "e1000-82544gc",
> +        .device_id = E1000_DEV_ID_82544GC_COPPER,
> +        .revision  = 0x03,
> +        .phy_id2   = E1000_PHY_ID2_82544x,
> +    },
> +    {
> +        .name      = "e1000-82545em",
> +        .device_id = E1000_DEV_ID_82545EM_COPPER,
> +        .revision  = 0x03,
> +        .phy_id2   = E1000_PHY_ID2_8254xx_DEFAULT,
> +    },
> +    {
> +        .name      = "e1000-82573l",
> +        .device_id = E1000_DEV_ID_82573L,
> +        .revision  = 0x03,
> +        .phy_id2   = E1000_PHY_ID2_82573x,
> +        .is_8257xx = true,
> +    },
> +};
> +
> +static const TypeInfo e1000_default_info = {
> +    .name          = "e1000",
> +    .parent        = "e1000-82540em",
>  };
>  
>  static void e1000_register_types(void)
>  {
> -    type_register_static(&e1000_info);
> +    int i;
> +
> +    type_register_static(&e1000_base_info);
> +    for (i = 0; i < ARRAY_SIZE(e1000_devices); i++) {
> +        const E1000Info *info = &e1000_devices[i];
> +        TypeInfo type_info = {};
> +
> +        type_info.name = info->name;
> +        type_info.parent = TYPE_E1000_BASE;
> +        type_info.class_data = (void *)info;
> +        type_info.class_init = e1000_class_init;
> +
> +        type_register(&type_info);
> +    }
> +    type_register_static(&e1000_default_info);
>  }
>  
>  type_init(e1000_register_types)
> diff --git a/hw/net/e1000_regs.h b/hw/net/e1000_regs.h
> index c9cb79e..13ac671 100644
> --- a/hw/net/e1000_regs.h
> +++ b/hw/net/e1000_regs.h
> @@ -99,6 +99,12 @@
>  #define E1000_DEV_ID_ICH8_IFE_G          0x10C5
>  #define E1000_DEV_ID_ICH8_IGP_M          0x104D
>  
> +/* Device Specific Register Defaults */
> +#define E1000_PHY_ID2_82541x 0x380
> +#define E1000_PHY_ID2_82544x 0xC30
> +#define E1000_PHY_ID2_8254xx_DEFAULT 0xC20 /* 82540x, 82545x, and 82546x */
> +#define E1000_PHY_ID2_82573x 0xCC0
> +
>  /* Register Set. (82543, 82544)
>   *
>   * Registers are defined to be 32 bits and  should be accessed as 32 bit values.
> -- 
> 1.9.3

Re: [Qemu-devel] [PATCH v3 0/3] e1000: allow model/device_id selection on command line

[Michael S. Tsirkin]

On Sat, May 31, 2014 at 11:33:13PM -0400, Gabriel L. Somlo wrote:
>     Allow selection of different card models from the qemu
>     command line, to better accomodate a wider range of guests.

Looks good to me.
If possible pls address a nit I noted in one of the patches.
Besides that:

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>


> New in v3:
> 
>   - 1/3 and 2/3 from v2 now merged into a single patch (1/3), with:
>       - s/TYPE_E1000/TYPE_E1000_BASE/ as suggested by Stefan
>       - improved QOM-ification as suggested by Peter Crosthwaite
> 
>   - *OPTIONAL* patch to remove stale support for 8257xx (see commit blurb
>     in patch 3/3 for details
>     (this can be squashed on top of 1/3, but I'm including it separately
>      here for clarity, and as an RFC).
> 
> Thanks,
>   Gabriel
> 
> 
> v2:
> 
>   - moved check for 8257x out of the way of QOM, as suggested by
>     Michael (patch 1/3)
> 
>   - resolved "Signed-off-by" misunderstanding and miscellaneous style
>     issues (patch 2/3)
> 
>   - modified e1000 test to check for all supported models, as suggested
>     by Andreas (patch 3/3). I used eepro100-test.c as an example for
>     this change.
> 
> 
> Gabriel L. Somlo (3):
>   e1000: allow command-line selection of card model
>   tests: e1000: test additional device IDs
>   e1000: remove broken support for 82573L
> 
>  hw/net/e1000.c      | 110 +++++++++++++++++++++++++++++++++++++++-------------
>  hw/net/e1000_regs.h |   6 +++
>  tests/e1000-test.c  |  33 ++++++++++++----
>  3 files changed, 114 insertions(+), 35 deletions(-)
> 
> -- 
> 1.9.3

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

[Peter Crosthwaite]

On Sun, Jun 1, 2014 at 1:33 PM, Gabriel L. Somlo <gsomlo@gmail.com> 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 <romain@dolbeau.org>
> Signed-off-by: Gabriel Somlo <somlo@cmu.edu>

Reviewed-by: Peter Crosthwaite <peter.crosthwaite@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..4e208e3 100644
> --- a/hw/net/e1000.c
> +++ b/hw/net/e1000.c
> @@ -69,23 +69,13 @@ static int debugflags = DBGBIT(TXERR) | DBGBIT(GENERAL);
>
>  /*
>   * HW models:
> - *  E1000_DEV_ID_82540EM works with Windows and Linux
> + *  E1000_DEV_ID_82540EM works with Windows, Linux, and OS X <= 10.8
>   *  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_82544GC_COPPER appears to work; not well tested
> + *  E1000_DEV_ID_82545EM_COPPER works with Linux and OS X >= 10.6
>   *  Others never tested
>   */
> -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
> -};
>
>  typedef struct E1000State_st {
>      /*< private >*/
> @@ -151,10 +141,21 @@ typedef struct E1000State_st {
>      uint32_t compat_flags;
>  } E1000State;
>
> -#define TYPE_E1000 "e1000"
> +typedef struct E1000DeviceClass {
> +    PCIDeviceClass parent_class;
> +    uint16_t phy_id2;
> +    bool is_8257xx;
> +} E1000DeviceClass;
> +
> +#define TYPE_E1000_BASE "e1000-base"
>
>  #define E1000(obj) \
> -    OBJECT_CHECK(E1000State, (obj), TYPE_E1000)
> +    OBJECT_CHECK(E1000State, (obj), TYPE_E1000_BASE)
> +
> +#define E1000_DEVICE_CLASS(klass) \
> +     OBJECT_CLASS_CHECK(E1000DeviceClass, (klass), TYPE_E1000_BASE)
> +#define E1000_DEVICE_GET_CLASS(obj) \
> +    OBJECT_GET_CLASS(E1000DeviceClass, (obj), TYPE_E1000_BASE)
>
>  #define        defreg(x)       x = (E1000_##x>>2)
>  enum {
> @@ -232,10 +233,11 @@ static const char phy_regcap[0x20] = {
>      [PHY_ID2] = PHY_R,         [M88E1000_PHY_SPEC_STATUS] = PHY_R
>  };
>
> +/* PHY_ID2 documented in 8254x_GBe_SDM.pdf, pp. 250 */
>  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,
> +    [PHY_ID1] = 0x141, /* [PHY_ID2] configured per DevId, from e1000_reset() */
>      [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,13 +271,15 @@ static void
>  set_interrupt_cause(E1000State *s, int index, uint32_t val)
>  {
>      PCIDevice *d = PCI_DEVICE(s);
> +    E1000DeviceClass *edc = E1000_DEVICE_GET_CLASS(d);

There was an earlier comment along the lines of avoiding QOM in the
data-path. Although I don't see a clean way around it TBH. Is there
any perf degredation in high interrupt scenarios?

My first guess would be no, as the class cast cache would be low
chance of evictions causing expensive lookup failures and the problem
probably evaporates #ifndef QOM_CAST_DEBUG anyways.

So I think this is ok.

Regards,
Peter

>      uint32_t pending_ints;
>      uint32_t mit_delay;
>
> -    if (val && (E1000_DEVID >= E1000_DEV_ID_82547EI_MOBILE)) {
> -        /* Only for 8257x */
> +    if (val && edc->is_8257xx) {
> +        /* hack only for 8257xx models */
>          val |= E1000_ICR_INT_ASSERTED;
>      }
> +
>      s->mac_reg[ICR] = val;
>
>      /*
> @@ -375,6 +379,7 @@ rxbufsize(uint32_t v)
>  static void e1000_reset(void *opaque)
>  {
>      E1000State *d = opaque;
> +    E1000DeviceClass *edc = E1000_DEVICE_GET_CLASS(d);
>      uint8_t *macaddr = d->conf.macaddr.a;
>      int i;
>
> @@ -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;
>      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 = {
>      }
>  };
>
> +/*
> + * EEPROM contents documented in Tables 5-2 and 5-3, pp. 98-102.
> + * Note: A valid DevId will be inserted during pci_e1000_init().
> + */
>  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,
> +    0x3000, 0x1000, 0x6403, 0 /*DevId*/, 0x8086, 0 /*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,
> @@ -1507,6 +1517,7 @@ static int pci_e1000_init(PCIDevice *pci_dev)
>  {
>      DeviceState *dev = DEVICE(pci_dev);
>      E1000State *d = E1000(pci_dev);
> +    PCIDeviceClass *pdc = PCI_DEVICE_GET_CLASS(pci_dev);
>      uint8_t *pci_conf;
>      uint16_t checksum = 0;
>      int i;
> @@ -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;
>      for (i = 0; i < EEPROM_CHECKSUM_REG; i++)
>          checksum += d->eeprom_data[i];
>      checksum = (uint16_t) EEPROM_SUM - checksum;
> @@ -1564,17 +1576,29 @@ static Property e1000_properties[] = {
>      DEFINE_PROP_END_OF_LIST(),
>  };
>
> +typedef struct E1000Info_st {
> +    const char *name;
> +    uint16_t   device_id;
> +    uint8_t    revision;
> +    uint16_t   phy_id2;
> +    bool       is_8257xx;
> +} E1000Info;
> +
>  static void e1000_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> +    E1000DeviceClass *e = E1000_DEVICE_CLASS(klass);
> +    const 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->device_id = info->device_id;
> +    k->revision = info->revision;
> +    e->phy_id2 = info->phy_id2;
> +    e->is_8257xx = info->is_8257xx;
>      k->class_id = PCI_CLASS_NETWORK_ETHERNET;
>      set_bit(DEVICE_CATEGORY_NETWORK, dc->categories);
>      dc->desc = "Intel Gigabit Ethernet";
> @@ -1583,16 +1607,64 @@ static void e1000_class_init(ObjectClass *klass, void *data)
>      dc->props = e1000_properties;
>  }
>
> -static const TypeInfo e1000_info = {
> -    .name          = TYPE_E1000,
> +static const TypeInfo e1000_base_info = {
> +    .name          = TYPE_E1000_BASE,
>      .parent        = TYPE_PCI_DEVICE,
>      .instance_size = sizeof(E1000State),
> -    .class_init    = e1000_class_init,
> +    .class_size    = sizeof(E1000DeviceClass),
> +    .abstract      = true,
> +};
> +
> +static const E1000Info e1000_devices[] = {
> +    {
> +        .name      = "e1000-82540em",
> +        .device_id = E1000_DEV_ID_82540EM,
> +        .revision  = 0x03,
> +        .phy_id2   = E1000_PHY_ID2_8254xx_DEFAULT,
> +    },
> +    {
> +        .name      = "e1000-82544gc",
> +        .device_id = E1000_DEV_ID_82544GC_COPPER,
> +        .revision  = 0x03,
> +        .phy_id2   = E1000_PHY_ID2_82544x,
> +    },
> +    {
> +        .name      = "e1000-82545em",
> +        .device_id = E1000_DEV_ID_82545EM_COPPER,
> +        .revision  = 0x03,
> +        .phy_id2   = E1000_PHY_ID2_8254xx_DEFAULT,
> +    },
> +    {
> +        .name      = "e1000-82573l",
> +        .device_id = E1000_DEV_ID_82573L,
> +        .revision  = 0x03,
> +        .phy_id2   = E1000_PHY_ID2_82573x,
> +        .is_8257xx = true,
> +    },
> +};
> +
> +static const TypeInfo e1000_default_info = {
> +    .name          = "e1000",
> +    .parent        = "e1000-82540em",
>  };
>
>  static void e1000_register_types(void)
>  {
> -    type_register_static(&e1000_info);
> +    int i;
> +
> +    type_register_static(&e1000_base_info);
> +    for (i = 0; i < ARRAY_SIZE(e1000_devices); i++) {
> +        const E1000Info *info = &e1000_devices[i];
> +        TypeInfo type_info = {};
> +
> +        type_info.name = info->name;
> +        type_info.parent = TYPE_E1000_BASE;
> +        type_info.class_data = (void *)info;
> +        type_info.class_init = e1000_class_init;
> +
> +        type_register(&type_info);
> +    }
> +    type_register_static(&e1000_default_info);
>  }
>
>  type_init(e1000_register_types)
> diff --git a/hw/net/e1000_regs.h b/hw/net/e1000_regs.h
> index c9cb79e..13ac671 100644
> --- a/hw/net/e1000_regs.h
> +++ b/hw/net/e1000_regs.h
> @@ -99,6 +99,12 @@
>  #define E1000_DEV_ID_ICH8_IFE_G          0x10C5
>  #define E1000_DEV_ID_ICH8_IGP_M          0x104D
>
> +/* Device Specific Register Defaults */
> +#define E1000_PHY_ID2_82541x 0x380
> +#define E1000_PHY_ID2_82544x 0xC30
> +#define E1000_PHY_ID2_8254xx_DEFAULT 0xC20 /* 82540x, 82545x, and 82546x */
> +#define E1000_PHY_ID2_82573x 0xCC0
> +
>  /* Register Set. (82543, 82544)
>   *
>   * Registers are defined to be 32 bits and  should be accessed as 32 bit values.
> --
> 1.9.3
>
>

Re: [Qemu-devel] [PATCH v3 2/3] tests: e1000: test additional device IDs

[Peter Crosthwaite]

On Sun, Jun 1, 2014 at 1:33 PM, Gabriel L. Somlo <gsomlo@gmail.com> wrote:
> Update e1000-test.c to check all currently supported devices.
>
> Suggested-by: Andreas Färber <afaerber@suse.de>
> Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

> ---
>  tests/e1000-test.c | 34 +++++++++++++++++++++++++++-------
>  1 file changed, 27 insertions(+), 7 deletions(-)
>
> diff --git a/tests/e1000-test.c b/tests/e1000-test.c
> index a8ba2fc..53c41f8 100644
> --- a/tests/e1000-test.c
> +++ b/tests/e1000-test.c
> @@ -13,21 +13,41 @@
>  #include "qemu/osdep.h"
>
>  /* Tests only initialization so far. TODO: Replace with functional tests */
> -static void nop(void)
> +static void test_device(gconstpointer data)
>  {
> +    const char *model = data;
> +    QTestState *s;
> +    char *args;
> +
> +    args = g_strdup_printf("-device %s", model);
> +    s = qtest_start(args);
> +
> +    if (s) {
> +        qtest_quit(s);
> +    }
> +    g_free(args);
>  }
>
> +static const char *models[] = {
> +    "e1000",
> +    "e1000-82540em",
> +    "e1000-82544gc",
> +    "e1000-82545em",
> +    "e1000-82573l",
> +};
> +
>  int main(int argc, char **argv)
>  {
> -    int ret;
> +    int i;
>
>      g_test_init(&argc, &argv, NULL);
> -    qtest_add_func("/e1000/nop", nop);
>
> -    qtest_start("-device e1000");
> -    ret = g_test_run();
> +    for (i = 0; i < ARRAY_SIZE(models); i++) {
> +        char *path;
>
> -    qtest_end();
> +        path = g_strdup_printf("/%s/e1000/%s", qtest_get_arch(), models[i]);
> +        g_test_add_data_func(path, models[i], test_device);
> +    }
>
> -    return ret;
> +    return g_test_run();
>  }
> --
> 1.9.3
>
>

Re: [Qemu-devel] [PATCH v3 3/3] e1000: remove broken support for 82573L

[Peter Crosthwaite]

On Sun, Jun 1, 2014 at 1:33 PM, Gabriel L. Somlo <gsomlo@gmail.com> wrote:
> Currently, e1000 support is based on the manual for the 8254xx
> model series. 82573x models are documented in a separate manual
> (see http://www.intel.com/content/dam/www/public/us/en/documents/manuals/pcie-gbe-controllers-open-source-manual.pdf)
> and the 82573L device ID no longer works correctly on either Linux
> (3.14.*) or Windows 7.
>
> This patch removes stale code claiming to support 82573L, cleaning
> up the code base for the remaining 8254xx model series.
>
> Signed-off-by: Gabriel Somlo <somlo@cmu.edu>

Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

> ---
>  hw/net/e1000.c     | 18 ------------------
>  tests/e1000-test.c |  1 -
>  2 files changed, 19 deletions(-)
>
> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
> index 4e208e3..3ae5b43 100644
> --- a/hw/net/e1000.c
> +++ b/hw/net/e1000.c
> @@ -70,8 +70,6 @@ static int debugflags = DBGBIT(TXERR) | DBGBIT(GENERAL);
>  /*
>   * HW models:
>   *  E1000_DEV_ID_82540EM works with Windows, Linux, and OS X <= 10.8
> - *  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_82544GC_COPPER appears to work; not well tested
>   *  E1000_DEV_ID_82545EM_COPPER works with Linux and OS X >= 10.6
>   *  Others never tested
> @@ -144,7 +142,6 @@ typedef struct E1000State_st {
>  typedef struct E1000DeviceClass {
>      PCIDeviceClass parent_class;
>      uint16_t phy_id2;
> -    bool is_8257xx;
>  } E1000DeviceClass;
>
>  #define TYPE_E1000_BASE "e1000-base"
> @@ -271,15 +268,9 @@ static void
>  set_interrupt_cause(E1000State *s, int index, uint32_t val)
>  {
>      PCIDevice *d = PCI_DEVICE(s);
> -    E1000DeviceClass *edc = E1000_DEVICE_GET_CLASS(d);
>      uint32_t pending_ints;
>      uint32_t mit_delay;
>
> -    if (val && edc->is_8257xx) {
> -        /* hack only for 8257xx models */
> -        val |= E1000_ICR_INT_ASSERTED;
> -    }
> -
>      s->mac_reg[ICR] = val;
>
>      /*
> @@ -1581,7 +1572,6 @@ typedef struct E1000Info_st {
>      uint16_t   device_id;
>      uint8_t    revision;
>      uint16_t   phy_id2;
> -    bool       is_8257xx;
>  } E1000Info;
>
>  static void e1000_class_init(ObjectClass *klass, void *data)
> @@ -1598,7 +1588,6 @@ static void e1000_class_init(ObjectClass *klass, void *data)
>      k->device_id = info->device_id;
>      k->revision = info->revision;
>      e->phy_id2 = info->phy_id2;
> -    e->is_8257xx = info->is_8257xx;
>      k->class_id = PCI_CLASS_NETWORK_ETHERNET;
>      set_bit(DEVICE_CATEGORY_NETWORK, dc->categories);
>      dc->desc = "Intel Gigabit Ethernet";
> @@ -1634,13 +1623,6 @@ static const E1000Info e1000_devices[] = {
>          .revision  = 0x03,
>          .phy_id2   = E1000_PHY_ID2_8254xx_DEFAULT,
>      },
> -    {
> -        .name      = "e1000-82573l",
> -        .device_id = E1000_DEV_ID_82573L,
> -        .revision  = 0x03,
> -        .phy_id2   = E1000_PHY_ID2_82573x,
> -        .is_8257xx = true,
> -    },
>  };
>
>  static const TypeInfo e1000_default_info = {
> diff --git a/tests/e1000-test.c b/tests/e1000-test.c
> index 53c41f8..81f164d 100644
> --- a/tests/e1000-test.c
> +++ b/tests/e1000-test.c
> @@ -33,7 +33,6 @@ static const char *models[] = {
>      "e1000-82540em",
>      "e1000-82544gc",
>      "e1000-82545em",
> -    "e1000-82573l",
>  };
>
>  int main(int argc, char **argv)
> --
> 1.9.3
>
>

Re: [Qemu-devel] [PATCH v3 0/3] e1000: allow model/device_id selection on command line

[Peter Crosthwaite]

On Sun, Jun 1, 2014 at 1:33 PM, Gabriel L. Somlo <gsomlo@gmail.com> wrote:
>     Allow selection of different card models from the qemu
>     command line, to better accomodate a wider range of guests.
>
> New in v3:
>
>   - 1/3 and 2/3 from v2 now merged into a single patch (1/3), with:
>       - s/TYPE_E1000/TYPE_E1000_BASE/ as suggested by Stefan
>       - improved QOM-ification as suggested by Peter Crosthwaite
>
>   - *OPTIONAL* patch to remove stale support for 8257xx (see commit blurb
>     in patch 3/3 for details
>     (this can be squashed on top of 1/3, but I'm including it separately
>      here for clarity, and as an RFC).
>

I think it's ok to merge as three separate patches organised the way
you have done it. It means if someone comes along later and decides to
repair the broken support they can revert just your last patch as
starting point, rather than having to disentangle it from the
QOMification work. The other alternative if you want to minimise
churn, is to do your defeaturing first then QOMifiy but that doesn't
do the repair guy any favours. Should probably still be three separate
patches though.

Regards,
Peter

> Thanks,
>   Gabriel
>
>
> v2:
>
>   - moved check for 8257x out of the way of QOM, as suggested by
>     Michael (patch 1/3)
>
>   - resolved "Signed-off-by" misunderstanding and miscellaneous style
>     issues (patch 2/3)
>
>   - modified e1000 test to check for all supported models, as suggested
>     by Andreas (patch 3/3). I used eepro100-test.c as an example for
>     this change.
>
>
> Gabriel L. Somlo (3):
>   e1000: allow command-line selection of card model
>   tests: e1000: test additional device IDs
>   e1000: remove broken support for 82573L
>
>  hw/net/e1000.c      | 110 +++++++++++++++++++++++++++++++++++++++-------------
>  hw/net/e1000_regs.h |   6 +++
>  tests/e1000-test.c  |  33 ++++++++++++----
>  3 files changed, 114 insertions(+), 35 deletions(-)
>
> --
> 1.9.3
>
>

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

[Stefan Hajnoczi]

On Sat, May 31, 2014 at 11:33:14PM -0400, Gabriel L. Somlo wrote:
> @@ -151,10 +141,21 @@ typedef struct E1000State_st {
>      uint32_t compat_flags;
>  } E1000State;
>  
> -#define TYPE_E1000 "e1000"
> +typedef struct E1000DeviceClass {

The QOM type is called TYPE_E1000_BASE ("e1000-base"), please name this
E1000BaseClass.  The reasoning is the same as my previous request for
consistent naming.  Other QOM class users in hw/ also follow this
pattern (type "X", class "XClass").

Re: [Qemu-devel] [PATCH v3 3/3] e1000: remove broken support for 82573L

[Stefan Hajnoczi]

On Sat, May 31, 2014 at 11:33:16PM -0400, Gabriel L. Somlo wrote:
> Currently, e1000 support is based on the manual for the 8254xx
> model series. 82573x models are documented in a separate manual
> (see http://www.intel.com/content/dam/www/public/us/en/documents/manuals/pcie-gbe-controllers-open-source-manual.pdf)
> and the 82573L device ID no longer works correctly on either Linux
> (3.14.*) or Windows 7.
> 
> This patch removes stale code claiming to support 82573L, cleaning
> up the code base for the remaining 8254xx model series.
> 
> Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> ---
>  hw/net/e1000.c     | 18 ------------------
>  tests/e1000-test.c |  1 -
>  2 files changed, 19 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Re: [Qemu-devel] [PATCH v3 2/3] tests: e1000: test additional device IDs

[Stefan Hajnoczi]

On Sat, May 31, 2014 at 11:33:15PM -0400, Gabriel L. Somlo wrote:
> Update e1000-test.c to check all currently supported devices.
> 
> Suggested-by: Andreas Färber <afaerber@suse.de>
> Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  tests/e1000-test.c | 34 +++++++++++++++++++++++++++-------
>  1 file changed, 27 insertions(+), 7 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Re: [Qemu-devel] [PATCH v3 0/3] e1000: allow model/device_id selection on command line

[Stefan Hajnoczi]

On Sat, May 31, 2014 at 11:33:13PM -0400, Gabriel L. Somlo wrote:
>     Allow selection of different card models from the qemu
>     command line, to better accomodate a wider range of guests.
> 
> New in v3:
> 
>   - 1/3 and 2/3 from v2 now merged into a single patch (1/3), with:
>       - s/TYPE_E1000/TYPE_E1000_BASE/ as suggested by Stefan
>       - improved QOM-ification as suggested by Peter Crosthwaite
> 
>   - *OPTIONAL* patch to remove stale support for 8257xx (see commit blurb
>     in patch 3/3 for details
>     (this can be squashed on top of 1/3, but I'm including it separately
>      here for clarity, and as an RFC).

Looks very close now.  I just had a comment about E1000DeviceClass,
which should be called E1000BaseClass.

Stefan