[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-arm] [PATCH 06/67] target/arm: Introduce pc_read
From: |
Richard Henderson |
Subject: |
Re: [Qemu-arm] [PATCH 06/67] target/arm: Introduce pc_read |
Date: |
Mon, 29 Jul 2019 17:38:57 -0700 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 |
On 7/29/19 7:05 AM, Peter Maydell wrote:
> 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...
Yes, well. I don't quite remember why "pc_next" was chosen for this field. It
is the "next" upon entry to tr_foo_disas_insn(). Often the target will
increment s->base.pc_next immediately, so it will also be the "next" insn
throughout translation. Though that isn't currently the case for ARM.
Once most of the uses of s->pc get moved to s->pc_read, it might be reasonable
to rename the remaining "s->base.pc_next" -> "s->pc_orig" and "s->pc" ->
"s->base.pc_next". Perhaps that would be clearer, I'm not sure.
>> + * pc_read -- the value for "PC" in the ARM ARM;
>
> nit: this line should end with a colon, rather than a semicolon
Oops, typo.
>> + * 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?
I didn't know if it would stay aarch32 only and it seemed more natural to match
the types.
r~
- [Qemu-arm] [PATCH 01/67] decodetree: Allow !function with no input bits, (continued)
- [Qemu-arm] [PATCH 01/67] decodetree: Allow !function with no input bits, Richard Henderson, 2019/07/26
- [Qemu-arm] [PATCH 02/67] target/arm: Remove offset argument to gen_exception_insn, Richard Henderson, 2019/07/26
- [Qemu-arm] [PATCH 04/67] target/arm: Remove offset argument to gen_exception_internal_insn, Richard Henderson, 2019/07/26
- [Qemu-arm] [PATCH 05/67] target/arm: Use the saved value of the insn address, Richard Henderson, 2019/07/26
- [Qemu-arm] [PATCH 06/67] target/arm: Introduce pc_read, Richard Henderson, 2019/07/26
- [Qemu-arm] [PATCH 08/67] target/arm: Use store_reg_from_load in thumb2 code, Richard Henderson, 2019/07/26
- [Qemu-arm] [PATCH 09/67] target/arm: Fold a pc load into load_reg, Richard Henderson, 2019/07/26
- [Qemu-arm] [PATCH 07/67] target/arm: Introduce add_reg_for_lit, Richard Henderson, 2019/07/26
- [Qemu-arm] [PATCH 12/67] target/arm: Introduce gen_illegal_op, Richard Henderson, 2019/07/26
- [Qemu-arm] [PATCH 11/67] target/arm: Add stubs for aa32 decodetree, Richard Henderson, 2019/07/26