[Qemu-devel] [PATCH] target/alpha: Switch to do_transaction_failed() hook

[Qemu-devel] [PATCH] target/alpha: Switch to do_transaction_failed() hook

[Peter Maydell]

Switch the alpha target from the old unassigned_access hook
to the new do_transaction_failed hook. This allows us to
resolve a ??? in the old hook implementation.

The only part of the alpha target that does physical
memory accesses is reading the page table -- add a
TODO comment there to the effect that we should handle
bus faults on page table walks. (Since the palcode
doesn't actually do anything useful on a bus fault anyway
it's a bit moot for now.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Based-on: 1501867249-1924-1-git-send-email-peter.maydell@linaro.org
This patch sits on top of the series adding the new hook.

The comment in the page walk code could probably be
rephrased by somebody who knows what the palcode
behaviour in the busfault-on-table-walk case is.

This patch isn't really tested (just 'make check' and
checking that qemu-system-alpha can start up).
---
 target/alpha/cpu.h        |  8 +++++---
 target/alpha/cpu.c        |  2 +-
 target/alpha/helper.c     |  8 ++++++++
 target/alpha/mem_helper.c | 19 ++++++++++---------
 4 files changed, 24 insertions(+), 13 deletions(-)

diff --git a/target/alpha/cpu.h b/target/alpha/cpu.h
index e95be2b..389e1a4 100644
--- a/target/alpha/cpu.h
+++ b/target/alpha/cpu.h
@@ -488,9 +488,11 @@ void cpu_alpha_store_fpcr (CPUAlphaState *env, uint64_t val);
 uint64_t cpu_alpha_load_gr(CPUAlphaState *env, unsigned reg);
 void cpu_alpha_store_gr(CPUAlphaState *env, unsigned reg, uint64_t val);
 #ifndef CONFIG_USER_ONLY
-QEMU_NORETURN void alpha_cpu_unassigned_access(CPUState *cpu, hwaddr addr,
-                                               bool is_write, bool is_exec,
-                                               int unused, unsigned size);
+void alpha_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr,
+                                     vaddr addr, unsigned size,
+                                     MMUAccessType access_type,
+                                     int mmu_idx, MemTxAttrs attrs,
+                                     MemTxResult response, uintptr_t retaddr);
 #endif
 
 static inline void cpu_get_tb_cpu_state(CPUAlphaState *env, target_ulong *pc,
diff --git a/target/alpha/cpu.c b/target/alpha/cpu.c
index 76150f4..4d49fd0 100644
--- a/target/alpha/cpu.c
+++ b/target/alpha/cpu.c
@@ -307,7 +307,7 @@ static void alpha_cpu_class_init(ObjectClass *oc, void *data)
 #ifdef CONFIG_USER_ONLY
     cc->handle_mmu_fault = alpha_cpu_handle_mmu_fault;
 #else
-    cc->do_unassigned_access = alpha_cpu_unassigned_access;
+    cc->do_transaction_failed = alpha_cpu_do_transaction_failed;
     cc->do_unaligned_access = alpha_cpu_do_unaligned_access;
     cc->get_phys_page_debug = alpha_cpu_get_phys_page_debug;
     dc->vmsd = &vmstate_alpha_cpu;
diff --git a/target/alpha/helper.c b/target/alpha/helper.c
index 34121f4..36407f7 100644
--- a/target/alpha/helper.c
+++ b/target/alpha/helper.c
@@ -163,6 +163,14 @@ static int get_physical_address(CPUAlphaState *env, target_ulong addr,
 
     pt = env->ptbr;
 
+    /* TODO: rather than using ldq_phys() to read the page table we should
+     * use address_space_ldq() so that we can handle the case when
+     * the page table read gives a bus fault, rather than ignoring it.
+     * For the existing code the zero data that ldq_phys will return for
+     * an access to invalid memory will result in our treating the page
+     * table as invalid, which may even be the right behaviour.
+     */
+
     /* L1 page table read.  */
     index = (addr >> (TARGET_PAGE_BITS + 20)) & 0x3ff;
     L1pte = ldq_phys(cs->as, pt + index*8);
diff --git a/target/alpha/mem_helper.c b/target/alpha/mem_helper.c
index 78a7d45..3c06baa 100644
--- a/target/alpha/mem_helper.c
+++ b/target/alpha/mem_helper.c
@@ -49,22 +49,23 @@ void alpha_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
     cpu_loop_exit(cs);
 }
 
-void alpha_cpu_unassigned_access(CPUState *cs, hwaddr addr,
-                                 bool is_write, bool is_exec, int unused,
-                                 unsigned size)
+void alpha_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr,
+                                     vaddr addr, unsigned size,
+                                     MMUAccessType access_type,
+                                     int mmu_idx, MemTxAttrs attrs,
+                                     MemTxResult response, uintptr_t retaddr)
 {
     AlphaCPU *cpu = ALPHA_CPU(cs);
     CPUAlphaState *env = &cpu->env;
 
+    if (retaddr) {
+        cpu_restore_state(cs, retaddr);
+    }
+
     env->trap_arg0 = addr;
-    env->trap_arg1 = is_write ? 1 : 0;
+    env->trap_arg1 = access_type == MMU_DATA_STORE ? 1 : 0;
     cs->exception_index = EXCP_MCHK;
     env->error_code = 0;
-
-    /* ??? We should cpu_restore_state to the faulting insn, but this hook
-       does not have access to the retaddr value from the original helper.
-       It's all moot until the QEMU PALcode grows an MCHK handler.  */
-
     cpu_loop_exit(cs);
 }
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH] target/alpha: Switch to do_transaction_failed() hook

[Richard Henderson]

On 08/08/2017 05:42 AM, Peter Maydell wrote:
> Switch the alpha target from the old unassigned_access hook
> to the new do_transaction_failed hook. This allows us to
> resolve a ??? in the old hook implementation.
> 
> The only part of the alpha target that does physical
> memory accesses is reading the page table -- add a
> TODO comment there to the effect that we should handle
> bus faults on page table walks. (Since the palcode
> doesn't actually do anything useful on a bus fault anyway
> it's a bit moot for now.)
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Based-on: 1501867249-1924-1-git-send-email-peter.maydell@linaro.org
> This patch sits on top of the series adding the new hook.
> 
> The comment in the page walk code could probably be
> rephrased by somebody who knows what the palcode
> behaviour in the busfault-on-table-walk case is.
> 
> This patch isn't really tested (just 'make check' and
> checking that qemu-system-alpha can start up).
> ---
>  target/alpha/cpu.h        |  8 +++++---
>  target/alpha/cpu.c        |  2 +-
>  target/alpha/helper.c     |  8 ++++++++
>  target/alpha/mem_helper.c | 19 ++++++++++---------
>  4 files changed, 24 insertions(+), 13 deletions(-)
> 
> diff --git a/target/alpha/cpu.h b/target/alpha/cpu.h
> index e95be2b..389e1a4 100644
> --- a/target/alpha/cpu.h
> +++ b/target/alpha/cpu.h
> @@ -488,9 +488,11 @@ void cpu_alpha_store_fpcr (CPUAlphaState *env, uint64_t val);
>  uint64_t cpu_alpha_load_gr(CPUAlphaState *env, unsigned reg);
>  void cpu_alpha_store_gr(CPUAlphaState *env, unsigned reg, uint64_t val);
>  #ifndef CONFIG_USER_ONLY
> -QEMU_NORETURN void alpha_cpu_unassigned_access(CPUState *cpu, hwaddr addr,
> -                                               bool is_write, bool is_exec,
> -                                               int unused, unsigned size);
> +void alpha_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr,
> +                                     vaddr addr, unsigned size,
> +                                     MMUAccessType access_type,
> +                                     int mmu_idx, MemTxAttrs attrs,
> +                                     MemTxResult response, uintptr_t retaddr);
>  #endif
>  
>  static inline void cpu_get_tb_cpu_state(CPUAlphaState *env, target_ulong *pc,
> diff --git a/target/alpha/cpu.c b/target/alpha/cpu.c
> index 76150f4..4d49fd0 100644
> --- a/target/alpha/cpu.c
> +++ b/target/alpha/cpu.c
> @@ -307,7 +307,7 @@ static void alpha_cpu_class_init(ObjectClass *oc, void *data)
>  #ifdef CONFIG_USER_ONLY
>      cc->handle_mmu_fault = alpha_cpu_handle_mmu_fault;
>  #else
> -    cc->do_unassigned_access = alpha_cpu_unassigned_access;
> +    cc->do_transaction_failed = alpha_cpu_do_transaction_failed;
>      cc->do_unaligned_access = alpha_cpu_do_unaligned_access;
>      cc->get_phys_page_debug = alpha_cpu_get_phys_page_debug;
>      dc->vmsd = &vmstate_alpha_cpu;
> diff --git a/target/alpha/helper.c b/target/alpha/helper.c
> index 34121f4..36407f7 100644
> --- a/target/alpha/helper.c
> +++ b/target/alpha/helper.c
> @@ -163,6 +163,14 @@ static int get_physical_address(CPUAlphaState *env, target_ulong addr,
>  
>      pt = env->ptbr;
>  
> +    /* TODO: rather than using ldq_phys() to read the page table we should
> +     * use address_space_ldq() so that we can handle the case when
> +     * the page table read gives a bus fault, rather than ignoring it.
> +     * For the existing code the zero data that ldq_phys will return for
> +     * an access to invalid memory will result in our treating the page
> +     * table as invalid, which may even be the right behaviour.
> +     */

FWIW, I believe the proper behaviour is to signal a machine check.
I'd have to re-read the MILO source to be sure.

That said, this looks good.  If I have a chance, I'll throw together a small
test case for this.

Reviewed-by: Richard Henderson <rth@twiddle.net>


r~

Re: [Qemu-devel] [PATCH] target/alpha: Switch to do_transaction_failed() hook

[Peter Maydell]

On 8 August 2017 at 13:42, Peter Maydell <peter.maydell@linaro.org> wrote:
> Switch the alpha target from the old unassigned_access hook
> to the new do_transaction_failed hook. This allows us to
> resolve a ??? in the old hook implementation.
>
> The only part of the alpha target that does physical
> memory accesses is reading the page table -- add a
> TODO comment there to the effect that we should handle
> bus faults on page table walks. (Since the palcode
> doesn't actually do anything useful on a bus fault anyway
> it's a bit moot for now.)
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Based-on: 1501867249-1924-1-git-send-email-peter.maydell@linaro.org
> This patch sits on top of the series adding the new hook.

The prerequisites for this patch are now all in git master.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] target/alpha: Switch to do_transaction_failed() hook

[Richard Henderson]

On 09/05/2017 08:54 AM, Peter Maydell wrote:
> On 8 August 2017 at 13:42, Peter Maydell <peter.maydell@linaro.org> wrote:
>> Switch the alpha target from the old unassigned_access hook
>> to the new do_transaction_failed hook. This allows us to
>> resolve a ??? in the old hook implementation.
>>
>> The only part of the alpha target that does physical
>> memory accesses is reading the page table -- add a
>> TODO comment there to the effect that we should handle
>> bus faults on page table walks. (Since the palcode
>> doesn't actually do anything useful on a bus fault anyway
>> it's a bit moot for now.)
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>> Based-on: 1501867249-1924-1-git-send-email-peter.maydell@linaro.org
>> This patch sits on top of the series adding the new hook.
> 
> The prerequisites for this patch are now all in git master.

Thanks.  Queued to tgt-axp.


r~