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

Jeff Kubascik posted 1 patch 4 years, 3 months ago
Test asan failed
Test checkpatch failed
Test FreeBSD failed
Test docker-mingw@fedora failed
Test docker-clang@ubuntu failed
Test docker-quick@centos7 failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20191217210230.99559-1-jeff.kubascik@dornerworks.com
Maintainers: Peter Maydell <peter.maydell@linaro.org>
target/arm/tlb_helper.c    | 2 +-
target/arm/translate-a64.c | 1 +
target/arm/translate.c     | 4 +++-
target/arm/translate.h     | 2 ++
4 files changed, 7 insertions(+), 2 deletions(-)
[PATCH] target/arm: fix IL bit for data abort exceptions
Posted by Jeff Kubascik 4 years, 3 months ago
The Instruction Length bit of the Exception Syndrome Register was fixed to 1
for data aborts. This bit is used by the Xen hypervisor to determine how to
increment the program counter after a mmio handler is successful and returns
control back to the guest virtual machine. With this value fixed to 1, the
hypervisor would always increment the program counter by 0x4. This is a
problem when the guest virtual machine is using Thumb instructions, as the
instruction that caused the exception may be 16 bits.

This adds a is_16bit flag to the disassembler context to keep track of the
current instruction length. For load/store instructions, the instruction
length bit is stored with the instruction syndrome data, to be later used if
the data abort occurs.

Signed-off-by: Jeff Kubascik <jeff.kubascik@dornerworks.com>
---
Hello,

I am using the ARMv8 version of QEMU to run the Xen hypervisor with a guest
virtual machine compiled for AArch32/Thumb code. I have noticed that when
the guest VM tries to write to an emulated PL011 register, the mmio handler
always increments the program counter by 4, even if the store instruction
that caused the exception was a 16-bit Thumb instruction.

I have traced this back to the IL bit in the ESR_EL2 register. Xen uses the
IL bit to determine how to increment the program counter. However, QEMU does
not correctly emulate this bit, always setting it to 1 (32-bit instruction).

The above patch works for my setup. However, I am not very familiar with the
QEMU code base, so it may not be the best way to do it, or even be correct.
Any feedback would be greatly appreciated.

Sincerely,
Jeff Kubascik
---
 target/arm/tlb_helper.c    | 2 +-
 target/arm/translate-a64.c | 1 +
 target/arm/translate.c     | 4 +++-
 target/arm/translate.h     | 2 ++
 4 files changed, 7 insertions(+), 2 deletions(-)

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;
     }
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index d4bebbe629..a3c618fdd9 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -14045,6 +14045,7 @@ static void disas_a64_insn(CPUARMState *env, DisasContext *s)
     s->pc_curr = s->base.pc_next;
     insn = arm_ldl_code(env, s->base.pc_next, s->sctlr_b);
     s->insn = insn;
+    s->is_16bit = false;
     s->base.pc_next += 4;
 
     s->fp_access_checked = false;
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;
     }
@@ -11057,6 +11057,7 @@ static void arm_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
     dc->pc_curr = dc->base.pc_next;
     insn = arm_ldl_code(env, dc->base.pc_next, dc->sctlr_b);
     dc->insn = insn;
+    dc->is_16bit = false;
     dc->base.pc_next += 4;
     disas_arm_insn(dc, insn);
 
@@ -11126,6 +11127,7 @@ static void thumb_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
     dc->pc_curr = dc->base.pc_next;
     insn = arm_lduw_code(env, dc->base.pc_next, dc->sctlr_b);
     is_16bit = thumb_insn_is_16bit(dc, dc->base.pc_next, insn);
+    dc->is_16bit = is_16bit;
     dc->base.pc_next += 2;
     if (!is_16bit) {
         uint32_t insn2 = arm_lduw_code(env, dc->base.pc_next, dc->sctlr_b);
diff --git a/target/arm/translate.h b/target/arm/translate.h
index b837b7fcbf..c16f434477 100644
--- a/target/arm/translate.h
+++ b/target/arm/translate.h
@@ -14,6 +14,8 @@ typedef struct DisasContext {
     target_ulong pc_curr;
     target_ulong page_start;
     uint32_t insn;
+    /* 16-bit instruction flag */
+    bool is_16bit;
     /* Nonzero if this instruction has been conditionally skipped.  */
     int condjmp;
     /* The label that will be jumped to when the instruction is skipped.  */
-- 
2.17.1


Re: [PATCH] target/arm: fix IL bit for data abort exceptions
Posted by Richard Henderson 4 years, 3 months ago
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?

> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> index d4bebbe629..a3c618fdd9 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -14045,6 +14045,7 @@ static void disas_a64_insn(CPUARMState *env, DisasContext *s)
>      s->pc_curr = s->base.pc_next;
>      insn = arm_ldl_code(env, s->base.pc_next, s->sctlr_b);
>      s->insn = insn;
> +    s->is_16bit = false;
>      s->base.pc_next += 4;

Should not be necessary, as the field is not read along any a64 path.  (Also,
while it's not yet in master, there's a patch on list that zero initializes the
entire structure.)

> 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;
>      }
> @@ -11057,6 +11057,7 @@ static void arm_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
>      dc->pc_curr = dc->base.pc_next;
>      insn = arm_ldl_code(env, dc->base.pc_next, dc->sctlr_b);
>      dc->insn = insn;
> +    dc->is_16bit = false;
>      dc->base.pc_next += 4;
>      disas_arm_insn(dc, insn);
>  
> @@ -11126,6 +11127,7 @@ static void thumb_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
>      dc->pc_curr = dc->base.pc_next;
>      insn = arm_lduw_code(env, dc->base.pc_next, dc->sctlr_b);
>      is_16bit = thumb_insn_is_16bit(dc, dc->base.pc_next, insn);
> +    dc->is_16bit = is_16bit;
>      dc->base.pc_next += 2;
>      if (!is_16bit) {
>          uint32_t insn2 = arm_lduw_code(env, dc->base.pc_next, dc->sctlr_b);
> diff --git a/target/arm/translate.h b/target/arm/translate.h
> index b837b7fcbf..c16f434477 100644
> --- a/target/arm/translate.h
> +++ b/target/arm/translate.h
> @@ -14,6 +14,8 @@ typedef struct DisasContext {
>      target_ulong pc_curr;
>      target_ulong page_start;
>      uint32_t insn;
> +    /* 16-bit instruction flag */
> +    bool is_16bit;
>      /* Nonzero if this instruction has been conditionally skipped.  */
>      int condjmp;
>      /* The label that will be jumped to when the instruction is skipped.  */

The rest of this looks both correct and necessary.


r~

Re: [PATCH] target/arm: fix IL bit for data abort exceptions
Posted by Peter Maydell 4 years, 3 months ago
On Wed, 18 Dec 2019 at 01:03, Richard Henderson
<richard.henderson@linaro.org> 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: qemu-stable@nongnu.org on the fix for this.)

> > 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

Re: [PATCH] target/arm: fix IL bit for data abort exceptions
Posted by Richard Henderson 4 years, 3 months ago
On 12/19/19 2:43 AM, Peter Maydell wrote:
> 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: qemu-stable@nongnu.org on the fix for this.)

Oops, yes, that would be my fault.

>>>      /* 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?

We could, yes.


r~

Re: [PATCH] target/arm: fix IL bit for data abort exceptions
Posted by Jeff Kubascik 4 years, 3 months ago
On 12/19/2019 7:43 AM, Peter Maydell wrote:
> On Wed, 18 Dec 2019 at 01:03, Richard Henderson
> <richard.henderson@linaro.org> 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: qemu-stable@nongnu.org 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

Re: [PATCH] target/arm: fix IL bit for data abort exceptions
Posted by Peter Maydell 4 years, 3 months ago
On Fri, 20 Dec 2019 at 13:53, Jeff Kubascik
<jeff.kubascik@dornerworks.com> wrote:
>
> On 12/19/2019 7:43 AM, Peter Maydell wrote:
> > On Wed, 18 Dec 2019 at 01:03, Richard Henderson
> > <richard.henderson@linaro.org> 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: qemu-stable@nongnu.org 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