08_scheduler doesn't work

[qianfan-Zhao]

08_scheduler doesn't work on my WSL2, no uart message anymore after Welcome to Chapter 8, Scheduling!.

Version:

• arm-none-eabi-gcc: gcc version 9.2.1 20191025 (release) [ARM/arm-9-branch revision 277599] (15:9-2019-q4-0ubuntu1)
• qemu: QEMU emulator version 7.2.90

I had debug the source code and found ptimer_isr is never enter, that will make the global variable systimie always zero. Appending -d guest_errors to qemu command line params, I got next error messages:

Welcome to Chapter 8, Scheduling!
gic_cpu_read: Bad offset 5
gic_cpu_write: Bad offset 5
gic_cpu_read: Bad offset 6
gic_cpu_write: Bad offset 6
gic_cpu_read: Bad offset 7
gic_cpu_write: Bad offset 7
gic_cpu_read: Bad offset 1
gic_cpu_write: Bad offset 1
gic_cpu_read: Bad offset 2
gic_cpu_write: Bad offset 2
gic_cpu_read: Bad offset 3
gic_cpu_write: Bad offset 3
Invalid read at addr 0x8, size 1, region 'arm_mptimer_timer', reason: invalid size (min:4 max:4)
Invalid read at addr 0x208, size 1, region '(null)', reason: rejected
Invalid write at addr 0x8, size 1, region 'arm_mptimer_timer', reason: invalid size (min:4 max:4)
Invalid write at addr 0x208, size 1, region '(null)', reason: rejected
Invalid read at addr 0x9, size 1, region 'arm_mptimer_timer', reason: invalid size (min:4 max:4)
Invalid read at addr 0x209, size 1, region '(null)', reason: rejected
Invalid write at addr 0x9, size 1, region 'arm_mptimer_timer', reason: invalid size (min:4 max:4)
Invalid write at addr 0x209, size 1, region '(null)', reason: rejected
Invalid read at addr 0xA, size 1, region 'arm_mptimer_timer', reason: invalid size (min:4 max:4)
Invalid read at addr 0x20A, size 1, region '(null)', reason: rejected
Invalid write at addr 0xA, size 1, region 'arm_mptimer_timer', reason: invalid size (min:4 max:4)
Invalid write at addr 0x20A, size 1, region '(null)', reason: rejected
Invalid read at addr 0xB, size 1, region 'arm_mptimer_timer', reason: invalid size (min:4 max:4)
Invalid read at addr 0x20B, size 1, region '(null)', reason: rejected
Invalid write at addr 0xB, size 1, region 'arm_mptimer_timer', reason: invalid size (min:4 max:4)
Invalid write at addr 0x20B, size 1, region '(null)', reason: rejected

The private timer registers should be accessed with uint32, but qemu report it was accessed by uint8, let's check the disassemble code:

$ arm-none-eabi-objdump -d -S 
...
600004a6 <ptimer_init>:
ptimer_error ptimer_init(uint16_t millisecs) {
600004a6:	b538      	push	{r3, r4, r5, lr}
600004a8:	4604      	mov	r4, r0
600004aa:	ee9f 5f10 	mrc	15, 4, r5, cr15, cr0, {0}
    regs = (private_timer_registers*)PTIMER_BASE;
600004ae:	f505 62c0 	add.w	r2, r5, #1536	; 0x600
600004b2:	f241 0308 	movw	r3, #4104	; 0x1008
600004b6:	f2c7 0300 	movt	r3, #28672	; 0x7000
600004ba:	601a      	str	r2, [r3, #0]
    if (!validate_config(millisecs)) {
600004bc:	f7ff ffbc 	bl	60000438 <validate_config>
600004c0:	b908      	cbnz	r0, 600004c6 <ptimer_init+0x20>
        return PTIMER_INVALID_PERIOD;
600004c2:	2001      	movs	r0, #1
}
600004c4:	bd38      	pop	{r3, r4, r5, pc}
    uint32_t load_val = millisecs_to_timer_value(millisecs);
600004c6:	4620      	mov	r0, r4
600004c8:	f7ff ffb8 	bl	6000043c <millisecs_to_timer_value>
    WRITE32(regs->LR, load_val); /* Load the initial timer value */
600004cc:	f8c5 0600 	str.w	r0, [r5, #1536]	; 0x600
    (void)irq_register_isr(PTIMER_INTERRUPT, ptimer_isr);
600004d0:	f240 4181 	movw	r1, #1153	; 0x481
600004d4:	f2c6 0100 	movt	r1, #24576	; 0x6000
600004d8:	201d      	movs	r0, #29
600004da:	f7ff ff96 	bl	6000040a <irq_register_isr>
    WRITE32(regs->CTRL, ctrl); /* Configure and start the timer */
600004de:	f241 0308 	movw	r3, #4104	; 0x1008
600004e2:	f2c7 0300 	movt	r3, #28672	; 0x7000
600004e6:	681b      	ldr	r3, [r3, #0]
600004e8:	7a1a      	ldrb	r2, [r3, #8]
600004ea:	2000      	movs	r0, #0
600004ec:	2207      	movs	r2, #7
600004ee:	721a      	strb	r2, [r3, #8]
600004f0:	7a5a      	ldrb	r2, [r3, #9]
600004f2:	7258      	strb	r0, [r3, #9]
600004f4:	7a9a      	ldrb	r2, [r3, #10]
600004f6:	7298      	strb	r0, [r3, #10]
600004f8:	7ada      	ldrb	r2, [r3, #11]
600004fa:	72d8      	strb	r0, [r3, #11]
    return PTIMER_OK;

Yes, it is accessed by uint8. Remove the __attribute__((packed)) from the register declare structure, the gcc will make those register accessed by uint32. Next is a patch:

diff --git a/src/08_scheduler/src/gic.h b/src/08_scheduler/src/gic.h
index 4620c05..cd5ff80 100644
--- a/src/08_scheduler/src/gic.h
+++ b/src/08_scheduler/src/gic.h
@@ -4,7 +4,7 @@
 #include <stdint.h>
 #include "cpu.h"
 
-typedef volatile struct __attribute__((packed)) {
+typedef volatile struct {
     uint32_t DCTLR;                 /* 0x0 Distributor Control register */
     const uint32_t DTYPER;          /* 0x4 Controller type register */
     const uint32_t DIIDR;           /* 0x8 Implementer identification register */
@@ -26,7 +26,7 @@ typedef volatile struct __attribute__((packed)) {
        Don't care about them */
 } gic_distributor_registers;
 
-typedef volatile struct __attribute__((packed)) {
+typedef volatile struct {
     uint32_t CCTLR;                 /* 0x0 CPU Interface control register */
     uint32_t CCPMR;                 /* 0x4 Interrupt priority mask register */
     uint32_t CBPR;                  /* 0x8 Binary point register */
diff --git a/src/08_scheduler/src/linkscript.ld b/src/08_scheduler/src/linkscript.ld
index 4e6586a..b4b9100 100644
--- a/src/08_scheduler/src/linkscript.ld
+++ b/src/08_scheduler/src/linkscript.ld
@@ -14,6 +14,7 @@ SECTIONS
         *(.vector_table)
         *(.text*)
         *(.rodata*)
+	. = ALIGN(4);
      } > ROM
     _text_end = .;
     .data : AT(ADDR(.text) + SIZEOF(.text))
diff --git a/src/08_scheduler/src/ptimer.h b/src/08_scheduler/src/ptimer.h
index 7b3d1f3..05d1957 100644
--- a/src/08_scheduler/src/ptimer.h
+++ b/src/08_scheduler/src/ptimer.h
@@ -4,7 +4,7 @@
 #include <stdint.h>
 #include "cpu.h"
 
-typedef volatile struct __attribute__((packed)) {
+typedef volatile struct {
     uint32_t LR;    /* 0x0 Private timer load register */
     uint32_t CR;    /* 0x4 Private timer counter regster */
     uint32_t CTRL;  /* 0x8 Private timer control register */

Disassemble code after apply this patch:

    WRITE32(regs->CTRL, ctrl); /* Configure and start the timer */
6000048e:	f241 0308 	movw	r3, #4104	; 0x1008
60000492:	f2c7 0300 	movt	r3, #28672	; 0x7000
60000496:	681b      	ldr	r3, [r3, #0]
60000498:	2207      	movs	r2, #7
6000049a:	609a      	str	r2, [r3, #8]

qemu work fine and no guest errors now:

Starting kernel ...

Welcome to Chapter 8, Scheduling!
Entering task 1... systime 2000
Exiting task 1...
Entering task 1... systime 4000
Exiting task 1...
Entering task 0... systime 5000
Exiting task 0...
Entering task 1... systime 6000
Exiting task 1...
Entering task 1... systime 8000
Exiting task 1...
Entering task 1... systime 10000
Exiting task 1...

[ghost]

Not bad... I'm impressed!

[painterner]

It has same issue when testing in ubuntu 20.04 with qemu 4.2.1, but I don't know the reason, however it works after remove "packed" and add "align(4)"

[MitchellThompkins]

Good idea to use -d guest_errors. I think unpacking the structs themselves is a bad idea because they are actually aligned in memory per the datasheet. Adding the alignment in the linker works obviously b/c it forces all data to be aligned on a 4-byte boundary, and because the structs are uint32_t they end up aligned. A bad compiler though would still be free to put padding in between the uint32_t, or if they weren't uint32_t padding would get added.

A better solution I think is to use typedef volatile struct __attribute__((packed, aligned(4))) which should align the stuct itself and leave it packed.

I tested this on my system with qemu 8.0.2 and gcc 12.2 and it started working.