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 <address@hidden> 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: 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..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
>
>