Re: [Qemu-devel] [PATCH 2/2] pseries: Use new hook to correct reset sequence

[Andreas Färber]

Am 02.08.2012 04:10, schrieb David Gibson:
> A number of things need to occur during reset of the PAPR paravirtualized
> platform in a specific order.  For example, the hash table needs to be
> cleared before the CPUs are reset, so that they initialize their register
> state correctly, and the CPUs need to have their main reset called before
> we set up the entry point state on the boot cpu.  We also need to have
> the main qdev reset happen before the creation and installation of the
> device tree for the new boot, because we need the state of the devices
> settled to correctly construct the device tree.
> 
> Currently reset of pseries is broken in a number of ways, and in other
> cases works largely by accident. This patch uses the new QEMUMachine reset
> hook to correct these problems, by replacing the several existing spapr
> reset hooks with one new machine hook which ensures that the various stages
> happen in the correct order.
> 
> Signed-off-by: David Gibson <address@hidden>
> ---
>  hw/spapr.c |   66 
> +++++++++++++++++++++++++++++++++---------------------------
>  1 file changed, 36 insertions(+), 30 deletions(-)
> 
> diff --git a/hw/spapr.c b/hw/spapr.c
> index 2453bae..1e60ec1 100644
> --- a/hw/spapr.c
> +++ b/hw/spapr.c
> @@ -582,29 +582,22 @@ static void spapr_reset_htab(sPAPREnvironment *spapr)
>      }
>  }
>  
> -static void spapr_reset(void *opaque)
> +static void spapr_reset_cpu(CPUPPCState *env)
>  {
> -    sPAPREnvironment *spapr = (sPAPREnvironment *)opaque;
> -
> -    /* Reset the hash table & recalc the RMA */
> -    spapr_reset_htab(spapr);
> -
> -    /* Load the fdt */
> -    spapr_finalize_fdt(spapr, spapr->fdt_addr, spapr->rtas_addr,
> -                       spapr->rtas_size);
> -}
> -
> -static void spapr_cpu_reset(void *opaque)
> -{
> -    PowerPCCPU *cpu = opaque;
> -    CPUPPCState *env = &cpu->env;
> +    PowerPCCPU *cpu = container_of(env, PowerPCCPU, env);

NACK. Please don't undo the cleanups I have applied! Functions should
take a QOM PowerPCCPU, not its internal CPUPPCState. Fields are
gradually being moved from CPUxxxState into CPUState.

>  
>      cpu_reset(CPU(cpu));

Also note the current discussion about CPU reset and ordering, e.g.:
http://patchwork.ozlabs.org/patch/174602/

Anthony was favoring moving reset code out of machines and expressed
dislike for looping through CPUs, which my above patch took into
account. The ordering issue between CPU and devices is still unsolved there.

Some on-list comments from Anthony would be nice, since we are moving
into opposing directions here - having the sPAPR machine be more in
control vs. moving code away from the PC machine into target-i386 CPU
and/or common CPU code.

Cheers,
Andreas

>  
>      env->external_htab = spapr->htab;
>      env->htab_base = -1;
>      env->htab_mask = HTAB_SIZE(spapr) - 1;
> +    /* CPUs need to start halted at reset, the platform reset code
> +     * will activate CPU0 then the rest are explicitly started by the
> +     * guest using RTAS */
> +    env->halted = 1;
>  
> +    /* Secondary CPUs get the CPU ID in r3 on entry */
> +    env->gpr[3] = env->cpu_index;
>      env->spr[SPR_SDR1] = (unsigned long)spapr->htab |
>          (spapr->htab_shift - 18);
>  
> @@ -612,14 +605,35 @@ static void spapr_cpu_reset(void *opaque)
>          kvmppc_update_sdr1(env);
>      }
>  
> -    /* Set up the entry state */
> -    if (env == first_cpu) {
> -        env->gpr[3] = spapr->fdt_addr;
> -        env->gpr[5] = 0;
> -        env->halted = 0;
> -        env->nip = spapr->entry_point;
> +    tb_flush(env);
> +}
> +
> +static void spapr_reset(bool report)
> +{
> +    CPUPPCState *env = first_cpu;
> +
> +    /* Reset the qdevs */
> +    qemu_default_system_reset(report);
> +
> +    /* Reset the hash table & recalc the RMA */
> +    spapr_reset_htab(spapr);
> +
> +    /* Reset the CPUs */
> +    for (env = first_cpu; env != NULL; env = env->next_cpu) {
> +        spapr_reset_cpu(env);
>      }
>  
> +    /* Load the fdt */
> +    spapr_finalize_fdt(spapr, spapr->fdt_addr, spapr->rtas_addr,
> +                       spapr->rtas_size);
> +
> +    /* Set up the entry state on CPU0 */
> +    env = first_cpu;
> +
> +    env->gpr[3] = spapr->fdt_addr;
> +    env->gpr[5] = 0;
> +    env->halted = 0;
> +    env->nip = spapr->entry_point;
>      tb_flush(env);
>  }
>  
> @@ -718,8 +732,6 @@ static void ppc_spapr_init(ram_addr_t ram_size,
>      /* FIXME: we should change this default based on RAM size */
>      spapr->htab_shift = 24;
>  
> -    qemu_register_reset(spapr_reset, spapr);
> -
>      /* init CPUs */
>      if (cpu_model == NULL) {
>          cpu_model = kvm_enabled() ? "host" : "POWER7";
> @@ -734,11 +746,9 @@ 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;
>          env->hreset_excp_prefix = 0;
> -        env->gpr[3] = env->cpu_index;
>      }
>  
>      /* allocate RAM */
> @@ -883,11 +893,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,
>                                              initrd_base, initrd_size,
> @@ -900,6 +905,7 @@ static QEMUMachine spapr_machine = {
>      .name = "pseries",
>      .desc = "pSeries Logical Partition (PAPR compliant)",
>      .init = ppc_spapr_init,
> +    .reset = spapr_reset,
>      .max_cpus = MAX_CPUS,
>      .no_parallel = 1,
>      .use_scsi = 1,
> 


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