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.