[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] target/alpha: Switch to do_transaction_failed()
From: |
Richard Henderson |
Subject: |
Re: [Qemu-devel] [PATCH] target/alpha: Switch to do_transaction_failed() hook |
Date: |
Tue, 8 Aug 2017 15:52:04 -0700 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 |
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 <address@hidden>
> ---
> Based-on: address@hidden
> 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 <address@hidden>
r~