Re: [Qemu-devel] [RFC PATCH v3 10/49] rtl8139: adding new fields to vmstate

[Paolo Bonzini <pbonzini@redhat.com>]

Il 31/07/2014 14:54, Pavel Dovgalyuk ha scritto:
> +static bool rtl8139_TimerExpire_needed(void *opaque)
> +{
> +    RTL8139State *s = (RTL8139State *)opaque;
> +    return s->TimerExpire != 0;
> +}
> +
> +static const VMStateDescription vmstate_rtl8139_TimerExpire = {
> +    .name = "rtl8139/TimerExpire",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_INT64(TimerExpire, RTL8139State),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static bool rtl8139_timer_needed(void *opaque)
> +{
> +    RTL8139State *s = (RTL8139State *)opaque;
> +    return timer_pending(s->timer);
> +}

I think this is needed only because of the complicated logic to compute
PCSTimeout on the fly when IntrStatus/IntrMask change or are read.
But we can just eliminate it, and leave the timer always set when
TimerInt is nonzero.

rtl8139_set_next_tctr_time should now be idempotent and deterministic.
Then it can be set in rtl8139_post_load.


---------------- 8< ---------------
From: Paolo Bonzini <pbonzini@redhat.com>
Subject: [PATCH] rtl8139: simplify timer logic

Pavel Dovgalyuk reports that TimerExpire and the timer are not restored
correctly on the receiving end of migration.

It is not clear to me whether this is really the case, but we can take
the occasion to get rid of the complicated code that computes PCSTimeout
on the fly upon changes to IntrStatus/IntrMask.  Just always keep a
timer running, it will fire every ~130 seconds at most if the interrupt
is masked with TimerInt != 0.

This makes rtl8139_set_next_tctr_time idempotent (when called with
wrap_on_overflow == false, as is the case from the post_load callback).

Not-tested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
index 90bc5ec..430eab3 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -508,7 +508,6 @@ typedef struct RTL8139State {
 
     /* PCI interrupt timer */
     QEMUTimer *timer;
-    int64_t TimerExpire;
 
     MemoryRegion bar_io;
     MemoryRegion bar_mem;
@@ -520,7 +519,7 @@ typedef struct RTL8139State {
 /* Writes tally counters to memory via DMA */
 static void RTL8139TallyCounters_dma_write(RTL8139State *s, dma_addr_t tc_addr);
 
-static void rtl8139_set_next_tctr_time(RTL8139State *s, int64_t current_time);
+static void rtl8139_set_next_tctr_time(RTL8139State *s, bool wrap_on_overflow);
 
 static void prom9346_decode_command(EEprom9346 *eeprom, uint8_t command)
 {
@@ -1282,6 +1281,7 @@ static void rtl8139_reset(DeviceState *d)
     s->TCTR = 0;
     s->TimerInt = 0;
     s->TCTR_base = 0;
+    rtl8139_set_next_tctr_time(s, false);
 
     /* reset tally counters */
     RTL8139TallyCounters_clear(&s->tally_counters);
@@ -2648,7 +2648,6 @@ static void rtl8139_IntrMask_write(RTL8139State *s, uint32_t val)
 
     s->IntrMask = val;
 
-    rtl8139_set_next_tctr_time(s, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
     rtl8139_update_irq(s);
 
 }
@@ -2683,13 +2682,7 @@ static void rtl8139_IntrStatus_write(RTL8139State *s, uint32_t val)
     rtl8139_update_irq(s);
 
     s->IntrStatus = newStatus;
-    /*
-     * Computing if we miss an interrupt here is not that correct but
-     * considered that we should have had already an interrupt
-     * and probably emulated is slower is better to assume this resetting was
-     * done before testing on previous rtl8139_update_irq lead to IRQ losing
-     */
-    rtl8139_set_next_tctr_time(s, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
+    rtl8139_set_next_tctr_time(s, true);
     rtl8139_update_irq(s);
 
 #endif
@@ -2697,8 +2690,6 @@ static void rtl8139_IntrStatus_write(RTL8139State *s, uint32_t val)
 
 static uint32_t rtl8139_IntrStatus_read(RTL8139State *s)
 {
-    rtl8139_set_next_tctr_time(s, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
-
     uint32_t ret = s->IntrStatus;
 
     DPRINTF("IntrStatus read(w) val=0x%04x\n", ret);
@@ -2881,43 +2872,25 @@ static void rtl8139_io_writew(void *opaque, uint8_t addr, uint32_t val)
     }
 }
 
-static void rtl8139_set_next_tctr_time(RTL8139State *s, int64_t current_time)
+static void rtl8139_set_next_tctr_time(RTL8139State *s, bool wrap_on_overflow)
 {
-    int64_t pci_time, next_time;
-    uint32_t low_pci;
+    int64_t delta;
 
     DPRINTF("entered rtl8139_set_next_tctr_time\n");
 
-    if (s->TimerExpire && current_time >= s->TimerExpire) {
-        s->IntrStatus |= PCSTimeout;
-        rtl8139_update_irq(s);
-    }
-
-    /* Set QEMU timer only if needed that is
-     * - TimerInt <> 0 (we have a timer)
-     * - mask = 1 (we want an interrupt timer)
-     * - irq = 0  (irq is not already active)
-     * If any of above change we need to compute timer again
-     * Also we must check if timer is passed without QEMU timer
-     */
-    s->TimerExpire = 0;
     if (!s->TimerInt) {
-        return;
-    }
-
-    pci_time = muldiv64(current_time - s->TCTR_base, PCI_FREQUENCY,
-                                get_ticks_per_sec());
-    low_pci = pci_time & 0xffffffff;
-    pci_time = pci_time - low_pci + s->TimerInt;
-    if (low_pci >= s->TimerInt) {
-        pci_time += 0x100000000LL;
-    }
-    next_time = s->TCTR_base + muldiv64(pci_time, get_ticks_per_sec(),
-                                                PCI_FREQUENCY);
-    s->TimerExpire = next_time;
-
-    if ((s->IntrMask & PCSTimeout) != 0 && (s->IntrStatus & PCSTimeout) == 0) {
-        timer_mod(s->timer, next_time);
+        timer_del(s->timer);
+    } else {
+        delta = muldiv64(s->TimerInt, get_ticks_per_sec(), PCI_FREQUENCY);
+        if (wrap_on_overflow &&
+            s->TCTR_base + delta >= qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)) {
+            /* The value in the Timer register does not change, but
+             * we're counting up by 2^32 ticks (approx. 130 seconds).
+             */
+            s->TCTR_base += muldiv64(0x100000000LL, get_ticks_per_sec(),
+                                     PCI_FREQUENCY);
+        }
+        timer_mod(s->timer, s->TCTR_base + delta);
     }
 }
 
@@ -2965,14 +2938,14 @@ static void rtl8139_io_writel(void *opaque, uint8_t addr, uint32_t val)
         case Timer:
             DPRINTF("TCTR Timer reset on write\n");
             s->TCTR_base = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
-            rtl8139_set_next_tctr_time(s, s->TCTR_base);
+            rtl8139_set_next_tctr_time(s, false);
             break;
 
         case FlashReg:
             DPRINTF("FlashReg TimerInt write val=0x%08x\n", val);
             if (s->TimerInt != val) {
                 s->TimerInt = val;
-                rtl8139_set_next_tctr_time(s, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
+                rtl8139_set_next_tctr_time(s, true);
             }
             break;
 
@@ -3249,7 +3222,7 @@ static uint32_t rtl8139_mmio_readl(void *opaque, hwaddr addr)
 static int rtl8139_post_load(void *opaque, int version_id)
 {
     RTL8139State* s = opaque;
-    rtl8139_set_next_tctr_time(s, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
+    rtl8139_set_next_tctr_time(s, false);
     if (version_id < 4) {
         s->cplus_enabled = s->CpCmd != 0;
     }
@@ -3280,8 +3253,7 @@ static void rtl8139_pre_save(void *opaque)
     RTL8139State* s = opaque;
     int64_t current_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
 
-    /* set IntrStatus correctly */
-    rtl8139_set_next_tctr_time(s, current_time);
+    /* for migration to older versions */
     s->TCTR = muldiv64(current_time - s->TCTR_base, PCI_FREQUENCY,
                        get_ticks_per_sec());
     s->rtl8139_mmio_io_addr_dummy = 0;
@@ -3448,7 +3420,7 @@ static void rtl8139_timer(void *opaque)
 
     s->IntrStatus |= PCSTimeout;
     rtl8139_update_irq(s);
-    rtl8139_set_next_tctr_time(s, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
+    rtl8139_set_next_tctr_time(s, true);
 }
 
 static void rtl8139_cleanup(NetClientState *nc)
@@ -3536,9 +3508,7 @@ static int pci_rtl8139_init(PCIDevice *dev)
     s->cplus_txbuffer_len = 0;
     s->cplus_txbuffer_offset = 0;
 
-    s->TimerExpire = 0;
     s->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, rtl8139_timer, s);
-    rtl8139_set_next_tctr_time(s, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
 
     add_boot_device_path(s->conf.bootindex, d, "/ethernet-phy@0");