----- Le 24 Mar 16, à 11:46, Peter Maydell [email protected] a écrit :
> On 24 March 2016 at 07:10, Jean-Christophe DUBOIS <[email protected]> > wrote: > > Le 23/03/2016 16:24, Peter Maydell a écrit : > >> On 19 March 2016 at 20:59, Jean-Christophe Dubois <[email protected]> > >> 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
