[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] target/arm: fix IL bit for data abort exceptions
From: |
Jeff Kubascik |
Subject: |
Re: [PATCH] target/arm: fix IL bit for data abort exceptions |
Date: |
Fri, 20 Dec 2019 08:54:30 -0500 |
User-agent: |
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.3.1 |
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