Re: [Qemu-devel] [PATCH v4 1/6] ARM: Factor out ARM on/off PSCI control functions

[Peter Maydell]

On 24 March 2016 at 13:54,  <address@hidden> wrote:
>
>
> ----- Le 24 Mar 16, à 11:46, Peter Maydell address@hidden a écrit :
>
>> On 24 March 2016 at 07:10, Jean-Christophe DUBOIS <address@hidden> wrote:
>> > Le 23/03/2016 16:24, Peter Maydell a écrit :
>
>> >> On 19 March 2016 at 20:59, Jean-Christophe Dubois <address@hidden>
>> >> wrote:
>> >>> +
>> >>> + /*
>> >>> + * The newly brought CPU is requested to enter the exception level
>> >>> + * "target_el" and be in the requested mode (aarch64 ou aarch32).
>> >>> + */
>> >>> +
>> >>> + /*
>> >>> + * Check that the CPU is supporting the requested level
>> >>> + */
>> >>> + switch (target_el) {
>> >>> + case 3:
>> >>> + if (arm_feature(&target_cpu->env, ARM_FEATURE_EL3)) {
>> >>> + if (is_a64(&target_cpu->env)) {
>
>> >> Why are we looking at the is_a64() state of the CPU as it comes
>> >> out of reset? What we care about is target_aa64 here (if we
>> >> are putting the CPU into a 64-bit state at a lower exception
>> >> level then we must ensure the state is consistent with all
>> >> the higher levels being 64-bit).
>
>> > Because we are trying to switch the CPU to the desired level and we don't 
>> > do
>> > it the same for aarch64 and aarch32. This is not about current mode but
>> > about the real architecture. Other functions like arm_is_secure() or
>> > arm_current_el() are doing the same. Even cpu_reset() is doing the same ...
>
>> No, this doesn't seem right to me. You're trying to set
>> the SCR_EL3.RW and HCR_EL2.RW bits, and the condition
>> for setting them is "are we trying to transition *to*
>> an EL which is 64 bits". Whether the current EL is 64 bits
>> or not is not relevant (and we know that it must be 64 bits
>> because 64-bit capable CPUs always reset into 64-bits,
>> assuming the caller didn't buggily ask us for a 64-bit
>> state on a 32-bit CPU).
>
> You mean that env->aarch64 will change dynamically depending on
> the CPU mode?

Yes, env->aarch64 tells you whether the CPU is currently in
AArch32 or AArch64. (You should use is_a64() rather than directly
looking at it, though.) If you want to know if the CPU
supports AArch64 in general, you can call
arm_feature(env, ARM_FEATURE_AARCH64).

>> arm_is_secure() and arm_current_el() are functions which
>> are asking questions about the current state of a CPU,
>> so obviously they look at the current state of the CPU.
>
>> >>> + /* We assert if the request cannot be fulfilled */
>> >>> + assert(target_aa64 == arm_el_is_aa64(&target_cpu->env, target_el));
>
>> >> So, when can this happen, or is it genuinely "can't happen" ?
>
>
>> > Well, for now I don't force the mode of the desired level. So if we are
>> > asked to go EL1 in arch32 but by default the CORE foes EL1 in arch64 this
>> > will fail.
>
>> Oh, I see. That needs at least a TODO comment, because it's
>> a missing feature that we'll need to implement at some point.
>> (The current PSCI code doesn't do it, but the conditions it
>> documents for why it can't do it aren't true any more -- we
>> do now implement more than just EL1.)
>
> I was wondeing if I could influence on the capability to get the requested 
> mode
> by tweaking target_cpu->env.cp15.scr_el3 and target_cpu->env.cp15.hcr_el2

The RW bits in SCR_EL3 and HCR_EL2 are there only to define
what the register width of lower exception levels are. If a
guest tries an exception return asking for a different register
width to what the system register say then this is an illegal
return event. So since we're manually adjusting the state of the
CPU to make it look as though we're in some lower exception level,
we need to adjust the RW bits to be consistent with the fact that
we're now in (say) AArch64 EL1; otherwise when the guest later does
an exception return it will break.

If you want to actually change the current mode then it's probably
sufficient to set the SCR_EL3.RW and HCR_EL2.RW bits correctly
and then set env->aarch64 to 0 for aarch32. You then need to make
sure you write a full correct cpsr. This is getting complicated
enough I'd rather leave it to a separate patch, though.

thanks
-- PMM