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

[Anthony Liguori]

Andreas Färber <address@hidden> writes:

> 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.

I already commented on the first patch because I had a feeling you'd
post something like this ;-)

Regarding reset:

1) Devices should implement DeviceState::reset()

2) If a device doesn't implement ::reset(), it should call
qemu_register_reset()

3) Reset should propagate through the device model, starting with the
top-level machine which is logically what's plugged into the wall and
is the source of power in the first place.

Regards,

Anthony Liguori

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