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.maydell@linaro.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);
}
--
2.7.4
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.maydell@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@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);
> }
>
On 15 December 2017 at 16:24, Peter Maydell <peter.maydell@linaro.org> 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.maydell@linaro.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. Reading through the Arm ARM for an unrelated reason, I discovered that this patch is necessary but not sufficient, because external aborts on translation table walks for ATS instructions are supposed to be reported as Data Aborts (unlike other kinds of translation fault) -- see DDI0487B.a section D4.2.11. So this patch is OK (reporting the abort in the PAR is no worse than ignoring it completely and using a zero descriptor value) but we should add another one after it that makes do_ats_write() special-case fi->type == ARMFault_SyncExternal and generate an exception. I won't get to that until next year, though. (Need to look up the various architecture versions to check whether they all behave the same here.) thanks -- PMM
© 2016 - 2024 Red Hat, Inc.