Re: [PATCH 1/7] hw/ppc/spapr: Restrict PPCTimebase structure declaration to sPAPR

[Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>]

On 13/10/2023 13:56, Philippe Mathieu-Daudé wrote:

> The PPCTimebase structure is only used by the sPAPR machine.
> Move its declaration to "hw/ppc/spapr.h".
> Move vmstate_ppc_timebase and the VMSTATE_PPC_TIMEBASE_V()
> macro to hw/ppc/spapr.c, along with the timebase_foo()
> migration helpers.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/hw/ppc/spapr.h |   6 +++
>   target/ppc/cpu-qom.h   |  22 --------
>   hw/ppc/ppc.c           | 107 -------------------------------------
>   hw/ppc/spapr.c         | 116 +++++++++++++++++++++++++++++++++++++++++
>   4 files changed, 122 insertions(+), 129 deletions(-)
> 
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index e91791a1a9..3cf9978cba 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -163,6 +163,12 @@ struct SpaprMachineClass {
>       SpaprIrq *irq;
>   };
>   
> +typedef struct PPCTimebase {
> +    uint64_t guest_timebase;
> +    int64_t time_of_the_day_ns;
> +    bool runstate_paused;
> +} PPCTimebase;
> +
>   #define WDT_MAX_WATCHDOGS       4      /* Maximum number of watchdog devices */
>   
>   #define TYPE_SPAPR_WDT "spapr-wdt"
> diff --git a/target/ppc/cpu-qom.h b/target/ppc/cpu-qom.h
> index be33786bd8..b5deef5ca5 100644
> --- a/target/ppc/cpu-qom.h
> +++ b/target/ppc/cpu-qom.h
> @@ -197,26 +197,4 @@ struct PowerPCCPUClass {
>       int  (*check_pow)(CPUPPCState *env);
>   };
>   
> -#ifndef CONFIG_USER_ONLY
> -typedef struct PPCTimebase {
> -    uint64_t guest_timebase;
> -    int64_t time_of_the_day_ns;
> -    bool runstate_paused;
> -} PPCTimebase;
> -
> -extern const VMStateDescription vmstate_ppc_timebase;
> -
> -#define VMSTATE_PPC_TIMEBASE_V(_field, _state, _version) {            \
> -    .name       = (stringify(_field)),                                \
> -    .version_id = (_version),                                         \
> -    .size       = sizeof(PPCTimebase),                                \
> -    .vmsd       = &vmstate_ppc_timebase,                              \
> -    .flags      = VMS_STRUCT,                                         \
> -    .offset     = vmstate_offset_value(_state, _field, PPCTimebase),  \
> -}
> -
> -void cpu_ppc_clock_vm_state_change(void *opaque, bool running,
> -                                   RunState state);
> -#endif
> -
>   #endif
> diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> index be167710a3..340cd6192f 100644
> --- a/hw/ppc/ppc.c
> +++ b/hw/ppc/ppc.c
> @@ -32,7 +32,6 @@
>   #include "qemu/main-loop.h"
>   #include "qemu/error-report.h"
>   #include "sysemu/kvm.h"
> -#include "sysemu/replay.h"
>   #include "sysemu/runstate.h"
>   #include "kvm_ppc.h"
>   #include "migration/vmstate.h"
> @@ -967,112 +966,6 @@ void cpu_ppc_store_purr(CPUPPCState *env, uint64_t value)
>       _cpu_ppc_store_purr(env, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), value);
>   }
>   
> -static void timebase_save(PPCTimebase *tb)
> -{
> -    uint64_t ticks = cpu_get_host_ticks();
> -    PowerPCCPU *first_ppc_cpu = POWERPC_CPU(first_cpu);
> -
> -    if (!first_ppc_cpu->env.tb_env) {
> -        error_report("No timebase object");
> -        return;
> -    }
> -
> -    if (replay_mode == REPLAY_MODE_NONE) {
> -        /* not used anymore, we keep it for compatibility */
> -        tb->time_of_the_day_ns = qemu_clock_get_ns(QEMU_CLOCK_HOST);
> -    } else {
> -        /* simpler for record-replay to avoid this event, compat not needed */
> -        tb->time_of_the_day_ns = 0;
> -    }
> -
> -    /*
> -     * 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->runstate_paused =
> -        runstate_check(RUN_STATE_PAUSED) || runstate_check(RUN_STATE_SAVE_VM);
> -}
> -
> -static void timebase_load(PPCTimebase *tb)
> -{
> -    CPUState *cpu;
> -    PowerPCCPU *first_ppc_cpu = POWERPC_CPU(first_cpu);
> -    int64_t tb_off_adj, tb_off;
> -    unsigned long freq;
> -
> -    if (!first_ppc_cpu->env.tb_env) {
> -        error_report("No timebase object");
> -        return;
> -    }
> -
> -    freq = first_ppc_cpu->env.tb_env->tb_freq;
> -
> -    tb_off_adj = tb->guest_timebase - cpu_get_host_ticks();
> -
> -    tb_off = first_ppc_cpu->env.tb_env->tb_offset;
> -    trace_ppc_tb_adjust(tb_off, tb_off_adj, tb_off_adj - tb_off,
> -                        (tb_off_adj - tb_off) / freq);
> -
> -    /* Set new offset to all CPUs */
> -    CPU_FOREACH(cpu) {
> -        PowerPCCPU *pcpu = POWERPC_CPU(cpu);
> -        pcpu->env.tb_env->tb_offset = tb_off_adj;
> -        kvmppc_set_reg_tb_offset(pcpu, pcpu->env.tb_env->tb_offset);
> -    }
> -}
> -
> -void cpu_ppc_clock_vm_state_change(void *opaque, bool running,
> -                                   RunState state)
> -{
> -    PPCTimebase *tb = opaque;
> -
> -    if (running) {
> -        timebase_load(tb);
> -    } else {
> -        timebase_save(tb);
> -    }
> -}
> -
> -/*
> - * When migrating a running guest, read the clock just
> - * before migration, so that the guest clock counts
> - * during the events between:
> - *
> - *  * vm_stop()
> - *  *
> - *  * pre_save()
> - *
> - *  This reduces clock difference on migration from 5s
> - *  to 0.1s (when max_downtime == 5s), because sending the
> - *  final pages of memory (which happens between vm_stop()
> - *  and pre_save()) takes max_downtime.
> - */
> -static int timebase_pre_save(void *opaque)
> -{
> -    PPCTimebase *tb = opaque;
> -
> -    /* guest_timebase won't be overridden in case of paused guest or savevm */
> -    if (!tb->runstate_paused) {
> -        timebase_save(tb);
> -    }
> -
> -    return 0;
> -}
> -
> -const VMStateDescription vmstate_ppc_timebase = {
> -    .name = "timebase",
> -    .version_id = 1,
> -    .minimum_version_id = 1,
> -    .pre_save = timebase_pre_save,
> -    .fields      = (VMStateField []) {
> -        VMSTATE_UINT64(guest_timebase, PPCTimebase),
> -        VMSTATE_INT64(time_of_the_day_ns, PPCTimebase),
> -        VMSTATE_END_OF_LIST()
> -    },
> -};
> -
>   /* Set up (once) timebase frequency (in Hz) */
>   void cpu_ppc_tb_init(CPUPPCState *env, uint32_t freq)
>   {
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index cb840676d3..fe8b425ffd 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -37,6 +37,7 @@
>   #include "sysemu/numa.h"
>   #include "sysemu/qtest.h"
>   #include "sysemu/reset.h"
> +#include "sysemu/replay.h"
>   #include "sysemu/runstate.h"
>   #include "qemu/log.h"
>   #include "hw/fw-path-provider.h"
> @@ -1809,6 +1810,100 @@ static bool spapr_vga_init(PCIBus *pci_bus, Error **errp)
>       }
>   }
>   
> +static void timebase_save(PPCTimebase *tb)
> +{
> +    uint64_t ticks = cpu_get_host_ticks();
> +    PowerPCCPU *first_ppc_cpu = POWERPC_CPU(first_cpu);
> +
> +    if (!first_ppc_cpu->env.tb_env) {
> +        error_report("No timebase object");
> +        return;
> +    }
> +
> +    if (replay_mode == REPLAY_MODE_NONE) {
> +        /* not used anymore, we keep it for compatibility */
> +        tb->time_of_the_day_ns = qemu_clock_get_ns(QEMU_CLOCK_HOST);
> +    } else {
> +        /* simpler for record-replay to avoid this event, compat not needed */
> +        tb->time_of_the_day_ns = 0;
> +    }
> +
> +    /*
> +     * 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->runstate_paused =
> +        runstate_check(RUN_STATE_PAUSED) || runstate_check(RUN_STATE_SAVE_VM);
> +}
> +
> +static void timebase_load(PPCTimebase *tb)
> +{
> +    CPUState *cpu;
> +    PowerPCCPU *first_ppc_cpu = POWERPC_CPU(first_cpu);
> +    int64_t tb_off_adj, tb_off;
> +    unsigned long freq;
> +
> +    if (!first_ppc_cpu->env.tb_env) {
> +        error_report("No timebase object");
> +        return;
> +    }
> +
> +    freq = first_ppc_cpu->env.tb_env->tb_freq;
> +
> +    tb_off_adj = tb->guest_timebase - cpu_get_host_ticks();
> +
> +    tb_off = first_ppc_cpu->env.tb_env->tb_offset;
> +    trace_ppc_tb_adjust(tb_off, tb_off_adj, tb_off_adj - tb_off,
> +                        (tb_off_adj - tb_off) / freq);
> +
> +    /* Set new offset to all CPUs */
> +    CPU_FOREACH(cpu) {
> +        PowerPCCPU *pcpu = POWERPC_CPU(cpu);
> +        pcpu->env.tb_env->tb_offset = tb_off_adj;
> +        kvmppc_set_reg_tb_offset(pcpu, pcpu->env.tb_env->tb_offset);
> +    }
> +}
> +
> +static void cpu_ppc_clock_vm_state_change(void *opaque, bool running,
> +                                          RunState state)
> +{
> +    PPCTimebase *tb = opaque;
> +
> +    if (running) {
> +        timebase_load(tb);
> +    } else {
> +        timebase_save(tb);
> +    }
> +}
> +
> +/*
> + * When migrating a running guest, read the clock just
> + * before migration, so that the guest clock counts
> + * during the events between:
> + *
> + *  * vm_stop()
> + *  *
> + *  * pre_save()
> + *
> + *  This reduces clock difference on migration from 5s
> + *  to 0.1s (when max_downtime == 5s), because sending the
> + *  final pages of memory (which happens between vm_stop()
> + *  and pre_save()) takes max_downtime.
> + */
> +static int timebase_pre_save(void *opaque)
> +{
> +    PPCTimebase *tb = opaque;
> +
> +    /* guest_timebase won't be overridden in case of paused guest or savevm */
> +    if (!tb->runstate_paused) {
> +        timebase_save(tb);
> +    }
> +
> +    return 0;
> +}
> +
>   static int spapr_pre_load(void *opaque)
>   {
>       int rc;
> @@ -2081,6 +2176,27 @@ static const VMStateDescription vmstate_spapr_fwnmi = {
>       },
>   };
>   
> +static const VMStateDescription vmstate_spapr_timebase = {
> +    .name = "timebase",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .pre_save = timebase_pre_save,
> +    .fields      = (VMStateField []) {
> +        VMSTATE_UINT64(guest_timebase, PPCTimebase),
> +        VMSTATE_INT64(time_of_the_day_ns, PPCTimebase),
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
> +#define VMSTATE_PPC_TIMEBASE_V(_field, _state, _version) {            \
> +    .name       = (stringify(_field)),                                \
> +    .version_id = (_version),                                         \
> +    .size       = sizeof(PPCTimebase),                                \
> +    .vmsd       = &vmstate_spapr_timebase,                            \
> +    .flags      = VMS_STRUCT,                                         \
> +    .offset     = vmstate_offset_value(_state, _field, PPCTimebase),  \
> +}
> +
>   static const VMStateDescription vmstate_spapr = {
>       .name = "spapr",
>       .version_id = 3,

I saw this series when it was original posted, but I failed to spot that it didn't 
apply to the PPC Mac machines. I have a feeling this should solve a long-running 
issue I've been having with decrementer migration, in which case can it be moved (or 
left) somewhere where this is still possible?


ATB,

Mark.