When we run in TCG icount mode, we calculate the number of instructions
to execute using tcg_get_icount_limit(), which ensures that we stop
execution at the next timer deadline. However there is a bug where
currently we do not recalculate that limit if the guest reprograms
a timer so that the next deadline moves closer, and so we will
continue execution until the original limit and fire the timer
later than we should.

Fix this bug in qemu_timer_notify_cb(): if we are currently running
a VCPU in icount mode, we simply need to kick it out of the main
loop and back to tcg_cpu_exec(), where it will recalculate the
icount limit. If we are not currently running a VCPU, then we
retain the existing logic for waking up a halted CPU.

Cc: qemu-stable@nongnu.org
Fixes: https://bugs.launchpad.net/qemu/+bug/1754038
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Thanks to Paolo for tracking down which function needed fixing!

 cpus.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/cpus.c b/cpus.c
index 2e6701795b..38eba8bff3 100644
--- a/cpus.c
+++ b/cpus.c
@@ -892,11 +892,19 @@ void qemu_timer_notify_cb(void *opaque, QEMUClockType type)
         return;
     }
 
-    if (!qemu_in_vcpu_thread() && first_cpu) {
+    if (qemu_in_vcpu_thread()) {
+        /* A CPU is currently running; kick it back out to the
+         * tcg_cpu_exec() loop so it will recalculate its
+         * icount deadline immediately.
+         */
+        qemu_cpu_kick(current_cpu);
+    } else if (first_cpu) {
         /* qemu_cpu_kick is not enough to kick a halted CPU out of
          * qemu_tcg_wait_io_event.  async_run_on_cpu, instead,
          * causes cpu_thread_is_idle to return false.  This way,
          * handle_icount_deadline can run.
+         * If we have no CPUs at all for some reason, we don't
+         * need to do anything.
          */
         async_run_on_cpu(first_cpu, do_nothing, RUN_ON_CPU_NULL);
     }
-- 
2.16.2

On 04/06/2018 10:38 PM, Peter Maydell wrote:
> When we run in TCG icount mode, we calculate the number of instructions
> to execute using tcg_get_icount_limit(), which ensures that we stop
> execution at the next timer deadline. However there is a bug where
> currently we do not recalculate that limit if the guest reprograms
> a timer so that the next deadline moves closer, and so we will
> continue execution until the original limit and fire the timer
> later than we should.
> 
> Fix this bug in qemu_timer_notify_cb(): if we are currently running
> a VCPU in icount mode, we simply need to kick it out of the main
> loop and back to tcg_cpu_exec(), where it will recalculate the
> icount limit. If we are not currently running a VCPU, then we
> retain the existing logic for waking up a halted CPU.
> 
> Cc: qemu-stable@nongnu.org
> Fixes: https://bugs.launchpad.net/qemu/+bug/1754038
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Thanks to Paolo for tracking down which function needed fixing!

Seconded.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Peter Maydell <peter.maydell@linaro.org> writes:

> When we run in TCG icount mode, we calculate the number of instructions
> to execute using tcg_get_icount_limit(), which ensures that we stop
> execution at the next timer deadline. However there is a bug where
> currently we do not recalculate that limit if the guest reprograms
> a timer so that the next deadline moves closer, and so we will
> continue execution until the original limit and fire the timer
> later than we should.
>
> Fix this bug in qemu_timer_notify_cb(): if we are currently running
> a VCPU in icount mode, we simply need to kick it out of the main
> loop and back to tcg_cpu_exec(), where it will recalculate the
> icount limit. If we are not currently running a VCPU, then we
> retain the existing logic for waking up a halted CPU.
>
> Cc: qemu-stable@nongnu.org
> Fixes: https://bugs.launchpad.net/qemu/+bug/1754038
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Thanks to Paolo for tracking down which function needed fixing!
>
>  cpus.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/cpus.c b/cpus.c
> index 2e6701795b..38eba8bff3 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -892,11 +892,19 @@ void qemu_timer_notify_cb(void *opaque, QEMUClockType type)
>          return;
>      }
>
> -    if (!qemu_in_vcpu_thread() && first_cpu) {
> +    if (qemu_in_vcpu_thread()) {
> +        /* A CPU is currently running; kick it back out to the
> +         * tcg_cpu_exec() loop so it will recalculate its
> +         * icount deadline immediately.
> +         */
> +        qemu_cpu_kick(current_cpu);

This is only totally true if whatever has caused the re-calculation has
ended the Translation Block, otherwise we won't finish until we've run a
few more instructions. I don't know if this could cause a problem if the
timeout was suddenly less than the remaining instructions in the block.

Anyway wording aside:

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

> +    } else if (first_cpu) {
>          /* qemu_cpu_kick is not enough to kick a halted CPU out of
>           * qemu_tcg_wait_io_event.  async_run_on_cpu, instead,
>           * causes cpu_thread_is_idle to return false.  This way,
>           * handle_icount_deadline can run.
> +         * If we have no CPUs at all for some reason, we don't
> +         * need to do anything.
>           */
>          async_run_on_cpu(first_cpu, do_nothing, RUN_ON_CPU_NULL);
>      }


--
Alex Bennée

On 6 April 2018 at 13:58, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>> -    if (!qemu_in_vcpu_thread() && first_cpu) {
>> +    if (qemu_in_vcpu_thread()) {
>> +        /* A CPU is currently running; kick it back out to the
>> +         * tcg_cpu_exec() loop so it will recalculate its
>> +         * icount deadline immediately.
>> +         */
>> +        qemu_cpu_kick(current_cpu);
>
> This is only totally true if whatever has caused the re-calculation has
> ended the Translation Block, otherwise we won't finish until we've run a
> few more instructions.

That should always be true, because it will be an IO instruction,
which we enforce is always the last insn in a TB.

thanks
-- PMM