Re: [Qemu-devel] [PATCH 2/7] pseries: Fix and cleanup CPU initialization and reset

["Andreas Färber" <afaerber@suse.de>]

Am 10.09.2012 08:38, schrieb David Gibson:
> The current pseries machine init function iterates over the CPUs at several
> points, doing various bits of initialization.  This is messy; these can
> and should be merged into a single iteration doing all the necessary per
> cpu initialization.  Worse, some of these initializations were setting up
> state which should be set on every reset, not just at machine init time.
> A few of the initializations simply weren't necessary at all.
> 
> This patch, therefore, moves those things that need to be to the
> per-cpu reset handler, and combines the remainder into two loops over
> the cpus (which also creates them).  The second loop is for setting up
> hash table information, and will be removed in a subsequent patch also
> making other fixes to the hash table setup.
> 
> This exposes a bug in our start-cpu RTAS routine (called by the guest to
> start up CPUs other than CPU0) under kvm.  Previously, this function did
> not make a call to ensure that it's changes to the new cpu's state were
> pushed into KVM in-kernel state.  We sort-of got away with this because
> some of the initializations had already placed the secondary CPUs into the
> right starting state for the sorts of Linux guests we've been running.
> 
> Nonetheless the start-cpu RTAS call's behaviour was not correct and could
> easily have been broken by guest changes.  This patch also fixes it.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/spapr.c      |   33 +++++++++++++++++++--------------
>  hw/spapr_rtas.c |    5 +++++
>  2 files changed, 24 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/spapr.c b/hw/spapr.c
> index c34b767..be9092a 100644
> --- a/hw/spapr.c
> +++ b/hw/spapr.c
> @@ -581,8 +581,15 @@ static void spapr_reset(void *opaque)
>  static void spapr_cpu_reset(void *opaque)
>  {
>      PowerPCCPU *cpu = opaque;
> +    CPUPPCState *env = &cpu->env;
>  
>      cpu_reset(CPU(cpu));
> +
> +    /* All CPUs start halted, except CPU0, the rest are explicitly
> +     * started up by the guest using an RTAS call */
> +    env->halted = 1;

That comment does not make sense to me. There is no differentiation
between CPU0 and other CPUs here, is an if (env->cpu_index != 0) missing
or is ->halted overwritten for CPU0 elsewhere? (then the comment should
probably go there instead?)

Andreas

> +
> +    env->spr[SPR_HIOR] = 0;
>  }
>  
>  /* Returns whether we want to use VGA or not */
> @@ -665,11 +672,16 @@ static void ppc_spapr_init(ram_addr_t ram_size,
>  
>          /* Set time-base frequency to 512 MHz */
>          cpu_ppc_tb_init(env, TIMEBASE_FREQ);
> -        qemu_register_reset(spapr_cpu_reset, cpu);
>  
> -        env->hreset_vector = 0x60;
> +        /* PAPR always has exception vectors in RAM not ROM */
>          env->hreset_excp_prefix = 0;
> -        env->gpr[3] = env->cpu_index;
> +
> +        /* Tell KVM that we're in PAPR mode */
> +        if (kvm_enabled()) {
> +            kvmppc_set_papr(env);
> +        }
> +
> +        qemu_register_reset(spapr_cpu_reset, cpu);
>      }
>  
>      /* allocate RAM */
> @@ -685,7 +697,10 @@ static void ppc_spapr_init(ram_addr_t ram_size,
>  
>      /* allocate hash page table.  For now we always make this 16mb,
>       * later we should probably make it scale to the size of guest
> -     * RAM */
> +     * RAM.  FIXME: setting the htab information in the CPU env really
> +     * belongs at CPU reset time, but we can get away with it for now
> +     * because the PAPR guest is not permitted to write SDR1 so in
> +     * fact these settings will never change during the run */
>      spapr->htab_size = 1ULL << (pteg_shift + 7);
>      spapr->htab = qemu_memalign(spapr->htab_size, spapr->htab_size);
>  
> @@ -697,11 +712,6 @@ static void ppc_spapr_init(ram_addr_t ram_size,
>          /* Tell KVM that we're in PAPR mode */
>          env->spr[SPR_SDR1] = (unsigned long)spapr->htab |
>                               ((pteg_shift + 7) - 18);
> -        env->spr[SPR_HIOR] = 0;
> -
> -        if (kvm_enabled()) {
> -            kvmppc_set_papr(env);
> -        }
>      }
>  
>      filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, "spapr-rtas.bin");
> @@ -827,11 +837,6 @@ static void ppc_spapr_init(ram_addr_t ram_size,
>  
>      spapr->entry_point = 0x100;
>  
> -    /* SLOF will startup the secondary CPUs using RTAS */
> -    for (env = first_cpu; env != NULL; env = env->next_cpu) {
> -        env->halted = 1;
> -    }
> -
>      /* Prepare the device tree */
>      spapr->fdt_skel = spapr_create_fdt_skel(cpu_model, rma_size,
>                                              initrd_base, initrd_size,
> diff --git a/hw/spapr_rtas.c b/hw/spapr_rtas.c
> index ae18595..b808f80 100644
> --- a/hw/spapr_rtas.c
> +++ b/hw/spapr_rtas.c
> @@ -184,6 +184,11 @@ static void rtas_start_cpu(sPAPREnvironment *spapr,
>              return;
>          }
>  
> +        /* This will make sure qemu state is up to date with kvm, and
> +         * mark it dirty so our changes get flushed back before the
> +         * new cpu enters */
> +        kvm_cpu_synchronize_state(env);
> +
>          env->msr = (1ULL << MSR_SF) | (1ULL << MSR_ME);
>          env->nip = start;
>          env->gpr[3] = r3;
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg