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

[Peter Crosthwaite]

On Thu, May 22, 2014 at 4:27 AM, 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>
> ---
>  hw/net/e1000.c | 95 
> ++++++++++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 79 insertions(+), 16 deletions(-)
>
> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
> index 1b8c513..74093ce 100644
> --- a/hw/net/e1000.c
> +++ b/hw/net/e1000.c
> @@ -69,23 +69,30 @@ 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
> -};
> +static inline uint16_t
> +e1000_phy_id2_init(uint16_t device_id)
> +{
> +    switch (device_id) {
> +    case E1000_DEV_ID_82573L:
> +        return 0xcc2;
> +    case E1000_DEV_ID_82544GC_COPPER:
> +        return 0xc30;
> +    default:
> +        return 0xc20; /* default for 82540EM and others */
> +    }
> +}

So I would guess with a bit more QOMification of the subtypes, it's
possible to promote the phy_id2 to class data and remove the (late)
lookup giving some decoupling between PCI dev id and this phy id.

I'll make some notes through the patch on the basic idea.

You could also do it for your is_8257x flag, just set it true in the
.info's that matter. Then grab it from class to obj at init time
(slightly optional, but avoids QOM-in-data-path).

>
>  typedef struct E1000State_st {
>      /*< private >*/
> @@ -153,7 +160,7 @@ typedef struct E1000State_st {
>      bool is_8257x;
>  } E1000State;
>
> -#define TYPE_E1000 "e1000"
> +#define TYPE_E1000 "e1000-base"
>
>  #define E1000(obj) \
>      OBJECT_CHECK(E1000State, (obj), TYPE_E1000)
> @@ -237,7 +244,7 @@ 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,
> +    [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,
> @@ -377,6 +384,7 @@ rxbufsize(uint32_t v)
>  static void e1000_reset(void *opaque)
>  {
>      E1000State *d = opaque;
> +    PCIDeviceClass *pdc = PCI_DEVICE_GET_CLASS(d);

Something like

E1000DeviceClass *ec = E1000_DEVICE_GET_CLASS(d);

You define the standard qom class caster boilerplate next to the E1000 macro.

>      uint8_t *macaddr = d->conf.macaddr.a;
>      int i;
>
> @@ -387,6 +395,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] = e1000_phy_id2_init(pdc->device_id);

d->phy_reg[PHY_ID2] = ec->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;
> @@ -1442,9 +1451,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,
> @@ -1535,6 +1548,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;
> @@ -1559,6 +1573,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;
> @@ -1594,17 +1609,25 @@ static Property e1000_properties[] = {
>      DEFINE_PROP_END_OF_LIST(),
>  };
>
> +typedef struct E1000Info_st {
> +    const char *name;
> +    uint16_t   vendor_id;

Curious, - is it valid to have a non-intel e1000? If not i'd probably
just drop the configurable vendor_id completely to save on the
repetition.

> +    uint16_t   device_id;
> +    uint8_t    revision;

uint16_t phy_id2;

> +} E1000Info;
> +

typedef struct E1000DeviceClass {
    PCIDeviceClass parent_class;
    uint16_t phy_id2;
} E1000DeviceClass;

This little bit of boilerplate is annoying when you have just one
class prop, but it becomes worth it when you start adding more E1000
specific variation between the different variants.

>  static void e1000_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);

E1000DeviceClass *ec = 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->vendor_id = info->vendor_id;
> +    k->device_id = info->device_id;
> +    k->revision = info->revision;

ec->phy_id2 = info->phy_id2

>      k->class_id = PCI_CLASS_NETWORK_ETHERNET;
>      set_bit(DEVICE_CATEGORY_NETWORK, dc->categories);
>      dc->desc = "Intel Gigabit Ethernet";
> @@ -1613,16 +1636,56 @@ static void e1000_class_init(ObjectClass *klass, void 
> *data)
>      dc->props = e1000_properties;
>  }
>
> -static const TypeInfo e1000_info = {
> +static const TypeInfo e1000_base_info = {
>      .name          = TYPE_E1000,
>      .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", /* default, alias for "e1000-82540em" */
> +        .vendor_id = PCI_VENDOR_ID_INTEL,
> +        .device_id = E1000_DEV_ID_82540EM,
> +        .revision  = 0x03,
> +    },

so is it as simple as just:

static const TypeInfo e1000_default_info = {
    .name = "e1000",
    .parent = "e1000-82540em"
}

To implement the default alias?

> +    {
> +        .name      = "e1000-82540em",
> +        .vendor_id = PCI_VENDOR_ID_INTEL,
> +        .device_id = E1000_DEV_ID_82540EM,
> +        .revision  = 0x03,

.phy_id2 = ...

Regards,
Peter

> +    },
> +    {
> +        .name      = "e1000-82544gc",
> +        .vendor_id = PCI_VENDOR_ID_INTEL,
> +        .device_id = E1000_DEV_ID_82544GC_COPPER,
> +        .revision  = 0x03,
> +    },
> +    {
> +        .name      = "e1000-82545em",
> +        .vendor_id = PCI_VENDOR_ID_INTEL,
> +        .device_id = E1000_DEV_ID_82545EM_COPPER,
> +        .revision  = 0x03,
> +    },
>  };
>
>  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;
> +        type_info.class_data = (void *)info;
> +        type_info.class_init = e1000_class_init;
> +
> +        type_register(&type_info);
> +    }
>  }
>
>  type_init(e1000_register_types)
> --
> 1.9.0
>
>