That is ok.
06.10.2015, 00:55, "Peter Maydell" <peter.mayd...@linaro.org>:
> On 2 October 2015 at 14:21, Sergey Sorokin <afaral...@yandex.ru> wrote:
>> It is incorrect to call arm_el_is_aa64() function for unimplemented EL.
>> This patch fixes several attempts to do so.
>>
>> Signed-off-by: Sergey Sorokin <afaral...@yandex.ru>
>> ---
>> target-arm/cpu.h | 8 +++++---
>> target-arm/helper.c | 15 +++++++++++++--
>> 2 files changed, 18 insertions(+), 5 deletions(-)
>>
>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>> index cc1578c..df456a5 100644
>> --- a/target-arm/cpu.h
>> +++ b/target-arm/cpu.h
>> @@ -1015,11 +1015,11 @@ static inline bool access_secure_reg(CPUARMState
>> *env)
>> */
>> #define A32_BANKED_CURRENT_REG_GET(_env, _regname) \
>> A32_BANKED_REG_GET((_env), _regname, \
>> - ((!arm_el_is_aa64((_env), 3) && arm_is_secure(_env))))
>> + (arm_is_secure(_env) && !arm_el_is_aa64((_env), 3)))
>>
>> #define A32_BANKED_CURRENT_REG_SET(_env, _regname, _val) \
>> A32_BANKED_REG_SET((_env), _regname, \
>> - ((!arm_el_is_aa64((_env), 3) && arm_is_secure(_env))), \
>> + (arm_is_secure(_env) && !arm_el_is_aa64((_env), 3)), \
>> (_val))
>>
>> void arm_cpu_list(FILE *f, fprintf_function cpu_fprintf);
>> @@ -1586,7 +1586,9 @@ static inline bool arm_excp_unmasked(CPUState *cs,
>> unsigned int excp_idx,
>> * interrupt.
>> */
>> if ((target_el > cur_el) && (target_el != 1)) {
>> - if (arm_el_is_aa64(env, 3) || ((scr || hcr) && (!secure))) {
>> + /* ARM_FEATURE_AARCH64 enabled means the higher EL is AArch64. */
>> + if (arm_feature(env, ARM_FEATURE_AARCH64) ||
>> + ((scr || hcr) && (!secure))) {
>> unmasked = 1;
>> }
>> }
>
> I know we discussed this one before, but having looked more carefully
> at it, I think the reason it's weird is that the original code is
> only correct if we're not implementing EL2.
>
> For instance (table D1-14 in the v8 ARMARM rev A.g)
> if we're in an all-AArch64 environment executing in Secure EL0
> then the interrupt mask is supposed to have an effect if the interrupt
> is targeting EL2. But the current code will always set 'unmasked' to 1 if
> EL3 is 64 bit.
>
> So I think what the code ought to read is:
>
> if ((target_el > cur_el) && (target_el != 1)) {
> /* Exceptions targeting a higher EL may not be maskable */
> if (arm_feature(env, ARM_FEATURE_AARCH64)) {
> /* 64-bit masking rules are simple: exceptions to EL3
> * can't be masked, and exceptions to EL2 can only be
> * masked from Secure state.
> */
> if (target_el == 3 || !secure) {
> unmasked = 1;
> }
> } else {
> /* The old 32-bit-only environment has a more complicated
> * masking setup.
> */
> if ((scr || hcr) && !secure) {
> unmasked = 1;
> }
> }
> }
>
> Except that then for the AArch64 case we've just calculated
> scr and hcr and then not needed them. I think most of the
> code calculating them ought to move into the else clause here.
>
> I'll write a patch for this and post it tomorrow.
>
> In the meantime, we can just make the comment say
>
> /* ARM_FEATURE_AARCH64 enabled means the highest EL is AArch64.
> * This code currently assumes that EL2 is not implemented
> * (and so that highest EL will be 3 and the target_el also 3).
> */
>
>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>> index 8367997..1f11dbd 100644
>> --- a/target-arm/helper.c
>> +++ b/target-arm/helper.c
>> @@ -5220,11 +5220,22 @@ 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);
>> + int rw;
>> int scr;
>> int hcr;
>> int target_el;
>> - int is64 = arm_el_is_aa64(env, 3);
>> + /* Is the higher EL AArch64? */
>
> "highest". I pointed this one out before...
>
>> + int is64 = arm_feature(env, ARM_FEATURE_AARCH64);
>> +
>> + /* If the highest EL is in AArch64 state, and EL3 is not implemented,
>> + * we must behave as if EL3 is implemented and is in AArch64 state.
>> + * Therefore we need appropriate RW bit.
>> + */
>
> I think we could put this comment into the else branch, and
> rephrase it a bit:
>
> + if (arm_feature(env, ARM_FEATURE_EL3)) {
> + rw = ((env->cp15.scr_el3 & SCR_RW) == SCR_RW);
> + } else {
> + /* Either EL2 is the highest EL (and so the EL2 register width
> + * is given by is64); or there is no EL2 or EL3, in which case
> + * the value of 'rw' does not affect the table lookup anyway.
> + */
> + rw = is64;
> + }
>
>> + if (arm_feature(env, ARM_FEATURE_EL3)) {
>> + rw = ((env->cp15.scr_el3 & SCR_RW) == SCR_RW);
>> + } else {
>> + rw = is64;
>> + }
>
> What I can do with this patch is:
> * make the minor comment tweaks above
> * apply the result to target-arm.next
>
> Is that OK?
>
>> switch (excp_idx) {
>> case EXCP_IRQ:
>> --
>> 1.9.3
>
> thanks
> -- PMM
