Re: [Qemu-devel] [PATCH RFC] spapr: ignore interrupts during reset state

[David Gibson]

On Thu, Jun 08, 2017 at 12:06:08PM +0530, Nikunj A Dadhania wrote:
> Rebooting a SMP TCG guest is broken for both single/multi threaded TCG.

Ouch.  When exactly did this happen?  I know that smp boot used to
work under TCG, albeit very slowly.

> When reset happens, all the CPUs are in halted state. First CPU is brought out
> of reset and secondary CPUs would be initialized by the guest kernel using a
> rtas call start-cpu.
> 
> However, in case of TCG, decrementer interrupts keep on coming and waking the
> secondary CPUs up.

Ok.. how is that happening given that the secondary CPUs should have
MSR[EE] == 0?

> These secondary CPUs would see the decrementer interrupt pending, which makes
> cpu::has_work() to bring them out of wait loop and start executing
> tcg_exec_cpu().
> 
> The problem with this is all the CPUs wake up and start booting SLOF image,
> causing the following exception(4 CPUs TCG VM):

[snip]
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index d10808d..eb88bcb 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1013,6 +1013,13 @@ struct CPUPPCState {
>      int access_type; /* when a memory exception occurs, the access
>                          type is stored here */
>  
> +    /* CPU in reset, shouldn't process any interrupts.
> +     *
> +     * Decrementer interrupts in TCG can still wake the CPU up. Make sure 
> that
> +     * when this variable is set, cpu_has_work_* should return false.
> +     */
> +    int in_reset;

So I'd really rather not add another flag to the cpu structure,
especially since we'd then need to migrate it as well.

I'm pretty sure there should be a way to inhibit the unwanted
interrupts using existing mechanisms.

> +
>      CPU_COMMON
>  
>      /* MMU context - only relevant for full system emulation */
> diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
> index 56a0ab2..64f4348 100644
> --- a/target/ppc/translate_init.c
> +++ b/target/ppc/translate_init.c
> @@ -8561,6 +8561,9 @@ static bool cpu_has_work_POWER7(CPUState *cs)
>      CPUPPCState *env = &cpu->env;
>  
>      if (cs->halted) {
> +        if (env->in_reset) {
> +            return false;
> +        }
>          if (!(cs->interrupt_request & CPU_INTERRUPT_HARD)) {
>              return false;
>          }
> @@ -8718,6 +8721,9 @@ static bool cpu_has_work_POWER8(CPUState *cs)
>      CPUPPCState *env = &cpu->env;
>  
>      if (cs->halted) {
> +        if (env->in_reset) {
> +            return false;
> +        }
>          if (!(cs->interrupt_request & CPU_INTERRUPT_HARD)) {
>              return false;
>          }
> @@ -8899,6 +8905,9 @@ static bool cpu_has_work_POWER9(CPUState *cs)
>      CPUPPCState *env = &cpu->env;
>  
>      if (cs->halted) {
> +        if (env->in_reset) {
> +            return false;
> +        }
>          if (!(cs->interrupt_request & CPU_INTERRUPT_HARD)) {
>              return false;
>          }

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature