[Qemu-devel] [PATCH] Fix ARM v7m gen_intermediate_code()

[Qemu-devel] [PATCH] Fix ARM v7m gen_intermediate_code()

[Christopher Friedt]

[-- Attachment #1: Type: text/plain, Size: 1775 bytes --]

Hi list,

I hope this message finds you well, as I'm currently on a lake in the
middle of nowhere relying on my flaky cellular connection. Roaming
sucks. In any case, I found a bug while trying to execute the "svc 0"
instruction for cortex-m3.

A UsageFault (EXCP_INVSTATE) is injected at
target/arm/translate.c:disas_arm_insn() without the patch. I noticed
because I added a log statement to the effect, so my pre-patch output
was:

$ qemu-system-arm -d int  -M netduino2 -cpu cortex-m3  -S -s
-semihosting -nographic -kernel hello.bin
Taking exception 2 [SVC]
... as 11
M variants do not implement ARM mode.
Taking exception 18 [v7M INVSTATE UsageFault]
... as 3
M variants do not implement ARM mode.
Taking exception 18 [v7M INVSTATE UsageFault]
qemu: fatal: Lockup: can't escalate 3 to HardFault (current priority -1)

R00=2000079c R01=0000008c R02=00000000 R03=000f0005
R04=2000071c R05=20000808 R06=00000000 R07=000f0005
R08=00000000 R09=00000000 R10=00000000 R11=00000000
R12=00000000 R13=200018e0 R14=fffffff1 R15=00000000
XPSR=00000003 ---- A handler
FPSCR: 00000000
Abort trap: 6

My post-patch output is:

$ qemu-system-arm -d int  -M netduino2 -cpu cortex-m3  -S -s
-semihosting -nographic -kernel hello.bin
Taking exception 2 [SVC]
... as 11
Taking exception 8 [QEMU v7M exception exit]
Exception return: magic PC fffffffd previous exception 11
...successful exception return
Taking exception 2 [SVC]
... as 11
Taking exception 8 [QEMU v7M exception exit]
Exception return: magic PC fffffffd previous exception 11
...successful exception return
qemu-system-arm: QEMU: Terminated via GDBstub

The patch is attached. Should be ok to go against master - i synced
before I went on vacation. Otherwise, I'd be happy to make any fixups
when I get back ;-)

Cheers,

C

[-- Attachment #2: qemu-system-arm-do-not-die-on-v7m-exception.patch --]
[-- Type: application/octet-stream, Size: 518 bytes --]

diff --git a/target/arm/translate.c b/target/arm/translate.c
index f845da7c63..655aa056d0 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -12878,7 +12878,7 @@ void gen_intermediate_code(CPUState *cpu, TranslationBlock *tb)
     DisasContext dc;
     const TranslatorOps *ops = &arm_translator_ops;
 
-    if (ARM_TBFLAG_THUMB(tb->flags)) {
+    if (ARM_TBFLAG_THUMB(tb->flags) || arm_feature(&ARM_CPU(cpu)->env, ARM_FEATURE_M)) {
         ops = &thumb_translator_ops;
     }
 #ifdef TARGET_AARCH64

Re: [Qemu-devel] [PATCH] Fix ARM v7m gen_intermediate_code()

[Peter Maydell]

On 23 August 2018 at 17:36, Christopher Friedt <chrisfriedt@gmail.com> wrote:
> I hope this message finds you well, as I'm currently on a lake in the
> middle of nowhere relying on my flaky cellular connection. Roaming
> sucks. In any case, I found a bug while trying to execute the "svc 0"
> instruction for cortex-m3.
>
> A UsageFault (EXCP_INVSTATE) is injected at
> target/arm/translate.c:disas_arm_insn() without the patch. I noticed
> because I added a log statement to the effect, so my pre-patch output
> was:
>
> $ qemu-system-arm -d int  -M netduino2 -cpu cortex-m3  -S -s
> -semihosting -nographic -kernel hello.bin
> Taking exception 2 [SVC]
> ... as 11
> M variants do not implement ARM mode.
> Taking exception 18 [v7M INVSTATE UsageFault]
> ... as 3
> M variants do not implement ARM mode.
> Taking exception 18 [v7M INVSTATE UsageFault]
> qemu: fatal: Lockup: can't escalate 3 to HardFault (current priority -1)
>
> R00=2000079c R01=0000008c R02=00000000 R03=000f0005
> R04=2000071c R05=20000808 R06=00000000 R07=000f0005
> R08=00000000 R09=00000000 R10=00000000 R11=00000000
> R12=00000000 R13=200018e0 R14=fffffff1 R15=00000000
> XPSR=00000003 ---- A handler
> FPSCR: 00000000
> Abort trap: 6

Hi; thanks for your patch, but I don't think it is correct.
What it does is to make QEMU ignore the T bit in the xPSR.
The architecture says that what should happen is that attempts
to execute with the T bit clear should cause an INVSTATE
UsageFault, which is exactly what we do. The reason we end up
aborting is because the CPU should really be going into
Lockup mode (where it basically hangs indefinitely),
and QEMU doesn't implement that.

Your guest code almost certainly has a bug where it is
not setting the low bit in the words in its exception
vector table. See the v7M ARM ARM section B1.5.3 if you
happen to have a copy on your lake, but the short answer
is that bit 1 must be set, exactly because this is what
defines whether EPSR.T is set on exception entry. If
you tried this on real hardware it would fail in the
same way (except that the hardware would lock up and sit
there like a lemon rather than calling abort()).

thanks
-- PMM

Re: [Qemu-devel] [PATCH] Fix ARM v7m gen_intermediate_code()

[Christopher Friedt]

On Thu., Aug. 23, 2018, 2:20 p.m. Peter Maydell, <peter.maydell@linaro.org>
wrote:

> On 23 August 2018 at 17:36, Christopher Friedt <chrisfriedt@gmail.com>
> wrote:
>
> Hi; thanks for your patch, but I don't think it is correct.
> What it does is to make QEMU ignore the T bit in the xPSR.
> The architecture says that what should happen is that attempts
> to execute with the T bit clear should cause an INVSTATE
> UsageFault, which is exactly what we do. The reason we end up
> aborting is because the CPU should really be going into
> Lockup mode (where it basically hangs indefinitely),
> and QEMU doesn't implement that.
>
> Your guest code almost certainly has a bug where it is
> not setting the low bit in the words in its exception
> vector table. See the v7M ARM ARM section B1.5.3 if you
> happen to have a copy on your lake, but the short answer
> is that bit 1 must be set, exactly because this is what
> defines whether EPSR.T is set on exception entry. If
> you tried this on real hardware it would fail in the
> same way (except that the hardware would lock up and sit
> there like a lemon rather than calling abort()).
>

Doh! You're right, although I checked for that in my rom vector table. As
it turns out, I relocated my vtable to ram and *then* zeroed bss, which
would obviously clear the T bit.

Cheers ;-)

>

Re: [Qemu-devel] [PATCH] Fix ARM v7m gen_intermediate_code()

[Christopher Friedt]

On Thu., Aug. 23, 2018, 3:50 p.m. Christopher Friedt, <chrisfriedt@gmail.com>
wrote:

>
>
> On Thu., Aug. 23, 2018, 2:20 p.m. Peter Maydell, <peter.maydell@linaro.org>
> wrote:
>
>> On 23 August 2018 at 17:36, Christopher Friedt <chrisfriedt@gmail.com>
>> wrote:
>>
>> happen to have a copy on your lake, but the short answer
>> is that bit 1 must be set, exactly because this is what
>> defines whether EPSR.T is set on exception entry.
>>
>
> Doh! You're right, although I checked for that in my rom vector table. As
> it turns out, I relocated my vtable to ram and *then* zeroed bss, which
> would obviously clear the T bit.
>

Ok, not right. The vtable entries all do have the T bit set and I was
zeroing the bss first after all. All isr entries are either 0x2e9 (no-op
isr), 0x2ff (no-op fault handler), or 0 (for reserved), so the T bit is set
everywhere it needs to be.

It would seem there is an underlying issue with the T bit not getting set
upon exception invocation, and my previous patch was more of a bandaid
solution. It would be fairly straightforward to come up with a short
assembly fragment that verifiably demonstrates the problem in this case.

Let me see what I can do.

C

>

Re: [Qemu-devel] [PATCH] Fix ARM v7m gen_intermediate_code()

[Christopher Friedt]

[-- Attachment #1: Type: text/plain, Size: 1409 bytes --]

On Thu, Aug 23, 2018 at 4:01 PM Christopher Friedt
<chrisfriedt@gmail.com> wrote:
> On Thu., Aug. 23, 2018, 3:50 p.m. Christopher Friedt, <chrisfriedt@gmail.com> wrote:
>> On Thu., Aug. 23, 2018, 2:20 p.m. Peter Maydell, <peter.maydell@linaro.org> wrote:
>>> On 23 August 2018 at 17:36, Christopher Friedt <chrisfriedt@gmail.com> wrote:
>>>
>>> happen to have a copy on your lake, but the short answer
>>> is that bit 1 must be set, exactly because this is what
>>> defines whether EPSR.T is set on exception entry.
>>
>> Doh! You're right, although I checked for that in my rom vector table. As it turns out, I relocated my vtable to ram and *then* zeroed bss, which would obviously clear the T bit.
>
> Ok, not right. The vtable entries all do have the T bit set and I was zeroing the bss first after all. All isr entries are either 0x2e9 (no-op isr), 0x2ff (no-op fault handler), or 0 (for reserved), so the T bit is set everywhere it needs to be.
>
> It would seem there is an underlying issue with the T bit not getting set upon exception invocation, and my previous patch was more of a bandaid solution. It would be fairly straightforward to come up with a short assembly fragment that verifiably demonstrates the problem in this case.

OK - assembly fragment worked fine. So yeah, it has to be something
wrong with the way that the C library I'm linking to (musl) is doing
syscalls.

[-- Attachment #2: foo.S --]
[-- Type: application/octet-stream, Size: 1747 bytes --]

        .syntax unified
        .cpu cortex-m3

        .section ".text"

		.global _start

        .type _vtable, %object
_vtable:
        .word   0x20020000 // initial sp
        .word   _start     // initial pc
        .word   _isr       // nmi
        .word   memcpy     // hardfault
        .word   _isr       // memmanage
        .word   memcpy     // busfault
        .word   memcpy     // usagefault
        .word   0          // reserved
        .word   0          // reserved
        .word   0          // reserved
        .word   0          // reserved
        .word   _isr       // svcall
        .word   _isr       // debugmonitor
        .word   0          // reserved
        .word   _isr       // pendsv
        .word   _isr       // systick
        .size _vtable, . - _vtable

		.type _start, %function
_start:
		// copy vtable to beginning of ram
		ldr r0, .L0        // destination
		mov r1, #0         // source
		mov r2, #(16*4)    // size (bytes)
		bl memcpy

		ldr r0, .L0        // beginning of ram
		ldr r1, .L1        // VTOR
		str r0, [ r1 ]
		svc 0
		b _start
		.size _start, . - _start

		.type _isr, %function
_isr:
		bx lr
		.size _isr, . - _isr

		.type memcpy, %function
memcpy:
		push	{r7}
		sub	sp, #20
		add	r7, sp, #0
		str	r0, [r7, #12]
		str	r1, [r7, #8]
		str	r2, [r7, #4]
		b.n	=2f
1:
		ldr	r3, [r7, #8]
		ldrb	r2, [r3, #0]
		ldr	r3, [r7, #12]
		strb	r2, [r3, #0]
		ldr	r3, [r7, #12]
		adds	r3, #1
		str	r3, [r7, #12]
		ldr	r3, [r7, #8]
		adds	r3, #1
		str	r3, [r7, #8]
		ldr	r3, [r7, #4]
		subs	r3, #1
		str	r3, [r7, #4]
		ldr	r3, [r7, #4]
2:
		cmp	r3, #0
		bne	1b
		nop
		adds	r7, #20
		mov	sp, r7
		pop	{r7}
		bx	lr

		.size memcpy, . - memcpy

.L0:
		.word 0x20000000

.L1:
		.word 0xe000ed08

		.end

Re: [Qemu-devel] [PATCH] Fix ARM v7m gen_intermediate_code()

[Christopher Friedt]

In the end it was just incorrect alignment for my vector table