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

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

Am 03.08.2012 04:31, schrieb David Gibson:
> On Thu, Aug 02, 2012 at 05:44:49PM +0200, Andreas Färber wrote:
>> 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 <david@gibson.dropbear.id.au>
>>> ---
>>>  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.
> 
> Um, ok.  So how do I iterate the PowerPCCPUs instead of the CPUPPCStates?

You can't, yet. The QOM CPUState part 4 series (that got stalled due to
APIC modelling) moved quite some fields to CPUState but not enough to
change the first_cpu type despite the really long (74?) series.

So the solution here is to iterate the CPUPPCState, call
ppc_env_get_cpu() on it and pass that as opaque as before.

I could add a cpu_foreach() function though if that helps in the
meantime? Either way the idea is to limit the number of places to touch
in the upcoming refactorings.

Andreas

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