Re: [PATCH 1/4] target/ppc: Add LPAR-per-core vs per-thread mode flag

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

Hi,

I'm afraid this patch breaks two Gitlab runners in different manners.


- the 'tsan-build' runner:

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

[2170/3531] Compiling C object libqemu-ppc64-softmmu.fa.p/target_ppc
_translate.c.o
FAILED: libqemu-ppc64-softmmu.fa.p/target_ppc_translate.c.o
clang -m64 -mcx16 -Ilibqemu-ppc64-softmmu.fa.p -I. -I.. -Itarget/ppc -I../target/ppc -Iqapi -Itrace -Iui -Iui/shader -I/usr/include/pixman-1 -I/usr/include/capstone -I/usr/include/spice-server -I/usr/include/spice-1 -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -fcolor-diagnostics -Wall -Winvalid-pch -Werror -std=gnu11 -O2 -g -fsanitize=thread -fstack-protector-strong -Wundef -Wwrite-strings -Wmissing-prototypes -Wstrict-prototypes -Wredundant-decls -Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs -Wendif-labels -Wexpansion-to-defined -Wmissing-format-attribute -Wno-initializer-overrides -Wno-missing-include-dirs -Wno-shift-negative-value -Wno-string-plus-int -Wno-typedef-redefinition -Wno-tautological-type-limit-compare -Wno-psabi -Wno-gnu-variable-sized-type-not-at-end -Wthread-safety -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="ppc64-softmmu-config-target.h"' '-DCONFIG_DEVICES="ppc64-softmmu-config-devices.h"' -MD -MQ libqemu-ppc64-softmmu.fa.p/target_ppc_translate.c.o -MF libqemu-ppc64-softmmu.fa.p/target_ppc_translate.c.o.d -o libqemu-ppc64-softmmu.fa.p/target_ppc_translate.c.o -c ../target/ppc/translate.c
../target/ppc/translate.c:249:20: error: unused function 'gen_serialize_core' [-Werror,-Wunused-function]
static inline bool gen_serialize_core(DisasContext *ctx)
                    ^
1 error generated.

And in fact, after this patch, gen_serialize_core() is now unused:


$ git grep 'gen_serialize_core'
target/ppc/translate.c:static inline bool gen_serialize_core(DisasContext *ctx)
target/ppc/translate.c:static inline bool gen_serialize_core_lpar(DisasContext *ctx)
target/ppc/translate.c:    if (!gen_serialize_core_lpar(ctx)) {
target/ppc/translate.c:    if (!gen_serialize_core_lpar(ctx)) {
$

I'm not sure why this was the only runner that catched this error. Anyway, this is easily
fixable by removing gen_serialize_core().



- the 'clang-user' Gitlab runner:

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

[1331/2617] 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
clang -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 -fcolor-diagnostics -Wall -Winvalid-pch -Werror -std=gnu11 -O2 -g -fstack-protector-strong -Wundef -Wwrite-strings -Wmissing-prototypes -Wstrict-prototypes -Wredundant-decls -Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs -Wendif-labels -Wexpansion-to-defined -Wmissing-format-attribute -Wno-initializer-overrides -Wno-missing-include-dirs -Wno-shift-negative-value -Wno-string-plus-int -Wno-typedef-redefinition -Wno-tautological-type-limit-compare -Wno-psabi -Wno-gnu-variable-sized-type-not-at-end -Wthread-safety -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 -fsanitize=undefined -fno-sanitize-recover=undefined -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:259:20: error: unused function 'gen_serialize_core_lpar' [-Werror,-Wunused-function]
static inline bool gen_serialize_core_lpar(DisasContext *ctx)
                    ^
1 error generated.


Given that the error was thrown when building the 32 bit linux user binary I think this
is a matter of wrapping the function with the adequate #ifdefs. Thanks,


Daniel



On 7/3/23 07:16, Nicholas Piggin wrote:
> The Power ISA has the concept of sub-processors:
> 
>    Hardware is allowed to sub-divide a multi-threaded processor into
>    "sub-processors" that appear to privileged programs as multi-threaded
>    processors with fewer threads.
> 
> POWER9 and POWER10 have two modes, either every thread is a
> sub-processor or all threads appear as one multi-threaded processor. In
> the user manuals these are known as "LPAR per thread" / "Thread LPAR",
> and "LPAR per core" / "1 LPAR", respectively.
> 
> The practical difference is: in thread LPAR mode, non-hypervisor SPRs
> are not shared between threads and msgsndp can not be used to message
> siblings. In 1 LPAR mode, some SPRs are shared and msgsndp is usable.
> Thrad LPAR allows multiple partitions to run concurrently on the same
> core, and is a requirement for KVM to run on POWER9/10 (which does not
> gang-schedule an LPAR on all threads of a core like POWER8 KVM).
> 
> Traditionally, SMT in PAPR environments including PowerVM and the
> pseries QEMU machine with KVM acceleration behaves as in 1 LPAR mode.
> In OPAL systems, Thread LPAR is used. When adding SMT to the powernv
> machine, it is therefore preferable to emulate Thread LPAR.
> 
> To account for this difference between pseries and powernv, an LPAR mode
> flag is added such that SPRs can be implemented as per-LPAR shared, and
> that becomes either per-thread or per-core depending on the flag.
> 
> Reviewed-by: Joel Stanley <joel@jms.id.au>
> Tested-by: Cédric Le Goater <clg@kaod.org>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>   hw/ppc/spapr_cpu_core.c |  2 ++
>   target/ppc/cpu.h        |  3 +++
>   target/ppc/cpu_init.c   | 12 ++++++++++++
>   target/ppc/translate.c  | 16 +++++++++++++---
>   4 files changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index a4e3c2fadd..b482d9754a 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -270,6 +270,8 @@ static bool spapr_realize_vcpu(PowerPCCPU *cpu, SpaprMachineState *spapr,
>       env->spr_cb[SPR_PIR].default_value = cs->cpu_index;
>       env->spr_cb[SPR_TIR].default_value = thread_index;
>   
> +    cpu_ppc_set_1lpar(cpu);
> +
>       /* Set time-base frequency to 512 MHz. vhyp must be set first. */
>       cpu_ppc_tb_init(env, SPAPR_TIMEBASE_FREQ);
>   
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index af12c93ebc..951f73d89d 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -674,6 +674,8 @@ enum {
>       POWERPC_FLAG_SCV      = 0x00200000,
>       /* Has >1 thread per core                                                */
>       POWERPC_FLAG_SMT      = 0x00400000,
> +    /* Using "LPAR per core" mode  (as opposed to per-thread)                */
> +    POWERPC_FLAG_SMT_1LPAR= 0x00800000,
>   };
>   
>   /*
> @@ -1437,6 +1439,7 @@ void store_booke_tsr(CPUPPCState *env, target_ulong val);
>   void ppc_tlb_invalidate_all(CPUPPCState *env);
>   void ppc_tlb_invalidate_one(CPUPPCState *env, target_ulong addr);
>   void cpu_ppc_set_vhyp(PowerPCCPU *cpu, PPCVirtualHypervisor *vhyp);
> +void cpu_ppc_set_1lpar(PowerPCCPU *cpu);
>   int ppcmas_tlb_check(CPUPPCState *env, ppcmas_tlb_t *tlb, hwaddr *raddrp,
>                        target_ulong address, uint32_t pid);
>   int ppcemb_tlb_search(CPUPPCState *env, target_ulong address, uint32_t pid);
> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index 5f4969664e..905a59aea9 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -6629,6 +6629,18 @@ void cpu_ppc_set_vhyp(PowerPCCPU *cpu, PPCVirtualHypervisor *vhyp)
>       env->msr_mask &= ~MSR_HVB;
>   }
>   
> +void cpu_ppc_set_1lpar(PowerPCCPU *cpu)
> +{
> +    CPUPPCState *env = &cpu->env;
> +
> +    /*
> +     * pseries SMT means "LPAR per core" mode, e.g., msgsndp is usable
> +     * between threads.
> +     */
> +    if (env->flags & POWERPC_FLAG_SMT) {
> +        env->flags |= POWERPC_FLAG_SMT_1LPAR;
> +    }
> +}
>   #endif /* !defined(CONFIG_USER_ONLY) */
>   
>   #endif /* defined(TARGET_PPC64) */
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index db0ba49bdc..10598cde40 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -256,6 +256,16 @@ static inline bool gen_serialize_core(DisasContext *ctx)
>   }
>   #endif
>   
> +static inline bool gen_serialize_core_lpar(DisasContext *ctx)
> +{
> +    /* 1LPAR implies SMT */
> +    if (ctx->flags & POWERPC_FLAG_SMT_1LPAR) {
> +        return gen_serialize(ctx);
> +    }
> +
> +    return true;
> +}
> +
>   /* SPR load/store helpers */
>   static inline void gen_load_spr(TCGv t, int reg)
>   {
> @@ -451,7 +461,7 @@ static void spr_write_CTRL_ST(DisasContext *ctx, int sprn, int gprn)
>   
>   void spr_write_CTRL(DisasContext *ctx, int sprn, int gprn)
>   {
> -    if (!(ctx->flags & POWERPC_FLAG_SMT)) {
> +    if (!(ctx->flags & POWERPC_FLAG_SMT_1LPAR)) {
>           spr_write_CTRL_ST(ctx, sprn, gprn);
>           goto out;
>       }
> @@ -815,7 +825,7 @@ void spr_write_pcr(DisasContext *ctx, int sprn, int gprn)
>   /* DPDES */
>   void spr_read_dpdes(DisasContext *ctx, int gprn, int sprn)
>   {
> -    if (!gen_serialize_core(ctx)) {
> +    if (!gen_serialize_core_lpar(ctx)) {
>           return;
>       }
>   
> @@ -824,7 +834,7 @@ void spr_read_dpdes(DisasContext *ctx, int gprn, int sprn)
>   
>   void spr_write_dpdes(DisasContext *ctx, int sprn, int gprn)
>   {
> -    if (!gen_serialize_core(ctx)) {
> +    if (!gen_serialize_core_lpar(ctx)) {
>           return;
>       }
>