Re: [Qemu-devel] [PATCH v1 14/16] target-arm: A64: Emulate the SMC insn

Re: [Qemu-devel] [PATCH v1 14/16] target-arm: A64: Emulate the SMC insn

[Edgar E. Iglesias]

On Fri, May 30, 2014 at 05:28:29PM +1000, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> 
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  target-arm/cpu.h           |  1 +
>  target-arm/helper-a64.c    |  1 +
>  target-arm/helper.c        |  6 ++++++
>  target-arm/helper.h        |  1 +
>  target-arm/internals.h     |  6 ++++++
>  target-arm/op_helper.c     | 11 +++++++++++
>  target-arm/translate-a64.c | 10 ++++++++++
>  7 files changed, 36 insertions(+)
> 
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 1a26ed4..b3631f2 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -52,6 +52,7 @@
>  #define EXCP_KERNEL_TRAP     9   /* Jumped to kernel code page.  */
>  #define EXCP_STREX          10
>  #define EXCP_HVC            11   /* HyperVisor Call */
> +#define EXCP_SMC            12   /* Secure Monitor Call */
>  
>  #define ARMV7M_EXCP_RESET   1
>  #define ARMV7M_EXCP_NMI     2
> diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
> index 974fa66..3894a6f 100644
> --- a/target-arm/helper-a64.c
> +++ b/target-arm/helper-a64.c
> @@ -476,6 +476,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
>      case EXCP_UDEF:
>      case EXCP_SWI:
>      case EXCP_HVC:
> +    case EXCP_SMC:
>          env->cp15.esr_el[new_el] = env->exception.syndrome;
>          break;
>      case EXCP_IRQ:
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 5b2070c..35091ea 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -3298,6 +3298,12 @@ unsigned int arm_excp_target_el(CPUState *cs, unsigned int excp_idx)
>      case EXCP_HVC:
>          target_el = MAX(target_el, 2);
>          break;
> +    case EXCP_SMC:
> +        target_el = 3;
> +        if (env->cp15.hcr_el2 & HCR_TSC) {

The HCR.TSC routing of SMC to EL2 only applies in NS EL1 mode,
will fix for v2 as:

    case EXCP_SMC:
        target_el = 3;
        if (!secure && cur_el == 1 && (env->cp15.hcr_el2 & HCR_TSC)) {
            target_el = 2;
        }
        break;


> +            target_el = 2;
> +        }
> +        break;
>      }
>      return target_el;
>  }
> diff --git a/target-arm/helper.h b/target-arm/helper.h
> index fb711be..6c3d84d 100644
> --- a/target-arm/helper.h
> +++ b/target-arm/helper.h
> @@ -51,6 +51,7 @@ DEF_HELPER_3(exception_with_syndrome, void, env, i32, i32)
>  DEF_HELPER_1(wfi, void, env)
>  DEF_HELPER_1(wfe, void, env)
>  DEF_HELPER_2(hvc, void, env, i32)
> +DEF_HELPER_2(smc, void, env, i32)
>  
>  DEF_HELPER_3(cpsr_write, void, env, i32, i32)
>  DEF_HELPER_1(cpsr_read, i32, env)
> diff --git a/target-arm/internals.h b/target-arm/internals.h
> index b08381c..e50a68e 100644
> --- a/target-arm/internals.h
> +++ b/target-arm/internals.h
> @@ -54,6 +54,7 @@ static const char * const excnames[] = {
>      [EXCP_KERNEL_TRAP] = "QEMU intercept of kernel commpage",
>      [EXCP_STREX] = "QEMU intercept of STREX",
>      [EXCP_HVC] = "Hypervisor Call",
> +    [EXCP_SMC] = "Secure Monitor Call",
>  };
>  
>  static inline void arm_log_exception(int idx)
> @@ -210,6 +211,11 @@ static inline uint32_t syn_aa64_hvc(uint32_t imm16)
>      return (EC_AA64_HVC << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff);
>  }
>  
> +static inline uint32_t syn_aa64_smc(uint32_t imm16)
> +{
> +    return (EC_AA64_SMC << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff);
> +}
> +
>  static inline uint32_t syn_aa32_svc(uint32_t imm16, bool is_thumb)
>  {
>      return (EC_AA32_SVC << ARM_EL_EC_SHIFT) | (imm16 & 0xffff)
> diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> index 6bf34b0..6840828 100644
> --- a/target-arm/op_helper.c
> +++ b/target-arm/op_helper.c
> @@ -405,6 +405,17 @@ void HELPER(hvc)(CPUARMState *env, uint32_t syndrome)
>      raise_exception(env, EXCP_HVC);
>  }
>  
> +void HELPER(smc)(CPUARMState *env, uint32_t syndrome)
> +{
> +    /* We've already checked that EL3 exists at translation time.  */
> +    if (env->cp15.scr_el3 & SCR_SMD) {
> +        env->exception.syndrome = syn_uncategorized();
> +        raise_exception(env, EXCP_UDEF);
> +    }
> +    env->exception.syndrome = syndrome;
> +    raise_exception(env, EXCP_SMC);
> +}
> +
>  void HELPER(exception_return)(CPUARMState *env)
>  {
>      int cur_el = arm_current_pl(env);
> diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
> index 3981ee1..a02fe06 100644
> --- a/target-arm/translate-a64.c
> +++ b/target-arm/translate-a64.c
> @@ -1451,6 +1451,16 @@ static void disas_exc(DisasContext *s, uint32_t insn)
>              gen_helper_hvc(cpu_env, tmp);
>              tcg_temp_free_i32(tmp);
>              break;
> +        case 3:
> +            if (!arm_dc_feature(s, ARM_FEATURE_EL3) || s->current_pl == 0) {
> +                unallocated_encoding(s);
> +                break;
> +            }
> +            tmp = tcg_const_i32(syn_aa64_smc(imm16));
> +            gen_a64_set_pc_im(s->pc);
> +            gen_helper_smc(cpu_env, tmp);
> +            tcg_temp_free_i32(tmp);
> +            break;
>          default:
>              unallocated_encoding(s);
>              break;
> -- 
> 1.8.3.2
> 

Re: [Qemu-devel] [PATCH v1 14/16] target-arm: A64: Emulate the SMC insn

[Greg Bellows]

[-- Attachment #1: Type: text/plain, Size: 7935 bytes --]

On 30 May 2014 22:49, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:

> On Fri, May 30, 2014 at 11:50:23AM -0500, Greg Bellows wrote:
> > On 30 May 2014 02:28, Edgar E. Iglesias <edgar.iglesias@gmail.com>
> wrote:
> >
> > > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> > >
> > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > > ---
> > >  target-arm/cpu.h           |  1 +
> > >  target-arm/helper-a64.c    |  1 +
> > >  target-arm/helper.c        |  6 ++++++
> > >  target-arm/helper.h        |  1 +
> > >  target-arm/internals.h     |  6 ++++++
> > >  target-arm/op_helper.c     | 11 +++++++++++
> > >  target-arm/translate-a64.c | 10 ++++++++++
> > >  7 files changed, 36 insertions(+)
> > >
> > > diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> > > index 1a26ed4..b3631f2 100644
> > > --- a/target-arm/cpu.h
> > > +++ b/target-arm/cpu.h
> > > @@ -52,6 +52,7 @@
> > >  #define EXCP_KERNEL_TRAP     9   /* Jumped to kernel code page.  */
> > >  #define EXCP_STREX          10
> > >  #define EXCP_HVC            11   /* HyperVisor Call */
> > > +#define EXCP_SMC            12   /* Secure Monitor Call */
> > >
> > >  #define ARMV7M_EXCP_RESET   1
> > >  #define ARMV7M_EXCP_NMI     2
> > > diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
> > > index 974fa66..3894a6f 100644
> > > --- a/target-arm/helper-a64.c
> > > +++ b/target-arm/helper-a64.c
> > > @@ -476,6 +476,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
> > >      case EXCP_UDEF:
> > >      case EXCP_SWI:
> > >      case EXCP_HVC:
> > > +    case EXCP_SMC:
> > >          env->cp15.esr_el[new_el] = env->exception.syndrome;
> > >          break;
> > >      case EXCP_IRQ:
> > > diff --git a/target-arm/helper.c b/target-arm/helper.c
> > > index 5b2070c..35091ea 100644
> > > --- a/target-arm/helper.c
> > > +++ b/target-arm/helper.c
> > > @@ -3298,6 +3298,12 @@ unsigned int arm_excp_target_el(CPUState *cs,
> > > unsigned int excp_idx)
> > >      case EXCP_HVC:
> > >          target_el = MAX(target_el, 2);
> > >          break;
> > > +    case EXCP_SMC:
> > > +        target_el = 3;
> > > +        if (env->cp15.hcr_el2 & HCR_TSC) {
> > > +            target_el = 2;
> > > +        }
> > > +        break;
> > >      }
> > >      return target_el;
> > >  }
> > > diff --git a/target-arm/helper.h b/target-arm/helper.h
> > > index fb711be..6c3d84d 100644
> > > --- a/target-arm/helper.h
> > > +++ b/target-arm/helper.h
> > > @@ -51,6 +51,7 @@ DEF_HELPER_3(exception_with_syndrome, void, env, i32,
> > > i32)
> > >  DEF_HELPER_1(wfi, void, env)
> > >  DEF_HELPER_1(wfe, void, env)
> > >  DEF_HELPER_2(hvc, void, env, i32)
> > > +DEF_HELPER_2(smc, void, env, i32)
> > >
> > >  DEF_HELPER_3(cpsr_write, void, env, i32, i32)
> > >  DEF_HELPER_1(cpsr_read, i32, env)
> > > diff --git a/target-arm/internals.h b/target-arm/internals.h
> > > index b08381c..e50a68e 100644
> > > --- a/target-arm/internals.h
> > > +++ b/target-arm/internals.h
> > > @@ -54,6 +54,7 @@ static const char * const excnames[] = {
> > >      [EXCP_KERNEL_TRAP] = "QEMU intercept of kernel commpage",
> > >      [EXCP_STREX] = "QEMU intercept of STREX",
> > >      [EXCP_HVC] = "Hypervisor Call",
> > > +    [EXCP_SMC] = "Secure Monitor Call",
> > >  };
> > >
> > >  static inline void arm_log_exception(int idx)
> > > @@ -210,6 +211,11 @@ static inline uint32_t syn_aa64_hvc(uint32_t
> imm16)
> > >      return (EC_AA64_HVC << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 &
> > > 0xffff);
> > >  }
> > >
> > > +static inline uint32_t syn_aa64_smc(uint32_t imm16)
> > > +{
> > > +    return (EC_AA64_SMC << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 &
> > > 0xffff);
> > > +}
> > > +
> > >  static inline uint32_t syn_aa32_svc(uint32_t imm16, bool is_thumb)
> > >  {
> > >      return (EC_AA32_SVC << ARM_EL_EC_SHIFT) | (imm16 & 0xffff)
> > > diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> > > index 6bf34b0..6840828 100644
> > > --- a/target-arm/op_helper.c
> > > +++ b/target-arm/op_helper.c
> > > @@ -405,6 +405,17 @@ void HELPER(hvc)(CPUARMState *env, uint32_t
> syndrome)
> > >      raise_exception(env, EXCP_HVC);
> > >  }
> > >
> > > +void HELPER(smc)(CPUARMState *env, uint32_t syndrome)
> > > +{
> > > +    /* We've already checked that EL3 exists at translation time.  */
> > > +    if (env->cp15.scr_el3 & SCR_SMD) {
> > >
> >
> > In ARMv7 isn't this is this only a valid check if the virtualization
> > extension is present?  If so, should we be checking for the virt
> extension
> > on AArch32 prior to raising undefined?
>
> My understanding is that on AArch32 SCR.SMD only applies
> to NS mode and only if HCR.TSC=0 (always the case if EL2 does not exist but
> can also be zero even if EL2 exists).
> So it's not dependant on virtual extensions/EL2 non-existance? Am I missing
> something?
>

I'm not sure how we handle this as the ARMv7 spec states the following for
the SCD bit:

"*SCD, bit[7], when implementation includes the Virtualization Extensions*"


This is not worded the same in the ARMv8 spec, possibly because the
extensions are assumed to always be present?  The QQMU code still supports
the notion of features in which case sometimes are extensions.


>
> I'm not adding AArch32 SMC here but am happy to try to get it right
> in preparation for the AArch32 support.
>

Sure, I suspect that Fabian's commits on top of yours will close any
AArch32 gaps.


>
> The patch misses that the HCR.TSC routing of SMC to EL2 takes priority over
> SMD for NS EL1. For AArch32, I need to check for non-secure state
> aswell. I'll update those for v2.
>
> Need to run a few tests but I'm considering something along these lines:
>
> void HELPER(smc)(CPUARMState *env, uint32_t syndrome)
> {
>     int cur_el = arm_current_pl(env);
>     /* FIXME: Use real secure state.  */
>     bool secure = false;
>     bool smd = env->cp15.scr_el3 & SCR_SMD;
>     /* On AArch32, SMD only applies to NS mode.  */
>     bool udef = is_a64(env) ? smd : !secure && smd;
>

Is it possible to get in here at EL0?  A quick check, did not turn up a
check for this in the code calling this helper, but I may have missed it.


>
>     /* In NS EL1, HCR controlled routing to EL2 has priority over SMD.  */
>     if (!secure && cur_el == 1 && (env->cp15.hcr_el2 & HCR_TSC)) {
>         udef = false;
>     }
>
>     /* We've already checked that EL3 exists at translation time.  */
>     if (udef) {
>         env->exception.syndrome = syn_uncategorized();
>         raise_exception(env, EXCP_UDEF);
>     }
>     env->exception.syndrome = syndrome;
>     raise_exception(env, EXCP_SMC);
> }
>
> Thanks for the comments,
> Edgar
>
>
> >
> >
> > > +        env->exception.syndrome = syn_uncategorized();
> > > +        raise_exception(env, EXCP_UDEF);
> > > +    }
> > > +    env->exception.syndrome = syndrome;
> > > +    raise_exception(env, EXCP_SMC);
> > > +}
> > > +
> > >  void HELPER(exception_return)(CPUARMState *env)
> > >  {
> > >      int cur_el = arm_current_pl(env);
> > > diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
> > > index 3981ee1..a02fe06 100644
> > > --- a/target-arm/translate-a64.c
> > > +++ b/target-arm/translate-a64.c
> > > @@ -1451,6 +1451,16 @@ static void disas_exc(DisasContext *s, uint32_t
> > > insn)
> > >              gen_helper_hvc(cpu_env, tmp);
> > >              tcg_temp_free_i32(tmp);
> > >              break;
> > > +        case 3:
> > > +            if (!arm_dc_feature(s, ARM_FEATURE_EL3) || s->current_pl
> ==
> > > 0) {
> > > +                unallocated_encoding(s);
> > > +                break;
> > > +            }
> > > +            tmp = tcg_const_i32(syn_aa64_smc(imm16));
> > > +            gen_a64_set_pc_im(s->pc);
> > > +            gen_helper_smc(cpu_env, tmp);
> > > +            tcg_temp_free_i32(tmp);
> > > +            break;
> > >          default:
> > >              unallocated_encoding(s);
> > >              break;
> > > --
> > > 1.8.3.2
> > >
> > >
>
>

[-- Attachment #2: Type: text/html, Size: 11501 bytes --]

Re: [Qemu-devel] [PATCH v1 14/16] target-arm: A64: Emulate the SMC insn

[Edgar E. Iglesias]

On Mon, Jun 02, 2014 at 11:12:11AM -0500, Greg Bellows wrote:
> On 30 May 2014 22:49, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> 
> > On Fri, May 30, 2014 at 11:50:23AM -0500, Greg Bellows wrote:
> > > On 30 May 2014 02:28, Edgar E. Iglesias <edgar.iglesias@gmail.com>
> > wrote:
> > >
> > > > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> > > >
> > > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > > > ---
> > > >  target-arm/cpu.h           |  1 +
> > > >  target-arm/helper-a64.c    |  1 +
> > > >  target-arm/helper.c        |  6 ++++++
> > > >  target-arm/helper.h        |  1 +
> > > >  target-arm/internals.h     |  6 ++++++
> > > >  target-arm/op_helper.c     | 11 +++++++++++
> > > >  target-arm/translate-a64.c | 10 ++++++++++
> > > >  7 files changed, 36 insertions(+)
> > > >
> > > > diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> > > > index 1a26ed4..b3631f2 100644
> > > > --- a/target-arm/cpu.h
> > > > +++ b/target-arm/cpu.h
> > > > @@ -52,6 +52,7 @@
> > > >  #define EXCP_KERNEL_TRAP     9   /* Jumped to kernel code page.  */
> > > >  #define EXCP_STREX          10
> > > >  #define EXCP_HVC            11   /* HyperVisor Call */
> > > > +#define EXCP_SMC            12   /* Secure Monitor Call */
> > > >
> > > >  #define ARMV7M_EXCP_RESET   1
> > > >  #define ARMV7M_EXCP_NMI     2
> > > > diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
> > > > index 974fa66..3894a6f 100644
> > > > --- a/target-arm/helper-a64.c
> > > > +++ b/target-arm/helper-a64.c
> > > > @@ -476,6 +476,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
> > > >      case EXCP_UDEF:
> > > >      case EXCP_SWI:
> > > >      case EXCP_HVC:
> > > > +    case EXCP_SMC:
> > > >          env->cp15.esr_el[new_el] = env->exception.syndrome;
> > > >          break;
> > > >      case EXCP_IRQ:
> > > > diff --git a/target-arm/helper.c b/target-arm/helper.c
> > > > index 5b2070c..35091ea 100644
> > > > --- a/target-arm/helper.c
> > > > +++ b/target-arm/helper.c
> > > > @@ -3298,6 +3298,12 @@ unsigned int arm_excp_target_el(CPUState *cs,
> > > > unsigned int excp_idx)
> > > >      case EXCP_HVC:
> > > >          target_el = MAX(target_el, 2);
> > > >          break;
> > > > +    case EXCP_SMC:
> > > > +        target_el = 3;
> > > > +        if (env->cp15.hcr_el2 & HCR_TSC) {
> > > > +            target_el = 2;
> > > > +        }
> > > > +        break;
> > > >      }
> > > >      return target_el;
> > > >  }
> > > > diff --git a/target-arm/helper.h b/target-arm/helper.h
> > > > index fb711be..6c3d84d 100644
> > > > --- a/target-arm/helper.h
> > > > +++ b/target-arm/helper.h
> > > > @@ -51,6 +51,7 @@ DEF_HELPER_3(exception_with_syndrome, void, env, i32,
> > > > i32)
> > > >  DEF_HELPER_1(wfi, void, env)
> > > >  DEF_HELPER_1(wfe, void, env)
> > > >  DEF_HELPER_2(hvc, void, env, i32)
> > > > +DEF_HELPER_2(smc, void, env, i32)
> > > >
> > > >  DEF_HELPER_3(cpsr_write, void, env, i32, i32)
> > > >  DEF_HELPER_1(cpsr_read, i32, env)
> > > > diff --git a/target-arm/internals.h b/target-arm/internals.h
> > > > index b08381c..e50a68e 100644
> > > > --- a/target-arm/internals.h
> > > > +++ b/target-arm/internals.h
> > > > @@ -54,6 +54,7 @@ static const char * const excnames[] = {
> > > >      [EXCP_KERNEL_TRAP] = "QEMU intercept of kernel commpage",
> > > >      [EXCP_STREX] = "QEMU intercept of STREX",
> > > >      [EXCP_HVC] = "Hypervisor Call",
> > > > +    [EXCP_SMC] = "Secure Monitor Call",
> > > >  };
> > > >
> > > >  static inline void arm_log_exception(int idx)
> > > > @@ -210,6 +211,11 @@ static inline uint32_t syn_aa64_hvc(uint32_t
> > imm16)
> > > >      return (EC_AA64_HVC << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 &
> > > > 0xffff);
> > > >  }
> > > >
> > > > +static inline uint32_t syn_aa64_smc(uint32_t imm16)
> > > > +{
> > > > +    return (EC_AA64_SMC << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 &
> > > > 0xffff);
> > > > +}
> > > > +
> > > >  static inline uint32_t syn_aa32_svc(uint32_t imm16, bool is_thumb)
> > > >  {
> > > >      return (EC_AA32_SVC << ARM_EL_EC_SHIFT) | (imm16 & 0xffff)
> > > > diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> > > > index 6bf34b0..6840828 100644
> > > > --- a/target-arm/op_helper.c
> > > > +++ b/target-arm/op_helper.c
> > > > @@ -405,6 +405,17 @@ void HELPER(hvc)(CPUARMState *env, uint32_t
> > syndrome)
> > > >      raise_exception(env, EXCP_HVC);
> > > >  }
> > > >
> > > > +void HELPER(smc)(CPUARMState *env, uint32_t syndrome)
> > > > +{
> > > > +    /* We've already checked that EL3 exists at translation time.  */
> > > > +    if (env->cp15.scr_el3 & SCR_SMD) {
> > > >
> > >
> > > In ARMv7 isn't this is this only a valid check if the virtualization
> > > extension is present?  If so, should we be checking for the virt
> > extension
> > > on AArch32 prior to raising undefined?
> >
> > My understanding is that on AArch32 SCR.SMD only applies
> > to NS mode and only if HCR.TSC=0 (always the case if EL2 does not exist but
> > can also be zero even if EL2 exists).
> > So it's not dependant on virtual extensions/EL2 non-existance? Am I missing
> > something?
> >
> 
> I'm not sure how we handle this as the ARMv7 spec states the following for
> the SCD bit:
> 
> "*SCD, bit[7], when implementation includes the Virtualization Extensions*"
> 
> 
> This is not worded the same in the ARMv8 spec, possibly because the
> extensions are assumed to always be present?  The QQMU code still supports
> the notion of features in which case sometimes are extensions.

Hi,

This is a bit confusing. I was looking at the spec for the Cortex A15
which does not mention the virtualization condition. It does refer
to the ARMv7 specs for "more" info though so I suspect we need to
differentiate them. I can do that in v2.


> 
> 
> >
> > I'm not adding AArch32 SMC here but am happy to try to get it right
> > in preparation for the AArch32 support.
> >
> 
> Sure, I suspect that Fabian's commits on top of yours will close any
> AArch32 gaps.
> 
> 
> >
> > The patch misses that the HCR.TSC routing of SMC to EL2 takes priority over
> > SMD for NS EL1. For AArch32, I need to check for non-secure state
> > aswell. I'll update those for v2.
> >
> > Need to run a few tests but I'm considering something along these lines:
> >
> > void HELPER(smc)(CPUARMState *env, uint32_t syndrome)
> > {
> >     int cur_el = arm_current_pl(env);
> >     /* FIXME: Use real secure state.  */
> >     bool secure = false;
> >     bool smd = env->cp15.scr_el3 & SCR_SMD;
> >     /* On AArch32, SMD only applies to NS mode.  */
> >     bool udef = is_a64(env) ? smd : !secure && smd;
> >
> 
> Is it possible to get in here at EL0?  A quick check, did not turn up a
> check for this in the code calling this helper, but I may have missed it.

We trap SMC from EL0 at translation time further down the patch.


> 
> 
> >
> >     /* In NS EL1, HCR controlled routing to EL2 has priority over SMD.  */
> >     if (!secure && cur_el == 1 && (env->cp15.hcr_el2 & HCR_TSC)) {
> >         udef = false;
> >     }
> >
> >     /* We've already checked that EL3 exists at translation time.  */
> >     if (udef) {
> >         env->exception.syndrome = syn_uncategorized();
> >         raise_exception(env, EXCP_UDEF);
> >     }
> >     env->exception.syndrome = syndrome;
> >     raise_exception(env, EXCP_SMC);
> > }
> >
> > Thanks for the comments,
> > Edgar
> >
> >
> > >
> > >
> > > > +        env->exception.syndrome = syn_uncategorized();
> > > > +        raise_exception(env, EXCP_UDEF);
> > > > +    }
> > > > +    env->exception.syndrome = syndrome;
> > > > +    raise_exception(env, EXCP_SMC);
> > > > +}
> > > > +
> > > >  void HELPER(exception_return)(CPUARMState *env)
> > > >  {
> > > >      int cur_el = arm_current_pl(env);
> > > > diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
> > > > index 3981ee1..a02fe06 100644
> > > > --- a/target-arm/translate-a64.c
> > > > +++ b/target-arm/translate-a64.c
> > > > @@ -1451,6 +1451,16 @@ static void disas_exc(DisasContext *s, uint32_t
> > > > insn)
> > > >              gen_helper_hvc(cpu_env, tmp);
> > > >              tcg_temp_free_i32(tmp);
> > > >              break;
> > > > +        case 3:
> > > > +            if (!arm_dc_feature(s, ARM_FEATURE_EL3) || s->current_pl
> > ==
> > > > 0) {

   ^^^^^ 
  here






> > > > +                unallocated_encoding(s);
> > > > +                break;
> > > > +            }
> > > > +            tmp = tcg_const_i32(syn_aa64_smc(imm16));
> > > > +            gen_a64_set_pc_im(s->pc);
> > > > +            gen_helper_smc(cpu_env, tmp);
> > > > +            tcg_temp_free_i32(tmp);
> > > > +            break;
> > > >          default:
> > > >              unallocated_encoding(s);
> > > >              break;
> > > > --
> > > > 1.8.3.2
> > > >
> > > >
> >
> >

Re: [Qemu-devel] [PATCH v1 01/16] target-arm: A64: Break out aarch64_save/restore_sp

[Alex Bennée]

Edgar E. Iglesias writes:

> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Break out code to save/restore AArch64 SP into functions.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  target-arm/internals.h | 29 ++++++++++++++++++++---------
>  target-arm/kvm64.c     | 13 +++----------
>  target-arm/op_helper.c |  6 +-----
>  3 files changed, 24 insertions(+), 24 deletions(-)
>
> diff --git a/target-arm/internals.h b/target-arm/internals.h
> index 564b5fa..08fa697 100644
> --- a/target-arm/internals.h
> +++ b/target-arm/internals.h
> @@ -105,6 +105,24 @@ enum arm_fprounding {
>  
>  int arm_rmode_to_sf(int rmode);
>  
> +static inline void aarch64_save_sp(CPUARMState *env, int el)
> +{
> +    if (env->pstate & PSTATE_SP) {
> +        env->sp_el[el] = env->xregs[31];
> +    } else {
> +        env->sp_el[0] = env->xregs[31];
> +    }
> +}
> +
> +static inline void aarch64_restore_sp(CPUARMState *env, int el)
> +{
> +    if (env->pstate & PSTATE_SP) {
> +        env->xregs[31] = env->sp_el[el];
> +    } else {
> +        env->xregs[31] = env->sp_el[0];
> +    }
> +}
> +

Just a note to say I'm currently looking at rationalising
env->pstate/env->uncached_cpsr and the various access functions. However
conveniently this moves everything to one place so I approve ;-)

>  static inline void update_spsel(CPUARMState *env, uint32_t imm)
>  {
>      unsigned int cur_el = arm_current_pl(env);
> @@ -114,21 +132,14 @@ static inline void update_spsel(CPUARMState *env, uint32_t imm)
>      if (!((imm ^ env->pstate) & PSTATE_SP)) {
>          return;
>      }
> +    aarch64_save_sp(env, cur_el);
>      env->pstate = deposit32(env->pstate, 0, 1, imm);
>  
>      /* We rely on illegal updates to SPsel from EL0 to get trapped
>       * at translation time.
>       */
>      assert(cur_el >= 1 && cur_el <= 3);
> -    if (env->pstate & PSTATE_SP) {
> -        /* Switch from using SP_EL0 to using SP_ELx */
> -        env->sp_el[0] = env->xregs[31];
> -        env->xregs[31] = env->sp_el[cur_el];
> -    } else {
> -        /* Switch from SP_EL0 to SP_ELx */
> -        env->sp_el[cur_el] = env->xregs[31];
> -        env->xregs[31] = env->sp_el[0];
> -    }
> +    aarch64_restore_sp(env, cur_el);
>  }
>  
>  /* Valid Syndrome Register EC field values */
> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
> index 70f311b..0542cd1 100644
> --- a/target-arm/kvm64.c
> +++ b/target-arm/kvm64.c
> @@ -21,6 +21,7 @@
>  #include "sysemu/kvm.h"
>  #include "kvm_arm.h"
>  #include "cpu.h"
> +#include "internals.h"
>  #include "hw/arm/arm.h"
>  
>  static inline void set_feature(uint64_t *features, int feature)
> @@ -124,11 +125,7 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>      /* KVM puts SP_EL0 in regs.sp and SP_EL1 in regs.sp_el1. On the
>       * QEMU side we keep the current SP in xregs[31] as well.
>       */
> -    if (env->pstate & PSTATE_SP) {
> -        env->sp_el[1] = env->xregs[31];
> -    } else {
> -        env->sp_el[0] = env->xregs[31];
> -    }
> +    aarch64_save_sp(env, 1);
>  
>      reg.id = AARCH64_CORE_REG(regs.sp);
>      reg.addr = (uintptr_t) &env->sp_el[0];
> @@ -227,11 +224,7 @@ int kvm_arch_get_registers(CPUState *cs)
>      /* KVM puts SP_EL0 in regs.sp and SP_EL1 in regs.sp_el1. On the
>       * QEMU side we keep the current SP in xregs[31] as well.
>       */
> -    if (env->pstate & PSTATE_SP) {
> -        env->xregs[31] = env->sp_el[1];
> -    } else {
> -        env->xregs[31] = env->sp_el[0];
> -    }
> +    aarch64_restore_sp(env, 1);
>  
>      reg.id = AARCH64_CORE_REG(regs.pc);
>      reg.addr = (uintptr_t) &env->pc;
> diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> index b28f694..2e2429a 100644
> --- a/target-arm/op_helper.c
> +++ b/target-arm/op_helper.c
> @@ -391,11 +391,7 @@ void HELPER(exception_return)(CPUARMState *env)
>      uint32_t spsr = env->banked_spsr[spsr_idx];
>      int new_el, i;
>  
> -    if (env->pstate & PSTATE_SP) {
> -        env->sp_el[cur_el] = env->xregs[31];
> -    } else {
> -        env->sp_el[0] = env->xregs[31];
> -    }
> +    aarch64_save_sp(env, cur_el);
>  
>      env->exclusive_addr = -1;

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>


-- 
Alex Bennée

Re: [Qemu-devel] [PATCH v1 02/16] target-arm: A64: Respect SPSEL in ERET SP restore

[Alex Bennée]

Edgar E. Iglesias writes:

> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  target-arm/op_helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> index 2e2429a..581dc09 100644
> --- a/target-arm/op_helper.c
> +++ b/target-arm/op_helper.c
> @@ -425,7 +425,7 @@ void HELPER(exception_return)(CPUARMState *env)
>          }
>          env->aarch64 = 1;
>          pstate_write(env, spsr);
> -        env->xregs[31] = env->sp_el[new_el];
> +        aarch64_restore_sp(env, new_el);
>          env->pc = env->elr_el[cur_el];
>      }

It might be worth putting the Aarch64 ARM reference next to the aarch64_
functions to make it easier to find in future.

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée

Re: [Qemu-devel] [PATCH v1 03/16] target-arm: A64: Respect SPSEL when taking exceptions

[Alex Bennée]

Edgar E. Iglesias writes:

> From: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  target-arm/helper-a64.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
> index cccda74..bc153cb 100644
> --- a/target-arm/helper-a64.c
> +++ b/target-arm/helper-a64.c
> @@ -489,8 +489,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
>  
>      if (is_a64(env)) {
>          env->banked_spsr[aarch64_banked_spsr_index(1)] = pstate_read(env);
> -        env->sp_el[arm_current_pl(env)] = env->xregs[31];
> -        env->xregs[31] = env->sp_el[1];
> +        aarch64_save_sp(env, arm_current_pl(env));
>          env->elr_el[1] = env->pc;
>      } else {
>          env->banked_spsr[0] = cpsr_read(env);
> @@ -508,6 +507,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
>  
>      pstate_write(env, PSTATE_DAIF | PSTATE_MODE_EL1h);
>      env->aarch64 = 1;
> +    aarch64_restore_sp(env, 1);
>  
>      env->pc = addr;
>      cs->interrupt_request |= CPU_INTERRUPT_EXITTB;

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée

Re: [Qemu-devel] [PATCH v1 04/16] target-arm: Make far_el1 an array

[Alex Bennée]

Edgar E. Iglesias writes:

> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> No functional change.
> Prepares for future additions of the EL2 and 3 versions of this reg.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  target-arm/cpu.c        |  2 +-
>  target-arm/cpu.h        |  2 +-
>  target-arm/helper-a64.c |  4 ++--
>  target-arm/helper.c     | 12 ++++++------
>  4 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index 794dcb9..93bd6a0 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -446,7 +446,7 @@ static void arm1026_initfn(Object *obj)
>          ARMCPRegInfo ifar = {
>              .name = "IFAR", .cp = 15, .crn = 6, .crm = 0, .opc1 = 0, .opc2 = 1,
>              .access = PL1_RW,
> -            .fieldoffset = offsetofhigh32(CPUARMState, cp15.far_el1),
> +            .fieldoffset = offsetofhigh32(CPUARMState, cp15.far_el[1]),
>              .resetvalue = 0
>          };
>          define_one_arm_cp_reg(cpu, &ifar);
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 8d04385..172a631 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -187,7 +187,7 @@ typedef struct CPUARMState {
>          uint32_t ifsr_el2; /* Fault status registers.  */
>          uint64_t esr_el[2];
>          uint32_t c6_region[8]; /* MPU base/size registers.  */
> -        uint64_t far_el1; /* Fault address registers.  */
> +        uint64_t far_el[2]; /* Fault address registers.  */

If there are EL1, 2 and 3 versions shouldn't this be [3]?

>          uint64_t par_el1;  /* Translation result. */
>          uint32_t c9_insn; /* Cache lockdown registers.  */
>          uint32_t c9_data;
> diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
> index bc153cb..d647441 100644
> --- a/target-arm/helper-a64.c
> +++ b/target-arm/helper-a64.c
> @@ -465,13 +465,13 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
>      }
>  
>      env->cp15.esr_el[1] = env->exception.syndrome;
> -    env->cp15.far_el1 = env->exception.vaddress;
> +    env->cp15.far_el[1] = env->exception.vaddress;
>  
>      switch (cs->exception_index) {
>      case EXCP_PREFETCH_ABORT:
>      case EXCP_DATA_ABORT:
>          qemu_log_mask(CPU_LOG_INT, "...with FAR 0x%" PRIx64 "\n",
> -                      env->cp15.far_el1);
> +                      env->cp15.far_el[1]);

As there is no FAR_EL0 shouldn't this be the first in the array (ie. 0?)

>          break;
>      case EXCP_BKPT:
>      case EXCP_UDEF:
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index ec031f5..5350a99 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -521,7 +521,7 @@ static const ARMCPRegInfo v6_cp_reginfo[] = {
>        .access = PL0_W, .type = ARM_CP_NOP },
>      { .name = "IFAR", .cp = 15, .crn = 6, .crm = 0, .opc1 = 0, .opc2 = 2,
>        .access = PL1_RW,
> -      .fieldoffset = offsetofhigh32(CPUARMState, cp15.far_el1),
> +      .fieldoffset = offsetofhigh32(CPUARMState, cp15.far_el[1]),
>        .resetvalue = 0, },
>      /* Watchpoint Fault Address Register : should actually only be present
>       * for 1136, 1176, 11MPCore.
> @@ -1505,7 +1505,7 @@ static const ARMCPRegInfo vmsa_cp_reginfo[] = {
>      /* 64-bit FAR; this entry also gives us the AArch32 DFAR */
>      { .name = "FAR_EL1", .state = ARM_CP_STATE_BOTH,
>        .opc0 = 3, .crn = 6, .crm = 0, .opc1 = 0, .opc2 = 0,
> -      .access = PL1_RW, .fieldoffset = offsetof(CPUARMState, cp15.far_el1),
> +      .access = PL1_RW, .fieldoffset = offsetof(CPUARMState, cp15.far_el[1]),
>        .resetvalue = 0, },
>      REGINFO_SENTINEL
>  };
> @@ -3414,8 +3414,8 @@ void arm_cpu_do_interrupt(CPUState *cs)
>          /* Fall through to prefetch abort.  */
>      case EXCP_PREFETCH_ABORT:
>          env->cp15.ifsr_el2 = env->exception.fsr;
> -        env->cp15.far_el1 = deposit64(env->cp15.far_el1, 32, 32,
> -                                      env->exception.vaddress);
> +        env->cp15.far_el[1] = deposit64(env->cp15.far_el[1], 32, 32,
> +                                        env->exception.vaddress);
>          qemu_log_mask(CPU_LOG_INT, "...with IFSR 0x%x IFAR 0x%x\n",
>                        env->cp15.ifsr_el2, (uint32_t)env->exception.vaddress);
>          new_mode = ARM_CPU_MODE_ABT;
> @@ -3425,8 +3425,8 @@ void arm_cpu_do_interrupt(CPUState *cs)
>          break;
>      case EXCP_DATA_ABORT:
>          env->cp15.esr_el[1] = env->exception.fsr;
> -        env->cp15.far_el1 = deposit64(env->cp15.far_el1, 0, 32,
> -                                      env->exception.vaddress);
> +        env->cp15.far_el[1] = deposit64(env->cp15.far_el[1], 0, 32,
> +                                        env->exception.vaddress);
>          qemu_log_mask(CPU_LOG_INT, "...with DFSR 0x%x DFAR 0x%x\n",
>                        (uint32_t)env->cp15.esr_el[1],
>                        (uint32_t)env->exception.vaddress);

-- 
Alex Bennée

Re: [Qemu-devel] [PATCH v1 04/16] target-arm: Make far_el1 an array

[Greg Bellows]

[-- Attachment #1: Type: text/plain, Size: 5683 bytes --]

On 3 June 2014 05:21, Alex Bennée <alex.bennee@linaro.org> wrote:

>
> Edgar E. Iglesias writes:
>
> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> >
> > No functional change.
> > Prepares for future additions of the EL2 and 3 versions of this reg.
> >
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > ---
> >  target-arm/cpu.c        |  2 +-
> >  target-arm/cpu.h        |  2 +-
> >  target-arm/helper-a64.c |  4 ++--
> >  target-arm/helper.c     | 12 ++++++------
> >  4 files changed, 10 insertions(+), 10 deletions(-)
> >
> > diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> > index 794dcb9..93bd6a0 100644
> > --- a/target-arm/cpu.c
> > +++ b/target-arm/cpu.c
> > @@ -446,7 +446,7 @@ static void arm1026_initfn(Object *obj)
> >          ARMCPRegInfo ifar = {
> >              .name = "IFAR", .cp = 15, .crn = 6, .crm = 0, .opc1 = 0,
> .opc2 = 1,
> >              .access = PL1_RW,
> > -            .fieldoffset = offsetofhigh32(CPUARMState, cp15.far_el1),
> > +            .fieldoffset = offsetofhigh32(CPUARMState, cp15.far_el[1]),
> >              .resetvalue = 0
> >          };
> >          define_one_arm_cp_reg(cpu, &ifar);
> > diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> > index 8d04385..172a631 100644
> > --- a/target-arm/cpu.h
> > +++ b/target-arm/cpu.h
> > @@ -187,7 +187,7 @@ typedef struct CPUARMState {
> >          uint32_t ifsr_el2; /* Fault status registers.  */
> >          uint64_t esr_el[2];
> >          uint32_t c6_region[8]; /* MPU base/size registers.  */
> > -        uint64_t far_el1; /* Fault address registers.  */
> > +        uint64_t far_el[2]; /* Fault address registers.  */
>
> If there are EL1, 2 and 3 versions shouldn't this be [3]?
>
>
Or [4]?  Even if we don't use all the EL slots, it would make for more
readable and consistent code if we kept the indices consistent with the
level value.  Otherwise, as we discussed previously, we end up with
different numbering schemes depending on the register.


> >          uint64_t par_el1;  /* Translation result. */
> >          uint32_t c9_insn; /* Cache lockdown registers.  */
> >          uint32_t c9_data;
> > diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
> > index bc153cb..d647441 100644
> > --- a/target-arm/helper-a64.c
> > +++ b/target-arm/helper-a64.c
> > @@ -465,13 +465,13 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
> >      }
> >
> >      env->cp15.esr_el[1] = env->exception.syndrome;
> > -    env->cp15.far_el1 = env->exception.vaddress;
> > +    env->cp15.far_el[1] = env->exception.vaddress;
> >
> >      switch (cs->exception_index) {
> >      case EXCP_PREFETCH_ABORT:
> >      case EXCP_DATA_ABORT:
> >          qemu_log_mask(CPU_LOG_INT, "...with FAR 0x%" PRIx64 "\n",
> > -                      env->cp15.far_el1);
> > +                      env->cp15.far_el[1]);
>
> As there is no FAR_EL0 shouldn't this be the first in the array (ie. 0?)
>

See above comment.


>
> >          break;
> >      case EXCP_BKPT:
> >      case EXCP_UDEF:
> > diff --git a/target-arm/helper.c b/target-arm/helper.c
> > index ec031f5..5350a99 100644
> > --- a/target-arm/helper.c
> > +++ b/target-arm/helper.c
> > @@ -521,7 +521,7 @@ static const ARMCPRegInfo v6_cp_reginfo[] = {
> >        .access = PL0_W, .type = ARM_CP_NOP },
> >      { .name = "IFAR", .cp = 15, .crn = 6, .crm = 0, .opc1 = 0, .opc2 =
> 2,
> >        .access = PL1_RW,
> > -      .fieldoffset = offsetofhigh32(CPUARMState, cp15.far_el1),
> > +      .fieldoffset = offsetofhigh32(CPUARMState, cp15.far_el[1]),
> >        .resetvalue = 0, },
> >      /* Watchpoint Fault Address Register : should actually only be
> present
> >       * for 1136, 1176, 11MPCore.
> > @@ -1505,7 +1505,7 @@ static const ARMCPRegInfo vmsa_cp_reginfo[] = {
> >      /* 64-bit FAR; this entry also gives us the AArch32 DFAR */
> >      { .name = "FAR_EL1", .state = ARM_CP_STATE_BOTH,
> >        .opc0 = 3, .crn = 6, .crm = 0, .opc1 = 0, .opc2 = 0,
> > -      .access = PL1_RW, .fieldoffset = offsetof(CPUARMState,
> cp15.far_el1),
> > +      .access = PL1_RW, .fieldoffset = offsetof(CPUARMState,
> cp15.far_el[1]),
> >        .resetvalue = 0, },
> >      REGINFO_SENTINEL
> >  };
> > @@ -3414,8 +3414,8 @@ void arm_cpu_do_interrupt(CPUState *cs)
> >          /* Fall through to prefetch abort.  */
> >      case EXCP_PREFETCH_ABORT:
> >          env->cp15.ifsr_el2 = env->exception.fsr;
> > -        env->cp15.far_el1 = deposit64(env->cp15.far_el1, 32, 32,
> > -                                      env->exception.vaddress);
> > +        env->cp15.far_el[1] = deposit64(env->cp15.far_el[1], 32, 32,
> > +                                        env->exception.vaddress);
> >          qemu_log_mask(CPU_LOG_INT, "...with IFSR 0x%x IFAR 0x%x\n",
> >                        env->cp15.ifsr_el2,
> (uint32_t)env->exception.vaddress);
> >          new_mode = ARM_CPU_MODE_ABT;
> > @@ -3425,8 +3425,8 @@ void arm_cpu_do_interrupt(CPUState *cs)
> >          break;
> >      case EXCP_DATA_ABORT:
> >          env->cp15.esr_el[1] = env->exception.fsr;
> > -        env->cp15.far_el1 = deposit64(env->cp15.far_el1, 0, 32,
> > -                                      env->exception.vaddress);
> > +        env->cp15.far_el[1] = deposit64(env->cp15.far_el[1], 0, 32,
> > +                                        env->exception.vaddress);
> >          qemu_log_mask(CPU_LOG_INT, "...with DFSR 0x%x DFAR 0x%x\n",
> >                        (uint32_t)env->cp15.esr_el[1],
> >                        (uint32_t)env->exception.vaddress);
>
> --
> Alex Bennée
>

[-- Attachment #2: Type: text/html, Size: 7678 bytes --]

Re: [Qemu-devel] [PATCH v1 04/16] target-arm: Make far_el1 an array

[Alex Bennée]

Greg Bellows writes:

> On 3 June 2014 05:21, Alex Bennée <alex.bennee@linaro.org> wrote:
>
>>
>> Edgar E. Iglesias writes:
>>
>> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>> >
>> > No functional change.
>> > Prepares for future additions of the EL2 and 3 versions of this reg.
>> >
>> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>> > ---
>> >  target-arm/cpu.c        |  2 +-
>> >  target-arm/cpu.h        |  2 +-
>> >  target-arm/helper-a64.c |  4 ++--
>> >  target-arm/helper.c     | 12 ++++++------
>> >  4 files changed, 10 insertions(+), 10 deletions(-)
>> >
>> > diff --git a/target-arm/cpu.c b/target-arm/cpu.c
>> > index 794dcb9..93bd6a0 100644
>> > --- a/target-arm/cpu.c
>> > +++ b/target-arm/cpu.c
>> > @@ -446,7 +446,7 @@ static void arm1026_initfn(Object *obj)
>> >          ARMCPRegInfo ifar = {
>> >              .name = "IFAR", .cp = 15, .crn = 6, .crm = 0, .opc1 = 0,
>> .opc2 = 1,
>> >              .access = PL1_RW,
>> > -            .fieldoffset = offsetofhigh32(CPUARMState, cp15.far_el1),
>> > +            .fieldoffset = offsetofhigh32(CPUARMState, cp15.far_el[1]),
>> >              .resetvalue = 0
>> >          };
>> >          define_one_arm_cp_reg(cpu, &ifar);
>> > diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>> > index 8d04385..172a631 100644
>> > --- a/target-arm/cpu.h
>> > +++ b/target-arm/cpu.h
>> > @@ -187,7 +187,7 @@ typedef struct CPUARMState {
>> >          uint32_t ifsr_el2; /* Fault status registers.  */
>> >          uint64_t esr_el[2];
>> >          uint32_t c6_region[8]; /* MPU base/size registers.  */
>> > -        uint64_t far_el1; /* Fault address registers.  */
>> > +        uint64_t far_el[2]; /* Fault address registers.  */
>>
>> If there are EL1, 2 and 3 versions shouldn't this be [3]?
>>
>>
> Or [4]?  Even if we don't use all the EL slots, it would make for more
> readable and consistent code if we kept the indices consistent with the
> level value.  Otherwise, as we discussed previously, we end up with
> different numbering schemes depending on the register.

Makes sense although it would be nice to mark the unused ones somehow
for debugging.

>
>
>> >          uint64_t par_el1;  /* Translation result. */
>> >          uint32_t c9_insn; /* Cache lockdown registers.  */
>> >          uint32_t c9_data;
>> > diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
>> > index bc153cb..d647441 100644
>> > --- a/target-arm/helper-a64.c
>> > +++ b/target-arm/helper-a64.c
>> > @@ -465,13 +465,13 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
>> >      }
>> >
>> >      env->cp15.esr_el[1] = env->exception.syndrome;
>> > -    env->cp15.far_el1 = env->exception.vaddress;
>> > +    env->cp15.far_el[1] = env->exception.vaddress;
>> >
>> >      switch (cs->exception_index) {
>> >      case EXCP_PREFETCH_ABORT:
>> >      case EXCP_DATA_ABORT:
>> >          qemu_log_mask(CPU_LOG_INT, "...with FAR 0x%" PRIx64 "\n",
>> > -                      env->cp15.far_el1);
>> > +                      env->cp15.far_el[1]);
>>
>> As there is no FAR_EL0 shouldn't this be the first in the array (ie. 0?)
>>
>
> See above comment.
>
>
>>
>> >          break;
>> >      case EXCP_BKPT:
>> >      case EXCP_UDEF:
>> > diff --git a/target-arm/helper.c b/target-arm/helper.c
>> > index ec031f5..5350a99 100644
>> > --- a/target-arm/helper.c
>> > +++ b/target-arm/helper.c
>> > @@ -521,7 +521,7 @@ static const ARMCPRegInfo v6_cp_reginfo[] = {
>> >        .access = PL0_W, .type = ARM_CP_NOP },
>> >      { .name = "IFAR", .cp = 15, .crn = 6, .crm = 0, .opc1 = 0, .opc2 =
>> 2,
>> >        .access = PL1_RW,
>> > -      .fieldoffset = offsetofhigh32(CPUARMState, cp15.far_el1),
>> > +      .fieldoffset = offsetofhigh32(CPUARMState, cp15.far_el[1]),
>> >        .resetvalue = 0, },
>> >      /* Watchpoint Fault Address Register : should actually only be
>> present
>> >       * for 1136, 1176, 11MPCore.
>> > @@ -1505,7 +1505,7 @@ static const ARMCPRegInfo vmsa_cp_reginfo[] = {
>> >      /* 64-bit FAR; this entry also gives us the AArch32 DFAR */
>> >      { .name = "FAR_EL1", .state = ARM_CP_STATE_BOTH,
>> >        .opc0 = 3, .crn = 6, .crm = 0, .opc1 = 0, .opc2 = 0,
>> > -      .access = PL1_RW, .fieldoffset = offsetof(CPUARMState,
>> cp15.far_el1),
>> > +      .access = PL1_RW, .fieldoffset = offsetof(CPUARMState,
>> cp15.far_el[1]),
>> >        .resetvalue = 0, },
>> >      REGINFO_SENTINEL
>> >  };
>> > @@ -3414,8 +3414,8 @@ void arm_cpu_do_interrupt(CPUState *cs)
>> >          /* Fall through to prefetch abort.  */
>> >      case EXCP_PREFETCH_ABORT:
>> >          env->cp15.ifsr_el2 = env->exception.fsr;
>> > -        env->cp15.far_el1 = deposit64(env->cp15.far_el1, 32, 32,
>> > -                                      env->exception.vaddress);
>> > +        env->cp15.far_el[1] = deposit64(env->cp15.far_el[1], 32, 32,
>> > +                                        env->exception.vaddress);
>> >          qemu_log_mask(CPU_LOG_INT, "...with IFSR 0x%x IFAR 0x%x\n",
>> >                        env->cp15.ifsr_el2,
>> (uint32_t)env->exception.vaddress);
>> >          new_mode = ARM_CPU_MODE_ABT;
>> > @@ -3425,8 +3425,8 @@ void arm_cpu_do_interrupt(CPUState *cs)
>> >          break;
>> >      case EXCP_DATA_ABORT:
>> >          env->cp15.esr_el[1] = env->exception.fsr;
>> > -        env->cp15.far_el1 = deposit64(env->cp15.far_el1, 0, 32,
>> > -                                      env->exception.vaddress);
>> > +        env->cp15.far_el[1] = deposit64(env->cp15.far_el[1], 0, 32,
>> > +                                        env->exception.vaddress);
>> >          qemu_log_mask(CPU_LOG_INT, "...with DFSR 0x%x DFAR 0x%x\n",
>> >                        (uint32_t)env->cp15.esr_el[1],
>> >                        (uint32_t)env->exception.vaddress);
>>
>> --
>> Alex Bennée
>>

-- 
Alex Bennée

Re: [Qemu-devel] [PATCH v1 04/16] target-arm: Make far_el1 an array

[Greg Bellows]

[-- Attachment #1: Type: text/plain, Size: 6482 bytes --]

Probably simple enough just to check that the value is always zero as I
believe that is what they are likely initialized to.  Perhaps during reset?


On 3 June 2014 08:35, Alex Bennée <alex.bennee@linaro.org> wrote:

>
> Greg Bellows writes:
>
> > On 3 June 2014 05:21, Alex Bennée <alex.bennee@linaro.org> wrote:
> >
> >>
> >> Edgar E. Iglesias writes:
> >>
> >> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> >> >
> >> > No functional change.
> >> > Prepares for future additions of the EL2 and 3 versions of this reg.
> >> >
> >> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> >> > ---
> >> >  target-arm/cpu.c        |  2 +-
> >> >  target-arm/cpu.h        |  2 +-
> >> >  target-arm/helper-a64.c |  4 ++--
> >> >  target-arm/helper.c     | 12 ++++++------
> >> >  4 files changed, 10 insertions(+), 10 deletions(-)
> >> >
> >> > diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> >> > index 794dcb9..93bd6a0 100644
> >> > --- a/target-arm/cpu.c
> >> > +++ b/target-arm/cpu.c
> >> > @@ -446,7 +446,7 @@ static void arm1026_initfn(Object *obj)
> >> >          ARMCPRegInfo ifar = {
> >> >              .name = "IFAR", .cp = 15, .crn = 6, .crm = 0, .opc1 = 0,
> >> .opc2 = 1,
> >> >              .access = PL1_RW,
> >> > -            .fieldoffset = offsetofhigh32(CPUARMState, cp15.far_el1),
> >> > +            .fieldoffset = offsetofhigh32(CPUARMState,
> cp15.far_el[1]),
> >> >              .resetvalue = 0
> >> >          };
> >> >          define_one_arm_cp_reg(cpu, &ifar);
> >> > diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> >> > index 8d04385..172a631 100644
> >> > --- a/target-arm/cpu.h
> >> > +++ b/target-arm/cpu.h
> >> > @@ -187,7 +187,7 @@ typedef struct CPUARMState {
> >> >          uint32_t ifsr_el2; /* Fault status registers.  */
> >> >          uint64_t esr_el[2];
> >> >          uint32_t c6_region[8]; /* MPU base/size registers.  */
> >> > -        uint64_t far_el1; /* Fault address registers.  */
> >> > +        uint64_t far_el[2]; /* Fault address registers.  */
> >>
> >> If there are EL1, 2 and 3 versions shouldn't this be [3]?
> >>
> >>
> > Or [4]?  Even if we don't use all the EL slots, it would make for more
> > readable and consistent code if we kept the indices consistent with the
> > level value.  Otherwise, as we discussed previously, we end up with
> > different numbering schemes depending on the register.
>
> Makes sense although it would be nice to mark the unused ones somehow
> for debugging.
>
> >
> >
> >> >          uint64_t par_el1;  /* Translation result. */
> >> >          uint32_t c9_insn; /* Cache lockdown registers.  */
> >> >          uint32_t c9_data;
> >> > diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
> >> > index bc153cb..d647441 100644
> >> > --- a/target-arm/helper-a64.c
> >> > +++ b/target-arm/helper-a64.c
> >> > @@ -465,13 +465,13 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
> >> >      }
> >> >
> >> >      env->cp15.esr_el[1] = env->exception.syndrome;
> >> > -    env->cp15.far_el1 = env->exception.vaddress;
> >> > +    env->cp15.far_el[1] = env->exception.vaddress;
> >> >
> >> >      switch (cs->exception_index) {
> >> >      case EXCP_PREFETCH_ABORT:
> >> >      case EXCP_DATA_ABORT:
> >> >          qemu_log_mask(CPU_LOG_INT, "...with FAR 0x%" PRIx64 "\n",
> >> > -                      env->cp15.far_el1);
> >> > +                      env->cp15.far_el[1]);
> >>
> >> As there is no FAR_EL0 shouldn't this be the first in the array (ie. 0?)
> >>
> >
> > See above comment.
> >
> >
> >>
> >> >          break;
> >> >      case EXCP_BKPT:
> >> >      case EXCP_UDEF:
> >> > diff --git a/target-arm/helper.c b/target-arm/helper.c
> >> > index ec031f5..5350a99 100644
> >> > --- a/target-arm/helper.c
> >> > +++ b/target-arm/helper.c
> >> > @@ -521,7 +521,7 @@ static const ARMCPRegInfo v6_cp_reginfo[] = {
> >> >        .access = PL0_W, .type = ARM_CP_NOP },
> >> >      { .name = "IFAR", .cp = 15, .crn = 6, .crm = 0, .opc1 = 0, .opc2
> =
> >> 2,
> >> >        .access = PL1_RW,
> >> > -      .fieldoffset = offsetofhigh32(CPUARMState, cp15.far_el1),
> >> > +      .fieldoffset = offsetofhigh32(CPUARMState, cp15.far_el[1]),
> >> >        .resetvalue = 0, },
> >> >      /* Watchpoint Fault Address Register : should actually only be
> >> present
> >> >       * for 1136, 1176, 11MPCore.
> >> > @@ -1505,7 +1505,7 @@ static const ARMCPRegInfo vmsa_cp_reginfo[] = {
> >> >      /* 64-bit FAR; this entry also gives us the AArch32 DFAR */
> >> >      { .name = "FAR_EL1", .state = ARM_CP_STATE_BOTH,
> >> >        .opc0 = 3, .crn = 6, .crm = 0, .opc1 = 0, .opc2 = 0,
> >> > -      .access = PL1_RW, .fieldoffset = offsetof(CPUARMState,
> >> cp15.far_el1),
> >> > +      .access = PL1_RW, .fieldoffset = offsetof(CPUARMState,
> >> cp15.far_el[1]),
> >> >        .resetvalue = 0, },
> >> >      REGINFO_SENTINEL
> >> >  };
> >> > @@ -3414,8 +3414,8 @@ void arm_cpu_do_interrupt(CPUState *cs)
> >> >          /* Fall through to prefetch abort.  */
> >> >      case EXCP_PREFETCH_ABORT:
> >> >          env->cp15.ifsr_el2 = env->exception.fsr;
> >> > -        env->cp15.far_el1 = deposit64(env->cp15.far_el1, 32, 32,
> >> > -                                      env->exception.vaddress);
> >> > +        env->cp15.far_el[1] = deposit64(env->cp15.far_el[1], 32, 32,
> >> > +                                        env->exception.vaddress);
> >> >          qemu_log_mask(CPU_LOG_INT, "...with IFSR 0x%x IFAR 0x%x\n",
> >> >                        env->cp15.ifsr_el2,
> >> (uint32_t)env->exception.vaddress);
> >> >          new_mode = ARM_CPU_MODE_ABT;
> >> > @@ -3425,8 +3425,8 @@ void arm_cpu_do_interrupt(CPUState *cs)
> >> >          break;
> >> >      case EXCP_DATA_ABORT:
> >> >          env->cp15.esr_el[1] = env->exception.fsr;
> >> > -        env->cp15.far_el1 = deposit64(env->cp15.far_el1, 0, 32,
> >> > -                                      env->exception.vaddress);
> >> > +        env->cp15.far_el[1] = deposit64(env->cp15.far_el[1], 0, 32,
> >> > +                                        env->exception.vaddress);
> >> >          qemu_log_mask(CPU_LOG_INT, "...with DFSR 0x%x DFAR 0x%x\n",
> >> >                        (uint32_t)env->cp15.esr_el[1],
> >> >                        (uint32_t)env->exception.vaddress);
> >>
> >> --
> >> Alex Bennée
> >>
>
> --
> Alex Bennée
>

[-- Attachment #2: Type: text/html, Size: 9042 bytes --]

Re: [Qemu-devel] [PATCH v1 06/16] target-arm: Add FAR_EL2 and 3

[Alex Bennée]

Edgar E. Iglesias writes:

> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  target-arm/cpu.h    | 2 +-
>  target-arm/helper.c | 6 ++++++
>  2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index f8ca1da..ef6a95d 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -187,7 +187,7 @@ typedef struct CPUARMState {
>          uint32_t ifsr_el2; /* Fault status registers.  */
>          uint64_t esr_el[4];
>          uint32_t c6_region[8]; /* MPU base/size registers.  */
> -        uint64_t far_el[2]; /* Fault address registers.  */
> +        uint64_t far_el[4]; /* Fault address registers.  */

Ahh my confusion from earlier is now clear. Perhaps the two commits
should be merged?

>          uint64_t par_el1;  /* Translation result. */
>          uint32_t c9_insn; /* Cache lockdown registers.  */
>          uint32_t c9_data;
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index da210b9..de5ee40 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -2120,6 +2120,9 @@ static const ARMCPRegInfo v8_el2_cp_reginfo[] = {
>        .type = ARM_CP_NO_MIGRATE,
>        .opc0 = 3, .opc1 = 4, .crn = 5, .crm = 2, .opc2 = 1,
>        .access = PL2_RW, .fieldoffset = offsetof(CPUARMState, cp15.esr_el[2]) },
> +    { .name = "FAR_EL2", .state = ARM_CP_STATE_AA64,
> +      .opc0 = 3, .opc1 = 4, .crn = 6, .crm = 0, .opc2 = 0,
> +      .access = PL2_RW, .fieldoffset = offsetof(CPUARMState, cp15.far_el[2]) },
>      { .name = "SPSR_EL2", .state = ARM_CP_STATE_AA64,
>        .type = ARM_CP_NO_MIGRATE,
>        .opc0 = 3, .opc1 = 4, .crn = 4, .crm = 0, .opc2 = 0,
> @@ -2142,6 +2145,9 @@ static const ARMCPRegInfo v8_el3_cp_reginfo[] = {
>        .type = ARM_CP_NO_MIGRATE,
>        .opc0 = 3, .opc1 = 6, .crn = 5, .crm = 2, .opc2 = 1,
>        .access = PL3_RW, .fieldoffset = offsetof(CPUARMState, cp15.esr_el[3]) },
> +    { .name = "FAR_EL3", .state = ARM_CP_STATE_AA64,
> +      .opc0 = 3, .opc1 = 6, .crn = 6, .crm = 0, .opc2 = 0,
> +      .access = PL3_RW, .fieldoffset = offsetof(CPUARMState, cp15.far_el[3]) },
>      { .name = "SPSR_EL3", .state = ARM_CP_STATE_AA64,
>        .type = ARM_CP_NO_MIGRATE,
>        .opc0 = 3, .opc1 = 6, .crn = 4, .crm = 0, .opc2 = 0,

-- 
Alex Bennée

Re: [Qemu-devel] [PATCH v1 06/16] target-arm: Add FAR_EL2 and 3

[Edgar E. Iglesias]

On Tue, Jun 03, 2014 at 11:22:51AM +0100, Alex Bennée wrote:
> 
> Edgar E. Iglesias writes:
> 
> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> >
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > ---
> >  target-arm/cpu.h    | 2 +-
> >  target-arm/helper.c | 6 ++++++
> >  2 files changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> > index f8ca1da..ef6a95d 100644
> > --- a/target-arm/cpu.h
> > +++ b/target-arm/cpu.h
> > @@ -187,7 +187,7 @@ typedef struct CPUARMState {
> >          uint32_t ifsr_el2; /* Fault status registers.  */
> >          uint64_t esr_el[4];
> >          uint32_t c6_region[8]; /* MPU base/size registers.  */
> > -        uint64_t far_el[2]; /* Fault address registers.  */
> > +        uint64_t far_el[4]; /* Fault address registers.  */
> 
> Ahh my confusion from earlier is now clear. Perhaps the two commits
> should be merged?


Hi,

The point is to have a non-functional diff and then incrementally add
the function to easy bisectability if something breaks. I don't
have a very strong opinion though, so if people insist I can squash.

Cheers,
Edgar

> 
> >          uint64_t par_el1;  /* Translation result. */
> >          uint32_t c9_insn; /* Cache lockdown registers.  */
> >          uint32_t c9_data;
> > diff --git a/target-arm/helper.c b/target-arm/helper.c
> > index da210b9..de5ee40 100644
> > --- a/target-arm/helper.c
> > +++ b/target-arm/helper.c
> > @@ -2120,6 +2120,9 @@ static const ARMCPRegInfo v8_el2_cp_reginfo[] = {
> >        .type = ARM_CP_NO_MIGRATE,
> >        .opc0 = 3, .opc1 = 4, .crn = 5, .crm = 2, .opc2 = 1,
> >        .access = PL2_RW, .fieldoffset = offsetof(CPUARMState, cp15.esr_el[2]) },
> > +    { .name = "FAR_EL2", .state = ARM_CP_STATE_AA64,
> > +      .opc0 = 3, .opc1 = 4, .crn = 6, .crm = 0, .opc2 = 0,
> > +      .access = PL2_RW, .fieldoffset = offsetof(CPUARMState, cp15.far_el[2]) },
> >      { .name = "SPSR_EL2", .state = ARM_CP_STATE_AA64,
> >        .type = ARM_CP_NO_MIGRATE,
> >        .opc0 = 3, .opc1 = 4, .crn = 4, .crm = 0, .opc2 = 0,
> > @@ -2142,6 +2145,9 @@ static const ARMCPRegInfo v8_el3_cp_reginfo[] = {
> >        .type = ARM_CP_NO_MIGRATE,
> >        .opc0 = 3, .opc1 = 6, .crn = 5, .crm = 2, .opc2 = 1,
> >        .access = PL3_RW, .fieldoffset = offsetof(CPUARMState, cp15.esr_el[3]) },
> > +    { .name = "FAR_EL3", .state = ARM_CP_STATE_AA64,
> > +      .opc0 = 3, .opc1 = 6, .crn = 6, .crm = 0, .opc2 = 0,
> > +      .access = PL3_RW, .fieldoffset = offsetof(CPUARMState, cp15.far_el[3]) },
> >      { .name = "SPSR_EL3", .state = ARM_CP_STATE_AA64,
> >        .type = ARM_CP_NO_MIGRATE,
> >        .opc0 = 3, .opc1 = 6, .crn = 4, .crm = 0, .opc2 = 0,
> 
> -- 
> Alex Bennée

Re: [Qemu-devel] [PATCH v1 06/16] target-arm: Add FAR_EL2 and 3

[Alex Bennée]

Edgar E. Iglesias writes:

> On Tue, Jun 03, 2014 at 11:22:51AM +0100, Alex Bennée wrote:
>> 
>> Edgar E. Iglesias writes:
>> 
>> 
>> Ahh my confusion from earlier is now clear. Perhaps the two commits
>> should be merged?
>
> Hi,
>
> The point is to have a non-functional diff and then incrementally add
> the function to easy bisectability if something breaks. I don't
> have a very strong opinion though, so if people insist I can squash.

Having each commit point be buildable and testable is certainly a
worthwhile goal from a bisect point of view. But for a simple no-op diff
(i.e. functionaly identical, just moving a few bits around) which will
then get updated with functional changes there is an argument to squash
the two together.

I like this patch series because the individual patches are narrow in
scope and not too big hence easier to review. I don't think squashing
some of non-function + functional diffs together detracts from that
nobel goal. As you say it's a judgement call.

-- 
Alex Bennée

Re: [Qemu-devel] [PATCH v1 06/16] target-arm: Add FAR_EL2 and 3

[Edgar E. Iglesias]

On Wed, Jun 04, 2014 at 08:55:30AM +0100, Alex Benn�e wrote:
> 
> Edgar E. Iglesias writes:
> 
> > On Tue, Jun 03, 2014 at 11:22:51AM +0100, Alex Benn?e wrote:
> >> 
> >> Edgar E. Iglesias writes:
> >> 
> >> 
> >> Ahh my confusion from earlier is now clear. Perhaps the two commits
> >> should be merged?
> >
> > Hi,
> >
> > The point is to have a non-functional diff and then incrementally add
> > the function to easy bisectability if something breaks. I don't
> > have a very strong opinion though, so if people insist I can squash.
> 
> Having each commit point be buildable and testable is certainly a
> worthwhile goal from a bisect point of view. But for a simple no-op diff
> (i.e. functionaly identical, just moving a few bits around) which will
> then get updated with functional changes there is an argument to squash
> the two together.


I disagree. IMO when patches include refactoring + changes, the refactoring
should be done with non functional changes (as far as possible) and then
followed up with small easily reviewable functional patches.


> 
> I like this patch series because the individual patches are narrow in
> scope and not too big hence easier to review. I don't think squashing
> some of non-function + functional diffs together detracts from that
> nobel goal. As you say it's a judgement call.

Re: [Qemu-devel] [PATCH v1 07/16] target-arm: Add HCR_EL2

[Alex Bennée]

Edgar E. Iglesias writes:

> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  target-arm/cpu.h    | 35 +++++++++++++++++++++++++++++++++++
>  target-arm/helper.c | 27 +++++++++++++++++++++++++++
>  2 files changed, 62 insertions(+)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index ef6a95d..b446478 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -184,6 +184,7 @@ typedef struct CPUARMState {
>                          MPU write buffer control.  */
>          uint32_t pmsav5_data_ap; /* PMSAv5 MPU data access permissions */
>          uint32_t pmsav5_insn_ap; /* PMSAv5 MPU insn access permissions */
> +        uint64_t hcr_el2; /* Hypervisor configuration register */
>          uint32_t ifsr_el2; /* Fault status registers.  */
>          uint64_t esr_el[4];
>          uint32_t c6_region[8]; /* MPU base/size registers.  */
> @@ -526,6 +527,40 @@ static inline void xpsr_write(CPUARMState *env, uint32_t val, uint32_t mask)
>      }
>  }
>  
> +#define HCR_VM        (1ULL << 0)
> +#define HCR_SWIO      (1ULL << 1)
> +#define HCR_PTW       (1ULL << 2)
> +#define HCR_FMO       (1ULL << 3)
> +#define HCR_IMO       (1ULL << 4)
> +#define HCR_AMO       (1ULL << 5)
> +#define HCR_VF        (1ULL << 6)
> +#define HCR_VI        (1ULL << 7)
> +#define HCR_VSE       (1ULL << 8)
> +#define HCR_FB        (1ULL << 9)
> +#define HCR_DC        (1ULL << 12)
> +#define HCR_TWI       (1ULL << 13)
> +#define HCR_TWE       (1ULL << 14)
> +#define HCR_TID0      (1ULL << 15)
> +#define HCR_TID1      (1ULL << 16)
> +#define HCR_TID2      (1ULL << 17)
> +#define HCR_TID3      (1ULL << 18)
> +#define HCR_TSC       (1ULL << 19)
> +#define HCR_TIDCP     (1ULL << 20)
> +#define HCR_TACR      (1ULL << 21)
> +#define HCR_TSW       (1ULL << 22)
> +#define HCR_TPC       (1ULL << 23)
> +#define HCR_TPU       (1ULL << 24)
> +#define HCR_TTLB      (1ULL << 25)
> +#define HCR_TVM       (1ULL << 26)
> +#define HCR_TGE       (1ULL << 27)
> +#define HCR_TDZ       (1ULL << 28)
> +#define HCR_HCD       (1ULL << 29)
> +#define HCR_TRVM      (1ULL << 30)
> +#define HCR_RW        (1ULL << 31)
> +#define HCR_CD        (1ULL << 32)
> +#define HCR_ID        (1ULL << 33)
> +#define HCR_RES0_MASK ((1ULL << 34) - 1)

Hmm isn't that actually HCR_MASK? I would expect the mask for the RES0
bits to be ~((1ULL << 34) - 1) but it's not actually used for that hence
the name confusion.

> +
>  /* Return the current FPSCR value.  */
>  uint32_t vfp_get_fpscr(CPUARMState *env);
>  void vfp_set_fpscr(CPUARMState *env, uint32_t val);
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index de5ee40..cf877ae 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -2107,10 +2107,37 @@ static const ARMCPRegInfo v8_el3_no_el2_cp_reginfo[] = {
>        .opc0 = 3, .opc1 = 4, .crn = 12, .crm = 0, .opc2 = 0,
>        .access = PL2_RW,
>        .readfn = arm_cp_read_zero, .writefn = arm_cp_write_ignore },
> +    { .name = "HCR_EL2", .state = ARM_CP_STATE_AA64,
> +      .type = ARM_CP_NO_MIGRATE,
> +      .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 0,
> +      .readfn = arm_cp_read_zero, .writefn = arm_cp_write_ignore },
>      REGINFO_SENTINEL
>  };
>  
> +static void hcr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
> +{
> +    ARMCPU *cpu = arm_env_get_cpu(env);
> +    uint64_t res0_mask = HCR_RES0_MASK;
> +
> +    if (!arm_feature(env, ARM_FEATURE_EL3)) {
> +        res0_mask &= ~HCR_HCD;
> +    }
> +
> +    /* Clear RES0 bits.  */
> +    value &= res0_mask;
> +
> +    if ((raw_read(env, ri) ^ value) & HCR_VM) {
> +        /* Flush the TLB when turning VM on/off.  */
> +        tlb_flush(CPU(cpu), 1);
> +    }
> +    raw_write(env, ri, value);
> +}
> +
>  static const ARMCPRegInfo v8_el2_cp_reginfo[] = {
> +    { .name = "HCR_EL2", .state = ARM_CP_STATE_AA64,
> +      .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 0,
> +      .access = PL2_RW, .fieldoffset = offsetof(CPUARMState, cp15.hcr_el2),
> +      .writefn = hcr_write },
>      { .name = "ELR_EL2", .state = ARM_CP_STATE_AA64,
>        .type = ARM_CP_NO_MIGRATE,
>        .opc0 = 3, .opc1 = 4, .crn = 4, .crm = 0, .opc2 = 1,

-- 
Alex Bennée

Re: [Qemu-devel] [PATCH v1 07/16] target-arm: Add HCR_EL2

[Edgar E. Iglesias]

On Tue, Jun 03, 2014 at 11:27:55AM +0100, Alex Bennée wrote:
> 
> Edgar E. Iglesias writes:
> 
> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> >
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > ---
> >  target-arm/cpu.h    | 35 +++++++++++++++++++++++++++++++++++
> >  target-arm/helper.c | 27 +++++++++++++++++++++++++++
> >  2 files changed, 62 insertions(+)
> >
> > diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> > index ef6a95d..b446478 100644
> > --- a/target-arm/cpu.h
> > +++ b/target-arm/cpu.h
> > @@ -184,6 +184,7 @@ typedef struct CPUARMState {
> >                          MPU write buffer control.  */
> >          uint32_t pmsav5_data_ap; /* PMSAv5 MPU data access permissions */
> >          uint32_t pmsav5_insn_ap; /* PMSAv5 MPU insn access permissions */
> > +        uint64_t hcr_el2; /* Hypervisor configuration register */
> >          uint32_t ifsr_el2; /* Fault status registers.  */
> >          uint64_t esr_el[4];
> >          uint32_t c6_region[8]; /* MPU base/size registers.  */
> > @@ -526,6 +527,40 @@ static inline void xpsr_write(CPUARMState *env, uint32_t val, uint32_t mask)
> >      }
> >  }
> >  
> > +#define HCR_VM        (1ULL << 0)
> > +#define HCR_SWIO      (1ULL << 1)
> > +#define HCR_PTW       (1ULL << 2)
> > +#define HCR_FMO       (1ULL << 3)
> > +#define HCR_IMO       (1ULL << 4)
> > +#define HCR_AMO       (1ULL << 5)
> > +#define HCR_VF        (1ULL << 6)
> > +#define HCR_VI        (1ULL << 7)
> > +#define HCR_VSE       (1ULL << 8)
> > +#define HCR_FB        (1ULL << 9)
> > +#define HCR_DC        (1ULL << 12)
> > +#define HCR_TWI       (1ULL << 13)
> > +#define HCR_TWE       (1ULL << 14)
> > +#define HCR_TID0      (1ULL << 15)
> > +#define HCR_TID1      (1ULL << 16)
> > +#define HCR_TID2      (1ULL << 17)
> > +#define HCR_TID3      (1ULL << 18)
> > +#define HCR_TSC       (1ULL << 19)
> > +#define HCR_TIDCP     (1ULL << 20)
> > +#define HCR_TACR      (1ULL << 21)
> > +#define HCR_TSW       (1ULL << 22)
> > +#define HCR_TPC       (1ULL << 23)
> > +#define HCR_TPU       (1ULL << 24)
> > +#define HCR_TTLB      (1ULL << 25)
> > +#define HCR_TVM       (1ULL << 26)
> > +#define HCR_TGE       (1ULL << 27)
> > +#define HCR_TDZ       (1ULL << 28)
> > +#define HCR_HCD       (1ULL << 29)
> > +#define HCR_TRVM      (1ULL << 30)
> > +#define HCR_RW        (1ULL << 31)
> > +#define HCR_CD        (1ULL << 32)
> > +#define HCR_ID        (1ULL << 33)
> > +#define HCR_RES0_MASK ((1ULL << 34) - 1)
> 
> Hmm isn't that actually HCR_MASK? I would expect the mask for the RES0
> bits to be ~((1ULL << 34) - 1) but it's not actually used for that hence
> the name confusion.


I can change the name for v2, thanks.


> 
> > +
> >  /* Return the current FPSCR value.  */
> >  uint32_t vfp_get_fpscr(CPUARMState *env);
> >  void vfp_set_fpscr(CPUARMState *env, uint32_t val);
> > diff --git a/target-arm/helper.c b/target-arm/helper.c
> > index de5ee40..cf877ae 100644
> > --- a/target-arm/helper.c
> > +++ b/target-arm/helper.c
> > @@ -2107,10 +2107,37 @@ static const ARMCPRegInfo v8_el3_no_el2_cp_reginfo[] = {
> >        .opc0 = 3, .opc1 = 4, .crn = 12, .crm = 0, .opc2 = 0,
> >        .access = PL2_RW,
> >        .readfn = arm_cp_read_zero, .writefn = arm_cp_write_ignore },
> > +    { .name = "HCR_EL2", .state = ARM_CP_STATE_AA64,
> > +      .type = ARM_CP_NO_MIGRATE,
> > +      .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 0,
> > +      .readfn = arm_cp_read_zero, .writefn = arm_cp_write_ignore },
> >      REGINFO_SENTINEL
> >  };
> >  
> > +static void hcr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
> > +{
> > +    ARMCPU *cpu = arm_env_get_cpu(env);
> > +    uint64_t res0_mask = HCR_RES0_MASK;
> > +
> > +    if (!arm_feature(env, ARM_FEATURE_EL3)) {
> > +        res0_mask &= ~HCR_HCD;
> > +    }
> > +
> > +    /* Clear RES0 bits.  */
> > +    value &= res0_mask;
> > +
> > +    if ((raw_read(env, ri) ^ value) & HCR_VM) {
> > +        /* Flush the TLB when turning VM on/off.  */
> > +        tlb_flush(CPU(cpu), 1);
> > +    }
> > +    raw_write(env, ri, value);
> > +}
> > +
> >  static const ARMCPRegInfo v8_el2_cp_reginfo[] = {
> > +    { .name = "HCR_EL2", .state = ARM_CP_STATE_AA64,
> > +      .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 0,
> > +      .access = PL2_RW, .fieldoffset = offsetof(CPUARMState, cp15.hcr_el2),
> > +      .writefn = hcr_write },
> >      { .name = "ELR_EL2", .state = ARM_CP_STATE_AA64,
> >        .type = ARM_CP_NO_MIGRATE,
> >        .opc0 = 3, .opc1 = 4, .crn = 4, .crm = 0, .opc2 = 1,
> 
> -- 
> Alex Bennée

Re: [Qemu-devel] [PATCH v1 08/16] target-arm: Add SCR_EL3

[Alex Bennée]

Edgar E. Iglesias writes:

> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  target-arm/cpu.h    | 15 +++++++++++++++
>  target-arm/helper.c | 20 ++++++++++++++++++++
>  2 files changed, 35 insertions(+)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index b446478..28521d4 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -185,6 +185,7 @@ typedef struct CPUARMState {
>          uint32_t pmsav5_data_ap; /* PMSAv5 MPU data access permissions */
>          uint32_t pmsav5_insn_ap; /* PMSAv5 MPU insn access permissions */
>          uint64_t hcr_el2; /* Hypervisor configuration register */
> +        uint32_t scr_el3; /* Secure configuration register.  */
>          uint32_t ifsr_el2; /* Fault status registers.  */
>          uint64_t esr_el[4];
>          uint32_t c6_region[8]; /* MPU base/size registers.  */
> @@ -561,6 +562,20 @@ static inline void xpsr_write(CPUARMState *env, uint32_t val, uint32_t mask)
>  #define HCR_ID        (1ULL << 33)
>  #define HCR_RES0_MASK ((1ULL << 34) - 1)
>  
> +#define SCR_NS        (1U << 0)
> +#define SCR_IRQ       (1U << 1)
> +#define SCR_FIQ       (1U << 2)
> +#define SCR_EA        (1U << 3)
> +#define SCR_SMD       (1U << 7)
> +#define SCR_HCE       (1U << 8)
> +#define SCR_SIF       (1U << 9)
> +#define SCR_RW        (1U << 10)
> +#define SCR_ST        (1U << 11)
> +#define SCR_TWI       (1U << 12)
> +#define SCR_TWE       (1U << 13)
> +#define SCR_RES1_MASK (3U << 4)
> +#define SCR_RES0_MASK (0x3fff & ~SCR_RES1_MASK)

Again I have similar cognitive dissonance with the naming of the mask
otherwise:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>


> +
>  /* Return the current FPSCR value.  */
>  uint32_t vfp_get_fpscr(CPUARMState *env);
>  void vfp_set_fpscr(CPUARMState *env, uint32_t val);
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index cf877ae..b760748 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -2162,6 +2162,22 @@ static const ARMCPRegInfo v8_el2_cp_reginfo[] = {
>      REGINFO_SENTINEL
>  };
>  
> +static void scr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
> +{
> +    uint32_t res0_mask = SCR_RES0_MASK;
> +
> +    if (!arm_feature(env, ARM_FEATURE_EL2)) {
> +        res0_mask &= ~SCR_HCE;
> +    }
> +
> +    /* Set RES1 bits.  */
> +    value |= SCR_RES1_MASK;
> +
> +    /* Clear RES0 bits.  */
> +    value &= res0_mask;
> +    raw_write(env, ri, value);
> +}
> +
>  static const ARMCPRegInfo v8_el3_cp_reginfo[] = {
>      { .name = "ELR_EL3", .state = ARM_CP_STATE_AA64,
>        .type = ARM_CP_NO_MIGRATE,
> @@ -2184,6 +2200,10 @@ static const ARMCPRegInfo v8_el3_cp_reginfo[] = {
>        .access = PL3_RW, .writefn = vbar_write,
>        .fieldoffset = offsetof(CPUARMState, cp15.vbar_el[3]),
>        .resetvalue = 0 },
> +    { .name = "SCR_EL3", .state = ARM_CP_STATE_AA64,
> +      .opc0 = 3, .opc1 = 6, .crn = 1, .crm = 1, .opc2 = 0,
> +      .access = PL3_RW, .fieldoffset = offsetof(CPUARMState, cp15.scr_el3),
> +      .writefn = scr_write },
>      REGINFO_SENTINEL
>  };

-- 
Alex Bennée

Re: [Qemu-devel] [PATCH v1 10/16] target-arm: Break out exception masking to a separate func

[Alex Bennée]

Edgar E. Iglesias writes:

> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  cpu-exec.c       |  5 ++---
>  target-arm/cpu.h | 16 ++++++++++++++++
>  2 files changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/cpu-exec.c b/cpu-exec.c
> index 38e5f02..a579ffc 100644
> --- a/cpu-exec.c
> +++ b/cpu-exec.c
> @@ -478,7 +478,7 @@ int cpu_exec(CPUArchState *env)
>                      }
>  #elif defined(TARGET_ARM)
>                      if (interrupt_request & CPU_INTERRUPT_FIQ
> -                        && !(env->daif & PSTATE_F)) {
> +                        && arm_excp_unmasked(cpu, EXCP_FIQ)) {
>                          cpu->exception_index = EXCP_FIQ;
>                          cc->do_interrupt(cpu);
>                          next_tb = 0;
> @@ -493,8 +493,7 @@ int cpu_exec(CPUArchState *env)
>                         We avoid this by disabling interrupts when
>                         pc contains a magic address.  */
>                      if (interrupt_request & CPU_INTERRUPT_HARD
> -                        && ((IS_M(env) && env->regs[15] < 0xfffffff0)
> -                            || !(env->daif & PSTATE_I))) {
> +                        && arm_excp_unmasked(cpu, EXCP_IRQ)) {
>                          cpu->exception_index = EXCP_IRQ;
>                          cc->do_interrupt(cpu);
>                          next_tb = 0;
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 5c74adc..9eddcc1 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -1130,6 +1130,22 @@ 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)
> +{
> +    CPUARMState *env = cs->env_ptr;
> +
> +    switch (excp_idx) {
> +    case EXCP_FIQ:
> +        return !(env->daif & PSTATE_F);
> +    case EXCP_IRQ:
> +        return ((IS_M(env) && env->regs[15] < 0xfffffff0)
> +                            || !(env->daif & PSTATE_I));
> +    default:
> +        assert(0);

g_assert_not_reached() is clearer about the intent here.

> +        break;
> +    }
> +}
> +
>  static inline CPUARMState *cpu_init(const char *cpu_model)
>  {
>      ARMCPU *cpu = cpu_arm_init(cpu_model);

-- 
Alex Bennée

Re: [Qemu-devel] [PATCH v1 10/16] target-arm: Break out exception masking to a separate func

[Edgar E. Iglesias]

On Tue, Jun 03, 2014 at 11:32:59AM +0100, Alex Bennée wrote:
> 
> Edgar E. Iglesias writes:
> 
> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> >
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > ---
> >  cpu-exec.c       |  5 ++---
> >  target-arm/cpu.h | 16 ++++++++++++++++
> >  2 files changed, 18 insertions(+), 3 deletions(-)
> >
> > diff --git a/cpu-exec.c b/cpu-exec.c
> > index 38e5f02..a579ffc 100644
> > --- a/cpu-exec.c
> > +++ b/cpu-exec.c
> > @@ -478,7 +478,7 @@ int cpu_exec(CPUArchState *env)
> >                      }
> >  #elif defined(TARGET_ARM)
> >                      if (interrupt_request & CPU_INTERRUPT_FIQ
> > -                        && !(env->daif & PSTATE_F)) {
> > +                        && arm_excp_unmasked(cpu, EXCP_FIQ)) {
> >                          cpu->exception_index = EXCP_FIQ;
> >                          cc->do_interrupt(cpu);
> >                          next_tb = 0;
> > @@ -493,8 +493,7 @@ int cpu_exec(CPUArchState *env)
> >                         We avoid this by disabling interrupts when
> >                         pc contains a magic address.  */
> >                      if (interrupt_request & CPU_INTERRUPT_HARD
> > -                        && ((IS_M(env) && env->regs[15] < 0xfffffff0)
> > -                            || !(env->daif & PSTATE_I))) {
> > +                        && arm_excp_unmasked(cpu, EXCP_IRQ)) {
> >                          cpu->exception_index = EXCP_IRQ;
> >                          cc->do_interrupt(cpu);
> >                          next_tb = 0;
> > diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> > index 5c74adc..9eddcc1 100644
> > --- a/target-arm/cpu.h
> > +++ b/target-arm/cpu.h
> > @@ -1130,6 +1130,22 @@ 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)
> > +{
> > +    CPUARMState *env = cs->env_ptr;
> > +
> > +    switch (excp_idx) {
> > +    case EXCP_FIQ:
> > +        return !(env->daif & PSTATE_F);
> > +    case EXCP_IRQ:
> > +        return ((IS_M(env) && env->regs[15] < 0xfffffff0)
> > +                            || !(env->daif & PSTATE_I));
> > +    default:
> > +        assert(0);
> 
> g_assert_not_reached() is clearer about the intent here.

Sounds good, will change it.

Thanks,
Edgar


> 
> > +        break;
> > +    }
> > +}
> > +
> >  static inline CPUARMState *cpu_init(const char *cpu_model)
> >  {
> >      ARMCPU *cpu = cpu_arm_init(cpu_model);
> 
> -- 
> Alex Bennée

Re: [Qemu-devel] [PATCH v1 12/16] target-arm: A64: Correct updates to FAR and ESR on exceptions

[Alex Bennée]

Edgar E. Iglesias writes:

> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Not all exception types update both FAR and ESR.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  target-arm/helper-a64.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
> index 7d94a74..c91005f 100644
> --- a/target-arm/helper-a64.c
> +++ b/target-arm/helper-a64.c
> @@ -466,18 +466,16 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
>                        env->exception.syndrome);
>      }
>  
> -    env->cp15.esr_el[new_el] = env->exception.syndrome;
> -    env->cp15.far_el[new_el] = env->exception.vaddress;
> -
>      switch (cs->exception_index) {
>      case EXCP_PREFETCH_ABORT:
>      case EXCP_DATA_ABORT:
> +        env->cp15.far_el[new_el] = env->exception.vaddress;
>          qemu_log_mask(CPU_LOG_INT, "...with FAR 0x%" PRIx64 "\n",
>                        env->cp15.far_el[new_el]);
> -        break;
>      case EXCP_BKPT:
>      case EXCP_UDEF:
>      case EXCP_SWI:
> +        env->cp15.esr_el[new_el] = env->exception.syndrome;
>          break;
>      case EXCP_IRQ:
>          addr += 0x80;


Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée

Re: [Qemu-devel] [PATCH v1 13/16] target-arm: A64: Emulate the HVC insn

[Alex Bennée]

Edgar E. Iglesias writes:

> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  target-arm/cpu.h           |  7 ++-----
>  target-arm/helper-a64.c    |  1 +
>  target-arm/helper.c        | 39 +++++++++++++++++++++++++++++++++++++++
>  target-arm/helper.h        |  1 +
>  target-arm/internals.h     |  6 ++++++
>  target-arm/op_helper.c     | 21 +++++++++++++++++++++
>  target-arm/translate-a64.c | 21 ++++++++++++++++-----
>  7 files changed, 86 insertions(+), 10 deletions(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 66c58bd..1a26ed4 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -51,6 +51,7 @@
>  #define EXCP_EXCEPTION_EXIT  8   /* Return from v7M exception.  */
>  #define EXCP_KERNEL_TRAP     9   /* Jumped to kernel code page.  */
>  #define EXCP_STREX          10
> +#define EXCP_HVC            11   /* HyperVisor Call */
>  
>  #define ARMV7M_EXCP_RESET   1
>  #define ARMV7M_EXCP_NMI     2
> @@ -715,11 +716,7 @@ static inline bool arm_el_is_aa64(CPUARMState *env, int el)
>  }
>  
>  void arm_cpu_list(FILE *f, fprintf_function cpu_fprintf);
> -static inline unsigned int arm_excp_target_el(CPUState *cs,
> -                                              unsigned int excp_idx)
> -{
> -    return 1;
> -}
> +unsigned int arm_excp_target_el(CPUState *cs, unsigned int excp_idx);

If the earlier commit had added this in the final place to start with
the functional diff would be clearer (although it's easy to eyeball in
this case).

>  
>  /* Interface between CPU and Interrupt controller.  */
>  void armv7m_nvic_set_pending(void *opaque, int irq);
> diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
> index c91005f..974fa66 100644
> --- a/target-arm/helper-a64.c
> +++ b/target-arm/helper-a64.c
> @@ -475,6 +475,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
>      case EXCP_BKPT:
>      case EXCP_UDEF:
>      case EXCP_SWI:
> +    case EXCP_HVC:
>          env->cp15.esr_el[new_el] = env->exception.syndrome;
>          break;
>      case EXCP_IRQ:
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index b760748..5b2070c 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -3208,6 +3208,11 @@ 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)
> +{
> +    return 1;
> +}
> +
>  #else
>  
>  /* Map CPU modes onto saved register banks.  */
> @@ -3263,6 +3268,40 @@ void switch_mode(CPUARMState *env, int mode)
>      env->spsr = env->banked_spsr[i];
>  }
>  
> +/*
> + * Determine the target EL for a given exception type.
> + */
> +unsigned int arm_excp_target_el(CPUState *cs, unsigned int excp_idx)
> +{
> +    CPUARMState *env = cs->env_ptr;
> +    unsigned int cur_el = arm_current_pl(env);
> +    unsigned int target_el = 1;
> +    bool route_to_el2 = false;
> +    /* FIXME: Use actual secure state.  */
> +    bool secure = false;

Should this be here?

<snip>
>  static inline void arm_log_exception(int idx)
> @@ -204,6 +205,11 @@ static inline uint32_t syn_aa64_svc(uint32_t imm16)
>      return (EC_AA64_SVC << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff);
>  }
>  
> +static inline uint32_t syn_aa64_hvc(uint32_t imm16)
> +{
> +    return (EC_AA64_HVC << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff);
> +}

The mask seems superfluous (as it is for arm_log_exception)

> +
>  static inline uint32_t syn_aa32_svc(uint32_t imm16, bool is_thumb)
>  {
>      return (EC_AA32_SVC << ARM_EL_EC_SHIFT) | (imm16 & 0xffff)
> diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> index 581dc09..6bf34b0 100644
> --- a/target-arm/op_helper.c
> +++ b/target-arm/op_helper.c
> @@ -384,6 +384,27 @@ void HELPER(msr_i_pstate)(CPUARMState *env, uint32_t op, uint32_t imm)
>      }
>  }
>  
> +void HELPER(hvc)(CPUARMState *env, uint32_t syndrome)
> +{
> +    bool udef;
> +
> +    /* We've already checked that EL2 exists at translation time.
> +     * EL3.HCE has priority over EL2.HCD.
> +     */
> +    if (arm_feature(env, ARM_FEATURE_EL3)) {
> +        udef = !(env->cp15.scr_el3 & SCR_HCE);
> +    } else {
> +        udef = env->cp15.hcr_el2 & HCR_HCD;
> +    }
> +
> +    if (udef) {
> +        env->exception.syndrome = syn_uncategorized();
> +        raise_exception(env, EXCP_UDEF);
> +    }
> +    env->exception.syndrome = syndrome;
> +    raise_exception(env, EXCP_HVC);
> +}
> +
>  void HELPER(exception_return)(CPUARMState *env)
>  {
>      int cur_el = arm_current_pl(env);
> diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
> index 9f964df..3981ee1 100644
> --- a/target-arm/translate-a64.c
> +++ b/target-arm/translate-a64.c
> @@ -1433,17 +1433,28 @@ static void disas_exc(DisasContext *s, uint32_t insn)
>      int opc = extract32(insn, 21, 3);
>      int op2_ll = extract32(insn, 0, 5);
>      int imm16 = extract32(insn, 5, 16);
> +    TCGv_i32 tmp;
>  
>      switch (opc) {
>      case 0:
> -        /* SVC, HVC, SMC; since we don't support the Virtualization
> -         * or TrustZone extensions these all UNDEF except SVC.
> -         */
> -        if (op2_ll != 1) {
> +        switch (op2_ll) {
> +        case 1:
> +            gen_exception_insn(s, 0, EXCP_SWI, syn_aa64_svc(imm16));
> +            break;
> +        case 2:
> +            if (!arm_dc_feature(s, ARM_FEATURE_EL2) || s->current_pl == 0) {
> +                unallocated_encoding(s);
> +                break;
> +            }
> +            tmp = tcg_const_i32(syn_aa64_hvc(imm16));
> +            gen_a64_set_pc_im(s->pc);
> +            gen_helper_hvc(cpu_env, tmp);
> +            tcg_temp_free_i32(tmp);
> +            break;
> +        default:
>              unallocated_encoding(s);
>              break;
>          }
> -        gen_exception_insn(s, 0, EXCP_SWI, syn_aa64_svc(imm16));
>          break;
>      case 1:
>          if (op2_ll != 0) {

-- 
Alex Bennée

Re: [Qemu-devel] [PATCH v1 13/16] target-arm: A64: Emulate the HVC insn

[Edgar E. Iglesias]

On Tue, Jun 03, 2014 at 11:41:25AM +0100, Alex Bennée wrote:
> 
> Edgar E. Iglesias writes:
> 
> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> >
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > ---
> >  target-arm/cpu.h           |  7 ++-----
> >  target-arm/helper-a64.c    |  1 +
> >  target-arm/helper.c        | 39 +++++++++++++++++++++++++++++++++++++++
> >  target-arm/helper.h        |  1 +
> >  target-arm/internals.h     |  6 ++++++
> >  target-arm/op_helper.c     | 21 +++++++++++++++++++++
> >  target-arm/translate-a64.c | 21 ++++++++++++++++-----
> >  7 files changed, 86 insertions(+), 10 deletions(-)
> >
> > diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> > index 66c58bd..1a26ed4 100644
> > --- a/target-arm/cpu.h
> > +++ b/target-arm/cpu.h
> > @@ -51,6 +51,7 @@
> >  #define EXCP_EXCEPTION_EXIT  8   /* Return from v7M exception.  */
> >  #define EXCP_KERNEL_TRAP     9   /* Jumped to kernel code page.  */
> >  #define EXCP_STREX          10
> > +#define EXCP_HVC            11   /* HyperVisor Call */
> >  
> >  #define ARMV7M_EXCP_RESET   1
> >  #define ARMV7M_EXCP_NMI     2
> > @@ -715,11 +716,7 @@ static inline bool arm_el_is_aa64(CPUARMState *env, int el)
> >  }
> >  
> >  void arm_cpu_list(FILE *f, fprintf_function cpu_fprintf);
> > -static inline unsigned int arm_excp_target_el(CPUState *cs,
> > -                                              unsigned int excp_idx)
> > -{
> > -    return 1;
> > -}
> > +unsigned int arm_excp_target_el(CPUState *cs, unsigned int excp_idx);
> 
> If the earlier commit had added this in the final place to start with
> the functional diff would be clearer (although it's easy to eyeball in
> this case).

Right, I can change that for v2.


> 
> >  
> >  /* Interface between CPU and Interrupt controller.  */
> >  void armv7m_nvic_set_pending(void *opaque, int irq);
> > diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
> > index c91005f..974fa66 100644
> > --- a/target-arm/helper-a64.c
> > +++ b/target-arm/helper-a64.c
> > @@ -475,6 +475,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
> >      case EXCP_BKPT:
> >      case EXCP_UDEF:
> >      case EXCP_SWI:
> > +    case EXCP_HVC:
> >          env->cp15.esr_el[new_el] = env->exception.syndrome;
> >          break;
> >      case EXCP_IRQ:
> > diff --git a/target-arm/helper.c b/target-arm/helper.c
> > index b760748..5b2070c 100644
> > --- a/target-arm/helper.c
> > +++ b/target-arm/helper.c
> > @@ -3208,6 +3208,11 @@ 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)
> > +{
> > +    return 1;
> > +}
> > +
> >  #else
> >  
> >  /* Map CPU modes onto saved register banks.  */
> > @@ -3263,6 +3268,40 @@ void switch_mode(CPUARMState *env, int mode)
> >      env->spsr = env->banked_spsr[i];
> >  }
> >  
> > +/*
> > + * Determine the target EL for a given exception type.
> > + */
> > +unsigned int arm_excp_target_el(CPUState *cs, unsigned int excp_idx)
> > +{
> > +    CPUARMState *env = cs->env_ptr;
> > +    unsigned int cur_el = arm_current_pl(env);
> > +    unsigned int target_el = 1;
> > +    bool route_to_el2 = false;
> > +    /* FIXME: Use actual secure state.  */
> > +    bool secure = false;
> 
> Should this be here?

I've put it there to make it easier for the TZ patches to identify the
places they need to update. + it allows me to code the conditions for
the exception routing (wrt S/NS).


> 
> <snip>
> >  static inline void arm_log_exception(int idx)
> > @@ -204,6 +205,11 @@ static inline uint32_t syn_aa64_svc(uint32_t imm16)
> >      return (EC_AA64_SVC << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff);
> >  }
> >  
> > +static inline uint32_t syn_aa64_hvc(uint32_t imm16)
> > +{
> > +    return (EC_AA64_HVC << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff);
> > +}
> 
> The mask seems superfluous (as it is for arm_log_exception)

Sorry, can you clarify what you mean here? Are you refering to the imm16?


> 
> > +
> >  static inline uint32_t syn_aa32_svc(uint32_t imm16, bool is_thumb)
> >  {
> >      return (EC_AA32_SVC << ARM_EL_EC_SHIFT) | (imm16 & 0xffff)
> > diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> > index 581dc09..6bf34b0 100644
> > --- a/target-arm/op_helper.c
> > +++ b/target-arm/op_helper.c
> > @@ -384,6 +384,27 @@ void HELPER(msr_i_pstate)(CPUARMState *env, uint32_t op, uint32_t imm)
> >      }
> >  }
> >  
> > +void HELPER(hvc)(CPUARMState *env, uint32_t syndrome)
> > +{
> > +    bool udef;
> > +
> > +    /* We've already checked that EL2 exists at translation time.
> > +     * EL3.HCE has priority over EL2.HCD.
> > +     */
> > +    if (arm_feature(env, ARM_FEATURE_EL3)) {
> > +        udef = !(env->cp15.scr_el3 & SCR_HCE);
> > +    } else {
> > +        udef = env->cp15.hcr_el2 & HCR_HCD;
> > +    }
> > +
> > +    if (udef) {
> > +        env->exception.syndrome = syn_uncategorized();
> > +        raise_exception(env, EXCP_UDEF);
> > +    }
> > +    env->exception.syndrome = syndrome;
> > +    raise_exception(env, EXCP_HVC);
> > +}
> > +
> >  void HELPER(exception_return)(CPUARMState *env)
> >  {
> >      int cur_el = arm_current_pl(env);
> > diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
> > index 9f964df..3981ee1 100644
> > --- a/target-arm/translate-a64.c
> > +++ b/target-arm/translate-a64.c
> > @@ -1433,17 +1433,28 @@ static void disas_exc(DisasContext *s, uint32_t insn)
> >      int opc = extract32(insn, 21, 3);
> >      int op2_ll = extract32(insn, 0, 5);
> >      int imm16 = extract32(insn, 5, 16);
> > +    TCGv_i32 tmp;
> >  
> >      switch (opc) {
> >      case 0:
> > -        /* SVC, HVC, SMC; since we don't support the Virtualization
> > -         * or TrustZone extensions these all UNDEF except SVC.
> > -         */
> > -        if (op2_ll != 1) {
> > +        switch (op2_ll) {
> > +        case 1:
> > +            gen_exception_insn(s, 0, EXCP_SWI, syn_aa64_svc(imm16));
> > +            break;
> > +        case 2:
> > +            if (!arm_dc_feature(s, ARM_FEATURE_EL2) || s->current_pl == 0) {
> > +                unallocated_encoding(s);
> > +                break;
> > +            }
> > +            tmp = tcg_const_i32(syn_aa64_hvc(imm16));
> > +            gen_a64_set_pc_im(s->pc);
> > +            gen_helper_hvc(cpu_env, tmp);
> > +            tcg_temp_free_i32(tmp);
> > +            break;
> > +        default:
> >              unallocated_encoding(s);
> >              break;
> >          }
> > -        gen_exception_insn(s, 0, EXCP_SWI, syn_aa64_svc(imm16));
> >          break;
> >      case 1:
> >          if (op2_ll != 0) {
> 
> -- 
> Alex Bennée

Re: [Qemu-devel] [PATCH v1 13/16] target-arm: A64: Emulate the HVC insn

[Alex Bennée]

Edgar E. Iglesias writes:

> On Tue, Jun 03, 2014 at 11:41:25AM +0100, Alex Bennée wrote:
>> 
>> Edgar E. Iglesias writes:
>> >  static inline void arm_log_exception(int idx)
>> > @@ -204,6 +205,11 @@ static inline uint32_t syn_aa64_svc(uint32_t imm16)
>> >      return (EC_AA64_SVC << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff);
>> >  }
>> >  
>> > +static inline uint32_t syn_aa64_hvc(uint32_t imm16)
>> > +{
>> > +    return (EC_AA64_HVC << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff);
>> > +}
>> 
>> The mask seems superfluous (as it is for arm_log_exception)
>
> Sorry, can you clarify what you mean here? Are you refering to the imm16?

Yes the imm16. It's the result of an extract32(..,..,16) so I can't see
how it wouldn't already be correctly masked.

-- 
Alex Bennée

Re: [Qemu-devel] [PATCH v1 13/16] target-arm: A64: Emulate the HVC insn

[Edgar E. Iglesias]

On Wed, Jun 04, 2014 at 08:26:51AM +0100, Alex Benn�e wrote:
> 
> Edgar E. Iglesias writes:
> 
> > On Tue, Jun 03, 2014 at 11:41:25AM +0100, Alex Benn?e wrote:
> >> 
> >> Edgar E. Iglesias writes:
> >> >  static inline void arm_log_exception(int idx)
> >> > @@ -204,6 +205,11 @@ static inline uint32_t syn_aa64_svc(uint32_t imm16)
> >> >      return (EC_AA64_SVC << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff);
> >> >  }
> >> >  
> >> > +static inline uint32_t syn_aa64_hvc(uint32_t imm16)
> >> > +{
> >> > +    return (EC_AA64_HVC << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff);
> >> > +}
> >> 
> >> The mask seems superfluous (as it is for arm_log_exception)
> >
> > Sorry, can you clarify what you mean here? Are you refering to the imm16?
> 
> Yes the imm16. It's the result of an extract32(..,..,16) so I can't see
> how it wouldn't already be correctly masked.

Right, so my first version here had a uint16_t imm16, but I changed
it to keep it consistent with the other functions. I'm happy to
change things to match their use with additional patches. I'll do
something for v2.

Re: [Qemu-devel] [PATCH v1 15/16] target-arm: Add IRQ and FIQ routing to EL2 and 3

[Alex Bennée]

Edgar E. Iglesias writes:

> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  target-arm/cpu.h    | 10 ++++++++++
>  target-arm/helper.c | 16 ++++++++++++++++
>  2 files changed, 26 insertions(+)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index b3631f2..d15e8d2 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -1133,6 +1133,8 @@ static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx)
>      CPUARMState *env = cs->env_ptr;
>      unsigned int cur_el = arm_current_pl(env);
>      unsigned int target_el = arm_excp_target_el(cs, excp_idx);
> +    /* FIXME: Use actual secure state.  */
> +    bool secure = false;

Again FIXME?

>  
>      /* Don't take exceptions if they target a lower EL.  */
>      if (cur_el > target_el) {
> @@ -1141,8 +1143,16 @@ static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx)
>  
>      switch (excp_idx) {
>      case EXCP_FIQ:
> +        if (!secure && cur_el < 2 && target_el == 2
> +            && (env->cp15.hcr_el2 & HCR_FMO)) {
> +            return true;
> +        }
>          return !(env->daif & PSTATE_F);
>      case EXCP_IRQ:
> +        if (!secure && cur_el < 2 && target_el == 2
> +            && (env->cp15.hcr_el2 & HCR_IMO)) {
> +            return true;
> +        }
>          return ((IS_M(env) && env->regs[15] < 0xfffffff0)
>                              || !(env->daif & PSTATE_I));

Hmm identical but subtly different tests may cause later trip ups.

> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 35091ea..649476b 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -3304,6 +3304,22 @@ unsigned int arm_excp_target_el(CPUState *cs, unsigned int excp_idx)
>              target_el = 2;
>          }
>          break;
> +    case EXCP_IRQ:
> +        if (!secure && (env->cp15.hcr_el2 & HCR_IMO)) {
> +            target_el = 2;
> +        }
> +        if (env->cp15.scr_el3 & SCR_IRQ) {
> +            target_el = 3;
> +        }
> +        break;
> +    case EXCP_FIQ:
> +        if (!secure && (env->cp15.hcr_el2 & HCR_FMO)) {
> +            target_el = 2;
> +        }
> +        if (env->cp15.scr_el3 & SCR_FIQ) {
> +            target_el = 3;
> +        }
> +        break;

I wonder if it's possible to make the common logic more common while
keeping the differences?

>      }
>      return target_el;
>  }

-- 
Alex Bennée

Re: [Qemu-devel] [PATCH v1 11/16] target-arm: Don't take interrupts targeting lower ELs

[Aggeler Fabian]

On 30 May 2014, at 09:28, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:

> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> 
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
> target-arm/cpu.h | 7 +++++++
> 1 file changed, 7 insertions(+)
> 
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 9eddcc1..66c58bd 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -1133,6 +1133,13 @@ bool write_cpustate_to_list(ARMCPU *cpu);
> static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx)
> {
>     CPUARMState *env = cs->env_ptr;
> +    unsigned int cur_el = arm_current_pl(env);
> +    unsigned int target_el = arm_excp_target_el(cs, excp_idx);
> +
> +    /* Don't take exceptions if they target a lower EL.  */
> +    if (cur_el > target_el) {
> +        return false;
> +    }

Hi Edgar

When making arm_excp_unmasked() reflect tables D1-13, D1-14, D1-15 
and G1-18, G1-19 in ARM ARMv8 this should not be necessary if I am 
not mistaken. Cases in which target_el is lower than cur_el are marked with 
a P (pending) in the table. Or am I missing something interpreting the
tables?

I extended your arm_excp_unmasked() and arm_excp_target_el() to reflect 
the behaviour shown in the tables in ARM ARMv8 and ARM ARMv7. I will 
send them with the TZ patches.

Best,
Fabian

> 
>     switch (excp_idx) {
>     case EXCP_FIQ:
> -- 
> 1.8.3.2
> 

Re: [Qemu-devel] [PATCH v1 11/16] target-arm: Don't take interrupts targeting lower ELs

[Edgar E. Iglesias]

On Sun, Jun 08, 2014 at 03:51:24PM +0000, Aggeler  Fabian wrote:
> 
> On 30 May 2014, at 09:28, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> 
> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> > 
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > ---
> > target-arm/cpu.h | 7 +++++++
> > 1 file changed, 7 insertions(+)
> > 
> > diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> > index 9eddcc1..66c58bd 100644
> > --- a/target-arm/cpu.h
> > +++ b/target-arm/cpu.h
> > @@ -1133,6 +1133,13 @@ bool write_cpustate_to_list(ARMCPU *cpu);
> > static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx)
> > {
> >     CPUARMState *env = cs->env_ptr;
> > +    unsigned int cur_el = arm_current_pl(env);
> > +    unsigned int target_el = arm_excp_target_el(cs, excp_idx);
> > +
> > +    /* Don't take exceptions if they target a lower EL.  */
> > +    if (cur_el > target_el) {
> > +        return false;
> > +    }
> 
> Hi Edgar

Hi Fabian,

> 
> When making arm_excp_unmasked() reflect tables D1-13, D1-14, D1-15 
> and G1-18, G1-19 in ARM ARMv8 this should not be necessary if I am 
> not mistaken. Cases in which target_el is lower than cur_el are marked with 
> a P (pending) in the table. Or am I missing something interpreting the
> tables?

This function is called to check if we can take a pending interrupt or
exception with current CPU state. It does not clear pending exceptions.
In this case, if the target_el is lower than the current EL we return false
and leave the exception pending (to be taken later).

> 
> I extended your arm_excp_unmasked() and arm_excp_target_el() to reflect 
> the behaviour shown in the tables in ARM ARMv8 and ARM ARMv7. I will 
> send them with the TZ patches.

Great, thanks.

Cheers,
Edgar


> 
> Best,
> Fabian
> 
> > 
> >     switch (excp_idx) {
> >     case EXCP_FIQ:
> > -- 
> > 1.8.3.2
> > 
> 

Re: [Qemu-devel] [PATCH v1 11/16] target-arm: Don't take interrupts targeting lower ELs

[Aggeler Fabian]

On 09 Jun 2014, at 01:43, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:

> On Sun, Jun 08, 2014 at 03:51:24PM +0000, Aggeler  Fabian wrote:
>> 
>> On 30 May 2014, at 09:28, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
>> 
>>> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>>> 
>>> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>>> ---
>>> target-arm/cpu.h | 7 +++++++
>>> 1 file changed, 7 insertions(+)
>>> 
>>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>>> index 9eddcc1..66c58bd 100644
>>> --- a/target-arm/cpu.h
>>> +++ b/target-arm/cpu.h
>>> @@ -1133,6 +1133,13 @@ bool write_cpustate_to_list(ARMCPU *cpu);
>>> static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx)
>>> {
>>>    CPUARMState *env = cs->env_ptr;
>>> +    unsigned int cur_el = arm_current_pl(env);
>>> +    unsigned int target_el = arm_excp_target_el(cs, excp_idx);
>>> +
>>> +    /* Don't take exceptions if they target a lower EL.  */
>>> +    if (cur_el > target_el) {
>>> +        return false;
>>> +    }
>> 
>> Hi Edgar
> 
> Hi Fabian,
> 
>> 
>> When making arm_excp_unmasked() reflect tables D1-13, D1-14, D1-15 
>> and G1-18, G1-19 in ARM ARMv8 this should not be necessary if I am 
>> not mistaken. Cases in which target_el is lower than cur_el are marked with 
>> a P (pending) in the table. Or am I missing something interpreting the
>> tables?
> 
> This function is called to check if we can take a pending interrupt or
> exception with current CPU state. It does not clear pending exceptions.
> In this case, if the target_el is lower than the current EL we return false
> and leave the exception pending (to be taken later).

I know, but what I want to say is, doesn’t the table which I mentioned reflect this
by marking it with a P for pending for cases where target_el would be lower than
the current EL it is taken from?

So if we handle these cases in arm_excp_unmasked() we don’t need to call 
arm_excp_target_el() to check.

> 
>> 
>> I extended your arm_excp_unmasked() and arm_excp_target_el() to reflect 
>> the behaviour shown in the tables in ARM ARMv8 and ARM ARMv7. I will 
>> send them with the TZ patches.
> 
> Great, thanks.
> 
> Cheers,
> Edgar
> 
> 
>> 
>> Best,
>> Fabian
>> 
>>> 
>>>    switch (excp_idx) {
>>>    case EXCP_FIQ:
>>> -- 
>>> 1.8.3.2

Re: [Qemu-devel] [PATCH v1 00/16] target-arm: Parts of the AArch64 EL2/3 exception model

[Peter Maydell]

On 30 May 2014 08:28, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> This is a second round of AArch64 EL2/3 patches working on the exception
> model. Among other things adding HVC/SMC, interrupt routing to EL2/3 and
> Virtual IRQs/FIQs. The VIRQ/VFIQ support only adds the external signal
> delivery method.
>
> Patch 3 is a bug fix.
> Patch 14 fails checkpatch, seems like a bug in checkpatch, CC:d Blue.
>
> This conflicts slightly with the PSCI emulation patches that Rob posted.
> A rebase should be trivial, hooking in the PSCI emulation calls in the
> HVC/SMC helpers.

Sorry for letting this patchset sit in my to-review queue for so long.
Patches 1-6 are good and I'm going to put them in target-arm.next.
Patches 9 and 11 I've added my R-by to. Patches 7, 8, 10, 12..16
I've replied to with review comments.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v1 00/16] target-arm: Parts of the AArch64 EL2/3 exception model

[Peter Maydell]

On 1 August 2014 15:35, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 30 May 2014 08:28, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
>> This is a second round of AArch64 EL2/3 patches working on the exception
>> model. Among other things adding HVC/SMC, interrupt routing to EL2/3 and
>> Virtual IRQs/FIQs. The VIRQ/VFIQ support only adds the external signal
>> delivery method.
>>
>> Patch 3 is a bug fix.
>> Patch 14 fails checkpatch, seems like a bug in checkpatch, CC:d Blue.
>>
>> This conflicts slightly with the PSCI emulation patches that Rob posted.
>> A rebase should be trivial, hooking in the PSCI emulation calls in the
>> HVC/SMC helpers.
>
> Sorry for letting this patchset sit in my to-review queue for so long.
> Patches 1-6 are good and I'm going to put them in target-arm.next.
> Patches 9 and 11 I've added my R-by to. Patches 7, 8, 10, 12..16
> I've replied to with review comments.

...this reply was to the v1 cover letter, but I did review v3 :-)

thanks
-- PMM

Re: [Qemu-devel] [PATCH v1 00/16] target-arm: Parts of the AArch64 EL2/3 exception model

[Edgar E. Iglesias]

On Fri, Aug 01, 2014 at 03:35:55PM +0100, Peter Maydell wrote:
> On 30 May 2014 08:28, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> > This is a second round of AArch64 EL2/3 patches working on the exception
> > model. Among other things adding HVC/SMC, interrupt routing to EL2/3 and
> > Virtual IRQs/FIQs. The VIRQ/VFIQ support only adds the external signal
> > delivery method.
> >
> > Patch 3 is a bug fix.
> > Patch 14 fails checkpatch, seems like a bug in checkpatch, CC:d Blue.
> >
> > This conflicts slightly with the PSCI emulation patches that Rob posted.
> > A rebase should be trivial, hooking in the PSCI emulation calls in the
> > HVC/SMC helpers.
> 
> Sorry for letting this patchset sit in my to-review queue for so long.
> Patches 1-6 are good and I'm going to put them in target-arm.next.
> Patches 9 and 11 I've added my R-by to. Patches 7, 8, 10, 12..16
> I've replied to with review comments.

Thanks Peter,

I think I've addressed most of your comments, sending out v4 now.

I fixed up the PC for trapped HVC/SMC and for SMC routed to EL2
but left the exception raising as is. I'm happy to update it if
you have a better idea.

Cheers,
Edgar