Re: [PATCH] target/arm: fix IL bit for data abort exceptions

[Jeff Kubascik]

On 12/19/2019 7:43 AM, Peter Maydell wrote:
> On Wed, 18 Dec 2019 at 01:03, Richard Henderson
> <address@hidden> wrote:
>>
>> On 12/17/19 11:02 AM, Jeff Kubascik wrote:
>>> diff --git a/target/arm/tlb_helper.c b/target/arm/tlb_helper.c
>>> index 5feb312941..e63f8bda29 100644
>>> --- a/target/arm/tlb_helper.c
>>> +++ b/target/arm/tlb_helper.c
>>> @@ -44,7 +44,7 @@ static inline uint32_t merge_syn_data_abort(uint32_t 
>>> template_syn,
>>>          syn = syn_data_abort_with_iss(same_el,
>>>                                        0, 0, 0, 0, 0,
>>>                                        ea, 0, s1ptw, is_write, fsc,
>>> -                                      false);
>>> +                                      true);
>>>          /* Merge the runtime syndrome with the template syndrome.  */
>>>          syn |= template_syn;
>>
>> This doesn't look correct.  Surely the IL bit should come from template_syn?
> 
> Yes. In translate.c we put it into the syndrome information by
> passing true/false to syn_data_abort_with_iss() depending on
> whether the issinfo passed in to disas_set_da_iss() has the
> ISSIs16Bit flag set.
> 
> I think this is a regression introduced in commit 46beb58efbb8a2a32
> when we converted the Thumb decoder over to decodetree. Before that
> 16 bit Thumb insns were in a different place in the old decoder and
> the 16-bit Thumb path passed ISSIs16Bit in with its issflags.
> (We should cc: address@hidden on the fix for this.)

The problem here was syn_data_abort_with_iss would return syn with the IL bit
set, which carries through when it gets or'd with template_syn. I had to change
the is_16bit argument to true so that it clear the IL bit.

>>> diff --git a/target/arm/translate.c b/target/arm/translate.c
>>> index 2b6c1f91bf..300480f1b7 100644
>>> --- a/target/arm/translate.c
>>> +++ b/target/arm/translate.c
>>> @@ -8555,7 +8555,7 @@ static ISSInfo make_issinfo(DisasContext *s, int rd, 
>>> bool p, bool w)
>>>
>>>      /* ISS not valid if writeback */
>>>      if (p && !w) {
>>> -        ret = rd;
>>> +        ret = rd | (s->is_16bit ? ISSIs16Bit : 0);
>>>      } else {
>>>          ret = ISSInvalid;
>>>      }
> 
> Rather than setting an is_16bit flag, we could just use
> "dc->base.pc_next - dc->pc_curr == 2", couldn't we?
> 
> thanks
> -- PMM
> 

Sincerely,
Jeff Kubascik