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

[Peter Maydell]

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).

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.)

It might be better to check this early and return failure to the
caller, since the guest can provoke it.

>> New global functions should all have properly formatted doc comments.
>
>
> I'll wok on it. Do you have a good citizen to point me to as a reference?

I usually use the extract/deposit functions in include/qemu/bitops.h
as a reference.

>>
>>> +
>>> +/*
>>> + * Start a give CPU. The CPU will start running at address "entry" with
>>> + * "context_id" in r0/x0. The CPU should start at exception level
>>> "target_el"
>>> + * and in mode aa64 or aa32 depending on "target_aa64"
>>> + */
>>> +int arm_set_cpu_on(uint64_t cpuid, uint64_t entry, uint64_t context_id,
>>> +                   int target_el, bool target_aa64);
>>
>> What's the return value? Do we start the CPU secure or nonsecure?
>> In AArch32, in which mode do we start? We need to either document
>> these things or let the caller specify.
>
> The idea is to start the CPU in the requested level/mode (or fail if it is
> not possible).
>
> I'll document return value/error.
>>
>>
>> (For PSCI we want to always start in non-secure, and we want to start
>> in the same execution state as the calling cpu, which I take to include
>> the mode. We won't ever try to start a cpu in EL3.)
>
>
> The PSCI call will take care of the requested level/mode.
>
> This is a more generic function that can be use for other purpose than PSCI.

Yes; I was just giving you the PSCI information to save you having to
go look up the spec. The generic function needs to be able to at least
provide sufficient functionality to implement the PSCI requirements.

thanks
-- PMM