Re: [RFC PATCH 3/4] hw/ppc: Take nested guest into account when saving timebase

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2594 bytes --]

On Thu, Feb 24, 2022 at 03:58:16PM -0300, Fabiano Rosas wrote:
> When saving the guest "timebase" we look to the first_cpu for its
> tb_offset. If that CPU happens to be running a nested guest at this
> time, the tb_offset will have the nested guest value.
> 
> This was caught by code inspection.

This doesn't seem right.  Isn't the real problem that nested_tb_offset
isn't being migrated?  If you migrate that, shouldn't everything be
fixed up when the L1 cpu leaves the nested guest on the destination
host?

> 
> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
> ---
>  hw/ppc/ppc.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> index 9e99625ea9..093cd87014 100644
> --- a/hw/ppc/ppc.c
> +++ b/hw/ppc/ppc.c
> @@ -36,6 +36,7 @@
>  #include "kvm_ppc.h"
>  #include "migration/vmstate.h"
>  #include "trace.h"
> +#include "hw/ppc/spapr_cpu_core.h"
>  
>  static void cpu_ppc_tb_stop (CPUPPCState *env);
>  static void cpu_ppc_tb_start (CPUPPCState *env);
> @@ -961,19 +962,33 @@ static void timebase_save(PPCTimebase *tb)
>  {
>      uint64_t ticks = cpu_get_host_ticks();
>      PowerPCCPU *first_ppc_cpu = POWERPC_CPU(first_cpu);
> +    int64_t tb_offset;
>  
>      if (!first_ppc_cpu->env.tb_env) {
>          error_report("No timebase object");
>          return;
>      }
>  
> +    tb_offset = first_ppc_cpu->env.tb_env->tb_offset;
> +
> +    if (first_ppc_cpu->vhyp && vhyp_cpu_in_nested(first_ppc_cpu)) {
> +        SpaprCpuState *spapr_cpu = spapr_cpu_state(first_ppc_cpu);
> +
> +        /*
> +         * If the first_cpu happens to be running a nested guest at
> +         * this time, tb_env->tb_offset will contain the nested guest
> +         * offset.
> +         */
> +        tb_offset -= spapr_cpu->nested_tb_offset;
> +    }
> +
>      /* not used anymore, we keep it for compatibility */
>      tb->time_of_the_day_ns = qemu_clock_get_ns(QEMU_CLOCK_HOST);
>      /*
>       * tb_offset is only expected to be changed by QEMU so
>       * there is no need to update it from KVM here
>       */
> -    tb->guest_timebase = ticks + first_ppc_cpu->env.tb_env->tb_offset;
> +    tb->guest_timebase = ticks + tb_offset;
>  
>      tb->runstate_paused =
>          runstate_check(RUN_STATE_PAUSED) || runstate_check(RUN_STATE_SAVE_VM);

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]