Thanks for the comments, responses inline.
On 31 October 2014 06:56, Peter Maydell <peter.mayd...@linaro.org> wrote:
> On 30 October 2014 21:28, Greg Bellows <greg.bell...@linaro.org> wrote:
> > Adds a dedicated function and a lookup table for determining the target
> > exception level of IRQ and FIQ exceptions. The lookup table is taken
> from the
> > ARMv7 and ARMv8 specification exception routing tables.
> >
> > Signed-off-by: Greg Bellows <greg.bell...@linaro.org>
> >
> > ---
> >
> > v7 -> v8
> > - Added target EL lookup table
> > - Rework arm_phys_excp_target_el to use an EL lookup table rather than
> > conditionals.
> >
> > v5 -> v6
> > - Removed unneeded arm_phys_excp_target_el() function prototype.
> > - Removed unneeded arm_phys_excp_target_el() USER_ONLY function.
> > - Fixed up arm_phys_excp_target_el() function definition to be static.
> > - Globally replace Aarch# with AArch#
> >
> > v4 -> v5
> > - Simplify target EL function including removal of mode which was unused
> > - Merged with patch that plugs in the use of the function
> >
> > v3 -> v4
> > - Fixed arm_phys_excp_target_el() 0/0/0 case to return excp_mode when
> EL<2
> > rather than ABORT.
> > ---
> > target-arm/helper.c | 111
> ++++++++++++++++++++++++++++++++++++++++++----------
> > 1 file changed, 91 insertions(+), 20 deletions(-)
> >
> > diff --git a/target-arm/helper.c b/target-arm/helper.c
> > index c47487a..e610466 100644
> > --- a/target-arm/helper.c
> > +++ b/target-arm/helper.c
> > @@ -3761,6 +3761,94 @@ void switch_mode(CPUARMState *env, int mode)
> > env->spsr = env->banked_spsr[i];
> > }
> >
> > +/* Physical Interrupt Target EL Lookup Table
> > + *
> > + * [ From ARM ARM section G1.13.4 (Table G1-15) ]
> > + *
> > + * The below multi-dimensional table is used for looking up the target
> > + * exception level given numerous condition criteria. Specifically, the
> > + * target EL is based on SCR and HCR routing controls as well as the
> > + * currently executing EL and secure state.
> > + *
> > + * Dimensions:
> > + * target_el_table[2][2][2][2][2][4]
> > + * | | | | | +--- Current EL
> > + * | | | | +------ Non-secure(0)/Secure(1)
> > + * | | | +--------- HCR mask override
> > + * | | +------------ SCR exec state control
> > + * | +--------------- SCR mask override
> > + * +------------------ 32-bit(0)/64-bit(1) EL3
> > + *
> > + * The table values are as such:
> > + * 0-3 = EL0-EL3
> > + * -1 = Cannot occur
> > + *
> > + * In the case of exceptions not being taken, EL1 is returned. These
> cases
> > + * will be caught by the checks for target being >= current.
>
> This could use rephrasing to make it clearer why returning 1 is ok:
>
> "The ARM ARM tables include some entries indicating 'exception not taken'.
> These are for the cases where we are currently at EL3 and the
> exception is not routed to EL3 by the SCR, or where we are currently
> at EL2 and the exception is not routed to EL3 or EL2. We can therefore
> put '1' in those entries in our array, and rely on the check for
> "target EL >= current EL" in the caller to ensure that the exception
> is not taken."
>
>
Added to v9.
> > + *
> > + * SCR HCR
> > + * 64 EA AMO From
> > + * BIT IRQ IMO Non-secure Secure
> > + * EL3 FIQ RW FMO EL0 EL1 EL2 EL3 EL0 EL1 EL2 EL3
> > + */
> > +const int8_t target_el_table[2][2][2][2][2][4] = {
> > + {{{{/* 0 0 0 0 */{ 1, 1, 2, -1 },{ 3, 3, -1, 3 },},
> > + {/* 0 0 0 1 */{ 2, 2, 2, -1 },{ 3, 3, -1, 3 },},},
> > + {{/* 0 0 1 0 */{ 1, 1, 2, -1 },{ 3, 3, -1, 3 },},
> > + {/* 0 0 1 1 */{ 2, 2, 2, -1 },{ 3, 3, -1, 3 },},},},
> > + {{{/* 0 1 0 0 */{ 3, 3, 3, -1 },{ 3, 3, -1, 3 },},
> > + {/* 0 1 0 1 */{ 3, 3, 3, -1 },{ 3, 3, -1, 3 },},},
> > + {{/* 0 1 1 0 */{ 3, 3, 3, -1 },{ 3, 3, -1, 3 },},
> > + {/* 0 1 1 1 */{ 3, 3, 3, -1 },{ 3, 3, -1, 3 },},},},},
>
> Your entries for 32-bit EL3, exception from Secure EL1 should all
> read "-1", not "3" -- it's not possible to have current_el be 1
> in this case (this is why table G1-15 has a "-" in those entries).
>
>
Hmmm... I copied directly from the table and remember having two columns of
-1, so I'm not sure where the 3s came from, but you are right. Fixed in v9.
> > + {{{{/* 1 0 0 0 */{ 1, 1, 2, -1 },{ 1, 1, -1, 1 },},
> > + {/* 1 0 0 1 */{ 2, 2, 2, -1 },{ 1, 1, -1, 1 },},},
> > + {{/* 1 0 1 0 */{ 1, 1, 1, -1 },{ 1, 1, -1, 1 },},
> > + {/* 1 0 1 1 */{ 2, 2, 2, -1 },{ 1, 1, -1, 1 },},},},
> > + {{{/* 1 1 0 0 */{ 3, 3, 3, -1 },{ 3, 3, -1, 3 },},
> > + {/* 1 1 0 1 */{ 3, 3, 3, -1 },{ 3, 3, -1, 3 },},},
> > + {{/* 1 1 1 0 */{ 3, 3, 3, -1 },{ 3, 3, -1, 3 },},
> > + {/* 1 1 1 1 */{ 3, 3, 3, -1 },{ 3, 3, -1, 3 },},},},},
> > +};
> > +
> > +/*
> > + * 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)
> > +{
> > + CPUARMState *env = cs->env_ptr;
> > + uint32_t rw = ((env->cp15.scr_el3 & SCR_RW) == SCR_RW);
> > + uint32_t scr;
> > + uint32_t hcr;
> > + uint32_t target_el;
> > + uint32_t is64 = arm_el_is_aa64(env, 3);
>
> You can just make these all "int".
>
Fixed in v9.
>
> > +
> > + switch (excp_idx) {
> > + case EXCP_IRQ:
> > + scr = ((env->cp15.scr_el3 & SCR_IRQ) == SCR_IRQ);
> > + hcr = ((env->cp15.hcr_el2 & HCR_IMO) == HCR_IMO);
> > + break;
> > + case EXCP_FIQ:
> > + scr = ((env->cp15.scr_el3 & SCR_FIQ) == SCR_FIQ);
> > + hcr = ((env->cp15.hcr_el2 & HCR_FMO) == HCR_FMO);
> > + break;
> > + default:
> > + scr = ((env->cp15.scr_el3 & SCR_EA) == SCR_EA);
> > + hcr = ((env->cp15.hcr_el2 & HCR_AMO) == HCR_AMO);
> > + break;
> > + };
> > +
> > + /* If HCR.TGE is set then HCR is treated as being 1 */
> > + hcr |= ((env->cp15.hcr_el2 & HCR_TGE) == HCR_TGE);
> > +
> > + /* Perform a table-lookup for the target EL given the current state
> */
> > + target_el = target_el_table[is64][scr][rw][hcr][secure][cur_el];
> > +
> > + assert(target_el > 0);
> > +
> > + return target_el;
> > +}
> > +
> > /*
> > * Determine the target EL for a given exception type.
> > */
> > @@ -3769,14 +3857,8 @@ 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;
> > - /* FIXME: Use actual secure state. */
> > - bool secure = false;
> > -
> > - if (!env->aarch64) {
> > - /* TODO: Add EL2 and 3 exception handling for AArch32. */
> > - return 1;
> > - }
> > + unsigned int target_el = 1;
>
> Do you actually need to initialize this?
>
I think this was leftover from before, not needed now. Fixed in v9.
>
> > + bool secure = arm_is_secure(env);
> >
> > switch (excp_idx) {
> > case EXCP_HVC:
> > @@ -3788,19 +3870,8 @@ unsigned int arm_excp_target_el(CPUState *cs,
> unsigned int excp_idx)
> > break;
> > case EXCP_FIQ:
> > case EXCP_IRQ:
> > - {
> > - const uint64_t hcr_mask = excp_idx == EXCP_FIQ ? HCR_FMO :
> HCR_IMO;
> > - const uint32_t scr_mask = excp_idx == EXCP_FIQ ? SCR_FIQ :
> SCR_IRQ;
> > -
> > - target_el = 1;
> > - if (!secure && (env->cp15.hcr_el2 & hcr_mask)) {
> > - target_el = 2;
> > - }
> > - if (env->cp15.scr_el3 & scr_mask) {
> > - target_el = 3;
> > - }
> > + target_el = arm_phys_excp_target_el(cs, excp_idx, cur_el,
> secure);
> > break;
> > - }
> > case EXCP_VIRQ:
> > case EXCP_VFIQ:
> > target_el = 1;
> > --
> > 1.8.3.2
> >
>
> Thanks for adding the table-driven code: I found this much easier
> to review against the ARM ARM. If you make those minor changes I
> mention above then you can add my reviewed-by tag.
>
> -- PMM
>
