[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-arm] [PATCH v9 23/25] target-arm: introduce ARM_CP_EXIT_PC
From: |
Alex Bennée |
Subject: |
Re: [Qemu-arm] [PATCH v9 23/25] target-arm: introduce ARM_CP_EXIT_PC |
Date: |
Fri, 03 Feb 2017 11:33:23 +0000 |
User-agent: |
mu4e 0.9.19; emacs 25.1.91.7 |
Peter Maydell <address@hidden> writes:
> On 1 February 2017 at 15:05, Alex Bennée <address@hidden> wrote:
>> Some helpers may trigger an immediate exit of the cpu_loop. If this
>> happens the PC need to be rectified to ensure the restart will begin
>> on the next instruction.
>>
>> Signed-off-by: Alex Bennée <address@hidden>
>> ---
>> target/arm/cpu.h | 3 ++-
>> target/arm/translate-a64.c | 4 ++++
>> target/arm/translate.c | 4 ++++
>> 3 files changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>> index d61793ca06..a3c4d07817 100644
>> --- a/target/arm/cpu.h
>> +++ b/target/arm/cpu.h
>> @@ -1465,7 +1465,8 @@ static inline uint64_t cpreg_to_kvm_id(uint32_t
>> cpregid)
>> #define ARM_CP_NZCV (ARM_CP_SPECIAL | (3 << 8))
>> #define ARM_CP_CURRENTEL (ARM_CP_SPECIAL | (4 << 8))
>> #define ARM_CP_DC_ZVA (ARM_CP_SPECIAL | (5 << 8))
>> -#define ARM_LAST_SPECIAL ARM_CP_DC_ZVA
>> +#define ARM_CP_EXIT_PC (ARM_CP_SPECIAL | (6 << 8))
>> +#define ARM_LAST_SPECIAL ARM_CP_EXIT_PC
>
> This shouldn't be a "special", because those are for
> "this is a special case that is handled entirely in the translate
> code", not "I need some extra behaviour on the code generated
> for calling the helper functions" (which is what the
> plain non-special ARM_CP flags do). Notice that all the other
> "special" cases completely define the behaviour of the cp that
> uses them, and the code implementing them ends the case
> statement with "return", not "break".
>
> Missing documentation comment change.
I posted this before you commented on the last version. Anyway see
bellow.
>
> That said, I'm definitely becoming more strongly of the
> opinion that longjumping out of this helper is not the
> best way to implement this, so these remarks are a bit moot.
Yep the tree:
https://github.com/stsquad/qemu/commits/mttcg/base-patches-v10
Reverts the this change and changes the cputlb flush code to return and
let the guest translation code exit the normal way. I was hoping to get
some feedback from Paolo and Richard before I roll the fixes together
and post v10 which will be later today.
>
>> /* Used only as a terminator for ARMCPRegInfo lists */
>> #define ARM_CP_SENTINEL 0xffff
>> /* Mask of only the flag bits in a type field */
>> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
>> index 7e7131fe2f..98d4fac070 100644
>> --- a/target/arm/translate-a64.c
>> +++ b/target/arm/translate-a64.c
>> @@ -1561,6 +1561,10 @@ static void handle_sys(DisasContext *s, uint32_t
>> insn, bool isread,
>> tcg_rt = cpu_reg(s, rt);
>> gen_helper_dc_zva(cpu_env, tcg_rt);
>> return;
>> + case ARM_CP_EXIT_PC:
>> + /* The helper may exit the cpu_loop so ensure PC is correct */
>> + gen_a64_set_pc_im(s->pc);
>> + break;
>> default:
>> break;
>> }
>> diff --git a/target/arm/translate.c b/target/arm/translate.c
>> index 24faa7c60c..e1f4a48720 100644
>> --- a/target/arm/translate.c
>> +++ b/target/arm/translate.c
>> @@ -7510,6 +7510,10 @@ static int disas_coproc_insn(DisasContext *s,
>> uint32_t insn)
>> gen_set_pc_im(s, s->pc);
>> s->is_jmp = DISAS_WFI;
>> return 0;
>> + case ARM_CP_EXIT_PC:
>> + /* The helper may exit the cpu_loop so ensure PC is correct */
>> + gen_set_pc_im(s, s->pc);
>> + break;
>> default:
>> break;
>> }
>
> thanks
> -- PMM
--
Alex Bennée