[Qemu-devel] [PATCH v1 0/2] arm: Extend PAR format determination

[Qemu-devel] [PATCH v1 0/2] arm: Extend PAR format determination

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Hi,

These are a couple of patches that implement more details of the
32 vs 64bit PAR format determination for AT operations.
This is due to issues we ran into when running Xen.

Best regards,
Edgar

Edgar E. Iglesias (2):
  target-arm: Move the regime_xxx helpers
  target-arm: Extend PAR format determination

 target/arm/helper.c | 434 ++++++++++++++++++++++++++++------------------------
 1 file changed, 231 insertions(+), 203 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH v1 1/2] target-arm: Move the regime_xxx helpers

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Move the regime_xxx helpers in preparation for future code
that will reuse them.

No functional change.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 target/arm/helper.c | 404 ++++++++++++++++++++++++++--------------------------
 1 file changed, 202 insertions(+), 202 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 2594faa..fd1027e 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -35,6 +35,208 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
 #define PMCRD   0x8
 #define PMCRC   0x4
 #define PMCRE   0x1
+
+/* Return the exception level which controls this address translation regime */
+static inline uint32_t regime_el(CPUARMState *env, ARMMMUIdx mmu_idx)
+{
+    switch (mmu_idx) {
+    case ARMMMUIdx_S2NS:
+    case ARMMMUIdx_S1E2:
+        return 2;
+    case ARMMMUIdx_S1E3:
+        return 3;
+    case ARMMMUIdx_S1SE0:
+        return arm_el_is_aa64(env, 3) ? 1 : 3;
+    case ARMMMUIdx_S1SE1:
+    case ARMMMUIdx_S1NSE0:
+    case ARMMMUIdx_S1NSE1:
+    case ARMMMUIdx_MPriv:
+    case ARMMMUIdx_MNegPri:
+    case ARMMMUIdx_MUser:
+        return 1;
+    default:
+        g_assert_not_reached();
+    }
+}
+
+/* Return true if this address translation regime is secure */
+static inline bool regime_is_secure(CPUARMState *env, ARMMMUIdx mmu_idx)
+{
+    switch (mmu_idx) {
+    case ARMMMUIdx_S12NSE0:
+    case ARMMMUIdx_S12NSE1:
+    case ARMMMUIdx_S1NSE0:
+    case ARMMMUIdx_S1NSE1:
+    case ARMMMUIdx_S1E2:
+    case ARMMMUIdx_S2NS:
+    case ARMMMUIdx_MPriv:
+    case ARMMMUIdx_MNegPri:
+    case ARMMMUIdx_MUser:
+        return false;
+    case ARMMMUIdx_S1E3:
+    case ARMMMUIdx_S1SE0:
+    case ARMMMUIdx_S1SE1:
+        return true;
+    default:
+        g_assert_not_reached();
+    }
+}
+
+/* Return the SCTLR value which controls this address translation regime */
+static inline uint32_t regime_sctlr(CPUARMState *env, ARMMMUIdx mmu_idx)
+{
+    return env->cp15.sctlr_el[regime_el(env, mmu_idx)];
+}
+
+/* Return true if the specified stage of address translation is disabled */
+static inline bool regime_translation_disabled(CPUARMState *env,
+                                               ARMMMUIdx mmu_idx)
+{
+    if (arm_feature(env, ARM_FEATURE_M)) {
+        switch (env->v7m.mpu_ctrl &
+                (R_V7M_MPU_CTRL_ENABLE_MASK | R_V7M_MPU_CTRL_HFNMIENA_MASK)) {
+        case R_V7M_MPU_CTRL_ENABLE_MASK:
+            /* Enabled, but not for HardFault and NMI */
+            return mmu_idx == ARMMMUIdx_MNegPri;
+        case R_V7M_MPU_CTRL_ENABLE_MASK | R_V7M_MPU_CTRL_HFNMIENA_MASK:
+            /* Enabled for all cases */
+            return false;
+        case 0:
+        default:
+            /* HFNMIENA set and ENABLE clear is UNPREDICTABLE, but
+             * we warned about that in armv7m_nvic.c when the guest set it.
+             */
+            return true;
+        }
+    }
+
+    if (mmu_idx == ARMMMUIdx_S2NS) {
+        return (env->cp15.hcr_el2 & HCR_VM) == 0;
+    }
+    return (regime_sctlr(env, mmu_idx) & SCTLR_M) == 0;
+}
+
+static inline bool regime_translation_big_endian(CPUARMState *env,
+                                                 ARMMMUIdx mmu_idx)
+{
+    return (regime_sctlr(env, mmu_idx) & SCTLR_EE) != 0;
+}
+
+/* Return the TCR controlling this translation regime */
+static inline TCR *regime_tcr(CPUARMState *env, ARMMMUIdx mmu_idx)
+{
+    if (mmu_idx == ARMMMUIdx_S2NS) {
+        return &env->cp15.vtcr_el2;
+    }
+    return &env->cp15.tcr_el[regime_el(env, mmu_idx)];
+}
+
+/* Convert a possible stage1+2 MMU index into the appropriate
+ * stage 1 MMU index
+ */
+static inline ARMMMUIdx stage_1_mmu_idx(ARMMMUIdx mmu_idx)
+{
+    if (mmu_idx == ARMMMUIdx_S12NSE0 || mmu_idx == ARMMMUIdx_S12NSE1) {
+        mmu_idx += (ARMMMUIdx_S1NSE0 - ARMMMUIdx_S12NSE0);
+    }
+    return mmu_idx;
+}
+
+/* Returns TBI0 value for current regime el */
+uint32_t arm_regime_tbi0(CPUARMState *env, ARMMMUIdx mmu_idx)
+{
+    TCR *tcr;
+    uint32_t el;
+
+    /* For EL0 and EL1, TBI is controlled by stage 1's TCR, so convert
+     * a stage 1+2 mmu index into the appropriate stage 1 mmu index.
+     */
+    mmu_idx = stage_1_mmu_idx(mmu_idx);
+
+    tcr = regime_tcr(env, mmu_idx);
+    el = regime_el(env, mmu_idx);
+
+    if (el > 1) {
+        return extract64(tcr->raw_tcr, 20, 1);
+    } else {
+        return extract64(tcr->raw_tcr, 37, 1);
+    }
+}
+
+/* Returns TBI1 value for current regime el */
+uint32_t arm_regime_tbi1(CPUARMState *env, ARMMMUIdx mmu_idx)
+{
+    TCR *tcr;
+    uint32_t el;
+
+    /* For EL0 and EL1, TBI is controlled by stage 1's TCR, so convert
+     * a stage 1+2 mmu index into the appropriate stage 1 mmu index.
+     */
+    mmu_idx = stage_1_mmu_idx(mmu_idx);
+
+    tcr = regime_tcr(env, mmu_idx);
+    el = regime_el(env, mmu_idx);
+
+    if (el > 1) {
+        return 0;
+    } else {
+        return extract64(tcr->raw_tcr, 38, 1);
+    }
+}
+
+/* Return the TTBR associated with this translation regime */
+static inline uint64_t regime_ttbr(CPUARMState *env, ARMMMUIdx mmu_idx,
+                                   int ttbrn)
+{
+    if (mmu_idx == ARMMMUIdx_S2NS) {
+        return env->cp15.vttbr_el2;
+    }
+    if (ttbrn == 0) {
+        return env->cp15.ttbr0_el[regime_el(env, mmu_idx)];
+    } else {
+        return env->cp15.ttbr1_el[regime_el(env, mmu_idx)];
+    }
+}
+
+/* Return true if the translation regime is using LPAE format page tables */
+static bool regime_using_lpae_format(CPUARMState *env,
+                                     ARMMMUIdx mmu_idx)
+{
+    int el = regime_el(env, mmu_idx);
+    if (el == 2 || arm_el_is_aa64(env, el)) {
+        return true;
+    }
+    if (arm_feature(env, ARM_FEATURE_LPAE)
+        && (regime_tcr(env, mmu_idx)->raw_tcr & TTBCR_EAE)) {
+        return true;
+    }
+    return false;
+}
+
+/* Returns true if the stage 1 translation regime is using LPAE format page
+ * tables. Used when raising alignment exceptions, whose FSR changes depending
+ * on whether the long or short descriptor format is in use. */
+bool arm_s1_regime_using_lpae_format(CPUARMState *env, ARMMMUIdx mmu_idx)
+{
+    mmu_idx = stage_1_mmu_idx(mmu_idx);
+
+    return regime_using_lpae_format(env, mmu_idx);
+}
+
+static inline bool regime_is_user(CPUARMState *env, ARMMMUIdx mmu_idx)
+{
+    switch (mmu_idx) {
+    case ARMMMUIdx_S1SE0:
+    case ARMMMUIdx_S1NSE0:
+    case ARMMMUIdx_MUser:
+        return true;
+    default:
+        return false;
+    case ARMMMUIdx_S12NSE0:
+    case ARMMMUIdx_S12NSE1:
+        g_assert_not_reached();
+    }
+}
 #endif
 
 static int vfp_gdb_get_reg(CPUARMState *env, uint8_t *buf, int reg)
@@ -7022,208 +7224,6 @@ void arm_cpu_do_interrupt(CPUState *cs)
     }
 }
 
-/* Return the exception level which controls this address translation regime */
-static inline uint32_t regime_el(CPUARMState *env, ARMMMUIdx mmu_idx)
-{
-    switch (mmu_idx) {
-    case ARMMMUIdx_S2NS:
-    case ARMMMUIdx_S1E2:
-        return 2;
-    case ARMMMUIdx_S1E3:
-        return 3;
-    case ARMMMUIdx_S1SE0:
-        return arm_el_is_aa64(env, 3) ? 1 : 3;
-    case ARMMMUIdx_S1SE1:
-    case ARMMMUIdx_S1NSE0:
-    case ARMMMUIdx_S1NSE1:
-    case ARMMMUIdx_MPriv:
-    case ARMMMUIdx_MNegPri:
-    case ARMMMUIdx_MUser:
-        return 1;
-    default:
-        g_assert_not_reached();
-    }
-}
-
-/* Return true if this address translation regime is secure */
-static inline bool regime_is_secure(CPUARMState *env, ARMMMUIdx mmu_idx)
-{
-    switch (mmu_idx) {
-    case ARMMMUIdx_S12NSE0:
-    case ARMMMUIdx_S12NSE1:
-    case ARMMMUIdx_S1NSE0:
-    case ARMMMUIdx_S1NSE1:
-    case ARMMMUIdx_S1E2:
-    case ARMMMUIdx_S2NS:
-    case ARMMMUIdx_MPriv:
-    case ARMMMUIdx_MNegPri:
-    case ARMMMUIdx_MUser:
-        return false;
-    case ARMMMUIdx_S1E3:
-    case ARMMMUIdx_S1SE0:
-    case ARMMMUIdx_S1SE1:
-        return true;
-    default:
-        g_assert_not_reached();
-    }
-}
-
-/* Return the SCTLR value which controls this address translation regime */
-static inline uint32_t regime_sctlr(CPUARMState *env, ARMMMUIdx mmu_idx)
-{
-    return env->cp15.sctlr_el[regime_el(env, mmu_idx)];
-}
-
-/* Return true if the specified stage of address translation is disabled */
-static inline bool regime_translation_disabled(CPUARMState *env,
-                                               ARMMMUIdx mmu_idx)
-{
-    if (arm_feature(env, ARM_FEATURE_M)) {
-        switch (env->v7m.mpu_ctrl &
-                (R_V7M_MPU_CTRL_ENABLE_MASK | R_V7M_MPU_CTRL_HFNMIENA_MASK)) {
-        case R_V7M_MPU_CTRL_ENABLE_MASK:
-            /* Enabled, but not for HardFault and NMI */
-            return mmu_idx == ARMMMUIdx_MNegPri;
-        case R_V7M_MPU_CTRL_ENABLE_MASK | R_V7M_MPU_CTRL_HFNMIENA_MASK:
-            /* Enabled for all cases */
-            return false;
-        case 0:
-        default:
-            /* HFNMIENA set and ENABLE clear is UNPREDICTABLE, but
-             * we warned about that in armv7m_nvic.c when the guest set it.
-             */
-            return true;
-        }
-    }
-
-    if (mmu_idx == ARMMMUIdx_S2NS) {
-        return (env->cp15.hcr_el2 & HCR_VM) == 0;
-    }
-    return (regime_sctlr(env, mmu_idx) & SCTLR_M) == 0;
-}
-
-static inline bool regime_translation_big_endian(CPUARMState *env,
-                                                 ARMMMUIdx mmu_idx)
-{
-    return (regime_sctlr(env, mmu_idx) & SCTLR_EE) != 0;
-}
-
-/* Return the TCR controlling this translation regime */
-static inline TCR *regime_tcr(CPUARMState *env, ARMMMUIdx mmu_idx)
-{
-    if (mmu_idx == ARMMMUIdx_S2NS) {
-        return &env->cp15.vtcr_el2;
-    }
-    return &env->cp15.tcr_el[regime_el(env, mmu_idx)];
-}
-
-/* Convert a possible stage1+2 MMU index into the appropriate
- * stage 1 MMU index
- */
-static inline ARMMMUIdx stage_1_mmu_idx(ARMMMUIdx mmu_idx)
-{
-    if (mmu_idx == ARMMMUIdx_S12NSE0 || mmu_idx == ARMMMUIdx_S12NSE1) {
-        mmu_idx += (ARMMMUIdx_S1NSE0 - ARMMMUIdx_S12NSE0);
-    }
-    return mmu_idx;
-}
-
-/* Returns TBI0 value for current regime el */
-uint32_t arm_regime_tbi0(CPUARMState *env, ARMMMUIdx mmu_idx)
-{
-    TCR *tcr;
-    uint32_t el;
-
-    /* For EL0 and EL1, TBI is controlled by stage 1's TCR, so convert
-     * a stage 1+2 mmu index into the appropriate stage 1 mmu index.
-     */
-    mmu_idx = stage_1_mmu_idx(mmu_idx);
-
-    tcr = regime_tcr(env, mmu_idx);
-    el = regime_el(env, mmu_idx);
-
-    if (el > 1) {
-        return extract64(tcr->raw_tcr, 20, 1);
-    } else {
-        return extract64(tcr->raw_tcr, 37, 1);
-    }
-}
-
-/* Returns TBI1 value for current regime el */
-uint32_t arm_regime_tbi1(CPUARMState *env, ARMMMUIdx mmu_idx)
-{
-    TCR *tcr;
-    uint32_t el;
-
-    /* For EL0 and EL1, TBI is controlled by stage 1's TCR, so convert
-     * a stage 1+2 mmu index into the appropriate stage 1 mmu index.
-     */
-    mmu_idx = stage_1_mmu_idx(mmu_idx);
-
-    tcr = regime_tcr(env, mmu_idx);
-    el = regime_el(env, mmu_idx);
-
-    if (el > 1) {
-        return 0;
-    } else {
-        return extract64(tcr->raw_tcr, 38, 1);
-    }
-}
-
-/* Return the TTBR associated with this translation regime */
-static inline uint64_t regime_ttbr(CPUARMState *env, ARMMMUIdx mmu_idx,
-                                   int ttbrn)
-{
-    if (mmu_idx == ARMMMUIdx_S2NS) {
-        return env->cp15.vttbr_el2;
-    }
-    if (ttbrn == 0) {
-        return env->cp15.ttbr0_el[regime_el(env, mmu_idx)];
-    } else {
-        return env->cp15.ttbr1_el[regime_el(env, mmu_idx)];
-    }
-}
-
-/* Return true if the translation regime is using LPAE format page tables */
-static inline bool regime_using_lpae_format(CPUARMState *env,
-                                            ARMMMUIdx mmu_idx)
-{
-    int el = regime_el(env, mmu_idx);
-    if (el == 2 || arm_el_is_aa64(env, el)) {
-        return true;
-    }
-    if (arm_feature(env, ARM_FEATURE_LPAE)
-        && (regime_tcr(env, mmu_idx)->raw_tcr & TTBCR_EAE)) {
-        return true;
-    }
-    return false;
-}
-
-/* Returns true if the stage 1 translation regime is using LPAE format page
- * tables. Used when raising alignment exceptions, whose FSR changes depending
- * on whether the long or short descriptor format is in use. */
-bool arm_s1_regime_using_lpae_format(CPUARMState *env, ARMMMUIdx mmu_idx)
-{
-    mmu_idx = stage_1_mmu_idx(mmu_idx);
-
-    return regime_using_lpae_format(env, mmu_idx);
-}
-
-static inline bool regime_is_user(CPUARMState *env, ARMMMUIdx mmu_idx)
-{
-    switch (mmu_idx) {
-    case ARMMMUIdx_S1SE0:
-    case ARMMMUIdx_S1NSE0:
-    case ARMMMUIdx_MUser:
-        return true;
-    default:
-        return false;
-    case ARMMMUIdx_S12NSE0:
-    case ARMMMUIdx_S12NSE1:
-        g_assert_not_reached();
-    }
-}
-
 /* Translate section/page access permissions to page
  * R/W protection flags
  *
-- 
2.7.4

[Qemu-devel] [PATCH v1 2/2] target-arm: Extend PAR format determination

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Extend PAR format determination to handle more cases.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 target/arm/helper.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index fd1027e..6a1fffe 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -2345,12 +2345,40 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
     uint32_t fsr;
     bool ret;
     uint64_t par64;
+    bool format64 = false;
     MemTxAttrs attrs = {};
     ARMMMUFaultInfo fi = {};
 
     ret = get_phys_addr(env, value, access_type, mmu_idx,
                         &phys_addr, &attrs, &prot, &page_size, &fsr, &fi);
-    if (extended_addresses_enabled(env)) {
+
+    if (is_a64(env)) {
+        format64 = true;
+    } else if (arm_feature(env, ARM_FEATURE_LPAE)) {
+        /*
+         * ATS1Cxx:
+         * * TTBCR.EAE determines whether the result is returned using the
+         *   32-bit or the 64-bit PAR format
+         * * Instructions executed in Hyp mode always use the 64bit format
+         *
+         * ATS1S2NSOxx uses the 64bit format if any of the following is true:
+         * * The Non-secure TTBCR.EAE bit is set to 1
+         * * The implementation includes EL2, and the value of HCR.VM is 1
+         *
+         * ATS1Hx always uses the 64bit format (not supported yet).
+         */
+        format64 = regime_using_lpae_format(env, mmu_idx);
+
+        if (arm_feature(env, ARM_FEATURE_EL2)) {
+            if (mmu_idx == ARMMMUIdx_S12NSE0 || mmu_idx == ARMMMUIdx_S12NSE1) {
+                format64 |= env->cp15.hcr_el2 & HCR_VM;
+            } else {
+                format64 |= arm_current_el(env) == 2;
+            }
+        }
+    }
+
+    if (format64) {
         /* fsr is a DFSR/IFSR value for the long descriptor
          * translation table format, but with WnR always clear.
          * Convert it to a 64-bit PAR.
-- 
2.7.4

Re: [Qemu-devel] [PATCH v1 1/2] target-arm: Move the regime_xxx helpers

[Alistair Francis]

On Fri, Jun 30, 2017 at 6:45 AM, Edgar E. Iglesias
<edgar.iglesias@gmail.com> wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Move the regime_xxx helpers in preparation for future code
> that will reuse them.
>
> No functional change.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>

Thanks,
Alistair

> ---
>  target/arm/helper.c | 404 ++++++++++++++++++++++++++--------------------------
>  1 file changed, 202 insertions(+), 202 deletions(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 2594faa..fd1027e 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -35,6 +35,208 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>  #define PMCRD   0x8
>  #define PMCRC   0x4
>  #define PMCRE   0x1
> +
> +/* Return the exception level which controls this address translation regime */
> +static inline uint32_t regime_el(CPUARMState *env, ARMMMUIdx mmu_idx)
> +{
> +    switch (mmu_idx) {
> +    case ARMMMUIdx_S2NS:
> +    case ARMMMUIdx_S1E2:
> +        return 2;
> +    case ARMMMUIdx_S1E3:
> +        return 3;
> +    case ARMMMUIdx_S1SE0:
> +        return arm_el_is_aa64(env, 3) ? 1 : 3;
> +    case ARMMMUIdx_S1SE1:
> +    case ARMMMUIdx_S1NSE0:
> +    case ARMMMUIdx_S1NSE1:
> +    case ARMMMUIdx_MPriv:
> +    case ARMMMUIdx_MNegPri:
> +    case ARMMMUIdx_MUser:
> +        return 1;
> +    default:
> +        g_assert_not_reached();
> +    }
> +}
> +
> +/* Return true if this address translation regime is secure */
> +static inline bool regime_is_secure(CPUARMState *env, ARMMMUIdx mmu_idx)
> +{
> +    switch (mmu_idx) {
> +    case ARMMMUIdx_S12NSE0:
> +    case ARMMMUIdx_S12NSE1:
> +    case ARMMMUIdx_S1NSE0:
> +    case ARMMMUIdx_S1NSE1:
> +    case ARMMMUIdx_S1E2:
> +    case ARMMMUIdx_S2NS:
> +    case ARMMMUIdx_MPriv:
> +    case ARMMMUIdx_MNegPri:
> +    case ARMMMUIdx_MUser:
> +        return false;
> +    case ARMMMUIdx_S1E3:
> +    case ARMMMUIdx_S1SE0:
> +    case ARMMMUIdx_S1SE1:
> +        return true;
> +    default:
> +        g_assert_not_reached();
> +    }
> +}
> +
> +/* Return the SCTLR value which controls this address translation regime */
> +static inline uint32_t regime_sctlr(CPUARMState *env, ARMMMUIdx mmu_idx)
> +{
> +    return env->cp15.sctlr_el[regime_el(env, mmu_idx)];
> +}
> +
> +/* Return true if the specified stage of address translation is disabled */
> +static inline bool regime_translation_disabled(CPUARMState *env,
> +                                               ARMMMUIdx mmu_idx)
> +{
> +    if (arm_feature(env, ARM_FEATURE_M)) {
> +        switch (env->v7m.mpu_ctrl &
> +                (R_V7M_MPU_CTRL_ENABLE_MASK | R_V7M_MPU_CTRL_HFNMIENA_MASK)) {
> +        case R_V7M_MPU_CTRL_ENABLE_MASK:
> +            /* Enabled, but not for HardFault and NMI */
> +            return mmu_idx == ARMMMUIdx_MNegPri;
> +        case R_V7M_MPU_CTRL_ENABLE_MASK | R_V7M_MPU_CTRL_HFNMIENA_MASK:
> +            /* Enabled for all cases */
> +            return false;
> +        case 0:
> +        default:
> +            /* HFNMIENA set and ENABLE clear is UNPREDICTABLE, but
> +             * we warned about that in armv7m_nvic.c when the guest set it.
> +             */
> +            return true;
> +        }
> +    }
> +
> +    if (mmu_idx == ARMMMUIdx_S2NS) {
> +        return (env->cp15.hcr_el2 & HCR_VM) == 0;
> +    }
> +    return (regime_sctlr(env, mmu_idx) & SCTLR_M) == 0;
> +}
> +
> +static inline bool regime_translation_big_endian(CPUARMState *env,
> +                                                 ARMMMUIdx mmu_idx)
> +{
> +    return (regime_sctlr(env, mmu_idx) & SCTLR_EE) != 0;
> +}
> +
> +/* Return the TCR controlling this translation regime */
> +static inline TCR *regime_tcr(CPUARMState *env, ARMMMUIdx mmu_idx)
> +{
> +    if (mmu_idx == ARMMMUIdx_S2NS) {
> +        return &env->cp15.vtcr_el2;
> +    }
> +    return &env->cp15.tcr_el[regime_el(env, mmu_idx)];
> +}
> +
> +/* Convert a possible stage1+2 MMU index into the appropriate
> + * stage 1 MMU index
> + */
> +static inline ARMMMUIdx stage_1_mmu_idx(ARMMMUIdx mmu_idx)
> +{
> +    if (mmu_idx == ARMMMUIdx_S12NSE0 || mmu_idx == ARMMMUIdx_S12NSE1) {
> +        mmu_idx += (ARMMMUIdx_S1NSE0 - ARMMMUIdx_S12NSE0);
> +    }
> +    return mmu_idx;
> +}
> +
> +/* Returns TBI0 value for current regime el */
> +uint32_t arm_regime_tbi0(CPUARMState *env, ARMMMUIdx mmu_idx)
> +{
> +    TCR *tcr;
> +    uint32_t el;
> +
> +    /* For EL0 and EL1, TBI is controlled by stage 1's TCR, so convert
> +     * a stage 1+2 mmu index into the appropriate stage 1 mmu index.
> +     */
> +    mmu_idx = stage_1_mmu_idx(mmu_idx);
> +
> +    tcr = regime_tcr(env, mmu_idx);
> +    el = regime_el(env, mmu_idx);
> +
> +    if (el > 1) {
> +        return extract64(tcr->raw_tcr, 20, 1);
> +    } else {
> +        return extract64(tcr->raw_tcr, 37, 1);
> +    }
> +}
> +
> +/* Returns TBI1 value for current regime el */
> +uint32_t arm_regime_tbi1(CPUARMState *env, ARMMMUIdx mmu_idx)
> +{
> +    TCR *tcr;
> +    uint32_t el;
> +
> +    /* For EL0 and EL1, TBI is controlled by stage 1's TCR, so convert
> +     * a stage 1+2 mmu index into the appropriate stage 1 mmu index.
> +     */
> +    mmu_idx = stage_1_mmu_idx(mmu_idx);
> +
> +    tcr = regime_tcr(env, mmu_idx);
> +    el = regime_el(env, mmu_idx);
> +
> +    if (el > 1) {
> +        return 0;
> +    } else {
> +        return extract64(tcr->raw_tcr, 38, 1);
> +    }
> +}
> +
> +/* Return the TTBR associated with this translation regime */
> +static inline uint64_t regime_ttbr(CPUARMState *env, ARMMMUIdx mmu_idx,
> +                                   int ttbrn)
> +{
> +    if (mmu_idx == ARMMMUIdx_S2NS) {
> +        return env->cp15.vttbr_el2;
> +    }
> +    if (ttbrn == 0) {
> +        return env->cp15.ttbr0_el[regime_el(env, mmu_idx)];
> +    } else {
> +        return env->cp15.ttbr1_el[regime_el(env, mmu_idx)];
> +    }
> +}
> +
> +/* Return true if the translation regime is using LPAE format page tables */
> +static bool regime_using_lpae_format(CPUARMState *env,
> +                                     ARMMMUIdx mmu_idx)
> +{
> +    int el = regime_el(env, mmu_idx);
> +    if (el == 2 || arm_el_is_aa64(env, el)) {
> +        return true;
> +    }
> +    if (arm_feature(env, ARM_FEATURE_LPAE)
> +        && (regime_tcr(env, mmu_idx)->raw_tcr & TTBCR_EAE)) {
> +        return true;
> +    }
> +    return false;
> +}
> +
> +/* Returns true if the stage 1 translation regime is using LPAE format page
> + * tables. Used when raising alignment exceptions, whose FSR changes depending
> + * on whether the long or short descriptor format is in use. */
> +bool arm_s1_regime_using_lpae_format(CPUARMState *env, ARMMMUIdx mmu_idx)
> +{
> +    mmu_idx = stage_1_mmu_idx(mmu_idx);
> +
> +    return regime_using_lpae_format(env, mmu_idx);
> +}
> +
> +static inline bool regime_is_user(CPUARMState *env, ARMMMUIdx mmu_idx)
> +{
> +    switch (mmu_idx) {
> +    case ARMMMUIdx_S1SE0:
> +    case ARMMMUIdx_S1NSE0:
> +    case ARMMMUIdx_MUser:
> +        return true;
> +    default:
> +        return false;
> +    case ARMMMUIdx_S12NSE0:
> +    case ARMMMUIdx_S12NSE1:
> +        g_assert_not_reached();
> +    }
> +}
>  #endif
>
>  static int vfp_gdb_get_reg(CPUARMState *env, uint8_t *buf, int reg)
> @@ -7022,208 +7224,6 @@ void arm_cpu_do_interrupt(CPUState *cs)
>      }
>  }
>
> -/* Return the exception level which controls this address translation regime */
> -static inline uint32_t regime_el(CPUARMState *env, ARMMMUIdx mmu_idx)
> -{
> -    switch (mmu_idx) {
> -    case ARMMMUIdx_S2NS:
> -    case ARMMMUIdx_S1E2:
> -        return 2;
> -    case ARMMMUIdx_S1E3:
> -        return 3;
> -    case ARMMMUIdx_S1SE0:
> -        return arm_el_is_aa64(env, 3) ? 1 : 3;
> -    case ARMMMUIdx_S1SE1:
> -    case ARMMMUIdx_S1NSE0:
> -    case ARMMMUIdx_S1NSE1:
> -    case ARMMMUIdx_MPriv:
> -    case ARMMMUIdx_MNegPri:
> -    case ARMMMUIdx_MUser:
> -        return 1;
> -    default:
> -        g_assert_not_reached();
> -    }
> -}
> -
> -/* Return true if this address translation regime is secure */
> -static inline bool regime_is_secure(CPUARMState *env, ARMMMUIdx mmu_idx)
> -{
> -    switch (mmu_idx) {
> -    case ARMMMUIdx_S12NSE0:
> -    case ARMMMUIdx_S12NSE1:
> -    case ARMMMUIdx_S1NSE0:
> -    case ARMMMUIdx_S1NSE1:
> -    case ARMMMUIdx_S1E2:
> -    case ARMMMUIdx_S2NS:
> -    case ARMMMUIdx_MPriv:
> -    case ARMMMUIdx_MNegPri:
> -    case ARMMMUIdx_MUser:
> -        return false;
> -    case ARMMMUIdx_S1E3:
> -    case ARMMMUIdx_S1SE0:
> -    case ARMMMUIdx_S1SE1:
> -        return true;
> -    default:
> -        g_assert_not_reached();
> -    }
> -}
> -
> -/* Return the SCTLR value which controls this address translation regime */
> -static inline uint32_t regime_sctlr(CPUARMState *env, ARMMMUIdx mmu_idx)
> -{
> -    return env->cp15.sctlr_el[regime_el(env, mmu_idx)];
> -}
> -
> -/* Return true if the specified stage of address translation is disabled */
> -static inline bool regime_translation_disabled(CPUARMState *env,
> -                                               ARMMMUIdx mmu_idx)
> -{
> -    if (arm_feature(env, ARM_FEATURE_M)) {
> -        switch (env->v7m.mpu_ctrl &
> -                (R_V7M_MPU_CTRL_ENABLE_MASK | R_V7M_MPU_CTRL_HFNMIENA_MASK)) {
> -        case R_V7M_MPU_CTRL_ENABLE_MASK:
> -            /* Enabled, but not for HardFault and NMI */
> -            return mmu_idx == ARMMMUIdx_MNegPri;
> -        case R_V7M_MPU_CTRL_ENABLE_MASK | R_V7M_MPU_CTRL_HFNMIENA_MASK:
> -            /* Enabled for all cases */
> -            return false;
> -        case 0:
> -        default:
> -            /* HFNMIENA set and ENABLE clear is UNPREDICTABLE, but
> -             * we warned about that in armv7m_nvic.c when the guest set it.
> -             */
> -            return true;
> -        }
> -    }
> -
> -    if (mmu_idx == ARMMMUIdx_S2NS) {
> -        return (env->cp15.hcr_el2 & HCR_VM) == 0;
> -    }
> -    return (regime_sctlr(env, mmu_idx) & SCTLR_M) == 0;
> -}
> -
> -static inline bool regime_translation_big_endian(CPUARMState *env,
> -                                                 ARMMMUIdx mmu_idx)
> -{
> -    return (regime_sctlr(env, mmu_idx) & SCTLR_EE) != 0;
> -}
> -
> -/* Return the TCR controlling this translation regime */
> -static inline TCR *regime_tcr(CPUARMState *env, ARMMMUIdx mmu_idx)
> -{
> -    if (mmu_idx == ARMMMUIdx_S2NS) {
> -        return &env->cp15.vtcr_el2;
> -    }
> -    return &env->cp15.tcr_el[regime_el(env, mmu_idx)];
> -}
> -
> -/* Convert a possible stage1+2 MMU index into the appropriate
> - * stage 1 MMU index
> - */
> -static inline ARMMMUIdx stage_1_mmu_idx(ARMMMUIdx mmu_idx)
> -{
> -    if (mmu_idx == ARMMMUIdx_S12NSE0 || mmu_idx == ARMMMUIdx_S12NSE1) {
> -        mmu_idx += (ARMMMUIdx_S1NSE0 - ARMMMUIdx_S12NSE0);
> -    }
> -    return mmu_idx;
> -}
> -
> -/* Returns TBI0 value for current regime el */
> -uint32_t arm_regime_tbi0(CPUARMState *env, ARMMMUIdx mmu_idx)
> -{
> -    TCR *tcr;
> -    uint32_t el;
> -
> -    /* For EL0 and EL1, TBI is controlled by stage 1's TCR, so convert
> -     * a stage 1+2 mmu index into the appropriate stage 1 mmu index.
> -     */
> -    mmu_idx = stage_1_mmu_idx(mmu_idx);
> -
> -    tcr = regime_tcr(env, mmu_idx);
> -    el = regime_el(env, mmu_idx);
> -
> -    if (el > 1) {
> -        return extract64(tcr->raw_tcr, 20, 1);
> -    } else {
> -        return extract64(tcr->raw_tcr, 37, 1);
> -    }
> -}
> -
> -/* Returns TBI1 value for current regime el */
> -uint32_t arm_regime_tbi1(CPUARMState *env, ARMMMUIdx mmu_idx)
> -{
> -    TCR *tcr;
> -    uint32_t el;
> -
> -    /* For EL0 and EL1, TBI is controlled by stage 1's TCR, so convert
> -     * a stage 1+2 mmu index into the appropriate stage 1 mmu index.
> -     */
> -    mmu_idx = stage_1_mmu_idx(mmu_idx);
> -
> -    tcr = regime_tcr(env, mmu_idx);
> -    el = regime_el(env, mmu_idx);
> -
> -    if (el > 1) {
> -        return 0;
> -    } else {
> -        return extract64(tcr->raw_tcr, 38, 1);
> -    }
> -}
> -
> -/* Return the TTBR associated with this translation regime */
> -static inline uint64_t regime_ttbr(CPUARMState *env, ARMMMUIdx mmu_idx,
> -                                   int ttbrn)
> -{
> -    if (mmu_idx == ARMMMUIdx_S2NS) {
> -        return env->cp15.vttbr_el2;
> -    }
> -    if (ttbrn == 0) {
> -        return env->cp15.ttbr0_el[regime_el(env, mmu_idx)];
> -    } else {
> -        return env->cp15.ttbr1_el[regime_el(env, mmu_idx)];
> -    }
> -}
> -
> -/* Return true if the translation regime is using LPAE format page tables */
> -static inline bool regime_using_lpae_format(CPUARMState *env,
> -                                            ARMMMUIdx mmu_idx)
> -{
> -    int el = regime_el(env, mmu_idx);
> -    if (el == 2 || arm_el_is_aa64(env, el)) {
> -        return true;
> -    }
> -    if (arm_feature(env, ARM_FEATURE_LPAE)
> -        && (regime_tcr(env, mmu_idx)->raw_tcr & TTBCR_EAE)) {
> -        return true;
> -    }
> -    return false;
> -}
> -
> -/* Returns true if the stage 1 translation regime is using LPAE format page
> - * tables. Used when raising alignment exceptions, whose FSR changes depending
> - * on whether the long or short descriptor format is in use. */
> -bool arm_s1_regime_using_lpae_format(CPUARMState *env, ARMMMUIdx mmu_idx)
> -{
> -    mmu_idx = stage_1_mmu_idx(mmu_idx);
> -
> -    return regime_using_lpae_format(env, mmu_idx);
> -}
> -
> -static inline bool regime_is_user(CPUARMState *env, ARMMMUIdx mmu_idx)
> -{
> -    switch (mmu_idx) {
> -    case ARMMMUIdx_S1SE0:
> -    case ARMMMUIdx_S1NSE0:
> -    case ARMMMUIdx_MUser:
> -        return true;
> -    default:
> -        return false;
> -    case ARMMMUIdx_S12NSE0:
> -    case ARMMMUIdx_S12NSE1:
> -        g_assert_not_reached();
> -    }
> -}
> -
>  /* Translate section/page access permissions to page
>   * R/W protection flags
>   *
> --
> 2.7.4
>
>

Re: [Qemu-devel] [PATCH v1 2/2] target-arm: Extend PAR format determination

[Peter Maydell]

On 30 June 2017 at 14:45, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Extend PAR format determination to handle more cases.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  target/arm/helper.c | 30 +++++++++++++++++++++++++++++-
>  1 file changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index fd1027e..6a1fffe 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -2345,12 +2345,40 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
>      uint32_t fsr;
>      bool ret;
>      uint64_t par64;
> +    bool format64 = false;
>      MemTxAttrs attrs = {};
>      ARMMMUFaultInfo fi = {};
>
>      ret = get_phys_addr(env, value, access_type, mmu_idx,
>                          &phys_addr, &attrs, &prot, &page_size, &fsr, &fi);
> -    if (extended_addresses_enabled(env)) {
> +
> +    if (is_a64(env)) {
> +        format64 = true;
> +    } else if (arm_feature(env, ARM_FEATURE_LPAE)) {
> +        /*
> +         * ATS1Cxx:
> +         * * TTBCR.EAE determines whether the result is returned using the
> +         *   32-bit or the 64-bit PAR format
> +         * * Instructions executed in Hyp mode always use the 64bit format
> +         *
> +         * ATS1S2NSOxx uses the 64bit format if any of the following is true:
> +         * * The Non-secure TTBCR.EAE bit is set to 1
> +         * * The implementation includes EL2, and the value of HCR.VM is 1
> +         *
> +         * ATS1Hx always uses the 64bit format (not supported yet).
> +         */
> +        format64 = regime_using_lpae_format(env, mmu_idx);
> +
> +        if (arm_feature(env, ARM_FEATURE_EL2)) {
> +            if (mmu_idx == ARMMMUIdx_S12NSE0 || mmu_idx == ARMMMUIdx_S12NSE1) {
> +                format64 |= env->cp15.hcr_el2 & HCR_VM;
> +            } else {
> +                format64 |= arm_current_el(env) == 2;
> +            }
> +        }
> +    }

So this kind of worries me, because what it's coded as is "determine
whether architecturally we should be returning a 64-bit or 32-bit
PAR format", but what the code below it uses the format64 flag for is
"manipulate whatever PAR we got handed back by get_phys_addr()".
So we have two separate bits of code that are both choosing
32 vs 64 bit PAR (the code in this patch, and the logic inside
get_phys_addr()), and they have to come to the same conclusion
or we'll silently mangle the PAR. It seems like it would be
better to either have get_phys_addr() explicitly tell us what kind
of format it is returning to us, or to have the caller tell it
what kind of PAR it needs.

PS: on the subject of virtualization, I notice there's a comment
a bit later on in do_ats_write() that says
    /* Note that S2WLK and FSTAGE are always zero, because we don't
     * implement virtualization and therefore there can't be a stage 2
     * fault.
     */
so we'll need to address that too at some point...

thanks
-- PMM

Re: [Qemu-devel] [PATCH v1 2/2] target-arm: Extend PAR format determination

[Edgar E. Iglesias]

On Mon, Jul 10, 2017 at 04:11:29PM +0100, Peter Maydell wrote:
> On 30 June 2017 at 14:45, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> >
> > Extend PAR format determination to handle more cases.
> >
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > ---
> >  target/arm/helper.c | 30 +++++++++++++++++++++++++++++-
> >  1 file changed, 29 insertions(+), 1 deletion(-)
> >
> > diff --git a/target/arm/helper.c b/target/arm/helper.c
> > index fd1027e..6a1fffe 100644
> > --- a/target/arm/helper.c
> > +++ b/target/arm/helper.c
> > @@ -2345,12 +2345,40 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
> >      uint32_t fsr;
> >      bool ret;
> >      uint64_t par64;
> > +    bool format64 = false;
> >      MemTxAttrs attrs = {};
> >      ARMMMUFaultInfo fi = {};
> >
> >      ret = get_phys_addr(env, value, access_type, mmu_idx,
> >                          &phys_addr, &attrs, &prot, &page_size, &fsr, &fi);
> > -    if (extended_addresses_enabled(env)) {
> > +
> > +    if (is_a64(env)) {
> > +        format64 = true;
> > +    } else if (arm_feature(env, ARM_FEATURE_LPAE)) {
> > +        /*
> > +         * ATS1Cxx:
> > +         * * TTBCR.EAE determines whether the result is returned using the
> > +         *   32-bit or the 64-bit PAR format
> > +         * * Instructions executed in Hyp mode always use the 64bit format
> > +         *
> > +         * ATS1S2NSOxx uses the 64bit format if any of the following is true:
> > +         * * The Non-secure TTBCR.EAE bit is set to 1
> > +         * * The implementation includes EL2, and the value of HCR.VM is 1
> > +         *
> > +         * ATS1Hx always uses the 64bit format (not supported yet).
> > +         */
> > +        format64 = regime_using_lpae_format(env, mmu_idx);
> > +
> > +        if (arm_feature(env, ARM_FEATURE_EL2)) {
> > +            if (mmu_idx == ARMMMUIdx_S12NSE0 || mmu_idx == ARMMMUIdx_S12NSE1) {
> > +                format64 |= env->cp15.hcr_el2 & HCR_VM;
> > +            } else {
> > +                format64 |= arm_current_el(env) == 2;
> > +            }
> > +        }
> > +    }
> 
> So this kind of worries me, because what it's coded as is "determine
> whether architecturally we should be returning a 64-bit or 32-bit
> PAR format", but what the code below it uses the format64 flag for is
> "manipulate whatever PAR we got handed back by get_phys_addr()".
> So we have two separate bits of code that are both choosing
> 32 vs 64 bit PAR (the code in this patch, and the logic inside
> get_phys_addr()), and they have to come to the same conclusion
> or we'll silently mangle the PAR. It seems like it would be
> better to either have get_phys_addr() explicitly tell us what kind
> of format it is returning to us, or to have the caller tell it
> what kind of PAR it needs.

Yes, I see your point and that's exactly what's happenning before the patch.
Some of these new checks are generic in the sense that they check for LPAE/64bitness
but others are I guess ATS specific for lack of a better term.
It feels a bit weird to put the ATS specific PAR format logic into get_phys_addr.

The basic idea here is that we never downgrade to the 32bit format, we only uprgade.
The following line was meant to get the initial format I think you are requesting:
format64 = regime_using_lpae_format(env, mmu_idx);

After that, we apply possible ATS specfic upgrades to 64bit PAR format if needed.

For clarity, perhaps we could make get_phys_addr return this same initial format, and then
we can follow up with the ATS specific upgrades. E.g:

    ret = get_phys_addr(env, value, access_type, mmu_idx,
                        &phys_addr, &attrs, &prot, &page_size, &fsr, &fi,
                        &format64);

    /* Apply possible ATS/PAR 64bit upgrades if format64 is false.  */
    if (is_a64(env)) {
        format64 = true;
    } else if (arm_feature(env, ARM_FEATURE_LPAE)) {
        if (arm_feature(env, ARM_FEATURE_EL2)) {
            if (mmu_idx == ARMMMUIdx_S12NSE0 || mmu_idx == ARMMMUIdx_S12NSE1) {
                format64 |= env->cp15.hcr_el2 & HCR_VM;
            } else {
                format64 |= arm_current_el(env) == 2;
            }
        }
    }


Does something like that sound OK?

Cheers,
Edgar


> >      ret = get_phys_addr(env, value, access_type, mmu_idx,
> >                          &phys_addr, &attrs, &prot, &page_size, &fsr, &fi);
> > -    if (extended_addresses_enabled(env)) {
> > +
> > +    if (is_a64(env)) {
> > +        format64 = true;
> > +    } else if (arm_feature(env, ARM_FEATURE_LPAE)) {
> > +        /*
> > +         * ATS1Cxx:
> > +         * * TTBCR.EAE determines whether the result is returned using the
> > +         *   32-bit or the 64-bit PAR format
> > +         * * Instructions executed in Hyp mode always use the 64bit format
> > +         *
> > +         * ATS1S2NSOxx uses the 64bit format if any of the following is true:
> > +         * * The Non-secure TTBCR.EAE bit is set to 1
> > +         * * The implementation includes EL2, and the value of HCR.VM is 1
> > +         *
> > +         * ATS1Hx always uses the 64bit format (not supported yet).
> > +         */
> > +        format64 = regime_using_lpae_format(env, mmu_idx);
> > +
> > +        if (arm_feature(env, ARM_FEATURE_EL2)) {
> > +            if (mmu_idx == ARMMMUIdx_S12NSE0 || mmu_idx == ARMMMUIdx_S12NSE1) {
> > +                format64 |= env->cp15.hcr_el2 & HCR_VM;
> > +            } else {
> > +                format64 |= arm_current_el(env) == 2;
> > +            }
>

> 
> PS: on the subject of virtualization, I notice there's a comment
> a bit later on in do_ats_write() that says
>     /* Note that S2WLK and FSTAGE are always zero, because we don't
>      * implement virtualization and therefore there can't be a stage 2
>      * fault.
>      */
> so we'll need to address that too at some point...
> 
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH v1 2/2] target-arm: Extend PAR format determination

[Peter Maydell]

On 11 July 2017 at 11:03, Edgar E. Iglesias <edgar.iglesias@xilinx.com> wrote:
> On Mon, Jul 10, 2017 at 04:11:29PM +0100, Peter Maydell wrote:
>> So this kind of worries me, because what it's coded as is "determine
>> whether architecturally we should be returning a 64-bit or 32-bit
>> PAR format", but what the code below it uses the format64 flag for is
>> "manipulate whatever PAR we got handed back by get_phys_addr()".
>> So we have two separate bits of code that are both choosing
>> 32 vs 64 bit PAR (the code in this patch, and the logic inside
>> get_phys_addr()), and they have to come to the same conclusion
>> or we'll silently mangle the PAR. It seems like it would be
>> better to either have get_phys_addr() explicitly tell us what kind
>> of format it is returning to us, or to have the caller tell it
>> what kind of PAR it needs.
>
> Yes, I see your point and that's exactly what's happenning before the patch.
> Some of these new checks are generic in the sense that they check for LPAE/64bitness
> but others are I guess ATS specific for lack of a better term.
> It feels a bit weird to put the ATS specific PAR format logic into get_phys_addr.
>
> The basic idea here is that we never downgrade to the 32bit format, we only uprgade.
> The following line was meant to get the initial format I think you are requesting:
> format64 = regime_using_lpae_format(env, mmu_idx);
>
> After that, we apply possible ATS specfic upgrades to 64bit PAR format if needed.
>
> For clarity, perhaps we could make get_phys_addr return this same initial format, and then
> we can follow up with the ATS specific upgrades. E.g:
>
>     ret = get_phys_addr(env, value, access_type, mmu_idx,
>                         &phys_addr, &attrs, &prot, &page_size, &fsr, &fi,
>                         &format64);
>
>     /* Apply possible ATS/PAR 64bit upgrades if format64 is false.  */
>     if (is_a64(env)) {
>         format64 = true;
>     } else if (arm_feature(env, ARM_FEATURE_LPAE)) {
>         if (arm_feature(env, ARM_FEATURE_EL2)) {
>             if (mmu_idx == ARMMMUIdx_S12NSE0 || mmu_idx == ARMMMUIdx_S12NSE1) {
>                 format64 |= env->cp15.hcr_el2 & HCR_VM;
>             } else {
>                 format64 |= arm_current_el(env) == 2;
>             }
>         }
>     }

This still has the same problem, doesn't it? If get_phys_addr()
has given you back a short-descriptor format PAR then you cannot
simply "upgrade" it to a long-descriptor format PAR -- the
fault status codes are all different. I think the "short desc
vs long desc" condition used to be simple but the various
upgrades to get_phys_addr() to handle EL2 have made it much
more complicated, and so we'll be much better off just handing
get_phys_addr() a flag to say how we want the status reported,
if it's really dependent on ATS vs not-ATS.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v1 2/2] target-arm: Extend PAR format determination

[Edgar E. Iglesias]

On Tue, Jul 11, 2017 at 11:14:04AM +0100, Peter Maydell wrote:
> On 11 July 2017 at 11:03, Edgar E. Iglesias <edgar.iglesias@xilinx.com> wrote:
> > On Mon, Jul 10, 2017 at 04:11:29PM +0100, Peter Maydell wrote:
> >> So this kind of worries me, because what it's coded as is "determine
> >> whether architecturally we should be returning a 64-bit or 32-bit
> >> PAR format", but what the code below it uses the format64 flag for is
> >> "manipulate whatever PAR we got handed back by get_phys_addr()".
> >> So we have two separate bits of code that are both choosing
> >> 32 vs 64 bit PAR (the code in this patch, and the logic inside
> >> get_phys_addr()), and they have to come to the same conclusion
> >> or we'll silently mangle the PAR. It seems like it would be
> >> better to either have get_phys_addr() explicitly tell us what kind
> >> of format it is returning to us, or to have the caller tell it
> >> what kind of PAR it needs.
> >
> > Yes, I see your point and that's exactly what's happenning before the patch.
> > Some of these new checks are generic in the sense that they check for LPAE/64bitness
> > but others are I guess ATS specific for lack of a better term.
> > It feels a bit weird to put the ATS specific PAR format logic into get_phys_addr.
> >
> > The basic idea here is that we never downgrade to the 32bit format, we only uprgade.
> > The following line was meant to get the initial format I think you are requesting:
> > format64 = regime_using_lpae_format(env, mmu_idx);
> >
> > After that, we apply possible ATS specfic upgrades to 64bit PAR format if needed.
> >
> > For clarity, perhaps we could make get_phys_addr return this same initial format, and then
> > we can follow up with the ATS specific upgrades. E.g:
> >
> >     ret = get_phys_addr(env, value, access_type, mmu_idx,
> >                         &phys_addr, &attrs, &prot, &page_size, &fsr, &fi,
> >                         &format64);
> >
> >     /* Apply possible ATS/PAR 64bit upgrades if format64 is false.  */
> >     if (is_a64(env)) {
> >         format64 = true;
> >     } else if (arm_feature(env, ARM_FEATURE_LPAE)) {
> >         if (arm_feature(env, ARM_FEATURE_EL2)) {
> >             if (mmu_idx == ARMMMUIdx_S12NSE0 || mmu_idx == ARMMMUIdx_S12NSE1) {
> >                 format64 |= env->cp15.hcr_el2 & HCR_VM;
> >             } else {
> >                 format64 |= arm_current_el(env) == 2;
> >             }
> >         }
> >     }
> 
> This still has the same problem, doesn't it? If get_phys_addr()
> has given you back a short-descriptor format PAR then you cannot
> simply "upgrade" it to a long-descriptor format PAR -- the
> fault status codes are all different. I think the "short desc
> vs long desc" condition used to be simple but the various
> upgrades to get_phys_addr() to handle EL2 have made it much
> more complicated, and so we'll be much better off just handing
> get_phys_addr() a flag to say how we want the status reported,

OK, yes, the codes will be a problem.

Telling get_phys_addr() what formatsounds good then. Will have a look.

Thanks,
Edgar

Re: [Qemu-devel] [PATCH v1 2/2] target-arm: Extend PAR format determination

[Edgar E. Iglesias]

On Tue, Jul 11, 2017 at 11:14:04AM +0100, Peter Maydell wrote:
> On 11 July 2017 at 11:03, Edgar E. Iglesias <edgar.iglesias@xilinx.com> wrote:
> > On Mon, Jul 10, 2017 at 04:11:29PM +0100, Peter Maydell wrote:
> >> So this kind of worries me, because what it's coded as is "determine
> >> whether architecturally we should be returning a 64-bit or 32-bit
> >> PAR format", but what the code below it uses the format64 flag for is
> >> "manipulate whatever PAR we got handed back by get_phys_addr()".
> >> So we have two separate bits of code that are both choosing
> >> 32 vs 64 bit PAR (the code in this patch, and the logic inside
> >> get_phys_addr()), and they have to come to the same conclusion
> >> or we'll silently mangle the PAR. It seems like it would be
> >> better to either have get_phys_addr() explicitly tell us what kind
> >> of format it is returning to us, or to have the caller tell it
> >> what kind of PAR it needs.
> >
> > Yes, I see your point and that's exactly what's happenning before the patch.
> > Some of these new checks are generic in the sense that they check for LPAE/64bitness
> > but others are I guess ATS specific for lack of a better term.
> > It feels a bit weird to put the ATS specific PAR format logic into get_phys_addr.
> >
> > The basic idea here is that we never downgrade to the 32bit format, we only uprgade.
> > The following line was meant to get the initial format I think you are requesting:
> > format64 = regime_using_lpae_format(env, mmu_idx);
> >
> > After that, we apply possible ATS specfic upgrades to 64bit PAR format if needed.
> >
> > For clarity, perhaps we could make get_phys_addr return this same initial format, and then
> > we can follow up with the ATS specific upgrades. E.g:
> >
> >     ret = get_phys_addr(env, value, access_type, mmu_idx,
> >                         &phys_addr, &attrs, &prot, &page_size, &fsr, &fi,
> >                         &format64);
> >
> >     /* Apply possible ATS/PAR 64bit upgrades if format64 is false.  */
> >     if (is_a64(env)) {
> >         format64 = true;
> >     } else if (arm_feature(env, ARM_FEATURE_LPAE)) {
> >         if (arm_feature(env, ARM_FEATURE_EL2)) {
> >             if (mmu_idx == ARMMMUIdx_S12NSE0 || mmu_idx == ARMMMUIdx_S12NSE1) {
> >                 format64 |= env->cp15.hcr_el2 & HCR_VM;
> >             } else {
> >                 format64 |= arm_current_el(env) == 2;
> >             }
> >         }
> >     }
> 
> This still has the same problem, doesn't it? If get_phys_addr()
> has given you back a short-descriptor format PAR then you cannot
> simply "upgrade" it to a long-descriptor format PAR -- the
> fault status codes are all different. I think the "short desc
> vs long desc" condition used to be simple but the various
> upgrades to get_phys_addr() to handle EL2 have made it much
> more complicated, and so we'll be much better off just handing
> get_phys_addr() a flag to say how we want the status reported,
> if it's really dependent on ATS vs not-ATS.
>

Another way could also be to have get_phys_addr() fill in generic
fields in the FaultInfo struct and then have a faultinfo_to_fsr
mapping function to populate FSR/PAR. Do you see any issues with that?

Perhaps that's better, not sure.

Best regards,
Edgar 

Re: [Qemu-devel] [PATCH v1 2/2] target-arm: Extend PAR format determination

[Peter Maydell]

On 11 July 2017 at 11:38, Edgar E. Iglesias <edgar.iglesias@xilinx.com> wrote:
> Another way could also be to have get_phys_addr() fill in generic
> fields in the FaultInfo struct and then have a faultinfo_to_fsr
> mapping function to populate FSR/PAR. Do you see any issues with that?

Yes, that's probably better. It's how the pseudocode in the ARM ARM
deals with the problem: there's a FaultRecord structure which is
populated by AArch32.CreateFaultRecord() and AArch64.CreateFaultRecord(),
and then it isn't actually turned into an FSR or ESR_ELx format
value until needed (eg in AArch64.FaultSyndrome which calls EncodeLDFSC
to get the long-form fault code bits, or similar functions on the AArch32
side which end up calling EncodeSDFSC() for short-form code bits).

thanks
-- PMM

Re: [Qemu-devel] [PATCH v1 2/2] target-arm: Extend PAR format determination

[Peter Maydell]

On 11 July 2017 at 11:38, Edgar E. Iglesias <edgar.iglesias@xilinx.com> wrote:
> Another way could also be to have get_phys_addr() fill in generic
> fields in the FaultInfo struct and then have a faultinfo_to_fsr
> mapping function to populate FSR/PAR. Do you see any issues with that?

Edgar, did you ever have a go at implementing this?
I'm currently running into a similar issue with M profile,
where at the moment we stuff the information about what
kind of fault the MPU generates into a v7PMSA format
FSR value and reinterpret it into M profile exception
types and fault status register bits later. This works
OK, but for v8M we want to start reporting kinds of fault
(like SecureFault) that don't have equivalents in v7PMSA
at all, and maybe it would be better to clean this up rather
than assigning arbitrary bogus fsr values for internal use...

thanks
-- PMM

Re: [Qemu-devel] [PATCH v1 2/2] target-arm: Extend PAR format determination

[Edgar E. Iglesias]

On Mon, Sep 18, 2017 at 04:50:23PM +0100, Peter Maydell wrote:
> On 11 July 2017 at 11:38, Edgar E. Iglesias <edgar.iglesias@xilinx.com> wrote:
> > Another way could also be to have get_phys_addr() fill in generic
> > fields in the FaultInfo struct and then have a faultinfo_to_fsr
> > mapping function to populate FSR/PAR. Do you see any issues with that?
> 
> Edgar, did you ever have a go at implementing this?

Hi Peter,

No, I haven't looked at it yet.
I'm a bit behind on everything here so I probably won't get a chance
to look at it soonish...



> I'm currently running into a similar issue with M profile,
> where at the moment we stuff the information about what
> kind of fault the MPU generates into a v7PMSA format
> FSR value and reinterpret it into M profile exception
> types and fault status register bits later. This works
> OK, but for v8M we want to start reporting kinds of fault
> (like SecureFault) that don't have equivalents in v7PMSA
> at all, and maybe it would be better to clean this up rather
> than assigning arbitrary bogus fsr values for internal use...

I see, yes that sounds like a similar issue.
If you'd like to take over this, that'd be great :-)

Cheers,
Edgar

Re: [Qemu-devel] [PATCH v1 2/2] target-arm: Extend PAR format determination

[Peter Maydell]

On 19 September 2017 at 05:43, Edgar E. Iglesias
<edgar.iglesias@xilinx.com> wrote:
> On Mon, Sep 18, 2017 at 04:50:23PM +0100, Peter Maydell wrote:
>> I'm currently running into a similar issue with M profile,
>> where at the moment we stuff the information about what
>> kind of fault the MPU generates into a v7PMSA format
>> FSR value and reinterpret it into M profile exception
>> types and fault status register bits later. This works
>> OK, but for v8M we want to start reporting kinds of fault
>> (like SecureFault) that don't have equivalents in v7PMSA
>> at all, and maybe it would be better to clean this up rather
>> than assigning arbitrary bogus fsr values for internal use...
>
> I see, yes that sounds like a similar issue.
> If you'd like to take over this, that'd be great :-)

For the moment I've taken the easy approach of using dummy
FSR values. We'll see if that gets through code review :-)

thanks
-- PMM