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

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

[Gabriel L. Somlo]

This started out as a single patch (now patch 2/3):

    Allow selection of different card models from the qemu
    command line, to better accomodate a wider range of guests.

New in 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.

On Wed, May 21, 2014 at 12:04:57PM +0300, Michael S. Tsirkin wrote:
> On Wed, May 21, 2014 at 10:40:54AM +0200, Andreas F?rber wrote:
> >    static inline uint16_t maybe?
> 
> inline in c file is an optimization hint for a compiler, so
> really useless without a benchmark.
> It's useful in headers since some compilers warn about unused
> non-inline static functions.

I went with "static inline uint16_t" in both 1/3 and 2/3, since each
only get called once, and I'm only using a function instead of a
preprocessor macro for aesthetic reasons. I'm not religious about it
though, so if anyone still objects I can change it to whatever we all
agree on :)

> > > +    d->phy_reg[PHY_ID2] = e1000_phy_id2_init(dev_id);
> 
> I would just pass E1000State to e1000_phy_id2_init.

So I'm still passing "uint16_t device_id" instead of E1000State,
because e1000_phy_id2_init() replaces "enum { PHY_ID2_INIT = ...}",
which goes before the E1000State typedef in the source, and I wanted
to have the new code show up in the same spot in the patch as the code
it replaces. That, and we wouldn't save anything either way, I'd still
have to de-ref E1000State to get at the device_id, either in
e1000_reset() or in e1000_phy_id2_init(). Again, not religious about
it, so I can change it if you really want me to :)

Thanks again,
  Gabriel

Gabriel L. Somlo (3):
  e1000: avoid relying on device id (and soon, QOM) on data path
  e1000: allow command-line selection of card model
  tests: e1000: test additional device IDs

 hw/net/e1000.c     | 127 ++++++++++++++++++++++++++++++++++++++++++++++-------
 tests/e1000-test.c |  33 +++++++++++---
 2 files changed, 136 insertions(+), 24 deletions(-)

-- 
1.9.0

[Qemu-devel] [PATCH v2 1/3] e1000: avoid relying on device id (and soon, QOM) on data path

[Gabriel L. Somlo]

Introduce "is_8257x" boolean flag to E1000State, and set it during
pci_e1000_init(), to avoid having to dynamically figure out device_id
during set_interrupt_cause(). This will come in handy once we have
a wider range of possible device IDs, and begin using QOM.

Suggested-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
---
 hw/net/e1000.c | 32 +++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index 8387443..1b8c513 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -149,6 +149,8 @@ typedef struct E1000State_st {
 #define E1000_FLAG_AUTONEG (1 << E1000_FLAG_AUTONEG_BIT)
 #define E1000_FLAG_MIT (1 << E1000_FLAG_MIT_BIT)
     uint32_t compat_flags;
+
+    bool is_8257x;
 } E1000State;
 
 #define TYPE_E1000 "e1000"
@@ -272,7 +274,7 @@ set_interrupt_cause(E1000State *s, int index, uint32_t val)
     uint32_t pending_ints;
     uint32_t mit_delay;
 
-    if (val && (E1000_DEVID >= E1000_DEV_ID_82547EI_MOBILE)) {
+    if (val && s->is_8257x) {
         /* Only for 8257x */
         val |= E1000_ICR_INT_ASSERTED;
     }
@@ -1503,6 +1505,32 @@ static NetClientInfo net_e1000_info = {
     .link_status_changed = e1000_set_link_status,
 };
 
+static inline bool
+e1000_is_8257x(uint16_t device_id)
+{
+    switch (device_id) {
+    case E1000_DEV_ID_82571EB_COPPER:
+    case E1000_DEV_ID_82571EB_FIBER:
+    case E1000_DEV_ID_82571EB_SERDES:
+    case E1000_DEV_ID_82571EB_QUAD_COPPER:
+    case E1000_DEV_ID_82571PT_QUAD_COPPER:
+    case E1000_DEV_ID_82571EB_QUAD_FIBER:
+    case E1000_DEV_ID_82571EB_QUAD_COPPER_LOWPROFILE:
+    case E1000_DEV_ID_82571EB_SERDES_DUAL:
+    case E1000_DEV_ID_82571EB_SERDES_QUAD:
+    case E1000_DEV_ID_82572EI_COPPER:
+    case E1000_DEV_ID_82572EI_FIBER:
+    case E1000_DEV_ID_82572EI_SERDES:
+    case E1000_DEV_ID_82572EI:
+    case E1000_DEV_ID_82573E:
+    case E1000_DEV_ID_82573E_IAMT:
+    case E1000_DEV_ID_82573L:
+        return true;
+    default:
+        return false;
+    }
+}
+
 static int pci_e1000_init(PCIDevice *pci_dev)
 {
     DeviceState *dev = DEVICE(pci_dev);
@@ -1546,6 +1574,8 @@ static int pci_e1000_init(PCIDevice *pci_dev)
     d->autoneg_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL, e1000_autoneg_timer, d);
     d->mit_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, e1000_mit_timer, d);
 
+    d->is_8257x = e1000_is_8257x(pdc->device_id);
+
     return 0;
 }
 
-- 
1.9.0

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

[Gabriel L. Somlo]

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 | 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 */
+    }
+}
 
 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);
     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);
     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;
+    uint16_t   device_id;
+    uint8_t    revision;
+} E1000Info;
+
 static void e1000_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_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;
     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,
+    .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,
+    },
+    {
+        .name      = "e1000-82540em",
+        .vendor_id = PCI_VENDOR_ID_INTEL,
+        .device_id = E1000_DEV_ID_82540EM,
+        .revision  = 0x03,
+    },
+    {
+        .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

[Qemu-devel] [PATCH v2 3/3] tests: e1000: test additional device IDs

[Gabriel L. Somlo]

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>
---
 tests/e1000-test.c | 33 ++++++++++++++++++++++++++-------
 1 file changed, 26 insertions(+), 7 deletions(-)

diff --git a/tests/e1000-test.c b/tests/e1000-test.c
index a8ba2fc..81f164d 100644
--- a/tests/e1000-test.c
+++ b/tests/e1000-test.c
@@ -13,21 +13,40 @@
 #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",
+};
+
 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.0