On 12/15/2017 01:24 PM, Peter Maydell wrote:
> Instead of ignoring the response from address_space_ld*()
> (indicating an attempt to read a page table descriptor from
> an invalid physical address), use it to report the failure
> correctly.
>
> Since this is another couple of locations where we need to
> decide the value of the ARMMMUFaultInfo ea bit based on a
> MemTxResult, we factor out that operation into a helper
> function.
>
> Signed-off-by: Peter Maydell <peter.mayd...@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <f4...@amsat.org>
> ---
> Now we've fixed the get-phys-addr functions to always report
> errors via the ARMMMUFaultInfo struct, it's pretty easy to
> detect and report external aborts on translation table walks.
>
> target/arm/internals.h | 10 ++++++++++
> target/arm/helper.c | 39 ++++++++++++++++++++++++++++++++++-----
> target/arm/op_helper.c | 7 +------
> 3 files changed, 45 insertions(+), 11 deletions(-)
>
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 876854d..89f5d2f 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -687,6 +687,16 @@ static inline uint32_t arm_fi_to_lfsc(ARMMMUFaultInfo
> *fi)
> return fsc;
> }
>
> +static inline bool arm_extabort_type(MemTxResult result)
> +{
> + /* The EA bit in syndromes and fault status registers is an
> + * IMPDEF classification of external aborts. ARM implementations
> + * usually use this to indicate AXI bus Decode error (0) or
> + * Slave error (1); in QEMU we follow that.
> + */
> + return result != MEMTX_DECODE_ERROR;
> +}
> +
> /* Do a page table walk and add page to TLB if possible */
> bool arm_tlb_fill(CPUState *cpu, vaddr address,
> MMUAccessType access_type, int mmu_idx,
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index d1395f9..2575a83 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -8305,6 +8305,7 @@ static hwaddr S1_ptw_translate(CPUARMState *env,
> ARMMMUIdx mmu_idx,
> ret = get_phys_addr_lpae(env, addr, 0, ARMMMUIdx_S2NS, &s2pa,
> &txattrs, &s2prot, &s2size, fi, NULL);
> if (ret) {
> + assert(fi->type != ARMFault_None);
> fi->s2addr = addr;
> fi->stage2 = true;
> fi->s1ptw = true;
> @@ -8328,7 +8329,9 @@ static uint32_t arm_ldl_ptw(CPUState *cs, hwaddr addr,
> bool is_secure,
> ARMCPU *cpu = ARM_CPU(cs);
> CPUARMState *env = &cpu->env;
> MemTxAttrs attrs = {};
> + MemTxResult result = MEMTX_OK;
> AddressSpace *as;
> + uint32_t data;
>
> attrs.secure = is_secure;
> as = arm_addressspace(cs, attrs);
> @@ -8337,10 +8340,16 @@ static uint32_t arm_ldl_ptw(CPUState *cs, hwaddr
> addr, bool is_secure,
> return 0;
> }
> if (regime_translation_big_endian(env, mmu_idx)) {
> - return address_space_ldl_be(as, addr, attrs, NULL);
> + data = address_space_ldl_be(as, addr, attrs, &result);
> } else {
> - return address_space_ldl_le(as, addr, attrs, NULL);
> + data = address_space_ldl_le(as, addr, attrs, &result);
> }
> + if (result == MEMTX_OK) {
> + return data;
> + }
> + fi->type = ARMFault_SyncExternalOnWalk;
> + fi->ea = arm_extabort_type(result);
> + return 0;
> }
>
> static uint64_t arm_ldq_ptw(CPUState *cs, hwaddr addr, bool is_secure,
> @@ -8349,7 +8358,9 @@ static uint64_t arm_ldq_ptw(CPUState *cs, hwaddr addr,
> bool is_secure,
> ARMCPU *cpu = ARM_CPU(cs);
> CPUARMState *env = &cpu->env;
> MemTxAttrs attrs = {};
> + MemTxResult result = MEMTX_OK;
> AddressSpace *as;
> + uint32_t data;
>
> attrs.secure = is_secure;
> as = arm_addressspace(cs, attrs);
> @@ -8358,10 +8369,16 @@ static uint64_t arm_ldq_ptw(CPUState *cs, hwaddr
> addr, bool is_secure,
> return 0;
> }
> if (regime_translation_big_endian(env, mmu_idx)) {
> - return address_space_ldq_be(as, addr, attrs, NULL);
> + data = address_space_ldq_be(as, addr, attrs, &result);
> } else {
> - return address_space_ldq_le(as, addr, attrs, NULL);
> + data = address_space_ldq_le(as, addr, attrs, &result);
> + }
> + if (result == MEMTX_OK) {
> + return data;
> }
> + fi->type = ARMFault_SyncExternalOnWalk;
> + fi->ea = arm_extabort_type(result);
> + return 0;
> }
>
> static bool get_phys_addr_v5(CPUARMState *env, uint32_t address,
> @@ -8390,6 +8407,9 @@ static bool get_phys_addr_v5(CPUARMState *env, uint32_t
> address,
> }
> desc = arm_ldl_ptw(cs, table, regime_is_secure(env, mmu_idx),
> mmu_idx, fi);
> + if (fi->type != ARMFault_None) {
> + goto do_fault;
> + }
> type = (desc & 3);
> domain = (desc >> 5) & 0x0f;
> if (regime_el(env, mmu_idx) == 1) {
> @@ -8426,6 +8446,9 @@ static bool get_phys_addr_v5(CPUARMState *env, uint32_t
> address,
> }
> desc = arm_ldl_ptw(cs, table, regime_is_secure(env, mmu_idx),
> mmu_idx, fi);
> + if (fi->type != ARMFault_None) {
> + goto do_fault;
> + }
> switch (desc & 3) {
> case 0: /* Page translation fault. */
> fi->type = ARMFault_Translation;
> @@ -8508,6 +8531,9 @@ static bool get_phys_addr_v6(CPUARMState *env, uint32_t
> address,
> }
> desc = arm_ldl_ptw(cs, table, regime_is_secure(env, mmu_idx),
> mmu_idx, fi);
> + if (fi->type != ARMFault_None) {
> + goto do_fault;
> + }
> type = (desc & 3);
> if (type == 0 || (type == 3 && !arm_feature(env, ARM_FEATURE_PXN))) {
> /* Section translation fault, or attempt to use the encoding
> @@ -8559,6 +8585,9 @@ static bool get_phys_addr_v6(CPUARMState *env, uint32_t
> address,
> table = (desc & 0xfffffc00) | ((address >> 10) & 0x3fc);
> desc = arm_ldl_ptw(cs, table, regime_is_secure(env, mmu_idx),
> mmu_idx, fi);
> + if (fi->type != ARMFault_None) {
> + goto do_fault;
> + }
> ap = ((desc >> 4) & 3) | ((desc >> 7) & 4);
> switch (desc & 3) {
> case 0: /* Page translation fault. */
> @@ -8964,7 +8993,7 @@ static bool get_phys_addr_lpae(CPUARMState *env,
> target_ulong address,
> descaddr &= ~7ULL;
> nstable = extract32(tableattrs, 4, 1);
> descriptor = arm_ldq_ptw(cs, descaddr, !nstable, mmu_idx, fi);
> - if (fi->s1ptw) {
> + if (fi->type != ARMFault_None) {
> goto do_fault;
> }
>
> diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
> index c2bb4f3..2f35ff7 100644
> --- a/target/arm/op_helper.c
> +++ b/target/arm/op_helper.c
> @@ -226,12 +226,7 @@ void arm_cpu_do_transaction_failed(CPUState *cs, hwaddr
> physaddr,
> cpu_restore_state(cs, retaddr);
> }
>
> - /* The EA bit in syndromes and fault status registers is an
> - * IMPDEF classification of external aborts. ARM implementations
> - * usually use this to indicate AXI bus Decode error (0) or
> - * Slave error (1); in QEMU we follow that.
> - */
> - fi.ea = (response != MEMTX_DECODE_ERROR);
> + fi.ea = arm_extabort_type(response);
> fi.type = ARMFault_SyncExternal;
> deliver_fault(cpu, addr, access_type, mmu_idx, &fi);
> }
>
