diff mbox series

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

Message ID 20230703101700.24064-2-npiggin@gmail.com
State New
Headers show
Series ppc/pnv: SMT support for powernv | expand

Commit Message

Nicholas Piggin July 3, 2023, 10:16 a.m. UTC 
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(-)

Comments

Cédric Le Goater July 3, 2023, 11:43 a.m. UTC | #1 
On 7/3/23 12: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>




Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

> ---
>   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;
>       }
>
Daniel Henrique Barboza July 3, 2023, 12:23 p.m. UTC | #2 
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;
>       }
>
Cédric Le Goater July 3, 2023, 12:55 p.m. UTC | #3 
On 7/3/23 14:23, Daniel Henrique Barboza wrote:
> 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,
> 
> 


I added a fix in b769d4c8f4c6 for a similar issue.

Nick, could you please take a look and compile with a recent clang ?

Thanks,

C.



> 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;
>>       }
Nicholas Piggin July 3, 2023, 1:02 p.m. UTC | #4 
On Mon Jul 3, 2023 at 10:23 PM AEST, Daniel Henrique Barboza wrote:
> 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:

Sorry Daniel :( I keep losing my test config. I'll have to set up
something a bit more permanent and reliable. Will resubmit.

Thanks,
Nick
diff mbox series

Patch

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;
     }