Re: [Qemu-devel] [RFC] ppc: qdev-ify CPU creation

[Andreas Färber]

Am 25.07.2011 um 12:09 schrieb Alexander Graf:

> On 24.07.2011, at 21:08, Hervé Poussineau wrote:
>
> > Alexander Graf a écrit :
> > > On 21.12.2010, at 21:01, Andreas Färber wrote:
> > >
> > > > From: Hervé Poussineau <address@hidden>
> > > >
> > > > v1:
> > > > * Coding style fixes.
> > > >
> > > > Signed-off-by: Hervé Poussineau <address@hidden>
> > > > Cc: Alexander Graf <address@hidden>
> > > > Signed-off-by: Andreas Färber <address@hidden>
> > > > ---
> > > >
> > > > Hello Alex,
> > > >
> > > > Seeing the discussions about Leon3, is this the way to go for  
> > > > ppc? Is ppc.[hc] right?
> > > >
> > > > The unconditional use of 6xx looks suspicious to me, no?
> > > > Should we rename cpu_device_irq_request() to  
> > > > cpu_device_irq_request_6xx()?
> > > >
> > > > Regards,
> > > > Andreas

[snip]

> > > > +    if (env->flags & POWERPC_FLAG_RTC_CLK) {
> > >
> > > Where does this flag suddenly come from? Is this related to  
> > > qdev'ification?
> > It's not related to qdev'ification. It is already set in CPU  
> > definitions since a long time.
>
> Hrm. So where is it usually read out then? Shouldn't that code go  
> away?

This is code moved 1:1 from PReP code to general ppc code, which I  
referred to above. I fear this is wrong.

> > > > +        /* POWER / PowerPC 601 RTC clock frequency is 7.8125 MHz  
> > > > */
> > > > +        cpu_ppc_tb_init(env, 7812500UL);
> > > > +    } else {
> > > > +        /* Set time-base frequency to 100 Mhz */
> > > > +        cpu_ppc_tb_init(env, 100UL * 1000UL * 1000UL);
> > >
> > > Usually we have a TB frequency of 400Mhz in our board/devtrees  
> > > hardcoded in the TCG case. How about a qdev property that the  
> > > creator could just modify to its needs? We won't need the special  
> > > 601 flag then either - just move that into the PREP code.
> > This code has been extracted from ppc_prep.c ; a qdev property is  
> > also fine.
>
> Yes, please :)

> > > > +CPUPPCState *cpu_ppc_init(const char *cpu_model)
> > > > +{
> > > > +    CPUPPCState *env;
> > > > +
> > > > +    env = qemu_mallocz(sizeof(CPUPPCState));
> > > > +    if (cpu_ppc_init_inplace(env, cpu_model) < 0) {
> > >
> > > Why would we need this function again if the CPUs are qdev'ified?
> > This function is not added ; it is already an existing one (see 25  
> > lines before). I kept it to not put in the same patch the CPU  
> > qdev'ification and the change of all the callers.
> > Indeed, a second patch may be created to change all callers to use  
> > cpu_ppc_create_simple() and to remove this function.
>
> Ah, I see. Please write up a patch set that goes through the bits of  
> fully converting it to Qdev :)

Could you please consider pulling my more recent patch of a PReP-local  
static function for shared CPU initialization into ppc-next? That way  
we can share code between PReP machines now and easily convert it in  
one central place when CPU qdev'ification has been completed.

Andreas