Re: [PATCH 1/7] target/ppc: Fix CPU reservation migration for record-replay

[Daniel Henrique Barboza <danielhb413@gmail.com>]

Nick,

On 6/23/23 09:57, Nicholas Piggin wrote:
> ppc only migrates reserve_addr, so the destination machine can get a
> valid reservation with an incorrect reservation value of 0. Prior to
> commit 392d328abe753 ("target/ppc: Ensure stcx size matches larx"),
> this could permit a stcx. to incorrectly succeed. That commit
> inadvertently fixed that bug because the target machine starts with an
> impossible reservation size of 0, so any stcx. will fail.
> 
> This behaviour is permitted by the ISA because reservation loss may
> have implementation-dependent cause. What's more, with KVM machines it
> is impossible save or reasonably restore reservation state. However if
> the vmstate is being used for record-replay, the reservation must be
> saved and restored exactly in order for execution from snapshot to
> match the record.
> 
> This patch deprecates the existing incomplete reserve_addr vmstate,
> and adds a new vmstate subsection with complete reservation state.
> The new vmstate is needed only when record-replay mode is active.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>   target/ppc/cpu.h       |  2 ++
>   target/ppc/machine.c   | 26 ++++++++++++++++++++++++--
>   target/ppc/translate.c |  2 ++
>   3 files changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 4138a25801..0087ce66e2 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1119,7 +1119,9 @@ struct CPUArchState {
>       target_ulong reserve_addr;   /* Reservation address */
>       target_ulong reserve_length; /* Reservation larx op size (bytes) */
>       target_ulong reserve_val;    /* Reservation value */
> +#if defined(TARGET_PPC64)
>       target_ulong reserve_val2;
> +#endif

^ this ifdef breaks gitlab on a handful of runners with this error:

https://gitlab.com/danielhb/qemu/-/jobs/4612030964

[1637/2985] Compiling C object libqemu-ppc-linux-user.fa.p/target_ppc_translate.c.o
FAILED: libqemu-ppc-linux-user.fa.p/target_ppc_translate.c.o
cc -m64 -mcx16 -Ilibqemu-ppc-linux-user.fa.p -I. -I.. -Itarget/ppc -I../target/ppc -I../common-user/host/x86_64 -I../linux-user/include/host/x86_64 -I../linux-user/include -Ilinux-user -I../linux-user -Ilinux-user/ppc -I../linux-user/ppc -Iqapi -Itrace -Iui/shader -I/usr/include/capstone -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -fdiagnostics-color=auto -Wall -Winvalid-pch -Werror -std=gnu11 -O2 -g -fstack-protector-strong -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Wundef -Wwrite-strings -Wmissing-prototypes -Wstrict-prototypes -Wredundant-decls -Wold-style-declaration -Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs -Wendif-labels -Wexpansion-to-defined -Wimplicit-fallthrough=2 -Wmissing-format-attribute -Wno-missing-include-dirs -Wno-shift-negative-value -Wno-psabi -isystem /builds/danielhb/qemu/linux-headers -isystem linux-headers -iquote . -iquote /builds/danielhb/qemu -iquote /builds/danielhb/qemu/include -iquote /builds/danielhb/qemu/host/include/x86_64 -iquote /builds/danielhb/qemu/host/include/generic -iquote /builds/danielhb/qemu/tcg/i386 -pthread -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -fno-strict-aliasing -fno-common -fwrapv -fPIE -isystem../linux-headers -isystemlinux-headers -DNEED_CPU_H '-DCONFIG_TARGET="ppc-linux-user-config-target.h"' '-DCONFIG_DEVICES="ppc-linux-user-config-devices.h"' -MD -MQ libqemu-ppc-linux-user.fa.p/target_ppc_translate.c.o -MF libqemu-ppc-linux-user.fa.p/target_ppc_translate.c.o.d -o libqemu-ppc-linux-user.fa.p/target_ppc_translate.c.o -c ../target/ppc/translate.c
../target/ppc/translate.c: In function 'ppc_translate_init':
../target/ppc/translate.c:156:5: error: 'cpu_reserve_val2' undeclared (first use in this function); did you mean 'cpu_reserve_val'?
   156 |     cpu_reserve_val2 = tcg_global_mem_new(cpu_env,
       |     ^~~~~~~~~~~~~~~~
       |     cpu_reserve_val
../target/ppc/translate.c:156:5: note: each undeclared identifier is reported only once for each function it appears in
In file included from /usr/include/rpc/netdb.h:42,
                  from /usr/include/netdb.h:32,
                  from /builds/danielhb/qemu/include/sysemu/os-posix.h:34,
                  from /builds/danielhb/qemu/include/qemu/osdep.h:151,
                  from ../target/ppc/translate.c:21:
../target/ppc/translate.c:157:65: error: 'CPUPPCState' {aka 'struct CPUArchState'} has no member named 'reserve_val2'; did you mean 'reserve_val'?
   157 |                                           offsetof(CPUPPCState, reserve_val2),
       |                                                                 ^~~~~~~~~~~~
[1638/2985] Compiling C object libqemu-ppc-linux-user.fa.p/libdecnumber_decNumber.c.o


I'll leave patch 1 and 5 behind for now. Since they're marked as fixes you
can send them during the freeze. Thanks,


Daniel


>   
>       /* These are used in supervisor mode only */
>       target_ulong msr;      /* machine state register */
> diff --git a/target/ppc/machine.c b/target/ppc/machine.c
> index 134b16c625..a817532e5b 100644
> --- a/target/ppc/machine.c
> +++ b/target/ppc/machine.c
> @@ -10,6 +10,7 @@
>   #include "qemu/main-loop.h"
>   #include "kvm_ppc.h"
>   #include "power8-pmu.h"
> +#include "sysemu/replay.h"
>   
>   static void post_load_update_msr(CPUPPCState *env)
>   {
> @@ -671,6 +672,27 @@ static const VMStateDescription vmstate_compat = {
>       }
>   };
>   
> +static bool reservation_needed(void *opaque)
> +{
> +    return (replay_mode != REPLAY_MODE_NONE);
> +}
> +
> +static const VMStateDescription vmstate_reservation = {
> +    .name = "cpu/reservation",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = reservation_needed,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINTTL(env.reserve_addr, PowerPCCPU),
> +        VMSTATE_UINTTL(env.reserve_length, PowerPCCPU),
> +        VMSTATE_UINTTL(env.reserve_val, PowerPCCPU),
> +#if defined(TARGET_PPC64)
> +        VMSTATE_UINTTL(env.reserve_val2, PowerPCCPU),
> +#endif
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>   const VMStateDescription vmstate_ppc_cpu = {
>       .name = "cpu",
>       .version_id = 5,
> @@ -692,8 +714,7 @@ const VMStateDescription vmstate_ppc_cpu = {
>           VMSTATE_UINTTL_ARRAY(env.spr, PowerPCCPU, 1024),
>           VMSTATE_UINT64(env.spe_acc, PowerPCCPU),
>   
> -        /* Reservation */
> -        VMSTATE_UINTTL(env.reserve_addr, PowerPCCPU),
> +        VMSTATE_UNUSED(sizeof(target_ulong)), /* was env.reserve_addr */
>   
>           /* Supervisor mode architected state */
>           VMSTATE_UINTTL(env.msr, PowerPCCPU),
> @@ -722,6 +743,7 @@ const VMStateDescription vmstate_ppc_cpu = {
>           &vmstate_tlbemb,
>           &vmstate_tlbmas,
>           &vmstate_compat,
> +        &vmstate_reservation,
>           NULL
>       }
>   };
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index c9fb7b40a5..eb278c2683 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -77,7 +77,9 @@ static TCGv cpu_xer, cpu_so, cpu_ov, cpu_ca, cpu_ov32, cpu_ca32;
>   static TCGv cpu_reserve;
>   static TCGv cpu_reserve_length;
>   static TCGv cpu_reserve_val;
> +#if defined(TARGET_PPC64)
>   static TCGv cpu_reserve_val2;
> +#endif
>   static TCGv cpu_fpscr;
>   static TCGv_i32 cpu_access_type;
>