diff mbox

[RFC,v3,10/49] rtl8139: adding new fields to vmstate

Message ID 3p4rhou9g4v7a9ufvf2ed5wo.1406821520941@email.android.com
State New
Headers show

Commit Message

Pavel Dovgalyuk July 31, 2014, 3:49 p.m. UTC 
We cannot calculate timers in post_load, because it requires correct value of virtual clock. And at that moment replay data structures could be not loaded yet.


Sent from mobile device



-------- Исходное сообщение --------
От: Paolo Bonzini <pbonzini@redhat.com> 
Дата:31.07.2014  18:14  (GMT+04:00) 
Кому: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>,qemu-devel@nongnu.org 
Копия: peter.maydell@linaro.org,peter.crosthwaite@xilinx.com,mark.burton@greensocs.com,real@ispras.ru,batuzovk@ispras.ru,maria.klimushenkova@ispras.ru,eblake@redhat.com,afaerber@suse.de,fred.konrad@greensocs.com 
Тема: Re: [RFC PATCH v3 10/49] rtl8139: adding new fields to vmstate 

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>

Comments

Eric Blake July 31, 2014, 3:55 p.m. UTC | #1 
[meta-comment]

On 07/31/2014 09:49 AM, Pavel Dovgalyuk wrote:
> We cannot calculate timers in post_load, because it requires correct value of virtual clock. And at that moment replay data structures could be not loaded yet.
> 
> 
> Sent from mobile device

Your mobile device is breaking threading.  It would be nice to stick to
standards-compliant mailers, for the sake of reviewers trying to read
your messages in threaded context.
Paolo Bonzini July 31, 2014, 4:20 p.m. UTC | #2 
Il 31/07/2014 17:49, Pavel Dovgalyuk ha scritto:
> We cannot calculate timers in post_load, because it requires correct
> value of virtual clock. And at that moment replay data structures could
> be not loaded yet.

Then this patch (and similarly others in this early part of the series)
should be properly named "work around deficiencies of replay", not "add
new fields to vmstate".

Replay of course has to do _more_ things than migration.  However,
anything that makes replay _different_ from migration, for things that
migration already does, is not going to fly.  It means instant bitrot,
and will not be accepted without very good justification.

timers_state is serialized before everything else for a reason.  If you
need to serialize more state for replay to work, add it to a subsection
of timers_state that is only included during record.

Paolo
diff mbox

Patch

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");