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

[Alexander Graf]

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 have the same issue in the previous patch with "1ULL < MSR_SF",
> though I don't know if those chips actually do reset into 64-bit mode.

They do :).

> 
>> +    }
>> 
>> #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 :).

> Speaking of which, I don't see where you change MAS2_EPN_MASK to take
> MSR_CM into account (or to accept 64-bit MAS2 at all).

Ugh. Good question why this works at all. Do we by accident truncate everything 
to 32bit?


Alex