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

[Peter Maydell]

On Fri, 20 Dec 2019 at 13:53, Jeff Kubascik
<address@hidden> wrote:
>
> 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.

Interesting. I think that's an entirely separate (and long
standing) bug to the regression where we forgot to fill in ISSIs16Bit
in the issinfo field, and it masks the other bug...

thanks
-- PMM