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

[Gabriel L. Somlo]

Hi Peter,

Thanks for the QOM crash course !

So I did s/TYPE_E1000/TYPE_E1000_BASE/ as suggested by Stefan, and
also moved PCI_VENDOR_ID_INTEL back into e1000_class_init() and away
from e1000_devices[] as you suggested. That's going to be part of
v3, as soon as we're done talking about QOM, see below :)

On Wed, May 28, 2014 at 11:02:10PM +1000, Peter Crosthwaite wrote:
> 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).

I've attached a new patch (could be fourth in the series, or merged in
with the main dev_id patch. My only concern regarding a replacement of
e1000_phy_id2_init() and e1000_is_8257x() is that now I'll *have* to
include a .phy_id2 (and possibly a .is_8257x) field with every entry
in the e1000_devices[] array (unless there's a way to set a default
"phy_id2 = 0xc20" and a default "is_8257x = false" somewhere, and
inherit it unless explicitly overridden for the relevant device IDs
during initialization ?

Note that we're not even supporting any 8257x cards, so the switch
statement is just there for documentation purposes at this time...

Thanks much,

--Gabriel


diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index 7a1a681..33a93ba 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -160,11 +160,21 @@ typedef struct E1000State_st {
     bool is_8257x;
 } E1000State;
 
+typedef struct E1000DeviceClass {
+    PCIDeviceClass parent_class;
+    uint16_t phy_id2;
+} E1000DeviceClass;
+
 #define TYPE_E1000_BASE "e1000-base"
 
 #define E1000(obj) \
     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 {
     defreg(CTRL),      defreg(EECD),   defreg(EERD),   defreg(GPRC),
@@ -384,7 +394,7 @@ rxbufsize(uint32_t v)
 static void e1000_reset(void *opaque)
 {
     E1000State *d = opaque;
-    PCIDeviceClass *pdc = PCI_DEVICE_GET_CLASS(d);
+    E1000DeviceClass *edc = E1000_DEVICE_GET_CLASS(d);
     uint8_t *macaddr = d->conf.macaddr.a;
     int i;
 
@@ -395,7 +405,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] = 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;
@@ -1613,12 +1623,14 @@ typedef struct E1000Info_st {
     const char *name;
     uint16_t   device_id;
     uint8_t    revision;
+    uint16_t   phy_id2;
 } 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;
@@ -1627,6 +1639,7 @@ static void e1000_class_init(ObjectClass *klass, void 
*data)
     k->vendor_id = PCI_VENDOR_ID_INTEL;
     k->device_id = info->device_id;
     k->revision = info->revision;
+    e->phy_id2 = info->phy_id2;
     k->class_id = PCI_CLASS_NETWORK_ETHERNET;
     set_bit(DEVICE_CATEGORY_NETWORK, dc->categories);
     dc->desc = "Intel Gigabit Ethernet";
@@ -1639,29 +1652,33 @@ static const TypeInfo e1000_base_info = {
     .name          = TYPE_E1000_BASE,
     .parent        = TYPE_PCI_DEVICE,
     .instance_size = sizeof(E1000State),
+    .class_size    = sizeof(E1000DeviceClass),
     .abstract      = true,
 };
 
+static const TypeInfo e1000_default_info = {
+    .name          = "e1000",
+    .parent        = "e1000-82540em",
+};
+
 static const E1000Info e1000_devices[] = {
     {
-        .name      = "e1000", /* default, alias for "e1000-82540em" */
-        .device_id = E1000_DEV_ID_82540EM,
-        .revision  = 0x03,
-    },
-    {
         .name      = "e1000-82540em",
         .device_id = E1000_DEV_ID_82540EM,
         .revision  = 0x03,
+        .phy_id2   = 0xc20,
     },
     {
         .name      = "e1000-82544gc",
         .device_id = E1000_DEV_ID_82544GC_COPPER,
         .revision  = 0x03,
+        .phy_id2   = 0xc30,
     },
     {
         .name      = "e1000-82545em",
         .device_id = E1000_DEV_ID_82545EM_COPPER,
         .revision  = 0x03,
+        .phy_id2   = 0xc20,
     },
 };
 
@@ -1670,6 +1687,7 @@ static void e1000_register_types(void)
     int i;
 
     type_register_static(&e1000_base_info);
+    type_register_static(&e1000_default_info);
     for (i = 0; i < ARRAY_SIZE(e1000_devices); i++) {
         const E1000Info *info = &e1000_devices[i];
         TypeInfo type_info = {};