[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v1 2/3] spapr: Set LPCR to current AIL mode when starting a n
From: |
Greg Kurz |
Subject: |
Re: [PATCH v1 2/3] spapr: Set LPCR to current AIL mode when starting a new CPU |
Date: |
Wed, 26 May 2021 18:03:09 +0200 |
On Wed, 26 May 2021 19:16:25 +1000
Nicholas Piggin <npiggin@gmail.com> wrote:
> TCG does not keep track of AIL mode in a central place, it's based on
> the current LPCR[AIL] bits. Synchronize the new CPU's LPCR to the
> current LPCR in rtas_start_cpu(), similarly to the way the ILE bit is
> synchronized.
>
> Open-code the ILE setting as well now that the caller's LPCR is
> available directly, there is no need for the indirection.
>
> Without this, under both TCG and KVM, adding a POWER8/9/10 class CPU
> with a new core ID after a modern Linux has booted results in the new
> CPU's LPCR missing the LPCR[AIL]=0b11 setting that the other CPUs have.
> This can cause crashes and unexpected behaviour.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> hw/ppc/spapr_rtas.c | 14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index 63d96955c0..b476382ae6 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -132,8 +132,8 @@ static void rtas_start_cpu(PowerPCCPU *callcpu,
> SpaprMachineState *spapr,
> target_ulong id, start, r3;
> PowerPCCPU *newcpu;
> CPUPPCState *env;
> - PowerPCCPUClass *pcc;
> target_ulong lpcr;
> + target_ulong caller_lpcr;
>
> if (nargs != 3 || nret != 1) {
> rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> @@ -152,7 +152,6 @@ static void rtas_start_cpu(PowerPCCPU *callcpu,
> SpaprMachineState *spapr,
> }
>
> env = &newcpu->env;
> - pcc = POWERPC_CPU_GET_CLASS(newcpu);
>
> if (!CPU(newcpu)->halted) {
> rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> @@ -164,10 +163,15 @@ static void rtas_start_cpu(PowerPCCPU *callcpu,
> SpaprMachineState *spapr,
> env->msr = (1ULL << MSR_SF) | (1ULL << MSR_ME);
> hreg_compute_hflags(env);
>
> + caller_lpcr = callcpu->env.spr[SPR_LPCR];
> lpcr = env->spr[SPR_LPCR];
> - if (!pcc->interrupts_big_endian(callcpu)) {
> - lpcr |= LPCR_ILE;
> - }
> +
> + /* Set ILE the same way */
> + lpcr = (lpcr & ~LPCR_ILE) | (caller_lpcr & LPCR_ILE);
> +
Unrelated change as Cedric already noted but that's nice :)
/me starting to think we might do the same elsewhere and
maybe get rid of PowerPCCPUClass::interrupts_big_endian()
> + /* Set AIL the same way */
> + lpcr = (lpcr & ~LPCR_AIL) | (caller_lpcr & LPCR_AIL);
> +
It seems better indeed to rely on the calling CPU here rather
than the arbitrary first_cpu in the hotplug handler.
Reviewed-by: Greg Kurz <groug@kaod.org>
> if (env->mmu_model == POWERPC_MMU_3_00) {
> /*
> * New cpus are expected to start in the same radix/hash mode