On 11/20/2015 08:47 AM, Peter Maydell wrote:
> On 9 November 2015 at 01:11, Michael Davidsaver <mdavidsa...@gmail.com> wrote:
>> Despite having the same notation, these bits
>> have completely different meaning than -AR.
>>
>> Add armv7m_excp_unmasked()
>> to calculate the currently runable exception priority
>> taking into account masks and active handlers.
>> Use this in conjunction with the pending exception
>> priority to determine if the pending exception
>> can interrupt execution.
>
> This function is used by code added in earlier patches in
> this series, so this patch needs to be moved earlier in the
> series, or those patches won't compile.
Should be fixed.
>> Signed-off-by: Michael Davidsaver <mdavidsa...@gmail.com>
>> ---
>> target-arm/cpu.c | 26 +++++++-------------------
>> target-arm/cpu.h | 27 ++++++++++++++++++++++++++-
>> 2 files changed, 33 insertions(+), 20 deletions(-)
>>
>> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
>> index be026bc..5d03117 100644
>> --- a/target-arm/cpu.c
>> +++ b/target-arm/cpu.c
>> @@ -173,6 +173,8 @@ static void arm_cpu_reset(CPUState *s)
>> uint32_t initial_pc; /* Loaded from 0x4 */
>> uint8_t *rom;
>>
>> + env->v7m.exception_prio = env->v7m.pending_prio = 0x100;
>> +
>> env->daif &= ~PSTATE_I;
>> rom = rom_ptr(0);
>> if (rom) {
>> @@ -301,29 +303,15 @@ static bool arm_v7m_cpu_exec_interrupt(CPUState *cs,
>> int interrupt_request)
>> {
>> CPUClass *cc = CPU_GET_CLASS(cs);
>> ARMCPU *cpu = ARM_CPU(cs);
>> - CPUARMState *env = &cpu->env;
>> bool ret = false;
>>
>> -
>> - if (interrupt_request & CPU_INTERRUPT_FIQ
>> - && !(env->daif & PSTATE_F)) {
>> - cs->exception_index = EXCP_FIQ;
>> - cc->do_interrupt(cs);
>> - ret = true;
>> - }
>> - /* ARMv7-M interrupt return works by loading a magic value
>> - * into the PC. On real hardware the load causes the
>> - * return to occur. The qemu implementation performs the
>> - * jump normally, then does the exception return when the
>> - * CPU tries to execute code at the magic address.
>> - * This will cause the magic PC value to be pushed to
>> - * the stack if an interrupt occurred at the wrong time.
>> - * We avoid this by disabling interrupts when
>> - * pc contains a magic address.
>
> This (removing this comment and the checks for the magic address)
> seem to be part of a separate change [probably the one in
> "armv7m: Undo armv7m.hack"] and shouldn't be in this patch.
Relocated.
>> + /* ARMv7-M interrupt masking works differently than -A or -R.
>> + * There is no FIQ/IRQ distinction.
>> + * Instead of masking interrupt sources, the I and F bits
>> + * (along with basepri) mask certain exception priority levels.
>> */
>> if (interrupt_request & CPU_INTERRUPT_HARD
>> - && !(env->daif & PSTATE_I)
>> - && (env->regs[15] < 0xfffffff0)) {
>> + && (armv7m_excp_unmasked(cpu) >= cpu->env.v7m.pending_prio)) {
>> cs->exception_index = EXCP_IRQ;
>> cc->do_interrupt(cs);
>> ret = true;
>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>> index c193fbb..29d89ce 100644
>> --- a/target-arm/cpu.h
>> +++ b/target-arm/cpu.h
>> @@ -1033,9 +1033,34 @@ void arm_cpu_list(FILE *f, fprintf_function
>> cpu_fprintf);
>> uint32_t arm_phys_excp_target_el(CPUState *cs, uint32_t excp_idx,
>> uint32_t cur_el, bool secure);
>>
>> +/* @returns highest (numerically lowest) unmasked exception priority
>> + */
>> +static inline
>> +int armv7m_excp_unmasked(ARMCPU *cpu)
>
> What this is really calculating is the current execution
> priority (running priority) of the CPU, so I think a better
> name would be armv7m_current_exec_priority() or
> armv7m_current_priority() or armv7m_running_priority() or similar.
Now armv7m_excp_running_prio()
>> +{
>> + CPUARMState *env = &cpu->env;
>> + int runnable;
>> +
>> + /* find highest (numerically lowest) priority which could
>> + * run based on masks
>> + */
>> + if (env->daif&PSTATE_F) { /* FAULTMASK */
>
> Style issue -- operands should have spaces around them.
>
>> + runnable = -2;
>
> These all seem to be off by one: FAULTMASK sets the
> running priority to -1, not -2, PRIMASK sets it to 0,
> not -1, and so on.
The off by one was due to my confusing runnable vs. running distinction, now
gone.
>> + } else if (env->daif&PSTATE_I) { /* PRIMASK */
>> + runnable = -1;
>> + } else if (env->v7m.basepri > 0) {
>> + /* BASEPRI==1 -> runnable==-1 (same as PRIMASK==1) */
>
> (applies to operands in comments too)
>
>> + runnable = env->v7m.basepri-2;
>
> Where is this - 2 from? Also, BASEPRI values honour the
> PRIGROUP setting. (Compare the ExecutionPriority pseudocode).
The off by two for BASEPRI was my mis-reading the definition.
>> + } else {
>> + runnable = 0x100; /* lower than any possible priority */
>> + }
>> + /* consider priority of active handler */
>> + return MIN(runnable, env->v7m.exception_prio-1);
>
> I don't think this -1 should be here.
It is gone.
>> +}
>> +
>> /* Interface between CPU and Interrupt controller. */
>> void armv7m_nvic_set_pending(void *opaque, int irq);
>> -int armv7m_nvic_acknowledge_irq(void *opaque);
>> +void armv7m_nvic_acknowledge_irq(void *opaque);
>> void armv7m_nvic_complete_irq(void *opaque, int irq);
>>
>> /* Interface for defining coprocessor registers.
>
> thanks
> -- PMM
>
