[Qemu-devel] [RFC PATCH 2/2] rtl8139: update HMP only when the address is fully written

[Vlad Yasevich <vyasevic@redhat.com>]

rtl8139 hardware requires 9346 config register to be set into
write mode before mac address can be changed even though it is
not documented.  Every driver inspected so far appears to do
this along with comments that this is an undocumented requirement.

We can use this to help us identify when the mac address has been
completely written.  Simple set a flag whenever mac has changed
and at the next transition of 9346 register from Write to Normal
mode, we update the HMP.

Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
---
 hw/i386/pc_piix.c    |  4 ++++
 hw/i386/pc_q35.c     |  4 ++++
 hw/net/rtl8139.c     | 50 +++++++++++++++++++++++++++++++++++++++++++++++++-
 include/hw/i386/pc.h |  8 ++++++++
 4 files changed, 65 insertions(+), 1 deletion(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 2daa111..731ae3b 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -372,6 +372,10 @@ static QEMUMachine pc_i440fx_machine_v1_7 = {
     PC_I440FX_1_7_MACHINE_OPTIONS,
     .name = "pc-i440fx-1.7",
     .init = pc_init_pci_1_7,
+    .compat_props = (GlobalProperty[]) {
+        PC_COMPAT_1_7,
+        { /* end of list */ }
+    },
 };
 
 #define PC_I440FX_1_6_MACHINE_OPTIONS PC_I440FX_1_7_MACHINE_OPTIONS
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index e2b8907..7b6aedf 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -292,6 +292,10 @@ static QEMUMachine pc_q35_machine_v1_7 = {
     PC_Q35_1_7_MACHINE_OPTIONS,
     .name = "pc-q35-1.7",
     .init = pc_q35_init_1_7,
+    .compat_props = (GlobalProperty[]) {
+        PC_COMPAT_1_7,
+        { /* end of list */ }
+    },
 };
 
 #define PC_Q35_1_6_MACHINE_OPTIONS PC_Q35_1_7_MACHINE_OPTIONS
diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
index 7f2b4db..5c4caec 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -476,6 +476,7 @@ typedef struct RTL8139State {
 
     uint16_t CpCmd;
     uint8_t  TxThresh;
+    bool     mac_changed;
 
     NICState *nic;
     NICConf conf;
@@ -515,6 +516,9 @@ typedef struct RTL8139State {
 
     /* Support migration to/from old versions */
     int rtl8139_mmio_io_addr_dummy;
+#define RTL8139_FLAG_MAC_BIT 0
+#define RTL8139_FLAG_MAC_COMPLETE (1 << RTL8139_FLAG_MAC_BIT)
+    uint32_t    compat_flags;
 } RTL8139State;
 
 /* Writes tally counters to memory via DMA */
@@ -1215,6 +1219,7 @@ static void rtl8139_reset(DeviceState *d)
     /* restore MAC address */
     memcpy(s->phys, s->conf.macaddr.a, 6);
     qemu_format_nic_info_str(qemu_get_queue(s->nic), s->phys);
+    s->mac_changed = false;
 
     /* reset interrupt mask */
     s->IntrStatus = 0;
@@ -1563,6 +1568,14 @@ static void rtl8139_Cfg9346_write(RTL8139State *s, uint32_t val)
         /* Reset.  */
         val = 0;
         rtl8139_reset(d);
+    } else if (opmode == Cfg9346_Normal && s->mac_changed) {
+        /* Even though it is not documented, it is required to set
+         * opmode to Cfg9346_ConfigWrite when changing the mac address
+         * of the card and to set to Cfg9346_Normal when done.  We
+         * can use this as an idication to kick off the notification event.
+         */
+        qemu_format_nic_info_str(qemu_get_queue(s->nic), s->phys);
+        s->mac_changed = false;
     }
 
     s->Cfg9346 = val;
@@ -2743,7 +2756,12 @@ static void rtl8139_io_writeb(void *opaque, uint8_t addr, uint32_t val)
     {
         case MAC0 ... MAC0+5:
             s->phys[addr - MAC0] = val;
-            qemu_format_nic_info_str(qemu_get_queue(s->nic), s->phys);
+            if (s->compat_flags & RTL8139_FLAG_MAC_COMPLETE) {
+                s->mac_changed = true;
+            } else if (addr == MAC0+5) {
+                /* Emulate old style updates on the last write */
+                qemu_format_nic_info_str(qemu_get_queue(s->nic), s->phys);
+            }
             break;
         case MAC0+6 ... MAC0+7:
             /* reserved */
@@ -3256,6 +3274,13 @@ static int rtl8139_post_load(void *opaque, int version_id)
      * to link status bit in BasicModeStatus */
     qemu_get_queue(s->nic)->link_down = (s->BasicModeStatus & 0x04) == 0;
 
+    /* Emulate old behavior if we don't support mac change completion
+     * tracking
+     */
+    if (!(s->compat_flags & RTL8139_FLAG_MAC_COMPLETE)) {
+        s->mac_changed = false;
+    }
+
     return 0;
 }
 
@@ -3286,6 +3311,24 @@ static void rtl8139_pre_save(void *opaque)
     s->rtl8139_mmio_io_addr_dummy = 0;
 }
 
+static bool rtl8139_mac_state_needed(void *opaque)
+{
+    RTL8139State *s = opaque;
+
+    return (s->compat_flags & RTL8139_FLAG_MAC_COMPLETE) && s->mac_changed;
+}
+
+static const VMStateDescription vmstate_rtl8139_mac_state ={
+    .name = "rtl8139/mac_state",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .fields      = (VMStateField []) {
+        VMSTATE_BOOL(mac_changed, RTL8139State),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const VMStateDescription vmstate_rtl8139 = {
     .name = "rtl8139",
     .version_id = 4,
@@ -3371,6 +3414,9 @@ static const VMStateDescription vmstate_rtl8139 = {
             .vmsd = &vmstate_rtl8139_hotplug_ready,
             .needed = rtl8139_hotplug_ready_needed,
         }, {
+            .vmsd = &vmstate_rtl8139_mac_state,
+            .needed = rtl8139_mac_state_needed,
+        }, {
             /* empty */
         }
     }
@@ -3547,6 +3593,8 @@ static int pci_rtl8139_init(PCIDevice *dev)
 
 static Property rtl8139_properties[] = {
     DEFINE_NIC_PROPERTIES(RTL8139State, conf),
+    DEFINE_PROP_BIT("mac_complete", RTL8139State,
+                    compat_flags, RTL8139_FLAG_MAC_BIT, true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 57e8d16..d1cfdde 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -247,7 +247,15 @@ uint16_t pvpanic_port(void);
 
 int e820_add_entry(uint64_t, uint64_t, uint32_t);
 
+#define PC_COMPAT_1_7 \
+        {\
+            .driver = "rtl8139",\
+            .property="mac_complete",\
+            .value = "off",\
+        }
+
 #define PC_COMPAT_1_6 \
+        PC_COMPAT_1_7, \
         {\
             .driver   = "e1000",\
             .property = "mitigation",\
-- 
1.8.4.2