[Qemu-devel] [PATCH v2 00/12] icount and misc MTTCG fixes for 2.9-rc4

[Qemu-devel] [PATCH v2 00/12] icount and misc MTTCG fixes for 2.9-rc4

[Alex Bennée]

Hi,

Here is my current queue for icount and miscellaneous MTTCG related
fixes. The main difference from the RFC series is I have dropped the
graceful handling of backward time events in the replay log in favour
of updating the global qemu_icount more frequently. The graceful
handling is now replaced with an assertion about the arrow of time.

With these changes I was able to dispenses with additional BQL locking
which I had been applying during cpu_handle_exception.

It doesn't completely solve the problem of keeping the main-loop and
vCPU events stream in sync but it works pretty reliably in my test
case. Given the current release time-line I think this is the best
solution for now. Options for post-2.9 include either replacing the
BQL mediated sequencing with a new lock (pushing replay_lock up the
tree?) or marshalling events through a single thread which can then
keep things in order.

Aside from that other patches have had various r-b and a-b tags
applied from the original posting.

I hope to roll a pull request with these fixes by the end of the week
so we there isn't a rush for next Tuesday's rc4. Expedient comments
and reviews are therefor gratefully received ;-)

Regards,


Alex Bennée (12):
  scripts/qemugdb/mtree.py: fix up mtree dump
  scripts/qemu-gdb/timers.py: new helper to dump timer state
  scripts/replay-dump.py: replay log dumper
  target/i386/misc_helper: wrap BQL around another IRQ generator
  cpus: remove icount handling from qemu_tcg_cpu_thread_fn
  cpus: check cpu->running in cpu_get_icount_raw()
  cpus: move icount preparation out of tcg_exec_cpu
  cpus: don't credit executed instructions before they have run
  cpus: introduce cpu_update_icount helper
  cpu-exec: update icount after each TB_EXIT
  cpus: call cpu_update_icount on read
  replay: assert time only goes forward

 cpu-exec.c                |  14 +--
 cpus.c                    |  98 ++++++++++++-----
 include/qemu/timer.h      |   1 +
 include/qom/cpu.h         |   1 +
 replay/replay-internal.c  |   4 +
 replay/replay.c           |   4 +
 scripts/qemu-gdb.py       |   3 +-
 scripts/qemugdb/mtree.py  |  12 +-
 scripts/qemugdb/timers.py |  54 +++++++++
 scripts/replay-dump.py    | 272 ++++++++++++++++++++++++++++++++++++++++++++++
 target/i386/misc_helper.c |   3 +
 11 files changed, 426 insertions(+), 40 deletions(-)
 create mode 100644 scripts/qemugdb/timers.py
 create mode 100755 scripts/replay-dump.py

-- 
2.11.0

[Qemu-devel] [PATCH v2 01/12] scripts/qemugdb/mtree.py: fix up mtree dump

[Alex Bennée]

Since QEMU has been able to build with native Int128 support this was
broken as it attempts to fish values out of the non-existent
structure. Also the alias print was trying to make a %x out of
gdb.ValueType directly which didn't seem to work.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 scripts/qemugdb/mtree.py | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/scripts/qemugdb/mtree.py b/scripts/qemugdb/mtree.py
index cc8131c2e7..e6791b7885 100644
--- a/scripts/qemugdb/mtree.py
+++ b/scripts/qemugdb/mtree.py
@@ -21,7 +21,15 @@ def isnull(ptr):
     return ptr == gdb.Value(0).cast(ptr.type)
 
 def int128(p):
-    return int(p['lo']) + (int(p['hi']) << 64)
+    '''Read an Int128 type to a python integer.
+
+    QEMU can be built with native Int128 support so we need to detect
+    if the value is a structure or the native type.
+    '''
+    if p.type.code == gdb.TYPE_CODE_STRUCT:
+        return int(p['lo']) + (int(p['hi']) << 64)
+    else:
+        return int(("%s" % p), 16)
 
 class MtreeCommand(gdb.Command):
     '''Display the memory tree hierarchy'''
@@ -69,7 +77,7 @@ class MtreeCommand(gdb.Command):
             gdb.write('%s    alias: %s@%016x (@ %s)\n' %
                       ('  ' * level,
                        alias['name'].string(),
-                       ptr['alias_offset'],
+                       int(ptr['alias_offset']),
                        alias,
                        ),
                       gdb.STDOUT)
-- 
2.11.0

[Qemu-devel] [PATCH v2 02/12] scripts/qemu-gdb/timers.py: new helper to dump timer state

[Alex Bennée]

This introduces the qemu-gdb command "qemu timers" which will dump the
state of the main timers in the system.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 scripts/qemu-gdb.py       |  3 ++-
 scripts/qemugdb/timers.py | 54 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+), 1 deletion(-)
 create mode 100644 scripts/qemugdb/timers.py

diff --git a/scripts/qemu-gdb.py b/scripts/qemu-gdb.py
index b3f8e04f77..3e7adb87dc 100644
--- a/scripts/qemu-gdb.py
+++ b/scripts/qemu-gdb.py
@@ -26,7 +26,7 @@ import os, sys
 
 sys.path.append(os.path.dirname(__file__))
 
-from qemugdb import aio, mtree, coroutine
+from qemugdb import aio, mtree, coroutine, timers
 
 class QemuCommand(gdb.Command):
     '''Prefix for QEMU debug support commands'''
@@ -38,6 +38,7 @@ QemuCommand()
 coroutine.CoroutineCommand()
 mtree.MtreeCommand()
 aio.HandlersCommand()
+timers.TimersCommand()
 
 coroutine.CoroutineSPFunction()
 coroutine.CoroutinePCFunction()
diff --git a/scripts/qemugdb/timers.py b/scripts/qemugdb/timers.py
new file mode 100644
index 0000000000..be71a001e3
--- /dev/null
+++ b/scripts/qemugdb/timers.py
@@ -0,0 +1,54 @@
+#!/usr/bin/python
+# GDB debugging support
+#
+# Copyright 2017 Linaro Ltd
+#
+# Author: Alex Bennée <alex.bennee@linaro.org>
+#
+# This work is licensed under the terms of the GNU GPL, version 2.  See
+# the COPYING file in the top-level directory.
+
+# 'qemu timers' -- display the current timerlists
+
+import gdb
+
+class TimersCommand(gdb.Command):
+    '''Display the current QEMU timers'''
+
+    def __init__(self):
+        'Register the class as a gdb command'
+        gdb.Command.__init__(self, 'qemu timers', gdb.COMMAND_DATA,
+                             gdb.COMPLETE_NONE)
+
+    def dump_timers(self, timer):
+        "Follow a timer and recursively dump each one in the list."
+        # timer should be of type QemuTimer
+        gdb.write("    timer %s/%s (cb:%s,opq:%s)\n" % (
+            timer['expire_time'],
+            timer['scale'],
+            timer['cb'],
+            timer['opaque']))
+
+        if int(timer['next']) > 0:
+            self.dump_timers(timer['next'])
+
+
+    def process_timerlist(self, tlist, ttype):
+        gdb.write("Processing %s timers\n" % (ttype))
+        gdb.write("  clock %s is enabled:%s, last:%s\n" % (
+            tlist['clock']['type'],
+            tlist['clock']['enabled'],
+            tlist['clock']['last']))
+        if int(tlist['active_timers']) > 0:
+            self.dump_timers(tlist['active_timers'])
+
+
+    def invoke(self, arg, from_tty):
+        'Run the command'
+        main_timers = gdb.parse_and_eval("main_loop_tlg")
+
+        # This will break if QEMUClockType in timer.h is redfined
+        self.process_timerlist(main_timers['tl'][0], "Realtime")
+        self.process_timerlist(main_timers['tl'][1], "Virtual")
+        self.process_timerlist(main_timers['tl'][2], "Host")
+        self.process_timerlist(main_timers['tl'][3], "Virtual RT")
-- 
2.11.0

[Qemu-devel] [PATCH v2 03/12] scripts/replay-dump.py: replay log dumper

[Alex Bennée]

This script is a debugging tool for looking through the contents of a
replay log file. It is incomplete but should fail gracefully at events
it doesn't understand.

It currently understands two different log formats as the audio
record/replay support was merged during since MTTCG. It was written to
help debug what has caused the BQL changes to break replay support.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 scripts/replay-dump.py | 272 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 272 insertions(+)
 create mode 100755 scripts/replay-dump.py

diff --git a/scripts/replay-dump.py b/scripts/replay-dump.py
new file mode 100755
index 0000000000..fdd178aba0
--- /dev/null
+++ b/scripts/replay-dump.py
@@ -0,0 +1,272 @@
+#!/usr/bin/env python
+# -*- coding: utf-8 -*-
+#
+# Dump the contents of a recorded execution stream
+#
+#  Copyright (c) 2017 Alex Bennée <alex.bennee@linaro.org>
+#
+# This library is free software; you can redistribute it and/or
+# modify it under the terms of the GNU Lesser General Public
+# License as published by the Free Software Foundation; either
+# version 2 of the License, or (at your option) any later version.
+#
+# This library is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+# Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public
+# License along with this library; if not, see <http://www.gnu.org/licenses/>.
+
+import argparse
+import struct
+from collections import namedtuple
+
+# This mirrors some of the global replay state which some of the
+# stream loading refers to. Some decoders may read the next event so
+# we need handle that case. Calling reuse_event will ensure the next
+# event is read from the cache rather than advancing the file.
+
+class ReplayState(object):
+    def __init__(self):
+        self.event = -1
+        self.event_count = 0
+        self.already_read = False
+        self.current_checkpoint = 0
+        self.checkpoint = 0
+
+    def set_event(self, ev):
+        self.event = ev
+        self.event_count += 1
+
+    def get_event(self):
+        self.already_read = False
+        return self.event
+
+    def reuse_event(self, ev):
+        self.event = ev
+        self.already_read = True
+
+    def set_checkpoint(self):
+        self.checkpoint = self.event - self.checkpoint_start
+
+    def get_checkpoint(self):
+        return self.checkpoint
+
+replay_state = ReplayState()
+
+# Simple read functions that mirror replay-internal.c
+# The file-stream is big-endian and manually written out a byte at a time.
+
+def read_byte(fin):
+    "Read a single byte"
+    return struct.unpack('>B', fin.read(1))[0]
+
+def read_event(fin):
+    "Read a single byte event, but save some state"
+    if replay_state.already_read:
+        return replay_state.get_event()
+    else:
+        replay_state.set_event(read_byte(fin))
+        return replay_state.event
+
+def read_word(fin):
+    "Read a 16 bit word"
+    return struct.unpack('>H', fin.read(2))[0]
+
+def read_dword(fin):
+    "Read a 32 bit word"
+    return struct.unpack('>I', fin.read(4))[0]
+
+def read_qword(fin):
+    "Read a 64 bit word"
+    return struct.unpack('>Q', fin.read(8))[0]
+
+# Generic decoder structure
+Decoder = namedtuple("Decoder", "eid name fn")
+
+def call_decode(table, index, dumpfile):
+    "Search decode table for next step"
+    decoder = next((d for d in table if d.eid == index), None)
+    if not decoder:
+        print "Could not decode index: %d" % (index)
+        print "Entry is: %s" % (decoder)
+        print "Decode Table is:\n%s" % (table)
+        return False
+    else:
+        return decoder.fn(decoder.eid, decoder.name, dumpfile)
+
+# Print event
+def print_event(eid, name, string=None, event_count=None):
+    "Print event with count"
+    if not event_count:
+        event_count = replay_state.event_count
+
+    if string:
+        print "%d:%s(%d) %s" % (event_count, name, eid, string)
+    else:
+        print "%d:%s(%d)" % (event_count, name, eid)
+
+
+# Decoders for each event type
+
+def decode_unimp(eid, name, _unused_dumpfile):
+    "Unimplimented decoder, will trigger exit"
+    print "%s not handled - will now stop" % (name)
+    return False
+
+# Checkpoint decoder
+def swallow_async_qword(eid, name, dumpfile):
+    "Swallow a qword of data without looking at it"
+    step_id = read_qword(dumpfile)
+    print "  %s(%d) @ %d" % (name, eid, step_id)
+    return True
+
+async_decode_table = [ Decoder(0, "REPLAY_ASYNC_EVENT_BH", swallow_async_qword),
+                       Decoder(1, "REPLAY_ASYNC_INPUT", decode_unimp),
+                       Decoder(2, "REPLAY_ASYNC_INPUT_SYNC", decode_unimp),
+                       Decoder(3, "REPLAY_ASYNC_CHAR_READ", decode_unimp),
+                       Decoder(4, "REPLAY_ASYNC_EVENT_BLOCK", decode_unimp),
+                       Decoder(5, "REPLAY_ASYNC_EVENT_NET", decode_unimp),
+]
+# See replay_read_events/replay_read_event
+def decode_async(eid, name, dumpfile):
+    """Decode an ASYNC event"""
+
+    print_event(eid, name)
+
+    async_event_kind = read_byte(dumpfile)
+    async_event_checkpoint = read_byte(dumpfile)
+
+    if async_event_checkpoint != replay_state.current_checkpoint:
+        print "  mismatch between checkpoint %d and async data %d" % (
+            replay_state.current_checkpoint, async_event_checkpoint)
+        return True
+
+    return call_decode(async_decode_table, async_event_kind, dumpfile)
+
+
+def decode_instruction(eid, name, dumpfile):
+    ins_diff = read_dword(dumpfile)
+    print_event(eid, name, "0x%x" % (ins_diff))
+    return True
+
+def decode_audio_out(eid, name, dumpfile):
+    audio_data = read_dword(dumpfile)
+    print_event(eid, name, "%d" % (audio_data))
+    return True
+
+def decode_checkpoint(eid, name, dumpfile):
+    """Decode a checkpoint.
+
+    Checkpoints contain a series of async events with their own specific data.
+    """
+    replay_state.set_checkpoint()
+    # save event count as we peek ahead
+    event_number = replay_state.event_count
+    next_event = read_event(dumpfile)
+
+    # if the next event is EVENT_ASYNC there are a bunch of
+    # async events to read, otherwise we are done
+    if next_event != 3:
+        print_event(eid, name, "no additional data", event_number)
+    else:
+        print_event(eid, name, "more data follows", event_number)
+
+    replay_state.reuse_event(next_event)
+    return True
+
+def decode_checkpoint_init(eid, name, dumpfile):
+    print_event(eid, name)
+    return True
+
+def decode_interrupt(eid, name, dumpfile):
+    print_event(eid, name)
+    return True
+
+def decode_clock(eid, name, dumpfile):
+    clock_data = read_qword(dumpfile)
+    print_event(eid, name, "0x%x" % (clock_data))
+    return True
+
+
+# pre-MTTCG merge
+v5_event_table = [Decoder(0, "EVENT_INSTRUCTION", decode_instruction),
+                  Decoder(1, "EVENT_INTERRUPT", decode_interrupt),
+                  Decoder(2, "EVENT_EXCEPTION", decode_unimp),
+                  Decoder(3, "EVENT_ASYNC", decode_async),
+                  Decoder(4, "EVENT_SHUTDOWN", decode_unimp),
+                  Decoder(5, "EVENT_CHAR_WRITE", decode_unimp),
+                  Decoder(6, "EVENT_CHAR_READ_ALL", decode_unimp),
+                  Decoder(7, "EVENT_CHAR_READ_ALL_ERROR", decode_unimp),
+                  Decoder(8, "EVENT_CLOCK_HOST", decode_clock),
+                  Decoder(9, "EVENT_CLOCK_VIRTUAL_RT", decode_clock),
+                  Decoder(10, "EVENT_CP_CLOCK_WARP_START", decode_checkpoint),
+                  Decoder(11, "EVENT_CP_CLOCK_WARP_ACCOUNT", decode_checkpoint),
+                  Decoder(12, "EVENT_CP_RESET_REQUESTED", decode_checkpoint),
+                  Decoder(13, "EVENT_CP_SUSPEND_REQUESTED", decode_checkpoint),
+                  Decoder(14, "EVENT_CP_CLOCK_VIRTUAL", decode_checkpoint),
+                  Decoder(15, "EVENT_CP_CLOCK_HOST", decode_checkpoint),
+                  Decoder(16, "EVENT_CP_CLOCK_VIRTUAL_RT", decode_checkpoint),
+                  Decoder(17, "EVENT_CP_INIT", decode_checkpoint_init),
+                  Decoder(18, "EVENT_CP_RESET", decode_checkpoint),
+]
+
+# post-MTTCG merge, AUDIO support added
+v6_event_table = [Decoder(0, "EVENT_INSTRUCTION", decode_instruction),
+                  Decoder(1, "EVENT_INTERRUPT", decode_interrupt),
+                  Decoder(2, "EVENT_EXCEPTION", decode_unimp),
+                  Decoder(3, "EVENT_ASYNC", decode_async),
+                  Decoder(4, "EVENT_SHUTDOWN", decode_unimp),
+                  Decoder(5, "EVENT_CHAR_WRITE", decode_unimp),
+                  Decoder(6, "EVENT_CHAR_READ_ALL", decode_unimp),
+                  Decoder(7, "EVENT_CHAR_READ_ALL_ERROR", decode_unimp),
+                  Decoder(8, "EVENT_AUDIO_OUT", decode_audio_out),
+                  Decoder(9, "EVENT_AUDIO_IN", decode_unimp),
+                  Decoder(10, "EVENT_CLOCK_HOST", decode_clock),
+                  Decoder(11, "EVENT_CLOCK_VIRTUAL_RT", decode_clock),
+                  Decoder(12, "EVENT_CP_CLOCK_WARP_START", decode_checkpoint),
+                  Decoder(13, "EVENT_CP_CLOCK_WARP_ACCOUNT", decode_checkpoint),
+                  Decoder(14, "EVENT_CP_RESET_REQUESTED", decode_checkpoint),
+                  Decoder(15, "EVENT_CP_SUSPEND_REQUESTED", decode_checkpoint),
+                  Decoder(16, "EVENT_CP_CLOCK_VIRTUAL", decode_checkpoint),
+                  Decoder(17, "EVENT_CP_CLOCK_HOST", decode_checkpoint),
+                  Decoder(18, "EVENT_CP_CLOCK_VIRTUAL_RT", decode_checkpoint),
+                  Decoder(19, "EVENT_CP_INIT", decode_checkpoint_init),
+                  Decoder(20, "EVENT_CP_RESET", decode_checkpoint),
+]
+
+def parse_arguments():
+    "Grab arguments for script"
+    parser = argparse.ArgumentParser()
+    parser.add_argument("-f", "--file", help='record/replay dump to read from',
+                        required=True)
+    return parser.parse_args()
+
+def decode_file(filename):
+    "Decode a record/replay dump"
+    dumpfile = open(filename, "rb")
+
+    # read and throwaway the header
+    version = read_dword(dumpfile)
+    junk = read_qword(dumpfile)
+
+    print "HEADER: version 0x%x" % (version)
+    if version == 0xe02006:
+        event_decode_table = v6_event_table
+        replay_state.checkpoint_start = 12
+    else:
+        event_decode_table = v5_event_table
+        replay_state.checkpoint_start = 10
+
+    try:
+        decode_ok = True
+        while decode_ok:
+            event = read_event(dumpfile)
+            decode_ok = call_decode(event_decode_table, event, dumpfile)
+    finally:
+        dumpfile.close()
+
+if __name__ == "__main__":
+    args = parse_arguments()
+    decode_file(args.file)
-- 
2.11.0

[Qemu-devel] [PATCH v2 04/12] target/i386/misc_helper: wrap BQL around another IRQ generator

[Alex Bennée]

Anything that calls into HW emulation must be protected by the BQL.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Richard Henderson <rth@twiddle.net>
Acked-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target/i386/misc_helper.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/target/i386/misc_helper.c b/target/i386/misc_helper.c
index ca2ea09f54..628f64aad5 100644
--- a/target/i386/misc_helper.c
+++ b/target/i386/misc_helper.c
@@ -18,6 +18,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/main-loop.h"
 #include "cpu.h"
 #include "exec/helper-proto.h"
 #include "exec/exec-all.h"
@@ -156,7 +157,9 @@ void helper_write_crN(CPUX86State *env, int reg, target_ulong t0)
         break;
     case 8:
         if (!(env->hflags2 & HF2_VINTR_MASK)) {
+            qemu_mutex_lock_iothread();
             cpu_set_apic_tpr(x86_env_get_cpu(env)->apic_state, t0);
+            qemu_mutex_unlock_iothread();
         }
         env->v_tpr = t0 & 0x0f;
         break;
-- 
2.11.0

[Qemu-devel] [PATCH v2 05/12] cpus: remove icount handling from qemu_tcg_cpu_thread_fn

[Alex Bennée]

We should never be running in multi-threaded mode with icount enabled.
There is no point calling handle_icount_deadline here so remove it and
assert !use_icount.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Richard Henderson <rth@twiddle.net>
---
 cpus.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/cpus.c b/cpus.c
index 68fdbc40b9..4e48b9c4ad 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1392,6 +1392,8 @@ static void *qemu_tcg_cpu_thread_fn(void *arg)
 {
     CPUState *cpu = arg;
 
+    g_assert(!use_icount);
+
     rcu_register_thread();
 
     qemu_mutex_lock_iothread();
@@ -1434,8 +1436,6 @@ static void *qemu_tcg_cpu_thread_fn(void *arg)
             }
         }
 
-        handle_icount_deadline();
-
         atomic_mb_set(&cpu->exit_request, 0);
         qemu_tcg_wait_io_event(cpu);
     }
-- 
2.11.0

[Qemu-devel] [PATCH v2 06/12] cpus: check cpu->running in cpu_get_icount_raw()

[Alex Bennée]

The lifetime of current_cpu is now the lifetime of the vCPU thread.
However get_icount_raw() can apply a fudge factor if called while code
is running to take into account the current executed instruction
count.

To ensure this is always the case we also check cpu->running.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Richard Henderson <rth@twiddle.net>
---
 cpus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cpus.c b/cpus.c
index 4e48b9c4ad..18b1746770 100644
--- a/cpus.c
+++ b/cpus.c
@@ -229,7 +229,7 @@ int64_t cpu_get_icount_raw(void)
     CPUState *cpu = current_cpu;
 
     icount = timers_state.qemu_icount;
-    if (cpu) {
+    if (cpu && cpu->running) {
         if (!cpu->can_do_io) {
             fprintf(stderr, "Bad icount read\n");
             exit(1);
-- 
2.11.0

[Qemu-devel] [PATCH v2 07/12] cpus: move icount preparation out of tcg_exec_cpu

[Alex Bennée]

As icount is only supported for single-threaded execution due to the
requirement for determinism let's remove it from the common
tcg_exec_cpu path.

Also remove the additional fiddling which shouldn't be required as the
icount counters should all be rectified as you enter the loop.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v2
  - only clear u16.low
  - drop BQL assert
---
 cpus.c | 67 ++++++++++++++++++++++++++++++++++++++++++++----------------------
 1 file changed, 45 insertions(+), 22 deletions(-)

diff --git a/cpus.c b/cpus.c
index 18b1746770..d9cb9407a2 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1179,47 +1179,64 @@ static void handle_icount_deadline(void)
     }
 }
 
-static int tcg_cpu_exec(CPUState *cpu)
+static void prepare_icount_for_run(CPUState *cpu)
 {
-    int ret;
-#ifdef CONFIG_PROFILER
-    int64_t ti;
-#endif
-
-#ifdef CONFIG_PROFILER
-    ti = profile_getclock();
-#endif
     if (use_icount) {
         int64_t count;
         int decr;
-        timers_state.qemu_icount -= (cpu->icount_decr.u16.low
-                                    + cpu->icount_extra);
-        cpu->icount_decr.u16.low = 0;
-        cpu->icount_extra = 0;
+
+        /* These should always be cleared by process_icount_data after
+         * each vCPU execution. However u16.high can be raised
+         * asynchronously by cpu_exit/cpu_interrupt/tcg_handle_interrupt
+         */
+        g_assert(cpu->icount_decr.u16.low == 0);
+        g_assert(cpu->icount_extra == 0);
+
+
         count = tcg_get_icount_limit();
+
         timers_state.qemu_icount += count;
         decr = (count > 0xffff) ? 0xffff : count;
         count -= decr;
         cpu->icount_decr.u16.low = decr;
         cpu->icount_extra = count;
     }
-    qemu_mutex_unlock_iothread();
-    cpu_exec_start(cpu);
-    ret = cpu_exec(cpu);
-    cpu_exec_end(cpu);
-    qemu_mutex_lock_iothread();
-#ifdef CONFIG_PROFILER
-    tcg_time += profile_getclock() - ti;
-#endif
+}
+
+static void process_icount_data(CPUState *cpu)
+{
     if (use_icount) {
         /* Fold pending instructions back into the
            instruction counter, and clear the interrupt flag.  */
         timers_state.qemu_icount -= (cpu->icount_decr.u16.low
                         + cpu->icount_extra);
-        cpu->icount_decr.u32 = 0;
+
+        /* Reset the counters */
+        cpu->icount_decr.u16.low = 0;
         cpu->icount_extra = 0;
         replay_account_executed_instructions();
     }
+}
+
+
+static int tcg_cpu_exec(CPUState *cpu)
+{
+    int ret;
+#ifdef CONFIG_PROFILER
+    int64_t ti;
+#endif
+
+#ifdef CONFIG_PROFILER
+    ti = profile_getclock();
+#endif
+    qemu_mutex_unlock_iothread();
+    cpu_exec_start(cpu);
+    ret = cpu_exec(cpu);
+    cpu_exec_end(cpu);
+    qemu_mutex_lock_iothread();
+#ifdef CONFIG_PROFILER
+    tcg_time += profile_getclock() - ti;
+#endif
     return ret;
 }
 
@@ -1306,7 +1323,13 @@ static void *qemu_tcg_rr_cpu_thread_fn(void *arg)
 
             if (cpu_can_run(cpu)) {
                 int r;
+
+                prepare_icount_for_run(cpu);
+
                 r = tcg_cpu_exec(cpu);
+
+                process_icount_data(cpu);
+
                 if (r == EXCP_DEBUG) {
                     cpu_handle_guest_debug(cpu);
                     break;
-- 
2.11.0

[Qemu-devel] [PATCH v2 08/12] cpus: don't credit executed instructions before they have run

[Alex Bennée]

Outside of the vCPU thread icount time will only be tracked against
timers_state.qemu_icount. We no longer credit cycles until they have
completed the run. Inside the vCPU thread we adjust for passage of
time by looking at how many have run so far. This is only valid inside
the vCPU thread while it is running.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 cpus.c            | 25 +++++++++++++++++++------
 include/qom/cpu.h |  1 +
 2 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/cpus.c b/cpus.c
index d9cb9407a2..88eabdc19f 100644
--- a/cpus.c
+++ b/cpus.c
@@ -223,6 +223,15 @@ void qemu_tcg_configure(QemuOpts *opts, Error **errp)
     }
 }
 
+/* The current number of executed instructions is based on what we
+ * originally budgeted minus the current state of the decrementing
+ * icount counters in extra/u16.low.
+ */
+static int64_t cpu_get_icount_executed(CPUState *cpu)
+{
+    return cpu->icount_budget - (cpu->icount_decr.u16.low + cpu->icount_extra);
+}
+
 int64_t cpu_get_icount_raw(void)
 {
     int64_t icount;
@@ -234,7 +243,8 @@ int64_t cpu_get_icount_raw(void)
             fprintf(stderr, "Bad icount read\n");
             exit(1);
         }
-        icount -= (cpu->icount_decr.u16.low + cpu->icount_extra);
+        /* Take into account what has run */
+        icount += cpu_get_icount_executed(cpu);
     }
     return icount;
 }
@@ -1195,7 +1205,10 @@ static void prepare_icount_for_run(CPUState *cpu)
 
         count = tcg_get_icount_limit();
 
-        timers_state.qemu_icount += count;
+        /* To calculate what we have executed so far we need to know
+         * what we originally budgeted to run this cycle */
+        cpu->icount_budget = count;
+
         decr = (count > 0xffff) ? 0xffff : count;
         count -= decr;
         cpu->icount_decr.u16.low = decr;
@@ -1206,14 +1219,14 @@ static void prepare_icount_for_run(CPUState *cpu)
 static void process_icount_data(CPUState *cpu)
 {
     if (use_icount) {
-        /* Fold pending instructions back into the
-           instruction counter, and clear the interrupt flag.  */
-        timers_state.qemu_icount -= (cpu->icount_decr.u16.low
-                        + cpu->icount_extra);
+        /* Account for executed instructions */
+        timers_state.qemu_icount += cpu_get_icount_executed(cpu);
 
         /* Reset the counters */
         cpu->icount_decr.u16.low = 0;
         cpu->icount_extra = 0;
+        cpu->icount_budget = 0;
+
         replay_account_executed_instructions();
     }
 }
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index c3292efe1c..5d10359c8f 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -332,6 +332,7 @@ struct CPUState {
     /* updates protected by BQL */
     uint32_t interrupt_request;
     int singlestep_enabled;
+    int64_t icount_budget;
     int64_t icount_extra;
     sigjmp_buf jmp_env;
 
-- 
2.11.0

[Qemu-devel] [PATCH v2 09/12] cpus: introduce cpu_update_icount helper

[Alex Bennée]

By holding off updates to timer_state.qemu_icount we can run into
trouble when the non-vCPU thread needs to know the time. This helper
ensures we atomically update timers_state.qemu_icount based on what
has been currently executed.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 cpus.c               | 16 ++++++++++++++--
 include/qemu/timer.h |  1 +
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/cpus.c b/cpus.c
index 88eabdc19f..71c3baba87 100644
--- a/cpus.c
+++ b/cpus.c
@@ -232,12 +232,24 @@ static int64_t cpu_get_icount_executed(CPUState *cpu)
     return cpu->icount_budget - (cpu->icount_decr.u16.low + cpu->icount_extra);
 }
 
+/*
+ * Update the global shared timer_state.qemu_icount to take into
+ * account executed instructions. This is done by the TCG vCPU
+ * thread so the main-loop can see time has moved forward.
+ */
+void cpu_update_icount(CPUState *cpu)
+{
+    int64_t executed = cpu_get_icount_executed(cpu);
+    cpu->icount_budget -= executed;
+    atomic_add(&timers_state.qemu_icount, executed);
+}
+
 int64_t cpu_get_icount_raw(void)
 {
     int64_t icount;
     CPUState *cpu = current_cpu;
 
-    icount = timers_state.qemu_icount;
+    icount = atomic_read(&timers_state.qemu_icount);
     if (cpu && cpu->running) {
         if (!cpu->can_do_io) {
             fprintf(stderr, "Bad icount read\n");
@@ -1220,7 +1232,7 @@ static void process_icount_data(CPUState *cpu)
 {
     if (use_icount) {
         /* Account for executed instructions */
-        timers_state.qemu_icount += cpu_get_icount_executed(cpu);
+        cpu_update_icount(cpu);
 
         /* Reset the counters */
         cpu->icount_decr.u16.low = 0;
diff --git a/include/qemu/timer.h b/include/qemu/timer.h
index e1742f2f3d..8a1eb74839 100644
--- a/include/qemu/timer.h
+++ b/include/qemu/timer.h
@@ -869,6 +869,7 @@ int64_t cpu_get_icount_raw(void);
 int64_t cpu_get_icount(void);
 int64_t cpu_get_clock(void);
 int64_t cpu_icount_to_ns(int64_t icount);
+void    cpu_update_icount(CPUState *cpu);
 
 /*******************************************/
 /* host CPU ticks (if available) */
-- 
2.11.0

[Qemu-devel] [PATCH v2 10/12] cpu-exec: update icount after each TB_EXIT

[Alex Bennée]

There is no particular reason we shouldn't update the global system
icount time as we exit each TranslationBlock run. This ensures the
main-loop doesn't have to wait until we exit to the outer loop for
executed instructions to be credited to timer_state.

The prepare_icount_for_run function is slightly tweaked to match the
logic we run in cpu_loop_exec_tb.

Based on Paolo's original suggestion.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 cpu-exec.c | 14 +++++++-------
 cpus.c     | 18 +++++-------------
 2 files changed, 12 insertions(+), 20 deletions(-)

diff --git a/cpu-exec.c b/cpu-exec.c
index 748cb66bca..63a56d0407 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -600,13 +600,13 @@ static inline void cpu_loop_exec_tb(CPUState *cpu, TranslationBlock *tb,
     /* Instruction counter expired.  */
     assert(use_icount);
 #ifndef CONFIG_USER_ONLY
-    if (cpu->icount_extra) {
-        /* Refill decrementer and continue execution.  */
-        cpu->icount_extra += insns_left;
-        insns_left = MIN(0xffff, cpu->icount_extra);
-        cpu->icount_extra -= insns_left;
-        cpu->icount_decr.u16.low = insns_left;
-    } else {
+    /* Ensure global icount has gone forward */
+    cpu_update_icount(cpu);
+    /* Refill decrementer and continue execution.  */
+    insns_left = MIN(0xffff, cpu->icount_budget);
+    cpu->icount_decr.u16.low = insns_left;
+    cpu->icount_extra = cpu->icount_budget - insns_left;
+    if (!cpu->icount_extra) {
         /* Execute any remaining instructions, then let the main loop
          * handle the next event.
          */
diff --git a/cpus.c b/cpus.c
index 71c3baba87..ff75af449a 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1204,8 +1204,7 @@ static void handle_icount_deadline(void)
 static void prepare_icount_for_run(CPUState *cpu)
 {
     if (use_icount) {
-        int64_t count;
-        int decr;
+        int insns_left;
 
         /* These should always be cleared by process_icount_data after
          * each vCPU execution. However u16.high can be raised
@@ -1214,17 +1213,10 @@ static void prepare_icount_for_run(CPUState *cpu)
         g_assert(cpu->icount_decr.u16.low == 0);
         g_assert(cpu->icount_extra == 0);
 
-
-        count = tcg_get_icount_limit();
-
-        /* To calculate what we have executed so far we need to know
-         * what we originally budgeted to run this cycle */
-        cpu->icount_budget = count;
-
-        decr = (count > 0xffff) ? 0xffff : count;
-        count -= decr;
-        cpu->icount_decr.u16.low = decr;
-        cpu->icount_extra = count;
+        cpu->icount_budget = tcg_get_icount_limit();
+        insns_left = MIN(0xffff, cpu->icount_budget);
+        cpu->icount_decr.u16.low = insns_left;
+        cpu->icount_extra = cpu->icount_budget - insns_left;
     }
 }
 
-- 
2.11.0

[Qemu-devel] [PATCH v2 11/12] cpus: call cpu_update_icount on read

[Alex Bennée]

This ensures each time the vCPU thread reads the icount we update the
master timer_state.qemu_icount field. This way as long as updates are
in BQL protected sections (which they should be) the main-loop can
never come to update the log and find time has gone backwards.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 cpus.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/cpus.c b/cpus.c
index ff75af449a..63de033cc8 100644
--- a/cpus.c
+++ b/cpus.c
@@ -246,19 +246,17 @@ void cpu_update_icount(CPUState *cpu)
 
 int64_t cpu_get_icount_raw(void)
 {
-    int64_t icount;
     CPUState *cpu = current_cpu;
 
-    icount = atomic_read(&timers_state.qemu_icount);
     if (cpu && cpu->running) {
         if (!cpu->can_do_io) {
             fprintf(stderr, "Bad icount read\n");
             exit(1);
         }
         /* Take into account what has run */
-        icount += cpu_get_icount_executed(cpu);
+        cpu_update_icount(cpu);
     }
-    return icount;
+    return atomic_read(&timers_state.qemu_icount);
 }
 
 /* Return the virtual CPU time, based on the instruction counter.  */
-- 
2.11.0

[Qemu-devel] [PATCH v2 12/12] replay: assert time only goes forward

[Alex Bennée]

If we find ourselves trying to add an event to the log where time has
gone backwards it is because a vCPU event has occurred and the
main-loop is not yet aware of time moving forward. This should not
happen and if it does its better to fail early than generate a log
that will have weird behaviour.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 replay/replay-internal.c | 4 ++++
 replay/replay.c          | 4 ++++
 2 files changed, 8 insertions(+)

diff --git a/replay/replay-internal.c b/replay/replay-internal.c
index bea7b4aa6b..fca8514012 100644
--- a/replay/replay-internal.c
+++ b/replay/replay-internal.c
@@ -195,6 +195,10 @@ void replay_save_instructions(void)
     if (replay_file && replay_mode == REPLAY_MODE_RECORD) {
         replay_mutex_lock();
         int diff = (int)(replay_get_current_step() - replay_state.current_step);
+
+        /* Time can only go forward */
+        assert(diff >= 0);
+
         if (diff > 0) {
             replay_put_event(EVENT_INSTRUCTION);
             replay_put_dword(diff);
diff --git a/replay/replay.c b/replay/replay.c
index 9e0724e756..f810628cac 100644
--- a/replay/replay.c
+++ b/replay/replay.c
@@ -84,6 +84,10 @@ void replay_account_executed_instructions(void)
         if (replay_state.instructions_count > 0) {
             int count = (int)(replay_get_current_step()
                               - replay_state.current_step);
+
+            /* Time can only go forward */
+            assert(count >= 0);
+
             replay_state.instructions_count -= count;
             replay_state.current_step += count;
             if (replay_state.instructions_count == 0) {
-- 
2.11.0

Re: [Qemu-devel] [PATCH v2 12/12] replay: assert time only goes forward

[Pavel Dovgalyuk]

> From: Alex Bennée [mailto:alex.bennee@linaro.org]
> 
> If we find ourselves trying to add an event to the log where time has
> gone backwards it is because a vCPU event has occurred and the
> main-loop is not yet aware of time moving forward. This should not
> happen and if it does its better to fail early than generate a log
> that will have weird behaviour.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  replay/replay-internal.c | 4 ++++
>  replay/replay.c          | 4 ++++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/replay/replay-internal.c b/replay/replay-internal.c
> index bea7b4aa6b..fca8514012 100644
> --- a/replay/replay-internal.c
> +++ b/replay/replay-internal.c
> @@ -195,6 +195,10 @@ void replay_save_instructions(void)
>      if (replay_file && replay_mode == REPLAY_MODE_RECORD) {
>          replay_mutex_lock();
>          int diff = (int)(replay_get_current_step() - replay_state.current_step);
> +
> +        /* Time can only go forward */
> +        assert(diff >= 0);
> +
>          if (diff > 0) {

This "if" is useless then.

>              replay_put_event(EVENT_INSTRUCTION);
>              replay_put_dword(diff);
> diff --git a/replay/replay.c b/replay/replay.c
> index 9e0724e756..f810628cac 100644
> --- a/replay/replay.c
> +++ b/replay/replay.c
> @@ -84,6 +84,10 @@ void replay_account_executed_instructions(void)
>          if (replay_state.instructions_count > 0) {
>              int count = (int)(replay_get_current_step()
>                                - replay_state.current_step);
> +
> +            /* Time can only go forward */
> +            assert(count >= 0);
> +
>              replay_state.instructions_count -= count;
>              replay_state.current_step += count;
>              if (replay_state.instructions_count == 0) {
> --
> 2.11.0


Pavel Dovgalyuk

Re: [Qemu-devel] [PATCH v2 12/12] replay: assert time only goes forward

[Paolo Bonzini]

On 05/04/2017 15:33, Pavel Dovgalyuk wrote:
>> +
>> +        /* Time can only go forward */
>> +        assert(diff >= 0);
>> +
>>          if (diff > 0) {
> This "if" is useless then.
> 

It isn't, "diff == 0" can happen.

Paolo

Re: [Qemu-devel] [PATCH v2 11/12] cpus: call cpu_update_icount on read

[Paolo Bonzini]

On 05/04/2017 15:25, Alex Bennée wrote:
> This ensures each time the vCPU thread reads the icount we update the
> master timer_state.qemu_icount field. This way as long as updates are
> in BQL protected sections (which they should be) the main-loop can
> never come to update the log and find time has gone backwards.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  cpus.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/cpus.c b/cpus.c
> index ff75af449a..63de033cc8 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -246,19 +246,17 @@ void cpu_update_icount(CPUState *cpu)
>  
>  int64_t cpu_get_icount_raw(void)
>  {
> -    int64_t icount;
>      CPUState *cpu = current_cpu;
>  
> -    icount = atomic_read(&timers_state.qemu_icount);
>      if (cpu && cpu->running) {
>          if (!cpu->can_do_io) {
>              fprintf(stderr, "Bad icount read\n");
>              exit(1);
>          }
>          /* Take into account what has run */
> -        icount += cpu_get_icount_executed(cpu);
> +        cpu_update_icount(cpu);
>      }
> -    return icount;
> +    return atomic_read(&timers_state.qemu_icount);
>  }
>  
>  /* Return the virtual CPU time, based on the instruction counter.  */
> 

Maybe the update should be done in gen_io_start instead.  There
shouldn't be any interference between vCPU and I/O threads except
between gen_io_start and gen_io_end.

Paolo

Re: [Qemu-devel] [PATCH v2 09/12] cpus: introduce cpu_update_icount helper

[Paolo Bonzini]

On 05/04/2017 15:25, Alex Bennée wrote:
> +{
> +    int64_t executed = cpu_get_icount_executed(cpu);
> +    cpu->icount_budget -= executed;
> +    atomic_add(&timers_state.qemu_icount, executed);
> +}

Since there's only one writer, it's also okay to do

	atomic_set(&timers_state.qemu_icount,
		   atomic_read(&timers_state.qemu_icount) + executed);

(also not just faster, but also simpler to turn into TCG code if we do
the update in gen_io_start).

Paolo

Re: [Qemu-devel] [PATCH v2 09/12] cpus: introduce cpu_update_icount helper

[Alex Bennée]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 05/04/2017 15:25, Alex Bennée wrote:
>> +{
>> +    int64_t executed = cpu_get_icount_executed(cpu);
>> +    cpu->icount_budget -= executed;
>> +    atomic_add(&timers_state.qemu_icount, executed);
>> +}
>
> Since there's only one writer, it's also okay to do
>
> 	atomic_set(&timers_state.qemu_icount,
> 		   atomic_read(&timers_state.qemu_icount) + executed);
>
> (also not just faster, but also simpler to turn into TCG code if we do
> the update in gen_io_start).

OK fair enough. Annoyingly I've just noticed this breaks for 32 bit
hosts because we might not have CONFIG_ATOMIC64. What's the best
approach? #ifdef and handwave?

--
Alex Bennée

Re: [Qemu-devel] [PATCH v2 12/12] replay: assert time only goes forward

[Alex Bennée]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 05/04/2017 15:33, Pavel Dovgalyuk wrote:
>>> +
>>> +        /* Time can only go forward */
>>> +        assert(diff >= 0);
>>> +
>>>          if (diff > 0) {
>> This "if" is useless then.
>>
>
> It isn't, "diff == 0" can happen.

In my previous patchset I actually output-ed a EVENT_INSTRUCTION with a
zero diff for the negative case although it does seem redundant to
output that if we can avoid it?

--
Alex Bennée

Re: [Qemu-devel] [PATCH v2 09/12] cpus: introduce cpu_update_icount helper

[Paolo Bonzini]

On 05/04/2017 16:34, Alex Bennée wrote:
>> On 05/04/2017 15:25, Alex Bennée wrote:
>>> +{
>>> +    int64_t executed = cpu_get_icount_executed(cpu);
>>> +    cpu->icount_budget -= executed;
>>> +    atomic_add(&timers_state.qemu_icount, executed);
>>> +}
>> Since there's only one writer, it's also okay to do
>>
>> 	atomic_set(&timers_state.qemu_icount,
>> 		   atomic_read(&timers_state.qemu_icount) + executed);
>>
>> (also not just faster, but also simpler to turn into TCG code if we do
>> the update in gen_io_start).
>
> OK fair enough. Annoyingly I've just noticed this breaks for 32 bit
> hosts because we might not have CONFIG_ATOMIC64. What's the best
> approach? #ifdef and handwave?

For 2.9 yes... For 2.10 I'm going to add a "Stat64" type that can
compute max or add of 63-bit values, and blocks on the read side if a
doubleword change is in progress.

Paolo

Re: [Qemu-devel] [PATCH v2 11/12] cpus: call cpu_update_icount on read

[Alex Bennée]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 05/04/2017 15:25, Alex Bennée wrote:
>> This ensures each time the vCPU thread reads the icount we update the
>> master timer_state.qemu_icount field. This way as long as updates are
>> in BQL protected sections (which they should be) the main-loop can
>> never come to update the log and find time has gone backwards.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>  cpus.c | 6 ++----
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/cpus.c b/cpus.c
>> index ff75af449a..63de033cc8 100644
>> --- a/cpus.c
>> +++ b/cpus.c
>> @@ -246,19 +246,17 @@ void cpu_update_icount(CPUState *cpu)
>>
>>  int64_t cpu_get_icount_raw(void)
>>  {
>> -    int64_t icount;
>>      CPUState *cpu = current_cpu;
>>
>> -    icount = atomic_read(&timers_state.qemu_icount);
>>      if (cpu && cpu->running) {
>>          if (!cpu->can_do_io) {
>>              fprintf(stderr, "Bad icount read\n");
>>              exit(1);
>>          }
>>          /* Take into account what has run */
>> -        icount += cpu_get_icount_executed(cpu);
>> +        cpu_update_icount(cpu);
>>      }
>> -    return icount;
>> +    return atomic_read(&timers_state.qemu_icount);
>>  }
>>
>>  /* Return the virtual CPU time, based on the instruction counter.  */
>>
>
> Maybe the update should be done in gen_io_start instead.  There
> shouldn't be any interference between vCPU and I/O threads except
> between gen_io_start and gen_io_end.

I'm not sure I follow. gen_io_start is a translation time thing. At
least here we ensure we update whenever the value is read.


--
Alex Bennée

Re: [Qemu-devel] [PATCH v2 11/12] cpus: call cpu_update_icount on read

[Paolo Bonzini]

----- Original Message -----
> From: "Alex Bennée" <alex.bennee@linaro.org>
> To: "Paolo Bonzini" <pbonzini@redhat.com>
> Cc: dovgaluk@ispras.ru, rth@twiddle.net, "peter maydell" <peter.maydell@linaro.org>, qemu-devel@nongnu.org,
> mttcg@greensocs.com, "fred konrad" <fred.konrad@greensocs.com>, "a rigo" <a.rigo@virtualopensystems.com>,
> cota@braap.org, "bobby prani" <bobby.prani@gmail.com>, nikunj@linux.vnet.ibm.com, "Peter Crosthwaite"
> <crosthwaite.peter@gmail.com>
> Sent: Friday, April 7, 2017 7:35:29 PM
> Subject: Re: [PATCH v2 11/12] cpus: call cpu_update_icount on read
> 
> 
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
> > On 05/04/2017 15:25, Alex Bennée wrote:
> >> This ensures each time the vCPU thread reads the icount we update the
> >> master timer_state.qemu_icount field. This way as long as updates are
> >> in BQL protected sections (which they should be) the main-loop can
> >> never come to update the log and find time has gone backwards.
> >>
> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> >> ---
> >>  cpus.c | 6 ++----
> >>  1 file changed, 2 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/cpus.c b/cpus.c
> >> index ff75af449a..63de033cc8 100644
> >> --- a/cpus.c
> >> +++ b/cpus.c
> >> @@ -246,19 +246,17 @@ void cpu_update_icount(CPUState *cpu)
> >>
> >>  int64_t cpu_get_icount_raw(void)
> >>  {
> >> -    int64_t icount;
> >>      CPUState *cpu = current_cpu;
> >>
> >> -    icount = atomic_read(&timers_state.qemu_icount);
> >>      if (cpu && cpu->running) {
> >>          if (!cpu->can_do_io) {
> >>              fprintf(stderr, "Bad icount read\n");
> >>              exit(1);
> >>          }
> >>          /* Take into account what has run */
> >> -        icount += cpu_get_icount_executed(cpu);
> >> +        cpu_update_icount(cpu);
> >>      }
> >> -    return icount;
> >> +    return atomic_read(&timers_state.qemu_icount);
> >>  }
> >>
> >>  /* Return the virtual CPU time, based on the instruction counter.  */
> >>
> >
> > Maybe the update should be done in gen_io_start instead.  There
> > shouldn't be any interference between vCPU and I/O threads except
> > between gen_io_start and gen_io_end.
> 
> I'm not sure I follow. gen_io_start is a translation time thing. At
> least here we ensure we update whenever the value is read.

Sorry, I meant we should generate TCG opcodes for the translation in
gen_io_start.

Paolo

Re: [Qemu-devel] [PATCH v2 11/12] cpus: call cpu_update_icount on read

[Alex Bennée]

Paolo Bonzini <pbonzini@redhat.com> writes:

> ----- Original Message -----
>> From: "Alex Bennée" <alex.bennee@linaro.org>
>> To: "Paolo Bonzini" <pbonzini@redhat.com>
>> Cc: dovgaluk@ispras.ru, rth@twiddle.net, "peter maydell" <peter.maydell@linaro.org>, qemu-devel@nongnu.org,
>> mttcg@greensocs.com, "fred konrad" <fred.konrad@greensocs.com>, "a rigo" <a.rigo@virtualopensystems.com>,
>> cota@braap.org, "bobby prani" <bobby.prani@gmail.com>, nikunj@linux.vnet.ibm.com, "Peter Crosthwaite"
>> <crosthwaite.peter@gmail.com>
>> Sent: Friday, April 7, 2017 7:35:29 PM
>> Subject: Re: [PATCH v2 11/12] cpus: call cpu_update_icount on read
>>
>>
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>
>> > On 05/04/2017 15:25, Alex Bennée wrote:
>> >> This ensures each time the vCPU thread reads the icount we update the
>> >> master timer_state.qemu_icount field. This way as long as updates are
>> >> in BQL protected sections (which they should be) the main-loop can
>> >> never come to update the log and find time has gone backwards.
>> >>
>> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> >> ---
>> >>  cpus.c | 6 ++----
>> >>  1 file changed, 2 insertions(+), 4 deletions(-)
>> >>
>> >> diff --git a/cpus.c b/cpus.c
>> >> index ff75af449a..63de033cc8 100644
>> >> --- a/cpus.c
>> >> +++ b/cpus.c
>> >> @@ -246,19 +246,17 @@ void cpu_update_icount(CPUState *cpu)
>> >>
>> >>  int64_t cpu_get_icount_raw(void)
>> >>  {
>> >> -    int64_t icount;
>> >>      CPUState *cpu = current_cpu;
>> >>
>> >> -    icount = atomic_read(&timers_state.qemu_icount);
>> >>      if (cpu && cpu->running) {
>> >>          if (!cpu->can_do_io) {
>> >>              fprintf(stderr, "Bad icount read\n");
>> >>              exit(1);
>> >>          }
>> >>          /* Take into account what has run */
>> >> -        icount += cpu_get_icount_executed(cpu);
>> >> +        cpu_update_icount(cpu);
>> >>      }
>> >> -    return icount;
>> >> +    return atomic_read(&timers_state.qemu_icount);
>> >>  }
>> >>
>> >>  /* Return the virtual CPU time, based on the instruction counter.  */
>> >>
>> >
>> > Maybe the update should be done in gen_io_start instead.  There
>> > shouldn't be any interference between vCPU and I/O threads except
>> > between gen_io_start and gen_io_end.
>>
>> I'm not sure I follow. gen_io_start is a translation time thing. At
>> least here we ensure we update whenever the value is read.
>
> Sorry, I meant we should generate TCG opcodes for the translation in
> gen_io_start.

Ahh OK. I think this is 2.10 stuff though right?

>
> Paolo


--
Alex Bennée

Re: [Qemu-devel] [PATCH v2 11/12] cpus: call cpu_update_icount on read

[Richard Henderson]

On 04/07/2017 06:14 AM, Alex Bennée wrote:
>> Sorry, I meant we should generate TCG opcodes for the translation in
>> gen_io_start.
> Ahh OK. I think this is 2.10 stuff though right?
>

Definitely.


r~