Re: [Qemu-arm] [PATCH 06/67] target/arm: Introduce pc_read

[Peter Maydell]

On Fri, 26 Jul 2019 at 18:50, Richard Henderson
<address@hidden> wrote:
>
> We currently have 3 different ways of computing the architectural
> value of "PC" as seen in the ARM ARM.
>
> The value of s->pc has been incremented past the current insn,
> but that is all.  Thus for a32, PC = s->pc + 4; for t32, PC = s->pc;
> for t16, PC = s->pc + 2.  These differing computations make it
> impossible at present to unify the various code paths.
>
> Let s->pc_read hold the architectural value of PC for all cases.
>
> Signed-off-by: Richard Henderson <address@hidden>
> ---
>  target/arm/translate.h | 10 ++++++++
>  target/arm/translate.c | 53 ++++++++++++++++++------------------------
>  2 files changed, 32 insertions(+), 31 deletions(-)
>
> diff --git a/target/arm/translate.h b/target/arm/translate.h
> index a20f6e2056..2dfdd8ca66 100644
> --- a/target/arm/translate.h
> +++ b/target/arm/translate.h
> @@ -9,7 +9,17 @@ typedef struct DisasContext {
>      DisasContextBase base;
>      const ARMISARegisters *isar;
>
> +    /*
> +     * Summary of the various values for "PC":
> +     * base.pc_next -- the start of the current insn
> +     * pc           -- the start of the next insn

These are confusingly named -- logically "pc_next" ought to
be the PC of the next instruction and "pc" ought to be
that of the current one...

> +     * pc_read      -- the value for "PC" in the ARM ARM;

nit: this line should end with a colon, rather than a semicolon

> +     *                 in arm mode, the current insn + 8;
> +     *                 in thumb mode, the current insn + 4;
> +     *                 in aa64 mode, unused.
> +     */
>      target_ulong pc;
> +    target_ulong pc_read;
>      target_ulong page_start;
>      uint32_t insn;

Why target_ulong rather than uint32_t, given this is
aarch32 only?

Otherwise
Reviewed-by: Peter Maydell <address@hidden>

thanks
-- PMM