[Qemu-devel] [PATCH v5 3/8] e1000: Introduced an array to control the access to the MAC registers

[Leonid Bloch]

The array of uint8_t's which is introduced here, contains useful metadata
about the MAC registers: if a register should be always accessible, or if
it is accessible, but partly implemented, or if the register requires a
certain compatibility flag to be accessed. Currently, 5 hypothetical flags
are supported (3 exist for e1000 so far) but if in the future more than 5
flags will be needed, the datatype of this array can simply be swapped for
a larger one.

This patch is intended to solve the following current problems:

1) On migration between different versions of QEMU, which differ by the
MAC registers implemented in them, some registers need not to be active if
a compatibility flag is set, in order to preserve the machine's state
perfectly for the older version. Checking this for each register
individually, would create a lot of clutter in the code.

2) Some registers are (or may be) only partly implemented (e.g.
placeholders that allow reading and writing, but lack other functions).
In such cases it is better to print a debug warning on read/write attempts.
As above, dealing with this functionality on a per-register level, would
require longer and more messy code.

Signed-off-by: Leonid Bloch <address@hidden>
Signed-off-by: Dmitry Fleytman <address@hidden>
---
 hw/net/e1000.c | 85 +++++++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 73 insertions(+), 12 deletions(-)

diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index 0e00afa..2bc533f 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -142,6 +142,8 @@ typedef struct E1000State_st {
     uint32_t compat_flags;
 } E1000State;
 
+#define chkflag(x)     (s->compat_flags & E1000_FLAG_##x)
+
 typedef struct E1000BaseClass {
     PCIDeviceClass parent_class;
     uint16_t phy_id2;
@@ -195,8 +197,7 @@ e1000_link_up(E1000State *s)
 static bool
 have_autoneg(E1000State *s)
 {
-    return (s->compat_flags & E1000_FLAG_AUTONEG) &&
-           (s->phy_reg[PHY_CTRL] & MII_CR_AUTO_NEG_EN);
+    return chkflag(AUTONEG) && (s->phy_reg[PHY_CTRL] & MII_CR_AUTO_NEG_EN);
 }
 
 static void
@@ -321,7 +322,7 @@ set_interrupt_cause(E1000State *s, int index, uint32_t val)
         if (s->mit_timer_on) {
             return;
         }
-        if (s->compat_flags & E1000_FLAG_MIT) {
+        if (chkflag(MIT)) {
             /* Compute the next mitigation delay according to pending
              * interrupts and the current values of RADV (provided
              * RDTR!=0), TADV and ITR.
@@ -1258,6 +1259,43 @@ static void (*macreg_writeops[])(E1000State *, int, 
uint32_t) = {
 
 enum { NWRITEOPS = ARRAY_SIZE(macreg_writeops) };
 
+enum { MAC_ACCESS_ALWAYS = 1, MAC_ACCESS_PARTIAL = 2,
+       MAC_ACCESS_FLAG_NEEDED = 4 };
+
+#define markflag(x)    ((E1000_FLAG_##x << 3) | MAC_ACCESS_FLAG_NEEDED)
+/* In the array below the meaning of the bits is: [f|f|f|f|f|n|p|a]
+ * f - flag bits (up to 5 possible flags)
+ * n - flag needed
+ * p - partially implenented
+ * a - access enabled always
+ * n=p=a=0 - not implemented or unknown */
+static const uint8_t mac_reg_chk[0x8000] = {
+    [CTRL]    = MAC_ACCESS_ALWAYS,    [EECD]   = MAC_ACCESS_ALWAYS,
+    [GPTC]    = MAC_ACCESS_ALWAYS,    [ICR]    = MAC_ACCESS_ALWAYS,
+    [IMS]     = MAC_ACCESS_ALWAYS,    [LEDCTL] = MAC_ACCESS_ALWAYS,
+    [MPC]     = MAC_ACCESS_ALWAYS,    [PBA]    = MAC_ACCESS_ALWAYS,
+    [RDBAL]   = MAC_ACCESS_ALWAYS,    [RDH]    = MAC_ACCESS_ALWAYS,
+    [STATUS]  = MAC_ACCESS_ALWAYS,    [SWSM]   = MAC_ACCESS_ALWAYS,
+    [TDBAL]   = MAC_ACCESS_ALWAYS,    [TDH]    = MAC_ACCESS_ALWAYS,
+    [TORH]    = MAC_ACCESS_ALWAYS,    [TORL]   = MAC_ACCESS_ALWAYS,
+    [TPR]     = MAC_ACCESS_ALWAYS,    [TPT]    = MAC_ACCESS_ALWAYS,
+    [RA]      = MAC_ACCESS_ALWAYS,    [MTA]    = MAC_ACCESS_ALWAYS,
+    [EERD]    = MAC_ACCESS_ALWAYS,    [GPRC]   = MAC_ACCESS_ALWAYS,
+    [ICS]     = MAC_ACCESS_ALWAYS,    [IMC]    = MAC_ACCESS_ALWAYS,
+    [MANC]    = MAC_ACCESS_ALWAYS,    [MDIC]   = MAC_ACCESS_ALWAYS,
+    [RCTL]    = MAC_ACCESS_ALWAYS,    [RDBAH]  = MAC_ACCESS_ALWAYS,
+    [RDLEN]   = MAC_ACCESS_ALWAYS,    [RDT]    = MAC_ACCESS_ALWAYS,
+    [TCTL]    = MAC_ACCESS_ALWAYS,    [TDBAH]  = MAC_ACCESS_ALWAYS,
+    [TDLEN]   = MAC_ACCESS_ALWAYS,    [TDT]    = MAC_ACCESS_ALWAYS,
+    [TOTH]    = MAC_ACCESS_ALWAYS,    [TOTL]   = MAC_ACCESS_ALWAYS,
+    [TXDCTL]  = MAC_ACCESS_ALWAYS,    [WUFC]   = MAC_ACCESS_ALWAYS,
+    [CRCERRS] = MAC_ACCESS_ALWAYS,    [VFTA]   = MAC_ACCESS_ALWAYS,
+    [VET]     = MAC_ACCESS_ALWAYS,
+
+    [RDTR]    = markflag(MIT),    [TADV]    = markflag(MIT),
+    [RADV]    = markflag(MIT),    [ITR]     = markflag(MIT),
+};
+
 static void
 e1000_mmio_write(void *opaque, hwaddr addr, uint64_t val,
                  unsigned size)
@@ -1266,9 +1304,21 @@ e1000_mmio_write(void *opaque, hwaddr addr, uint64_t val,
     unsigned int index = (addr & 0x1ffff) >> 2;
 
     if (index < NWRITEOPS && macreg_writeops[index]) {
-        macreg_writeops[index](s, index, val);
+        if ((mac_reg_chk[index] & MAC_ACCESS_ALWAYS)
+            || ((mac_reg_chk[index] & MAC_ACCESS_FLAG_NEEDED)
+                && (s->compat_flags & (mac_reg_chk[index] >> 3)))) {
+            if (mac_reg_chk[index] & MAC_ACCESS_PARTIAL) {
+                DBGOUT(GENERAL, "Writing to register at offset: 0x%08x. "
+                       "It is not fully implemented.\n", index<<2);
+            }
+            macreg_writeops[index](s, index, val);
+        } else if (mac_reg_chk[index] & MAC_ACCESS_FLAG_NEEDED) {
+            DBGOUT(MMIO, "MMIO write attempt to disabled reg. addr=0x%08x\n",
+                   index<<2);
+        }
     } else if (index < NREADOPS && macreg_readops[index]) {
-        DBGOUT(MMIO, "e1000_mmio_writel RO %x: 0x%04"PRIx64"\n", index<<2, 
val);
+        DBGOUT(MMIO, "e1000_mmio_writel RO %x: 0x%04"PRIx64"\n",
+               index<<2, val);
     } else {
         DBGOUT(UNKNOWN, "MMIO unknown write addr=0x%08x,val=0x%08"PRIx64"\n",
                index<<2, val);
@@ -1281,11 +1331,22 @@ e1000_mmio_read(void *opaque, hwaddr addr, unsigned 
size)
     E1000State *s = opaque;
     unsigned int index = (addr & 0x1ffff) >> 2;
 
-    if (index < NREADOPS && macreg_readops[index])
-    {
-        return macreg_readops[index](s, index);
+    if (index < NREADOPS && macreg_readops[index]) {
+        if ((mac_reg_chk[index] & MAC_ACCESS_ALWAYS)
+            || ((mac_reg_chk[index] & MAC_ACCESS_FLAG_NEEDED)
+                && (s->compat_flags & (mac_reg_chk[index] >> 3)))) {
+            if (mac_reg_chk[index] & MAC_ACCESS_PARTIAL) {
+                DBGOUT(GENERAL, "Reading register at offset: 0x%08x. "
+                       "It is not fully implemented.\n", index<<2);
+            }
+            return macreg_readops[index](s, index);
+        } else if (mac_reg_chk[index] & MAC_ACCESS_FLAG_NEEDED) {
+            DBGOUT(MMIO, "MMIO read attempt of disabled reg. addr=0x%08x\n",
+                   index<<2);
+        }
+    } else {
+        DBGOUT(UNKNOWN, "MMIO unknown read addr=0x%08x\n", index<<2);
     }
-    DBGOUT(UNKNOWN, "MMIO unknown read addr=0x%08x\n", index<<2);
     return 0;
 }
 
@@ -1352,7 +1413,7 @@ static int e1000_post_load(void *opaque, int version_id)
     E1000State *s = opaque;
     NetClientState *nc = qemu_get_queue(s->nic);
 
-    if (!(s->compat_flags & E1000_FLAG_MIT)) {
+    if (!chkflag(MIT)) {
         s->mac_reg[ITR] = s->mac_reg[RDTR] = s->mac_reg[RADV] =
             s->mac_reg[TADV] = 0;
         s->mit_irq_level = false;
@@ -1379,14 +1440,14 @@ static bool e1000_mit_state_needed(void *opaque)
 {
     E1000State *s = opaque;
 
-    return s->compat_flags & E1000_FLAG_MIT;
+    return chkflag(MIT);
 }
 
 static bool e1000_full_mac_needed(void *opaque)
 {
     E1000State *s = opaque;
 
-    return s->compat_flags & E1000_FLAG_MAC;
+    return chkflag(MAC);
 }
 
 static const VMStateDescription vmstate_e1000_mit_state = {
-- 
2.4.3