| Message ID | 1427483446-31900-4-git-send-email-greg.bellows@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 27 March 2015 at 19:10, Greg Bellows <greg.bellows@linaro.org> wrote: > Updated the interrupt handling to utilize and report through the target EL > exception field. This includes consolidating and cleaning up code where > needed. Target EL is now calculated once in arm_cpu_exec_interrupt() and > do_interrupt was updated to use the target_el exception field. The > necessary code from arm_excp_target_el() was merged in where needed and the > function removed. > --- a/target-arm/helper-a64.c > +++ b/target-arm/helper-a64.c > @@ -463,7 +463,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs) > { > ARMCPU *cpu = ARM_CPU(cs); > CPUARMState *env = &cpu->env; > - unsigned int new_el = arm_excp_target_el(cs, cs->exception_index); > + unsigned int new_el = MAX(env->exception.target_el, 1); Surely we should never be able to get here with target_el zero? Rest of the patch looks OK. -- PMM
On Thu, Apr 16, 2015 at 12:52 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 27 March 2015 at 19:10, Greg Bellows <greg.bellows@linaro.org> wrote: > > Updated the interrupt handling to utilize and report through the target > EL > > exception field. This includes consolidating and cleaning up code where > > needed. Target EL is now calculated once in arm_cpu_exec_interrupt() and > > do_interrupt was updated to use the target_el exception field. The > > necessary code from arm_excp_target_el() was merged in where needed and > the > > function removed. > > > --- a/target-arm/helper-a64.c > > +++ b/target-arm/helper-a64.c > > @@ -463,7 +463,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs) > > { > > ARMCPU *cpu = ARM_CPU(cs); > > CPUARMState *env = &cpu->env; > > - unsigned int new_el = arm_excp_target_el(cs, cs->exception_index); > > + unsigned int new_el = MAX(env->exception.target_el, 1); > > Surely we should never be able to get here with target_el zero? > Ideally that would be true and I wondered that myself so I took out the the MAX safety net in arm_excp_target_el() and later hit the assert in aarch64_banked_spsr_index() because new_el was 0. This is why I preserved the MAX behavior everywhere because just like the original code, there are cases where current_el is 0. I figured this was the safest alternative as it would catch all the cases where we were not specifying the target EL. > Rest of the patch looks OK. > > -- PMM >
On 16 April 2015 at 22:03, Greg Bellows <greg.bellows@linaro.org> wrote: > > > On Thu, Apr 16, 2015 at 12:52 PM, Peter Maydell <peter.maydell@linaro.org> > wrote: >> >> On 27 March 2015 at 19:10, Greg Bellows <greg.bellows@linaro.org> wrote: >> > Updated the interrupt handling to utilize and report through the target >> > EL >> > exception field. This includes consolidating and cleaning up code where >> > needed. Target EL is now calculated once in arm_cpu_exec_interrupt() and >> > do_interrupt was updated to use the target_el exception field. The >> > necessary code from arm_excp_target_el() was merged in where needed and >> > the >> > function removed. >> >> > --- a/target-arm/helper-a64.c >> > +++ b/target-arm/helper-a64.c >> > @@ -463,7 +463,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs) >> > { >> > ARMCPU *cpu = ARM_CPU(cs); >> > CPUARMState *env = &cpu->env; >> > - unsigned int new_el = arm_excp_target_el(cs, cs->exception_index); >> > + unsigned int new_el = MAX(env->exception.target_el, 1); >> >> Surely we should never be able to get here with target_el zero? > > > Ideally that would be true and I wondered that myself so I took out the the > MAX safety net in arm_excp_target_el() and later hit the assert in > aarch64_banked_spsr_index() because new_el was 0. This is why I preserved > the MAX behavior everywhere because just like the original code, there are > cases where current_el is 0. I guessed we might assert. But this is bad, because it suggests there's somewhere that's not setting target_el at all, which in turn probably means that after we've taken one exception, the next time around we'll just go to whichever target_el the first one set up... There should be a definite rule about who has to set up target_el, so when we're reviewing or changing code we can tell whether it's been done correctly. (Compare the rule for exception.syndrome, which is "must be set up before the exception is raised, unless excp_is_internal(excp)", which we have some assertions to try to enforce. There's also a less-documented rule for exception.vaddress, which is "must be set for EXCP_PREFETCH_ABORT and EXCP_DATA_ABORT".) -- PMM
diff --git a/target-arm/cpu.c b/target-arm/cpu.c index 986f04c..4aa71a7 100644 --- a/target-arm/cpu.c +++ b/target-arm/cpu.c @@ -206,31 +206,52 @@ static void arm_cpu_reset(CPUState *s) bool arm_cpu_exec_interrupt(CPUState *cs, int interrupt_request) { CPUClass *cc = CPU_GET_CLASS(cs); + CPUARMState *env = cs->env_ptr; + uint32_t cur_el = arm_current_el(env); + bool secure = arm_is_secure(env); + uint32_t target_el; + uint32_t excp_idx; bool ret = false; - if (interrupt_request & CPU_INTERRUPT_FIQ - && arm_excp_unmasked(cs, EXCP_FIQ)) { - cs->exception_index = EXCP_FIQ; - cc->do_interrupt(cs); - ret = true; + if (interrupt_request & CPU_INTERRUPT_FIQ) { + excp_idx = EXCP_FIQ; + target_el = arm_phys_excp_target_el(cs, excp_idx, cur_el, secure); + if (arm_excp_unmasked(cs, excp_idx, target_el)) { + cs->exception_index = excp_idx; + env->exception.target_el = target_el; + cc->do_interrupt(cs); + ret = true; + } } - if (interrupt_request & CPU_INTERRUPT_HARD - && arm_excp_unmasked(cs, EXCP_IRQ)) { - cs->exception_index = EXCP_IRQ; - cc->do_interrupt(cs); - ret = true; + if (interrupt_request & CPU_INTERRUPT_HARD) { + excp_idx = EXCP_IRQ; + target_el = arm_phys_excp_target_el(cs, excp_idx, cur_el, secure); + if (arm_excp_unmasked(cs, excp_idx, target_el)) { + cs->exception_index = excp_idx; + env->exception.target_el = target_el; + cc->do_interrupt(cs); + ret = true; + } } - if (interrupt_request & CPU_INTERRUPT_VIRQ - && arm_excp_unmasked(cs, EXCP_VIRQ)) { - cs->exception_index = EXCP_VIRQ; - cc->do_interrupt(cs); - ret = true; + if (interrupt_request & CPU_INTERRUPT_VIRQ) { + excp_idx = EXCP_VIRQ; + target_el = 1; + if (arm_excp_unmasked(cs, excp_idx, target_el)) { + cs->exception_index = excp_idx; + env->exception.target_el = target_el; + cc->do_interrupt(cs); + ret = true; + } } - if (interrupt_request & CPU_INTERRUPT_VFIQ - && arm_excp_unmasked(cs, EXCP_VFIQ)) { - cs->exception_index = EXCP_VFIQ; - cc->do_interrupt(cs); - ret = true; + if (interrupt_request & CPU_INTERRUPT_VFIQ) { + excp_idx = EXCP_VFIQ; + target_el = 1; + if (arm_excp_unmasked(cs, excp_idx, target_el)) { + cs->exception_index = excp_idx; + env->exception.target_el = target_el; + cc->do_interrupt(cs); + ret = true; + } } return ret; diff --git a/target-arm/cpu.h b/target-arm/cpu.h index 0b232ba..2178a1f 100644 --- a/target-arm/cpu.h +++ b/target-arm/cpu.h @@ -1007,7 +1007,8 @@ static inline bool access_secure_reg(CPUARMState *env) (_val)) void arm_cpu_list(FILE *f, fprintf_function cpu_fprintf); -unsigned int arm_excp_target_el(CPUState *cs, unsigned int excp_idx); +uint32_t arm_phys_excp_target_el(CPUState *cs, uint32_t excp_idx, + uint32_t cur_el, bool secure); /* Interface between CPU and Interrupt controller. */ void armv7m_nvic_set_pending(void *opaque, int irq); @@ -1489,11 +1490,11 @@ bool write_cpustate_to_list(ARMCPU *cpu); # define TARGET_VIRT_ADDR_SPACE_BITS 32 #endif -static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx) +static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx, + unsigned int target_el) { CPUARMState *env = cs->env_ptr; unsigned int cur_el = arm_current_el(env); - unsigned int target_el = arm_excp_target_el(cs, excp_idx); bool secure = arm_is_secure(env); uint32_t scr; uint32_t hcr; diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c index 7e0d038..07f9799 100644 --- a/target-arm/helper-a64.c +++ b/target-arm/helper-a64.c @@ -463,7 +463,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs) { ARMCPU *cpu = ARM_CPU(cs); CPUARMState *env = &cpu->env; - unsigned int new_el = arm_excp_target_el(cs, cs->exception_index); + unsigned int new_el = MAX(env->exception.target_el, 1); target_ulong addr = env->cp15.vbar_el[new_el]; unsigned int new_mode = aarch64_pstate_mode(new_el, true); diff --git a/target-arm/helper.c b/target-arm/helper.c index 10886c5..95383d5 100644 --- a/target-arm/helper.c +++ b/target-arm/helper.c @@ -4091,7 +4091,8 @@ uint32_t HELPER(get_r13_banked)(CPUARMState *env, uint32_t mode) return 0; } -unsigned int arm_excp_target_el(CPUState *cs, unsigned int excp_idx) +uint32_t arm_phys_excp_target_el(CPUState *cs, uint32_t excp_idx, + uint32_t cur_el, bool secure) { return 1; } @@ -4215,8 +4216,8 @@ const int8_t target_el_table[2][2][2][2][2][4] = { /* * Determine the target EL for physical exceptions */ -static inline uint32_t arm_phys_excp_target_el(CPUState *cs, uint32_t excp_idx, - uint32_t cur_el, bool secure) +uint32_t arm_phys_excp_target_el(CPUState *cs, uint32_t excp_idx, + uint32_t cur_el, bool secure) { CPUARMState *env = cs->env_ptr; int rw = ((env->cp15.scr_el3 & SCR_RW) == SCR_RW); @@ -4251,40 +4252,6 @@ static inline uint32_t arm_phys_excp_target_el(CPUState *cs, uint32_t excp_idx, return target_el; } -/* - * Determine the target EL for a given exception type. - */ -unsigned int arm_excp_target_el(CPUState *cs, unsigned int excp_idx) -{ - ARMCPU *cpu = ARM_CPU(cs); - CPUARMState *env = &cpu->env; - unsigned int cur_el = arm_current_el(env); - unsigned int target_el; - bool secure = arm_is_secure(env); - - switch (excp_idx) { - case EXCP_HVC: - case EXCP_HYP_TRAP: - target_el = 2; - break; - case EXCP_SMC: - target_el = 3; - break; - case EXCP_FIQ: - case EXCP_IRQ: - target_el = arm_phys_excp_target_el(cs, excp_idx, cur_el, secure); - break; - case EXCP_VIRQ: - case EXCP_VFIQ: - target_el = 1; - break; - default: - target_el = MAX(cur_el, 1); - break; - } - return target_el; -} - static void v7m_push(CPUARMState *env, uint32_t val) { CPUState *cs = CPU(arm_env_get_cpu(env));
Updated the interrupt handling to utilize and report through the target EL exception field. This includes consolidating and cleaning up code where needed. Target EL is now calculated once in arm_cpu_exec_interrupt() and do_interrupt was updated to use the target_el exception field. The necessary code from arm_excp_target_el() was merged in where needed and the function removed. Signed-off-by: Greg Bellows <greg.bellows@linaro.org> --- target-arm/cpu.c | 61 +++++++++++++++++++++++++++++++++---------------- target-arm/cpu.h | 7 +++--- target-arm/helper-a64.c | 2 +- target-arm/helper.c | 41 ++++----------------------------- 4 files changed, 50 insertions(+), 61 deletions(-)