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

[jcd]

----- 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?

> 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

> 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