qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH] fix WFI/WFE length in syndrome register


From: Peter Maydell
Subject: Re: [Qemu-devel] [PATCH] fix WFI/WFE length in syndrome register
Date: Fri, 20 Oct 2017 10:30:30 +0100

On 19 October 2017 at 21:56, Stefano Stabellini <address@hidden> wrote:
> On Thu, 19 Oct 2017, Peter Maydell wrote:
>> On 18 October 2017 at 23:03, Stefano Stabellini <address@hidden> wrote:
>> > WFI/E are 4 bytes long: set ARM_EL_IL_SHIFT in the syndrome.
>> >
>> > Signed-off-by: Stefano Stabellini <address@hidden>
>> >
>> > diff --git a/target/arm/internals.h b/target/arm/internals.h
>> > index 1f6efef..cf8c966 100644
>> > --- a/target/arm/internals.h
>> > +++ b/target/arm/internals.h
>> > @@ -398,6 +398,7 @@ static inline uint32_t syn_breakpoint(int same_el)
>> >  static inline uint32_t syn_wfx(int cv, int cond, int ti)
>> >  {
>> >      return (EC_WFX_TRAP << ARM_EL_EC_SHIFT) |
>> > +           (1 << ARM_EL_IL_SHIFT) |
>> >             (cv << 24) | (cond << 20) | ti;
>> >  }
>>
>> Hmm. What we do now is definitely wrong, but WFI and WFE can be 2 bytes:
>> there is a T1 Thumb encoding that is 2 bytes.
>>
>> HELPER(wfi) doesn't get that right, though:
>>     if (target_el) {
>>         env->pc -= 4;
>>         raise_exception(env, EXCP_UDEF, syn_wfx(1, 0xe, 0), target_el);
>>     }
>>
>> So I think that HELPER(wfi) needs to be passed an extra
>> parameter is_16bit, which it can then use both in its adjustment
>> of env->pc and to pass as an extra parameter to syn_wfx(),
>> which is then syn_wfx(int cv, int cond, int ti, bool is_16bit).
>>
>> (In theory HELPER(wfe) should also be passed is_16bit, but
>> since it doesn't currently ever raise an exception it
>> doesn't matter.)
>
> Wouldn't it be better to just check on env->thumb like
> HELPER(cpsr_write_eret) for example?

> diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
> index 3914145..ea16c9a 100644
> --- a/target/arm/op_helper.c
> +++ b/target/arm/op_helper.c
> @@ -476,8 +476,8 @@ void HELPER(wfi)(CPUARMState *env)
>      }
>
>      if (target_el) {
> -        env->pc -= 4;
> -        raise_exception(env, EXCP_UDEF, syn_wfx(1, 0xe, 0), target_el);
> +        env->pc -= env->thumb ? 2 : 4;
> +        raise_exception(env, EXCP_UDEF, syn_wfx(1, 0xe, 0, env->thumb), 
> target_el);
>      }
>
>      cs->exception_index = EXCP_HLT;

This doesn't work, because there is also a 32 bit Thumb
encoding of WFI. env->thumb says "we are in thumb mode",
which isn't the same as "this insn is 16 bits".

thanks
-- PMM



reply via email to

[Prev in Thread] Current Thread [Next in Thread]