Re: [Qemu-devel] [PATCH 8/8] PPC: Add e5500 CPU target

[Alexander Graf]

On 21.06.2012, at 01:07, Scott Wood wrote:

> On 06/20/2012 05:59 PM, Alexander Graf wrote:
>> 
>> On 21.06.2012, at 00:26, Scott Wood wrote:
>> 
>>> On 06/20/2012 03:11 PM, Alexander Graf wrote:
>>>> +    /* XXX better abstract into Emb.xxx features */
>>>> +    if (version == fsl_e5500) {
>>>> +        spr_register(env, SPR_BOOKE_EPCR, "EPCR",
>>>> +                     SPR_NOACCESS, SPR_NOACCESS,
>>>> +                     &spr_read_generic, &spr_write_generic,
>>>> +                     0x00000000);
>>>> +        spr_register(env, SPR_BOOKE_MAS7_MAS3, "MAS7_MAS3",
>>>> +                     SPR_NOACCESS, SPR_NOACCESS,
>>>> +                     &spr_read_mas73, &spr_write_mas73,
>>>> +                     0x00000000);
>>>> +        env->reset_msr = (1ULL < MSR_CM);
>>> 
>>> That's a funny way of writing "env->reset_msr = 0". :-)
>>> 
>>> Assuming you really meant "env->reset_msr = 1ULL << MSR_CM", why?  We
>>> enter the kernel in 32-bit mode.  It resets in 32-bit mode as well, if
>>> we ever implement that for e5500 QEMU.
>> 
>> Hrm. At least my self-compiled kernel did issue an "ld" instruction before 
>> going into MSR_CM mode, hence I figured we need it.
> 
> You don't need MSR_CM to run 64-bit instructions.  It just affects
> masking in certain places.

Wait - you don't? Is there a comprehensive description on what MSR_CM really 
does and does not?

> 
>>>> +    }
>>>> 
>>>> #if !defined(CONFIG_USER_ONLY)
>>>>    env->nb_tlb = 0;
>>>> @@ -4576,6 +4655,14 @@ static void init_proc_e500 (CPUPPCState *env, int 
>>>> version)
>>>> #endif
>>>> 
>>>>    init_excp_e200(env);
>>>> +
>>>> +#if !defined(CONFIG_USER_ONLY)
>>>> +    /* We support 64bit wide IVPR on 64bit platforms */
>>>> +    if (version == fsl_e5500) {
>>>> +        env->ivpr_mask = (target_ulong)~0xFFFFULL;
>>>> +    }
>>>> +#endif
>>> 
>>> So, I'm guessing you don't do this unconditionally because QEMU will
>>> generate 64-bit code if compiled that way, regardless of the actual
>>> target -- and you don't want stray garbage in the upper 32 bits being
>>> written into IVPR.  But why isn't this an issue with all the other SPRs?
>>> Why don't we have a problem with junk being written into the upper half
>>> of MAS3, for example (there's MAS3_RPN_MASK, but it's not used)?
>> 
>> I was thinking of making it unconditional, but this way seemed
>> cleaner to me, as it actually follows exactly what the spec says. Not
>> sure what would happen if you have -1 in your 32-bit register value
>> and you try to write that to IVPR otherwise. It'd probably break :).
> 
> It would only break because there doesn't seem to be any generic way of
> treating 32-bit SPRs as 32-bit.  We should probably have a separate
> spr_write_generic32().  For a register like IVPR we'd select 32 or
> full-size at init time, based on the type of CPU we're modelling.  For
> something like MAS3 we'd always use the 32-bit version.

Yup, that should work :)


Alex