[PATCH v2 0/5] sysemu/replay: Restrict icount to TCG system emulation

[PATCH v2 0/5] sysemu/replay: Restrict icount to TCG system emulation

[Philippe Mathieu-Daudé]

Slightly simplify non-TCG and user emulation code.

Since v1:
- Introduce enum of icount modes
- Fix ARM INST_RETIRED event

Philippe Mathieu-Daudé (5):
  sysemu/cpu-timers: Introduce ICountMode enumerator
  target/arm: Ensure icount is enabled when emulating INST_RETIRED
  util/async: Only call icount_notify_exit() if icount is enabled
  system/vl: Restrict icount to TCG emulation
  sysemu/replay: Restrict icount to system emulation

 include/sysemu/cpu-timers.h | 22 ++++++++++++++--------
 include/sysemu/replay.h     | 11 ++++++++---
 accel/tcg/icount-common.c   | 16 +++++++---------
 stubs/icount.c              | 29 ++---------------------------
 system/cpu-timers.c         |  2 +-
 system/vl.c                 |  6 +++++-
 target/arm/helper.c         |  5 ++++-
 util/async.c                | 16 +++++++++-------
 8 files changed, 50 insertions(+), 57 deletions(-)

-- 
2.41.0

[PATCH v2 1/5] sysemu/cpu-timers: Introduce ICountMode enumerator

[Philippe Mathieu-Daudé]

Rather than having to lookup for what the 0, 1, 2, ...
icount values are, use a enum definition.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/sysemu/cpu-timers.h | 20 +++++++++++++-------
 accel/tcg/icount-common.c   | 16 +++++++---------
 stubs/icount.c              |  2 +-
 system/cpu-timers.c         |  2 +-
 target/arm/helper.c         |  3 ++-
 5 files changed, 24 insertions(+), 19 deletions(-)

diff --git a/include/sysemu/cpu-timers.h b/include/sysemu/cpu-timers.h
index 2e786fe7fb..e909ffae47 100644
--- a/include/sysemu/cpu-timers.h
+++ b/include/sysemu/cpu-timers.h
@@ -17,18 +17,24 @@ void cpu_timers_init(void);
 
 /* icount - Instruction Counter API */
 
-/*
- * icount enablement state:
+/**
+ * ICountMode: icount enablement state:
  *
- * 0 = Disabled - Do not count executed instructions.
- * 1 = Enabled - Fixed conversion of insn to ns via "shift" option
- * 2 = Enabled - Runtime adaptive algorithm to compute shift
+ * @ICOUNT_DISABLED: Disabled - Do not count executed instructions.
+ * @ICOUNT_PRECISE: Enabled - Fixed conversion of insn to ns via "shift" option
+ * @ICOUNT_ADAPTATIVE: Enabled - Runtime adaptive algorithm to compute shift
  */
+typedef enum {
+    ICOUNT_DISABLED = 0,
+    ICOUNT_PRECISE = 1,
+    ICOUNT_ADAPTATIVE = 2,
+} ICountMode;
+
 #ifdef CONFIG_TCG
-extern int use_icount;
+extern ICountMode use_icount;
 #define icount_enabled() (use_icount)
 #else
-#define icount_enabled() 0
+#define icount_enabled() ICOUNT_DISABLED
 #endif
 
 /*
diff --git a/accel/tcg/icount-common.c b/accel/tcg/icount-common.c
index ec57192be8..067c93a9ae 100644
--- a/accel/tcg/icount-common.c
+++ b/accel/tcg/icount-common.c
@@ -49,21 +49,19 @@ static bool icount_sleep = true;
 /* Arbitrarily pick 1MIPS as the minimum allowable speed.  */
 #define MAX_ICOUNT_SHIFT 10
 
-/*
- * 0 = Do not count executed instructions.
- * 1 = Fixed conversion of insn to ns via "shift" option
- * 2 = Runtime adaptive algorithm to compute shift
- */
-int use_icount;
+/* Do not count executed instructions */
+ICountMode use_icount = ICOUNT_DISABLED;
 
 static void icount_enable_precise(void)
 {
-    use_icount = 1;
+    /* Fixed conversion of insn to ns via "shift" option */
+    use_icount = ICOUNT_PRECISE;
 }
 
 static void icount_enable_adaptive(void)
 {
-    use_icount = 2;
+    /* Runtime adaptive algorithm to compute shift */
+    use_icount = ICOUNT_ADAPTATIVE;
 }
 
 /*
@@ -256,7 +254,7 @@ static void icount_warp_rt(void)
         int64_t warp_delta;
 
         warp_delta = clock - timers_state.vm_clock_warp_start;
-        if (icount_enabled() == 2) {
+        if (icount_enabled() == ICOUNT_ADAPTATIVE) {
             /*
              * In adaptive mode, do not let QEMU_CLOCK_VIRTUAL run too far
              * ahead of real time (it might already be ahead so careful not
diff --git a/stubs/icount.c b/stubs/icount.c
index 6df8c2bf7d..f8e6a014b8 100644
--- a/stubs/icount.c
+++ b/stubs/icount.c
@@ -4,7 +4,7 @@
 
 /* icount - Instruction Counter API */
 
-int use_icount;
+ICountMode use_icount = ICOUNT_DISABLED;
 
 void icount_update(CPUState *cpu)
 {
diff --git a/system/cpu-timers.c b/system/cpu-timers.c
index 7452d97b67..6befb82e48 100644
--- a/system/cpu-timers.c
+++ b/system/cpu-timers.c
@@ -154,7 +154,7 @@ static bool adjust_timers_state_needed(void *opaque)
 
 static bool icount_shift_state_needed(void *opaque)
 {
-    return icount_enabled() == 2;
+    return icount_enabled() == ICOUNT_ADAPTATIVE;
 }
 
 /*
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 2746d3fdac..adb0960bba 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -934,7 +934,8 @@ static int64_t cycles_ns_per(uint64_t cycles)
 
 static bool instructions_supported(CPUARMState *env)
 {
-    return icount_enabled() == 1; /* Precise instruction counting */
+    /* Precise instruction counting */
+    return icount_enabled() == ICOUNT_PRECISE;
 }
 
 static uint64_t instructions_get_count(CPUARMState *env)
-- 
2.41.0

[PATCH v2 2/5] target/arm: Ensure icount is enabled when emulating INST_RETIRED

[Philippe Mathieu-Daudé]

pmu_init() register its event checking the pm_event::supported()
handler. For INST_RETIRED, the event is only registered and the
bit enabled in the PMU Common Event Identification register when
icount is enabled as ICOUNT_PRECISE.

Assert the pm_event::get_count() and pm_event::ns_per_count()
handler will only be called under this icount mode.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/arm/helper.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index adb0960bba..333fd5f4bf 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -940,11 +940,13 @@ static bool instructions_supported(CPUARMState *env)
 
 static uint64_t instructions_get_count(CPUARMState *env)
 {
+    assert(icount_enabled() == ICOUNT_PRECISE);
     return (uint64_t)icount_get_raw();
 }
 
 static int64_t instructions_ns_per(uint64_t icount)
 {
+    assert(icount_enabled() == ICOUNT_PRECISE);
     return icount_to_ns((int64_t)icount);
 }
 #endif
-- 
2.41.0

[PATCH v2 3/5] util/async: Only call icount_notify_exit() if icount is enabled

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 stubs/icount.c |  2 +-
 util/async.c   | 16 +++++++++-------
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/stubs/icount.c b/stubs/icount.c
index f8e6a014b8..a5202e2dd9 100644
--- a/stubs/icount.c
+++ b/stubs/icount.c
@@ -43,7 +43,7 @@ void icount_account_warp_timer(void)
 {
     abort();
 }
-
 void icount_notify_exit(void)
 {
+    abort();
 }
diff --git a/util/async.c b/util/async.c
index 8f90ddc304..9007642c27 100644
--- a/util/async.c
+++ b/util/async.c
@@ -94,13 +94,15 @@ static void aio_bh_enqueue(QEMUBH *bh, unsigned new_flags)
     }
 
     aio_notify(ctx);
-    /*
-     * Workaround for record/replay.
-     * vCPU execution should be suspended when new BH is set.
-     * This is needed to avoid guest timeouts caused
-     * by the long cycles of the execution.
-     */
-    icount_notify_exit();
+    if (unlikely(icount_enabled())) {
+        /*
+         * Workaround for record/replay.
+         * vCPU execution should be suspended when new BH is set.
+         * This is needed to avoid guest timeouts caused
+         * by the long cycles of the execution.
+         */
+        icount_notify_exit();
+    }
 }
 
 /* Only called from aio_bh_poll() and aio_ctx_finalize() */
-- 
2.41.0

[PATCH v2 4/5] system/vl: Restrict icount to TCG emulation

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 stubs/icount.c | 6 ------
 system/vl.c    | 6 +++++-
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/stubs/icount.c b/stubs/icount.c
index a5202e2dd9..b060b03a73 100644
--- a/stubs/icount.c
+++ b/stubs/icount.c
@@ -1,5 +1,4 @@
 #include "qemu/osdep.h"
-#include "qapi/error.h"
 #include "sysemu/cpu-timers.h"
 
 /* icount - Instruction Counter API */
@@ -10,11 +9,6 @@ void icount_update(CPUState *cpu)
 {
     abort();
 }
-void icount_configure(QemuOpts *opts, Error **errp)
-{
-    /* signal error */
-    error_setg(errp, "cannot configure icount, TCG support not available");
-}
 int64_t icount_get_raw(void)
 {
     abort();
diff --git a/system/vl.c b/system/vl.c
index 2bcd9efb9a..8c99c5f681 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -2270,7 +2270,11 @@ static void user_register_global_props(void)
 
 static int do_configure_icount(void *opaque, QemuOpts *opts, Error **errp)
 {
-    icount_configure(opts, errp);
+    if (tcg_enabled()) {
+        icount_configure(opts, errp);
+    } else {
+        error_setg(errp, "cannot configure icount, TCG support not available");
+    }
     return 0;
 }
 
-- 
2.41.0

[PATCH v2 5/5] sysemu/replay: Restrict icount to system emulation

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/sysemu/cpu-timers.h |  2 +-
 include/sysemu/replay.h     | 11 ++++++++---
 stubs/icount.c              | 19 -------------------
 3 files changed, 9 insertions(+), 23 deletions(-)

diff --git a/include/sysemu/cpu-timers.h b/include/sysemu/cpu-timers.h
index e909ffae47..25dfc76599 100644
--- a/include/sysemu/cpu-timers.h
+++ b/include/sysemu/cpu-timers.h
@@ -30,7 +30,7 @@ typedef enum {
     ICOUNT_ADAPTATIVE = 2,
 } ICountMode;
 
-#ifdef CONFIG_TCG
+#if defined(CONFIG_TCG) && !defined(CONFIG_USER_ONLY)
 extern ICountMode use_icount;
 #define icount_enabled() (use_icount)
 #else
diff --git a/include/sysemu/replay.h b/include/sysemu/replay.h
index 08aae5869f..8102fa54f0 100644
--- a/include/sysemu/replay.h
+++ b/include/sysemu/replay.h
@@ -1,6 +1,3 @@
-#ifndef SYSEMU_REPLAY_H
-#define SYSEMU_REPLAY_H
-
 /*
  * QEMU replay (system interface)
  *
@@ -11,6 +8,12 @@
  * See the COPYING file in the top-level directory.
  *
  */
+#ifndef SYSEMU_REPLAY_H
+#define SYSEMU_REPLAY_H
+
+#ifdef CONFIG_USER_ONLY
+#error Cannot include this header from user emulation
+#endif
 
 #include "exec/replay-core.h"
 #include "qapi/qapi-types-misc.h"
@@ -79,12 +82,14 @@ int64_t replay_save_clock(ReplayClockKind kind, int64_t clock,
 int64_t replay_read_clock(ReplayClockKind kind, int64_t raw_icount);
 /*! Saves or reads the clock depending on the current replay mode. */
 #define REPLAY_CLOCK(clock, value)                                      \
+    !icount_enabled() ? (value) :                                       \
     (replay_mode == REPLAY_MODE_PLAY                                    \
         ? replay_read_clock((clock), icount_get_raw())                  \
         : replay_mode == REPLAY_MODE_RECORD                             \
             ? replay_save_clock((clock), (value), icount_get_raw())     \
             : (value))
 #define REPLAY_CLOCK_LOCKED(clock, value)                               \
+    !icount_enabled() ? (value) :                                       \
     (replay_mode == REPLAY_MODE_PLAY                                    \
         ? replay_read_clock((clock), icount_get_raw_locked())           \
         : replay_mode == REPLAY_MODE_RECORD                             \
diff --git a/stubs/icount.c b/stubs/icount.c
index b060b03a73..9a29084ecc 100644
--- a/stubs/icount.c
+++ b/stubs/icount.c
@@ -5,30 +5,11 @@
 
 ICountMode use_icount = ICOUNT_DISABLED;
 
-void icount_update(CPUState *cpu)
-{
-    abort();
-}
 int64_t icount_get_raw(void)
 {
     abort();
     return 0;
 }
-int64_t icount_get(void)
-{
-    abort();
-    return 0;
-}
-int64_t icount_to_ns(int64_t icount)
-{
-    abort();
-    return 0;
-}
-int64_t icount_round(int64_t count)
-{
-    abort();
-    return 0;
-}
 void icount_start_warp_timer(void)
 {
     abort();
-- 
2.41.0

Re: [PATCH v2 1/5] sysemu/cpu-timers: Introduce ICountMode enumerator

[Richard Henderson]

On 12/7/23 07:45, Philippe Mathieu-Daudé wrote:
> Rather than having to lookup for what the 0, 1, 2, ...
> icount values are, use a enum definition.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/sysemu/cpu-timers.h | 20 +++++++++++++-------
>   accel/tcg/icount-common.c   | 16 +++++++---------
>   stubs/icount.c              |  2 +-
>   system/cpu-timers.c         |  2 +-
>   target/arm/helper.c         |  3 ++-
>   5 files changed, 24 insertions(+), 19 deletions(-)
> 
> diff --git a/include/sysemu/cpu-timers.h b/include/sysemu/cpu-timers.h
> index 2e786fe7fb..e909ffae47 100644
> --- a/include/sysemu/cpu-timers.h
> +++ b/include/sysemu/cpu-timers.h
> @@ -17,18 +17,24 @@ void cpu_timers_init(void);
>   
>   /* icount - Instruction Counter API */
>   
> -/*
> - * icount enablement state:
> +/**
> + * ICountMode: icount enablement state:
>    *
> - * 0 = Disabled - Do not count executed instructions.
> - * 1 = Enabled - Fixed conversion of insn to ns via "shift" option
> - * 2 = Enabled - Runtime adaptive algorithm to compute shift
> + * @ICOUNT_DISABLED: Disabled - Do not count executed instructions.
> + * @ICOUNT_PRECISE: Enabled - Fixed conversion of insn to ns via "shift" option
> + * @ICOUNT_ADAPTATIVE: Enabled - Runtime adaptive algorithm to compute shift
>    */
> +typedef enum {
> +    ICOUNT_DISABLED = 0,
> +    ICOUNT_PRECISE = 1,
> +    ICOUNT_ADAPTATIVE = 2,

No need for the assignments.  Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH v2 2/5] target/arm: Ensure icount is enabled when emulating INST_RETIRED

[Richard Henderson]

On 12/7/23 07:45, Philippe Mathieu-Daudé wrote:
> pmu_init() register its event checking the pm_event::supported()
> handler. For INST_RETIRED, the event is only registered and the
> bit enabled in the PMU Common Event Identification register when
> icount is enabled as ICOUNT_PRECISE.
> 
> Assert the pm_event::get_count() and pm_event::ns_per_count()
> handler will only be called under this icount mode.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/arm/helper.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index adb0960bba..333fd5f4bf 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -940,11 +940,13 @@ static bool instructions_supported(CPUARMState *env)
>   
>   static uint64_t instructions_get_count(CPUARMState *env)
>   {
> +    assert(icount_enabled() == ICOUNT_PRECISE);
>       return (uint64_t)icount_get_raw();
>   }
>   
>   static int64_t instructions_ns_per(uint64_t icount)
>   {
> +    assert(icount_enabled() == ICOUNT_PRECISE);
>       return icount_to_ns((int64_t)icount);
>   }
>   #endif

I don't think an assert is required -- that's exactly what the .supported field is for. 
If you think this needs additional clarification, a comment is sufficient.


r~

Re: [PATCH v2 3/5] util/async: Only call icount_notify_exit() if icount is enabled

[Richard Henderson]

On 12/7/23 07:45, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   stubs/icount.c |  2 +-
>   util/async.c   | 16 +++++++++-------
>   2 files changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/stubs/icount.c b/stubs/icount.c
> index f8e6a014b8..a5202e2dd9 100644
> --- a/stubs/icount.c
> +++ b/stubs/icount.c
> @@ -43,7 +43,7 @@ void icount_account_warp_timer(void)
>   {
>       abort();
>   }
> -
>   void icount_notify_exit(void)
>   {
> +    abort();
>   }
> diff --git a/util/async.c b/util/async.c
> index 8f90ddc304..9007642c27 100644
> --- a/util/async.c
> +++ b/util/async.c
> @@ -94,13 +94,15 @@ static void aio_bh_enqueue(QEMUBH *bh, unsigned new_flags)
>       }
>   
>       aio_notify(ctx);
> -    /*
> -     * Workaround for record/replay.
> -     * vCPU execution should be suspended when new BH is set.
> -     * This is needed to avoid guest timeouts caused
> -     * by the long cycles of the execution.
> -     */
> -    icount_notify_exit();
> +    if (unlikely(icount_enabled())) {
> +        /*
> +         * Workaround for record/replay.
> +         * vCPU execution should be suspended when new BH is set.
> +         * This is needed to avoid guest timeouts caused
> +         * by the long cycles of the execution.
> +         */
> +        icount_notify_exit();
> +    }

If you're going to do this, remove the test in the non-stub icount_notify_exit.


r~

Re: [PATCH v2 4/5] system/vl: Restrict icount to TCG emulation

[Richard Henderson]

On 12/7/23 07:45, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   stubs/icount.c | 6 ------
>   system/vl.c    | 6 +++++-
>   2 files changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/stubs/icount.c b/stubs/icount.c
> index a5202e2dd9..b060b03a73 100644
> --- a/stubs/icount.c
> +++ b/stubs/icount.c
> @@ -1,5 +1,4 @@
>   #include "qemu/osdep.h"
> -#include "qapi/error.h"
>   #include "sysemu/cpu-timers.h"
>   
>   /* icount - Instruction Counter API */
> @@ -10,11 +9,6 @@ void icount_update(CPUState *cpu)
>   {
>       abort();
>   }
> -void icount_configure(QemuOpts *opts, Error **errp)
> -{
> -    /* signal error */
> -    error_setg(errp, "cannot configure icount, TCG support not available");
> -}
>   int64_t icount_get_raw(void)
>   {
>       abort();
> diff --git a/system/vl.c b/system/vl.c
> index 2bcd9efb9a..8c99c5f681 100644
> --- a/system/vl.c
> +++ b/system/vl.c
> @@ -2270,7 +2270,11 @@ static void user_register_global_props(void)
>   
>   static int do_configure_icount(void *opaque, QemuOpts *opts, Error **errp)
>   {
> -    icount_configure(opts, errp);
> +    if (tcg_enabled()) {
> +        icount_configure(opts, errp);
> +    } else {
> +        error_setg(errp, "cannot configure icount, TCG support not available");
> +    }
>       return 0;
>   }

This is called before the accelerator is chosen -- even before the set of available 
accelerators is even found.  Indeed, that's the very next thing that 
configure_accelerators does.

OTOH, I don't see why icount_configure is being called so early.


r~

>   

Re: [PATCH v2 2/5] target/arm: Ensure icount is enabled when emulating INST_RETIRED

[Philippe Mathieu-Daudé]

On 7/12/23 23:12, Richard Henderson wrote:
> On 12/7/23 07:45, Philippe Mathieu-Daudé wrote:
>> pmu_init() register its event checking the pm_event::supported()
>> handler. For INST_RETIRED, the event is only registered and the
>> bit enabled in the PMU Common Event Identification register when
>> icount is enabled as ICOUNT_PRECISE.
>>
>> Assert the pm_event::get_count() and pm_event::ns_per_count()
>> handler will only be called under this icount mode.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   target/arm/helper.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/target/arm/helper.c b/target/arm/helper.c
>> index adb0960bba..333fd5f4bf 100644
>> --- a/target/arm/helper.c
>> +++ b/target/arm/helper.c
>> @@ -940,11 +940,13 @@ static bool instructions_supported(CPUARMState 
>> *env)
>>   static uint64_t instructions_get_count(CPUARMState *env)
>>   {
>> +    assert(icount_enabled() == ICOUNT_PRECISE);
>>       return (uint64_t)icount_get_raw();
>>   }
>>   static int64_t instructions_ns_per(uint64_t icount)
>>   {
>> +    assert(icount_enabled() == ICOUNT_PRECISE);
>>       return icount_to_ns((int64_t)icount);
>>   }
>>   #endif
> 
> I don't think an assert is required -- that's exactly what the 
> .supported field is for. If you think this needs additional 
> clarification, a comment is sufficient.

Without this I'm getting this link failure with TCG disabled:

ld: Undefined symbols:
   _icount_to_ns, referenced from:
       _instructions_ns_per in target_arm_helper.c.o
clang: error: linker command failed with exit code 1 (use -v to see 
invocation)

Re: [PATCH v2 2/5] target/arm: Ensure icount is enabled when emulating INST_RETIRED

[Peter Maydell]

On Fri, 8 Dec 2023 at 10:36, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> On 7/12/23 23:12, Richard Henderson wrote:
> > On 12/7/23 07:45, Philippe Mathieu-Daudé wrote:
> >> pmu_init() register its event checking the pm_event::supported()
> >> handler. For INST_RETIRED, the event is only registered and the
> >> bit enabled in the PMU Common Event Identification register when
> >> icount is enabled as ICOUNT_PRECISE.
> >>
> >> Assert the pm_event::get_count() and pm_event::ns_per_count()
> >> handler will only be called under this icount mode.
> >>
> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> >> ---
> >>   target/arm/helper.c | 2 ++
> >>   1 file changed, 2 insertions(+)
> >>
> >> diff --git a/target/arm/helper.c b/target/arm/helper.c
> >> index adb0960bba..333fd5f4bf 100644
> >> --- a/target/arm/helper.c
> >> +++ b/target/arm/helper.c
> >> @@ -940,11 +940,13 @@ static bool instructions_supported(CPUARMState
> >> *env)
> >>   static uint64_t instructions_get_count(CPUARMState *env)
> >>   {
> >> +    assert(icount_enabled() == ICOUNT_PRECISE);
> >>       return (uint64_t)icount_get_raw();
> >>   }
> >>   static int64_t instructions_ns_per(uint64_t icount)
> >>   {
> >> +    assert(icount_enabled() == ICOUNT_PRECISE);
> >>       return icount_to_ns((int64_t)icount);
> >>   }
> >>   #endif
> >
> > I don't think an assert is required -- that's exactly what the
> > .supported field is for. If you think this needs additional
> > clarification, a comment is sufficient.
>
> Without this I'm getting this link failure with TCG disabled:
>
> ld: Undefined symbols:
>    _icount_to_ns, referenced from:
>        _instructions_ns_per in target_arm_helper.c.o
> clang: error: linker command failed with exit code 1 (use -v to see
> invocation)

I think we should fix this earlier by not trying to enable
these TCG-only PMU event types in a non-TCG config.

-- PMM

Re: [PATCH v2 4/5] system/vl: Restrict icount to TCG emulation

[Philippe Mathieu-Daudé]

On 7/12/23 23:38, Richard Henderson wrote:
> On 12/7/23 07:45, Philippe Mathieu-Daudé wrote:
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   stubs/icount.c | 6 ------
>>   system/vl.c    | 6 +++++-
>>   2 files changed, 5 insertions(+), 7 deletions(-)
>>
>> diff --git a/stubs/icount.c b/stubs/icount.c
>> index a5202e2dd9..b060b03a73 100644
>> --- a/stubs/icount.c
>> +++ b/stubs/icount.c
>> @@ -1,5 +1,4 @@
>>   #include "qemu/osdep.h"
>> -#include "qapi/error.h"
>>   #include "sysemu/cpu-timers.h"
>>   /* icount - Instruction Counter API */
>> @@ -10,11 +9,6 @@ void icount_update(CPUState *cpu)
>>   {
>>       abort();
>>   }
>> -void icount_configure(QemuOpts *opts, Error **errp)
>> -{
>> -    /* signal error */
>> -    error_setg(errp, "cannot configure icount, TCG support not 
>> available");
>> -}
>>   int64_t icount_get_raw(void)
>>   {
>>       abort();
>> diff --git a/system/vl.c b/system/vl.c
>> index 2bcd9efb9a..8c99c5f681 100644
>> --- a/system/vl.c
>> +++ b/system/vl.c
>> @@ -2270,7 +2270,11 @@ static void user_register_global_props(void)
>>   static int do_configure_icount(void *opaque, QemuOpts *opts, Error 
>> **errp)
>>   {
>> -    icount_configure(opts, errp);
>> +    if (tcg_enabled()) {
>> +        icount_configure(opts, errp);
>> +    } else {
>> +        error_setg(errp, "cannot configure icount, TCG support not 
>> available");
>> +    }
>>       return 0;
>>   }
> 
> This is called before the accelerator is chosen -- even before the set 
> of available accelerators is even found.  Indeed, that's the very next 
> thing that configure_accelerators does.
> 
> OTOH, I don't see why icount_configure is being called so early.

See commit 7f8b6126e7:

     vl: move icount configuration earlier

     Once qemu_tcg_configure is turned into a QOM property setter,
     it will not be able to set a default value for mttcg_enabled.
     Setting the default will move to the TCG instance_init function,
     which currently runs before "-icount" is processed.

     However, it is harmless to do configure_icount for all accelerators;
     we will just fail later if a non-TCG accelerator is selected.  So do
     that.

But few commits after we have 28a0961757 ("vl: merge -accel processing
into configure_accelerators"), so it shouldn't be an issue now. I'll
consolidate.

Re: [PATCH v2 2/5] target/arm: Ensure icount is enabled when emulating INST_RETIRED

[Philippe Mathieu-Daudé]

Hi Peter,

On 8/12/23 11:59, Peter Maydell wrote:
> On Fri, 8 Dec 2023 at 10:36, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> On 7/12/23 23:12, Richard Henderson wrote:
>>> On 12/7/23 07:45, Philippe Mathieu-Daudé wrote:
>>>> pmu_init() register its event checking the pm_event::supported()
>>>> handler. For INST_RETIRED, the event is only registered and the
>>>> bit enabled in the PMU Common Event Identification register when
>>>> icount is enabled as ICOUNT_PRECISE.
>>>>
>>>> Assert the pm_event::get_count() and pm_event::ns_per_count()
>>>> handler will only be called under this icount mode.
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>> ---
>>>>    target/arm/helper.c | 2 ++
>>>>    1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/target/arm/helper.c b/target/arm/helper.c
>>>> index adb0960bba..333fd5f4bf 100644
>>>> --- a/target/arm/helper.c
>>>> +++ b/target/arm/helper.c
>>>> @@ -940,11 +940,13 @@ static bool instructions_supported(CPUARMState
>>>> *env)
>>>>    static uint64_t instructions_get_count(CPUARMState *env)
>>>>    {
>>>> +    assert(icount_enabled() == ICOUNT_PRECISE);
>>>>        return (uint64_t)icount_get_raw();
>>>>    }
>>>>    static int64_t instructions_ns_per(uint64_t icount)
>>>>    {
>>>> +    assert(icount_enabled() == ICOUNT_PRECISE);
>>>>        return icount_to_ns((int64_t)icount);
>>>>    }
>>>>    #endif
>>>
>>> I don't think an assert is required -- that's exactly what the
>>> .supported field is for. If you think this needs additional
>>> clarification, a comment is sufficient.
>>
>> Without this I'm getting this link failure with TCG disabled:
>>
>> ld: Undefined symbols:
>>     _icount_to_ns, referenced from:
>>         _instructions_ns_per in target_arm_helper.c.o
>> clang: error: linker command failed with exit code 1 (use -v to see
>> invocation)
> 
> I think we should fix this earlier by not trying to enable
> these TCG-only PMU event types in a non-TCG config.

I agree... but (as discussed yesterday on IRC), this is a bigger rework.

This icount cleanup blocks 60+ patches on top :/ Since I have a v3
addressing Richard's comments already done, I'll post it, mentioning the
PMU issue in the cover; then see if cleaning it isn't too invasive.
If I end in another rabbit hole, I'll suggest to accept this current
patch and clean the technical debt later, again.

Thanks,

Phil.

Re: [PATCH v2 2/5] target/arm: Ensure icount is enabled when emulating INST_RETIRED

[Philippe Mathieu-Daudé]

On 8/12/23 12:23, Philippe Mathieu-Daudé wrote:
> Hi Peter,
> 
> On 8/12/23 11:59, Peter Maydell wrote:
>> On Fri, 8 Dec 2023 at 10:36, Philippe Mathieu-Daudé 
>> <philmd@linaro.org> wrote:
>>>
>>> On 7/12/23 23:12, Richard Henderson wrote:
>>>> On 12/7/23 07:45, Philippe Mathieu-Daudé wrote:
>>>>> pmu_init() register its event checking the pm_event::supported()
>>>>> handler. For INST_RETIRED, the event is only registered and the
>>>>> bit enabled in the PMU Common Event Identification register when
>>>>> icount is enabled as ICOUNT_PRECISE.
>>>>>
>>>>> Assert the pm_event::get_count() and pm_event::ns_per_count()
>>>>> handler will only be called under this icount mode.
>>>>>
>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>> ---
>>>>>    target/arm/helper.c | 2 ++
>>>>>    1 file changed, 2 insertions(+)
>>>>>
>>>>> diff --git a/target/arm/helper.c b/target/arm/helper.c
>>>>> index adb0960bba..333fd5f4bf 100644
>>>>> --- a/target/arm/helper.c
>>>>> +++ b/target/arm/helper.c
>>>>> @@ -940,11 +940,13 @@ static bool instructions_supported(CPUARMState
>>>>> *env)
>>>>>    static uint64_t instructions_get_count(CPUARMState *env)
>>>>>    {
>>>>> +    assert(icount_enabled() == ICOUNT_PRECISE);
>>>>>        return (uint64_t)icount_get_raw();
>>>>>    }
>>>>>    static int64_t instructions_ns_per(uint64_t icount)
>>>>>    {
>>>>> +    assert(icount_enabled() == ICOUNT_PRECISE);
>>>>>        return icount_to_ns((int64_t)icount);
>>>>>    }
>>>>>    #endif
>>>>
>>>> I don't think an assert is required -- that's exactly what the
>>>> .supported field is for. If you think this needs additional
>>>> clarification, a comment is sufficient.
>>>
>>> Without this I'm getting this link failure with TCG disabled:
>>>
>>> ld: Undefined symbols:
>>>     _icount_to_ns, referenced from:
>>>         _instructions_ns_per in target_arm_helper.c.o
>>> clang: error: linker command failed with exit code 1 (use -v to see
>>> invocation)
>>
>> I think we should fix this earlier by not trying to enable
>> these TCG-only PMU event types in a non-TCG config.
> 
> I agree... but (as discussed yesterday on IRC), this is a bigger rework.

Giving it a try, I figured HVF emulates PMC (cycle counter) within
some vPMU, containing "a single event source: the cycle counter"
(per Alex).
Some helpers are duplicated, such pmu_update_irq().

pmu_counter_enabled() diff (-KVM +HVF):

  /*
   * Returns true if the counter (pass 31 for PMCCNTR) should count 
events using
   * the current EL, security state, and register configuration.
   */
  static bool pmu_counter_enabled(CPUARMState *env, uint8_t counter)
  {
      uint64_t filter;
-    bool e, p, u, nsk, nsu, nsh, m;
-    bool enabled, prohibited = false, filtered;
-    bool secure = arm_is_secure(env);
+    bool enabled, filtered = true;
      int el = arm_current_el(env);
-    uint64_t mdcr_el2 = arm_mdcr_el2_eff(env);
-    uint8_t hpmn = mdcr_el2 & MDCR_HPMN;

-    if (!arm_feature(env, ARM_FEATURE_PMU)) {
-        return false;
-    }
-
-    if (!arm_feature(env, ARM_FEATURE_EL2) ||
-            (counter < hpmn || counter == 31)) {
-        e = env->cp15.c9_pmcr & PMCRE;
-    } else {
-        e = mdcr_el2 & MDCR_HPME;
-    }
-    enabled = e && (env->cp15.c9_pmcnten & (1 << counter));
-
-    /* Is event counting prohibited? */
-    if (el == 2 && (counter < hpmn || counter == 31)) {
-        prohibited = mdcr_el2 & MDCR_HPMD;
-    }
-    if (secure) {
-        prohibited = prohibited || !(env->cp15.mdcr_el3 & MDCR_SPME);
-    }
-
-    if (counter == 31) {
-        /*
-         * The cycle counter defaults to running. PMCR.DP says "disable
-         * the cycle counter when event counting is prohibited".
-         * Some MDCR bits disable the cycle counter specifically.
-         */
-        prohibited = prohibited && env->cp15.c9_pmcr & PMCRDP;
-        if (cpu_isar_feature(any_pmuv3p5, env_archcpu(env))) {
-            if (secure) {
-                prohibited = prohibited || (env->cp15.mdcr_el3 & 
MDCR_SCCD);
-            }
-            if (el == 2) {
-                prohibited = prohibited || (mdcr_el2 & MDCR_HCCD);
-            }
-        }
-    }
+    enabled = (env->cp15.c9_pmcr & PMCRE) &&
+              (env->cp15.c9_pmcnten & (1 << counter));

      if (counter == 31) {
          filter = env->cp15.pmccfiltr_el0;
      } else {
          filter = env->cp15.c14_pmevtyper[counter];
      }

-    p   = filter & PMXEVTYPER_P;
-    u   = filter & PMXEVTYPER_U;
-    nsk = arm_feature(env, ARM_FEATURE_EL3) && (filter & PMXEVTYPER_NSK);
-    nsu = arm_feature(env, ARM_FEATURE_EL3) && (filter & PMXEVTYPER_NSU);
-    nsh = arm_feature(env, ARM_FEATURE_EL2) && (filter & PMXEVTYPER_NSH);
-    m   = arm_el_is_aa64(env, 1) &&
-              arm_feature(env, ARM_FEATURE_EL3) && (filter & PMXEVTYPER_M);
-
      if (el == 0) {
-        filtered = secure ? u : u != nsu;
+        filtered = filter & PMXEVTYPER_U;
      } else if (el == 1) {
-        filtered = secure ? p : p != nsk;
-    } else if (el == 2) {
-        filtered = !nsh;
-    } else { /* EL3 */
-        filtered = m != p;
+        filtered = filter & PMXEVTYPER_P;
      }

      if (counter != 31) {
          /*
           * If not checking PMCCNTR, ensure the counter is setup to an 
event we
           * support
           */
          uint16_t event = filter & PMXEVTYPER_EVTCOUNT;
-        if (!event_supported(event)) {
+        if (!pmu_event_supported(event)) {
              return false;
          }
      }

-    return enabled && !prohibited && !filtered;
+    return enabled && !filtered;
  }

I could link HVF without PMU a few surgery such:
---
@@ -1493,12 +1486,12 @@ static int hvf_sysreg_write(CPUState *cpu, 
uint32_t reg, uint64_t val)

      switch (reg) {
      case SYSREG_PMCCNTR_EL0:
-        pmu_op_start(env);
+        pmccntr_op_start(env);
          env->cp15.c15_ccnt = val;
-        pmu_op_finish(env);
+        pmccntr_op_finish(env);
          break;
      case SYSREG_PMCR_EL0:
-        pmu_op_start(env);
+        pmccntr_op_start(env);

---

I'll try to split as:

- target/arm/pmu_common_helper.c (?)
- target/arm/pmc_helper.c
- target/arm/tcg/pmu_helper.c

Ideally pmu_counter_enabled() should be unified, but I
don't feel confident enough to do it.

Regards,

Phil.