Re: [PATCH 4/4] target/ppc: Fix env->quiesced migration

[Fabiano Rosas <farosas@suse.de>]

Thomas Huth <thuth@redhat.com> writes:

> On 20/08/2025 00.39, Fabiano Rosas wrote:
>> The commit referenced (from QEMU 10.0) has changed the way the pseries
>> machine marks a cpu as quiesced. Previously, the cpu->halted value
>> from QEMU common cpu code was (incorrectly) used. With the fix, the
>> env->quiesced variable starts being used, which improves on the
>> original situation, but also causes a side effect after migration:
>> 
>> The env->quiesced is set at reset and never migrated, which causes the
>> destination QEMU to stop delivering interrupts and hang the machine.
>> 
>> To fix the issue from this point on, start migrating the env->quiesced
>> value.
>> 
>> For QEMU versions < 10.0, sending the new element on the stream would
>> cause migration to be aborted, so add the appropriate compatibility
>> property to omit the new subsection.
>> 
>> Independently of this patch, all migrations from QEMU versions < 10.0
>> will result in a hang since the older QEMU never migrates
>> env->quiesced. This is bad because it leaves machines already running
>> on the old QEMU without a migration path into newer versions.
>> 
>> As a workaround, clear env->quiesced in the new QEMU whenever
>> cpu->halted is also clear. This assumes rtas_stop_self() always sets
>> both flags at the same time. Migrations during secondaries bringup
>> (i.e. before rtas-start-cpu) will still cause a hang, but those are
>> early enough that requiring reboot would not be unreasonable.
>> 
>> Note that this was tested with -cpu power9 and -machine ic-mode=xive
>> due to another bug affecting migration of XICS guests. Tested both
>> forward and backward migration and savevm/loadvm from 9.2 and 10.0.
>> 
>> Reported-by: Fabian Vogt <fvogt@suse.de>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/3079
>> Fixes: fb802acdc8b ("ppc/spapr: Fix RTAS stopped state")
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>> The choice of PowerPCCPU to hold the compat property is dubious. This
>> only affects pseries, but it seems like a layering violation to access
>> SpaprMachine from target/ppc/, suggestions welcome.
>> ---
>>   hw/core/machine.c     |  1 +
>>   target/ppc/cpu.h      |  1 +
>>   target/ppc/cpu_init.c |  7 +++++++
>>   target/ppc/machine.c  | 40 ++++++++++++++++++++++++++++++++++++++++
>>   4 files changed, 49 insertions(+)
>> 
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index bd47527479..ea83c0876b 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -42,6 +42,7 @@ GlobalProperty hw_compat_10_0[] = {
>>       { "vfio-pci", "x-migration-load-config-after-iter", "off" },
>>       { "ramfb", "use-legacy-x86-rom", "true"},
>>       { "vfio-pci-nohotplug", "use-legacy-x86-rom", "true" },
>> +    { "powerpc64-cpu", "rtas-stopped-state", "false" },
>
> This is specific to ppc, so it should not go into the generic hw_compat_* array.
>

So arm-cpu in hw_compat_9_2 should not be there? 

> Please define a spapr_compat_10_0 array in 
> spapr_machine_10_0_class_options() and do another compat_props_add() for 
> that array there. (Similar to what is done for TYPE_SPAPR_PCI_HOST_BRIDGE in
> spapr_machine_5_0_class_options() for example)
>

Ok, thanks for the pointer.

>   Thanks,
>    Thomas
>
>
>>   };
>>   const size_t hw_compat_10_0_len = G_N_ELEMENTS(hw_compat_10_0);
>>   
>> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
>> index 6b90543811..8ff453024b 100644
>> --- a/target/ppc/cpu.h
>> +++ b/target/ppc/cpu.h
>> @@ -1470,6 +1470,7 @@ struct ArchCPU {
>>       void *machine_data;
>>       int32_t node_id; /* NUMA node this CPU belongs to */
>>       PPCHash64Options *hash64_opts;
>> +    bool rtas_stopped_state;
>>   
>>       /* Those resources are used only during code translation */
>>       /* opcode handlers */
>> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
>> index a0e77f2673..4380c6eb14 100644
>> --- a/target/ppc/cpu_init.c
>> +++ b/target/ppc/cpu_init.c
>> @@ -55,6 +55,11 @@
>>   /* #define PPC_DEBUG_SPR */
>>   /* #define USE_APPLE_GDB */
>>   
>> +static const Property powerpc_cpu_properties[] = {
>> +    DEFINE_PROP_BOOL("rtas-stopped-state", PowerPCCPU,
>> +                      rtas_stopped_state, true),
>> +};
>> +
>>   static inline void vscr_init(CPUPPCState *env, uint32_t val)
>>   {
>>       /* Altivec always uses round-to-nearest */
>> @@ -7525,6 +7530,8 @@ static void ppc_cpu_class_init(ObjectClass *oc, const void *data)
>>                                         &pcc->parent_unrealize);
>>       pcc->pvr_match = ppc_pvr_match_default;
>>   
>> +    device_class_set_props(dc, powerpc_cpu_properties);
>> +
>>       resettable_class_set_parent_phases(rc, NULL, ppc_cpu_reset_hold, NULL,
>>                                          &pcc->parent_phases);
>>   
>> diff --git a/target/ppc/machine.c b/target/ppc/machine.c
>> index d72e5ecb94..8797233ebe 100644
>> --- a/target/ppc/machine.c
>> +++ b/target/ppc/machine.c
>> @@ -257,6 +257,23 @@ static int cpu_post_load(void *opaque, int version_id)
>>           ppc_store_sdr1(env, env->spr[SPR_SDR1]);
>>       }
>>   
>> +    if (!cpu->rtas_stopped_state) {
>> +        /*
>> +         * The source QEMU doesn't have fb802acdc8 and still uses halt
>> +         * + PM bits in LPCR to implement RTAS stopped state. The new
>> +         * QEMU will have put the newly created vcpus in that state,
>> +         * waiting for the start-cpu RTAS call. Clear the quiesced
>> +         * flag if possible, otherwise the newly-loaded machine will
>> +         * hang indefinitely due to quiesced state ignoring
>> +         * interrupts.
>> +         */
>> +
>> +        if (!CPU(cpu)->halted) {
>> +            /* not halted, so definitely not in RTAS stopped state */
>> +            env->quiesced = 0;
>> +        }
>> +    }
>> +
>>       post_load_update_msr(env);
>>   
>>       if (tcg_enabled()) {
>> @@ -649,6 +666,28 @@ static const VMStateDescription vmstate_reservation = {
>>       }
>>   };
>>   
>> +static bool rtas_stopped_needed(void *opaque)
>> +{
>> +    PowerPCCPU *cpu = opaque;
>> +
>> +    return cpu->rtas_stopped_state && !cpu->env.quiesced;
>> +}
>> +
>> +static const VMStateDescription vmstate_rtas_stopped = {
>> +    .name = "cpu/rtas_stopped",
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .needed = rtas_stopped_needed,
>> +    .fields = (const VMStateField[]) {
>> +        /*
>> +         * "RTAS stopped" state, independent of halted state. For QEMU
>> +         * < 10.0, this is taken from cpu->halted at cpu_post_load()
>> +         */
>> +        VMSTATE_BOOL(env.quiesced, PowerPCCPU),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>> +
>>   #ifdef TARGET_PPC64
>>   static bool bhrb_needed(void *opaque)
>>   {
>> @@ -715,6 +754,7 @@ const VMStateDescription vmstate_ppc_cpu = {
>>           &vmstate_tlbmas,
>>           &vmstate_compat,
>>           &vmstate_reservation,
>> +        &vmstate_rtas_stopped,
>>           NULL
>>       }
>>   };