mini-os: initial ARM support

Otherwise, passing doubles to varargs functions causes a crash.

Signed-off-by Thomas Leonard <[email protected]>

+ /* Must ensure that (%rsp + 8) is 16-byte aligned at the start of thread_starter. */

+    thread->sp -= sizeof(unsigned long);
     
     stack_push(thread, (unsigned long) function);
     stack_push(thread, (unsigned long) data);

Quoted 5 lines · Thomas Leonard

Otherwise, passing doubles to varargs functions causes a crash.

Signed-off-by Thomas Leonard <[email protected]>

+ /* Must ensure that (%rsp + 8) is 16-byte aligned at the start of thread_starter. */

Is this really effectively a requirement for the stack to start out misaligned?

Quoted 9 lines · Ian Campbell, le Thu 17 Jul 2014 16:50:25 +0100, a écrit :

Quoted 5 lines · Thomas Leonard

Otherwise, passing doubles to varargs functions causes a crash.

Signed-off-by Thomas Leonard <[email protected]>

+ /* Must ensure that (%rsp + 8) is 16-byte aligned at the start of thread_starter. */

Is this really effectively a requirement for the stack to start out misaligned?

Yes. From the x86_64 ABI pdf I have here:

“
The end of the input argument area shall be aligned on a 16 byte boundary. In other words, the value (%rsp - 8) is always a multiple of 16 when control is transferred to the function entry point.
”

So the proposed comment is exactly the ABI requirement.

Maybe it should
even say

/* x86_64 ABI requires that (%rsp + 8) is 16-byte aligned at the start of thread_starter. */

I suppose this comes from the stack normally being 16-byte aligned but the actual call to the function adding another 8 by pushing the return address, so on funciton entry you are "misaligned" by 8 bytes.

Otherwise, passing doubles to varargs functions causes a crash.

Signed-off-by Thomas Leonard <[email protected]>

---
 extras/mini-os/arch/x86/sched.c | 3 +++
 1 file changed, 3 insertions(+)

extras/mini-os/arch/x86/sched.c +3 -0

index 8a05b58..e4a3dc2 100644
@@ -107,6 +107,9 @@ struct thread* arch_create_thread(char *name, void (*function)(void *),
     thread->sp = (unsigned long)thread->stack + STACK_SIZE;
     /* Save pointer to the thread on the stack, used by current macro */
     *((unsigned long *)thread->stack) = (unsigned long)thread;
+
+    /* Must ensure that (%rsp + 8) is 16-byte aligned at the start of thread_starter. */
+    thread->sp -= sizeof(unsigned long);
 
     stack_push(thread, (unsigned long) function);
     stack_push(thread, (unsigned long) data);

Signature · 1 line

2.0.1

This breaks the include cycle hypervisor.h -> hypercall-x86_32.h -> mm.h -> lib.h -> gntmap.h -> os.h -> hypervisor.h.

Quoted 2 lines · Thomas Leonard

This breaks the include cycle hypervisor.h -> hypercall-x86_32.h -> mm.h -> lib.h -> gntmap.h -> os.h -> hypervisor.h.

I've just realised this is missing an include in tpm_tis.c (which isn't built by default). Fixed patch attached.

Quoted 2 lines · Thomas Leonard

This breaks the include cycle hypervisor.h -> hypercall-x86_32.h -> mm.h -> lib.h -> gntmap.h -> os.h -> hypervisor.h.

I've just realised this is missing an include in tpm_tis.c (which isn't built by default). Fixed patch attached.

Signature · 25 lines

-- 
Dr Thomas Leonard        http://0install.net/
GPG: 9242 9807 C985 3C07 44A6  8B9A AE07 8280 59A5 3CC1
GPG: DA98 25AE CAD0 8975 7CDA  BD8E 0713 3F96 CA74 D8BA

From 98705b128feb2e46272c4d29205f384899108704 Mon Sep 17 00:00:00 2001
From: Thomas Leonard <[email protected]>
Date: Mon, 7 Jul 2014 13:20:43 +0100
Subject: [PATCH] mini-os: don't include lib.h from mm.h

This breaks the include cycle hypervisor.h -> hypercall-x86_32.h -> mm.h
-> lib.h -> gntmap.h -> os.h -> hypervisor.h.

Signed-off-by: Thomas Leonard <[email protected]>

---

- Added missing #include <mini-os/lib.h> to tpm_tis.c
---
 extras/mini-os/include/mm.h | 3 +--
 extras/mini-os/lock.c       | 1 +
 extras/mini-os/main.c       | 1 +
 extras/mini-os/tpm_tis.c    | 1 +
 extras/mini-os/tpmfront.c   | 1 +
 5 files changed, 5 insertions(+), 2 deletions(-)

extras/mini-os/include/mm.h +1 -2

index a94cd6d..a177251 100644
@@ -32,8 +32,7 @@
 #else
 #error "Unsupported architecture"
 #endif
-
-#include <mini-os/lib.h>
+#include <xen/xen.h>
 
 #include <mini-os/arch_limits.h>
 #include <mini-os/arch_mm.h>

extras/mini-os/lock.c +1 -0

index 71a4971..61194e5 100644
@@ -9,6 +9,7 @@
 #include <sys/lock.h>
 #include <sched.h>
 #include <wait.h>
+#include <mini-os/lib.h>
 
 int ___lock_init(_LOCK_T *lock)
 {

extras/mini-os/main.c +1 -0

index aec0586..4ec40b5 100644
@@ -15,6 +15,7 @@
 #include <unistd.h>
 #include <xenbus.h>
 #include <events.h>
+#include <mini-os/lib.h>
 
 extern int main(int argc, char *argv[], char *envp[]);
 extern void __libc_init_array(void);

extras/mini-os/tpm_tis.c +1 -0

index e8ca69f..09234e5 100644
@@ -26,6 +26,7 @@
 #include <mini-os/events.h>
 #include <mini-os/wait.h>
 #include <mini-os/xmalloc.h>
+#include <mini-os/lib.h>
 #include <errno.h>
 #include <stdbool.h>
 

extras/mini-os/tpmfront.c +1 -0

index ce5b3e1..6049244 100644
@@ -31,6 +31,7 @@
 #include <xen/io/xenbus.h>
 #include <xen/io/tpmif.h>
 #include <mini-os/tpmfront.h>
+#include <mini-os/lib.h>
 #include <fcntl.h>
 
 //#define TPMFRONT_PRINT_DEBUG

Signature · 1 line

2.0.1

extras/mini-os/include/arm/os.h new file +7 -0

new file mode 100644
index 0000000..6a1cc37
@@ -0,0 +1,216 @@
+#ifndef _OS_H_
+#define _OS_H_
+
+#ifndef __ASSEMBLY__
+
+#include <mini-os/hypervisor.h>
+#include <mini-os/types.h>

Hi Thomas,

Quoted 22 lines · Thomas Leonard

extras/mini-os/include/arm/os.h new file +14 -0

new file mode 100644
index 0000000..6a1cc37
@@ -0,0 +1,216 @@
+#ifndef _OS_H_
+#define _OS_H_
+
+#ifndef __ASSEMBLY__
+
+#include <mini-os/hypervisor.h>
+#include <mini-os/types.h>
+#include <mini-os/compiler.h>
+#include <mini-os/kernel.h>
+#include <xen/xen.h>
+
+void arch_fini(void);
+void timer_handler(evtchn_port_t port, struct pt_regs *regs, void *ign);
+

Quoted 4 lines

extras/mini-os/include/arm/os.h new file +3 -0

+extern void *device_tree;
+
+#define BUG() while(1){asm volatile (".word 0xe7f000f0\n");} /* Undefined

instruction; will call our fault handler. */

NIT: I would explain that ARM doesn't provide any undefined instruction so we use an encoding that is invalid on ARMv7.

The other part of this patch looks good to me.

FWIW: Acked-by: Julien Grall <[email protected]>

Regards,

--
Julien Grall

Quoted 26 lines · Julien Grall

Hi Thomas,

Quoted 22 lines · Thomas Leonard

extras/mini-os/include/arm/os.h new file +14 -0

new file mode 100644
index 0000000..6a1cc37
@@ -0,0 +1,216 @@
+#ifndef _OS_H_
+#define _OS_H_
+
+#ifndef __ASSEMBLY__
+
+#include <mini-os/hypervisor.h>
+#include <mini-os/types.h>
+#include <mini-os/compiler.h>
+#include <mini-os/kernel.h>
+#include <xen/xen.h>
+
+void arch_fini(void);
+void timer_handler(evtchn_port_t port, struct pt_regs *regs, void *ign);
+

Quoted 8 lines

Quoted 4 lines

extras/mini-os/include/arm/os.h new file +3 -0

+extern void *device_tree;
+
+#define BUG() while(1){asm volatile (".word 0xe7f000f0\n");} /* Undefined

instruction; will call our fault handler. */

NIT: I would explain that ARM doesn't provide any undefined instruction so we use an encoding that is invalid on ARMv7.

I think it does, although I can't find it in the ARMv7 reference at the moment.

But from
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0489c/CIHJIJCA.html
:

"UND pseudo-instruction

Prompted by this I took another look and there it is, A8.8.247 defines "UDF" as a permanently undefined encoding.

Quoted 5 lines

But from
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0489c/CIHJIJCA.html
:

"UND pseudo-instruction

From: Karim Raslan <[email protected]>

Adds header files for future ARM support.
Based on work by Karim Allah Ahmed.

Signed-off-by Karim Allah Ahmed <[email protected]>

Signed-off-by Thomas Leonard <[email protected]>

+/********************* common arm32 and arm64 ****************************/

extras/mini-os/include/hypervisor.h +2 -0

+#elif defined(__arm__) || defined(__aarch64__)
+#include <hypercall-arm.h>

Quoted 7 lines · Thomas Leonard

From: Karim Raslan <[email protected]>

Adds header files for future ARM support.
Based on work by Karim Allah Ahmed.

Signed-off-by Karim Allah Ahmed <[email protected]>

Signed-off-by Thomas Leonard <[email protected]>

Acked-by Ian Campbell <[email protected]>

[...]

Quoted 1 line

+/********************* common arm32 and arm64 ****************************/

[...]

Quoted 2 lines

extras/mini-os/include/hypervisor.h +2 -0

+#elif defined(__arm__) || defined(__aarch64__)
+#include <hypercall-arm.h>

[...]

extras/mini-os/include/mm.h line 33

Quoted 2 lines

extras/mini-os/include/mm.h +2 -0

+#elif defined(__arm__) || defined(__aarch64__)
+#include <xen/arch-arm.h>

This series isn't adding aarch64 as well, is it?

I don't object to these being here, I would just be pleasantly surprised to find arm64 being supported too ;-)

Quoted 7 lines · Thomas Leonard

From: Karim Raslan <[email protected]>

Adds header files for future ARM support.
Based on work by Karim Allah Ahmed.

Signed-off-by Karim Allah Ahmed <[email protected]>

Signed-off-by Thomas Leonard <[email protected]>

Acked-by Ian Campbell <[email protected]>

[...]

Quoted 1 line

+/********************* common arm32 and arm64 ****************************/

[...]

Quoted 2 lines

extras/mini-os/include/hypervisor.h +2 -0

+#elif defined(__arm__) || defined(__aarch64__)
+#include <hypercall-arm.h>

[...]

extras/mini-os/include/mm.h line 33

Quoted 2 lines

extras/mini-os/include/mm.h +2 -0

+#elif defined(__arm__) || defined(__aarch64__)
+#include <xen/arch-arm.h>

This series isn't adding aarch64 as well, is it?

I don't object to these being here, I would just be pleasantly surprised to find arm64 being supported too ;-)

+extern int physical_address_offset; /* Add this to a virtual address to get the physical address (wraps) */

Quoted 1 line · Thomas Leonard, le Wed 16 Jul 2014 12:07:44 +0100, a écrit :

+extern int physical_address_offset; /* Add this to a virtual address to get the physical address (wraps) */

int only?

From: Karim Raslan <[email protected]>

Looks like this is revision v1.3.0-47-gbe60268 from
http://git.jdl.com/gitweb/?p=dtc.git

The memmove implementation is from FreeBSD's
contrib/ldns/compat/memmove.c (r246827).

Signed-off-by Karim Allah Ahmed <[email protected]>

[[email protected]: split out FDT support into a separate patch]

[[email protected]: fixed "make clean" for FDT]

[[email protected]: replaced GPL memmove with BSD one]

Signed-off-by Thomas Leonard <[email protected]>

Quoted 8 lines · Thomas Leonard

From: Karim Raslan <[email protected]>

Looks like this is revision v1.3.0-47-gbe60268 from
http://git.jdl.com/gitweb/?p=dtc.git

The memmove implementation is from FreeBSD's
contrib/ldns/compat/memmove.c (r246827).

Quoted 5 lines

Signed-off-by Karim Allah Ahmed <[email protected]>

[[email protected]: split out FDT support into a separate patch]

[[email protected]: fixed "make clean" for FDT]

[[email protected]: replaced GPL memmove with BSD one]

Signed-off-by Thomas Leonard <[email protected]>

Xen already has a copy of libfdt in xen/common/libfdt.

Quoted 14 lines · Andrew Cooper

Quoted 13 lines · Thomas Leonard

From: Karim Raslan <[email protected]>

Looks like this is revision v1.3.0-47-gbe60268 from
http://git.jdl.com/gitweb/?p=dtc.git

The memmove implementation is from FreeBSD's
contrib/ldns/compat/memmove.c (r246827).

Signed-off-by Karim Allah Ahmed <[email protected]>

[[email protected]: split out FDT support into a separate patch]

[[email protected]: fixed "make clean" for FDT]

[[email protected]: replaced GPL memmove with BSD one]

Signed-off-by Thomas Leonard <[email protected]>

Quoted 1 line

Xen already has a copy of libfdt in xen/common/libfdt.

Please review the thread on this from the previous postings.

Ian.

Quoted 15 lines · Ian Campbell

Quoted 14 lines · Andrew Cooper

Quoted 13 lines · Thomas Leonard

From: Karim Raslan <[email protected]>

Looks like this is revision v1.3.0-47-gbe60268 from
http://git.jdl.com/gitweb/?p=dtc.git

The memmove implementation is from FreeBSD's
contrib/ldns/compat/memmove.c (r246827).

Signed-off-by Karim Allah Ahmed <[email protected]>

[[email protected]: split out FDT support into a separate patch]

[[email protected]: fixed "make clean" for FDT]

[[email protected]: replaced GPL memmove with BSD one]

Signed-off-by Thomas Leonard <[email protected]>

Quoted 5 lines

Quoted 1 line

Xen already has a copy of libfdt in xen/common/libfdt.

Please review the thread on this from the previous postings.

Ian.

Which appears to be unconcluded.

libfdt is a 3rd party library which, from what I can tell in the history, is unmodified. Therefore, my suggestion of moving it outside of the xen/ tree and having both minios and Xen VPATH themselves access to it helps with the end goal of preventing minios becoming dependent on Xen code. It further prevents both Xen and minios from accidentally gaining localisation hacks in the library itself, which makes it easier to updates to the base libdft if/when necessary.

Quoted 16 lines · Andrew Cooper

Quoted 15 lines · Ian Campbell

Quoted 14 lines · Andrew Cooper

Quoted 13 lines · Thomas Leonard

From: Karim Raslan <[email protected]>

Looks like this is revision v1.3.0-47-gbe60268 from
http://git.jdl.com/gitweb/?p=dtc.git

The memmove implementation is from FreeBSD's
contrib/ldns/compat/memmove.c (r246827).

Signed-off-by Karim Allah Ahmed <[email protected]>

[[email protected]: split out FDT support into a separate patch]

[[email protected]: fixed "make clean" for FDT]

[[email protected]: replaced GPL memmove with BSD one]

Signed-off-by Thomas Leonard <[email protected]>

Quoted 7 lines

Quoted 5 lines

Quoted 1 line

Xen already has a copy of libfdt in xen/common/libfdt.

Please review the thread on this from the previous postings.

Ian.

Which appears to be unconcluded.

http://article.gmane.org/gmane.comp.emulators.xen.devel/205280 looked conclusive to me.

Quoted 7 lines

libfdt is a 3rd party library which, from what I can tell in the history, is unmodified. Therefore, my suggestion of moving it outside of the xen/ tree and having both minios and Xen VPATH themselves access to it helps with the end goal of preventing minios becoming dependent on Xen code. It further prevents both Xen and minios from accidentally gaining localisation hacks in the library itself, which makes it easier to updates to the base libdft if/when necessary.

This would mean that splitting extras/mini-os off wouldn't be just a case of doing that, you'd also need arrange for the VPATH to be satisfied, I don't know if Anil et al will be happy with that.

Quoted 36 lines · Ian Campbell

Quoted 16 lines · Andrew Cooper

Quoted 15 lines · Ian Campbell

Quoted 14 lines · Andrew Cooper

Quoted 13 lines · Thomas Leonard

From: Karim Raslan <[email protected]>

Looks like this is revision v1.3.0-47-gbe60268 from
http://git.jdl.com/gitweb/?p=dtc.git

The memmove implementation is from FreeBSD's
contrib/ldns/compat/memmove.c (r246827).

Signed-off-by Karim Allah Ahmed <[email protected]>

[[email protected]: split out FDT support into a separate patch]

[[email protected]: fixed "make clean" for FDT]

[[email protected]: replaced GPL memmove with BSD one]

Signed-off-by Thomas Leonard <[email protected]>

Quoted 7 lines

Quoted 5 lines

Quoted 1 line

Xen already has a copy of libfdt in xen/common/libfdt.

Please review the thread on this from the previous postings.

Ian.

Which appears to be unconcluded.

http://article.gmane.org/gmane.comp.emulators.xen.devel/205280 looked conclusive to me.

Quoted 7 lines

libfdt is a 3rd party library which, from what I can tell in the history, is unmodified. Therefore, my suggestion of moving it outside of the xen/ tree and having both minios and Xen VPATH themselves access to it helps with the end goal of preventing minios becoming dependent on Xen code. It further prevents both Xen and minios from accidentally gaining localisation hacks in the library itself, which makes it easier to updates to the base libdft if/when necessary.

Quoted 3 lines

This would mean that splitting extras/mini-os off wouldn't be just a case of doing that, you'd also need arrange for the VPATH to be satisfied, I don't know if Anil et al will be happy with that.

As long as we can resist the urge to locally modify libfdt, that shouldn't be too onerous. However, if libfdt isn't being modified at all, then why import it into the tree at all? We could just clone the right revision in the build system as happens for some other upstream tools.

Quoted 16 lines · Andrew Cooper

Quoted 15 lines · Ian Campbell

Quoted 14 lines · Andrew Cooper

Quoted 13 lines · Thomas Leonard

From: Karim Raslan <[email protected]>

Looks like this is revision v1.3.0-47-gbe60268 from
http://git.jdl.com/gitweb/?p=dtc.git

The memmove implementation is from FreeBSD's
contrib/ldns/compat/memmove.c (r246827).

Signed-off-by Karim Allah Ahmed <[email protected]>

[[email protected]: split out FDT support into a separate patch]

[[email protected]: fixed "make clean" for FDT]

[[email protected]: replaced GPL memmove with BSD one]

Signed-off-by Thomas Leonard <[email protected]>

Quoted 7 lines

Quoted 5 lines

Quoted 1 line

Xen already has a copy of libfdt in xen/common/libfdt.

Please review the thread on this from the previous postings.

Ian.

Which appears to be unconcluded.

http://article.gmane.org/gmane.comp.emulators.xen.devel/205280 looked conclusive to me.

Quoted 7 lines

libfdt is a 3rd party library which, from what I can tell in the history, is unmodified. Therefore, my suggestion of moving it outside of the xen/ tree and having both minios and Xen VPATH themselves access to it helps with the end goal of preventing minios becoming dependent on Xen code. It further prevents both Xen and minios from accidentally gaining localisation hacks in the library itself, which makes it easier to updates to the base libdft if/when necessary.

This would mean that splitting extras/mini-os off wouldn't be just a case of doing that, you'd also need arrange for the VPATH to be satisfied, I don't know if Anil et al will be happy with that.

Ultimately this is up to Samuel if he is happy with the current approach or not.

__sti is x86 specific.

Signed-off-by Thomas Leonard <[email protected]>

__sti is x86 specific.

Signed-off-by Thomas Leonard <[email protected]>

---
 extras/mini-os/kernel.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

extras/mini-os/kernel.c +1 -1

index 9a30550..437e5b4 100644
@@ -121,7 +121,7 @@ void start_kernel(void)
     init_events();
 
     /* ENABLE EVENT DELIVERY. This is disabled at start of day. */
-    __sti();
+    local_irq_enable();
 
     setup_xen_features();
 

Signature · 1 line

2.0.1

extras/mini-os/arch/arm/arm32.S new file +22 -0

+	@ Set access permission for domains.
+	@ Domains are deprecated, but we have to configure them anyway.
+	@ We mark every page as being domain 0 and set domain 0 to "client mode"
+	@ (client mode = use access flags in page table).
+	mov	r0, #1			@ 1 = client
+	mcr	p15, 0, r0, c3, c0, 0	@ DACR
+
+	@ Template (flags) for a 1 MB page-table entry.
+	@ TEX[2:0] C B = 001 1 1 (outer and inner write-back, write-allocate)
+	ldr	r8, =(0x2 +  		/* Section entry */ \
+		      0xc +  		/* C B */ \
+		      (3 << 10) + 	/* Read/write */ \
+		      (1 << 12) +	/* TEX */ \
+		      (1 << 16) +	/* Sharable */ \
+		      (1<<19))		/* Non-secure */
+	@ r8 = template page table entry
+
+	@ Add an entry for the current physical section, at the old and new
+	@ addresses. It's OK if they're the same.
+	mov	r0, pc, lsr#20
+	mov	r0, r0, lsl#20		@ r0 = physical address of this code's section start
+	orr	r3, r0, r8		@ r3 = table entry for this section

Hi Thomas,

extras/mini-os/arch/arm/arm32.S line 49

Quoted 6 lines · Thomas Leonard

extras/mini-os/arch/arm/arm32.S new file +5 -0

+
+       @ Tell the system where our page table is located.
+       @ This is the 16 KB top-level translation table, in which
+       @ each word maps one 1MB virtual section to a physical section.
+       mcr     p15, 0, r1, c2, c0, 0   @ set ttbr0 = r1

I don't see any modification of TTBCR (so the initial value is 0). I guess you are using short-descriptor translation table format, right? If so, I would write somewhere.

Quoted 36 lines

extras/mini-os/arch/arm/arm32.S new file +3 -0

+       @ Set access permission for domains.
+       @ Domains are deprecated, but we have to configure them anyway.
+       @ We mark every page as being domain 0 and set domain 0 to "client

mode"

extras/mini-os/arch/arm/arm32.S new file +6 -0

+       @ (client mode = use access flags in page table).
+       mov     r0, #1                  @ 1 = client
+       mcr     p15, 0, r0, c3, c0, 0   @ DACR
+
+       @ Template (flags) for a 1 MB page-table entry.
+       @ TEX[2:0] C B = 001 1 1 (outer and inner write-back,

write-allocate)

extras/mini-os/arch/arm/arm32.S new file +9 -0

+       ldr     r8, =(0x2 +             /* Section entry */ \
+                     0xc +             /* C B */ \
+                     (3 << 10) +       /* Read/write */ \
+                     (1 << 12) +       /* TEX */ \
+                     (1 << 16) +       /* Sharable */ \
+                     (1<<19))          /* Non-secure */
+       @ r8 = template page table entry
+
+       @ Add an entry for the current physical section, at the old and

new

extras/mini-os/arch/arm/arm32.S new file +3 -0

+       @ addresses. It's OK if they're the same.
+       mov     r0, pc, lsr#20
+       mov     r0, r0, lsl#20          @ r0 = physical address of this

code's section start
+ orr r3, r0, r8 @ r3 = table entry for this
section
+ ldr r4, =_start @ r4 = desired virtual address of
this section
+ str r3, [r1, r4, lsr#18] @ map desired virtual section to
this code
+ str r3, [r1, r0, lsr#18] @ map current section to this code
too

extras/mini-os/arch/arm/arm32.S new file +3 -0

+
+       @ Invalidate TLB
+       mcr     p15, 0, r1, c8, c7, 0   @ TLBIALL

Quoted 5 lines

extras/mini-os/arch/arm/arm32.S new file +3 -0

+
+       @ Enable MMU / SCTLR
+       @ We leave caches off for now because we're going to be changing

the
+ @ TLB a lot and this avoids having to flush the caches each time.

Did you intend to mean page table rather than TLB?

[..]

extras/mini-os/arch/arm/arm32.S line 195

Quoted 7 lines

extras/mini-os/arch/arm/arm32.S new file +7 -0

+irq_handler:
+       ldr     sp, =irqstack_end
+       push    {r0 - r12, r14}
+
+       ldr     r0, IRQ_handler
+       cmp     r0, #0
+       .word   0x07f000f0      @ undeq - panic if no handler

Hrmmm, I didn't spot this earlier. How can this work? You unconditionally called an undefined encoding.

[..]

Quoted 10 lines

extras/mini-os/arch/arm/minios-arm32.lds new file +3 -0

new file mode 100755
index 0000000..21c3893
@@ -0,0 +1,79 @@
+OUTPUT_ARCH(arm)
+ENTRY(_start)
+SECTIONS

extras/mini-os/arch/arm/minios-arm32.lds line 7

Quoted 4 lines

extras/mini-os/arch/arm/minios-arm32.lds new file +4 -0

+{
+  _boot_stack   = 0x400000;    /* 16 KB boot stack */
+  _boot_stack_end = 0x404000;
+  _page_dir      = 0x404000;   /* 16 KB translation table */

It might be worse to explain that you are relying on there is a hole before the kernel address.

Quoted 11 lines

extras/mini-os/arch/arm/arm32.S line 195

Quoted 7 lines

extras/mini-os/arch/arm/arm32.S new file +7 -0

+irq_handler:
+       ldr     sp, =irqstack_end
+       push    {r0 - r12, r14}
+
+       ldr     r0, IRQ_handler
+       cmp     r0, #0
+       .word   0x07f000f0      @ undeq - panic if no handler

Hrmmm, I didn't spot this earlier. How can this work? You unconditionally called an undefined encoding.

Here, the leading "0" is EQ, so it's only executed if IRQ_handler is unset ("undEQ").

Quoted 12 lines · Thomas Leonard

Quoted 11 lines

extras/mini-os/arch/arm/arm32.S line 195

Quoted 7 lines

extras/mini-os/arch/arm/arm32.S new file +7 -0

+irq_handler:
+       ldr     sp, =irqstack_end
+       push    {r0 - r12, r14}
+
+       ldr     r0, IRQ_handler
+       cmp     r0, #0
+       .word   0x07f000f0      @ undeq - panic if no handler

Hrmmm, I didn't spot this earlier. How can this work? You unconditionally called an undefined encoding.

Quoted 2 lines

Here, the leading "0" is EQ, so it's only executed if IRQ_handler is unset ("undEQ").

ARMv7 ARM A8.8.247 says that UDF isn't conditional,so you probably odn't want to rely on that.

extras/mini-os/arch/arm/arm32.S new file +8 -0

+	@ Invalidate TLB
+	mcr	p15, 0, r1, c8, c7, 0	@ TLBIALL
+	isb
+
+	@ The new mapping has now taken effect:
+	@ r7 -> page_dir
+
+	@ Fill in the whole top-level translation table (at page_dir).

extras/mini-os/arch/arm/arm32.S line 82

Quoted 3 lines · Thomas Leonard

extras/mini-os/arch/arm/arm32.S new file +3 -0

+     @ Enable MMU / SCTLR
+     @ We leave caches off for now because we're going to be changing the
+     @ TLB a lot and this avoids having to flush the caches each time.

ARMv7 (or perhaps it was the virt extensions, either way you can rely on it here) makes the MMU cache coherent, so you don't need to flush the caches when doing a PTE write any more.

extras/mini-os/arch/arm/arm32.S line 84

Quoted 5 lines

extras/mini-os/arch/arm/arm32.S new file +5 -0

+       @ Enable MMU / SCTLR
+       @ We leave caches off for now because we're going to be changing the
+       @ TLB a lot and this avoids having to flush the caches each time.
+       mrc     p15, 0, r1, c1, c0, 0   @ SCTLR
+       orr     r1, r1, #0x1            @ Enable MMU

I think you want a dsb here to ensure that any previous PTE write you did when setting up the PTs have actually occurred.

extras/mini-os/arch/arm/arm32.S line 89

Quoted 6 lines

extras/mini-os/arch/arm/arm32.S new file +5 -0

+       mcr     p15, 0, r1, c1, c0, 0   @ SCTLR
+       isb
+
 
+     adr     r1, stage2              @ Physical address of stage2
+     sub     r1, r1, r9              @ Virtual address of stage2

ldr r1, =stage2

would do this in one, I think.

Quoted 7 lines

extras/mini-os/arch/arm/arm32.S new file +7 -0

+     bx      r1
+
+@ Called once the MMU is enabled. The boot code and the page table are mapped,
+@ but nothing else is yet.
+@
+@ => r2 -> dtb (physical)
+@    r7 = virtual address of page table

Quoted 4 lines

+@    r8 = section entry template (flags)
+@    r9 = desired physical - virtual offset
+@    pc -> somewhere in newly-mapped virtual code section

+stage2:

I think you want another dsb around here somewhere to ensure that none of the PT updates you do in a moment float up and interfere with the mapping of the code between the write to SCTLR until the bx r1.

Quoted 8 lines

extras/mini-os/arch/arm/arm32.S new file +8 -0

+     @ Invalidate TLB
+     mcr     p15, 0, r1, c8, c7, 0   @ TLBIALL
+     isb
+
+     @ The new mapping has now taken effect:
+     @ r7 -> page_dir
+
+     @ Fill in the whole top-level translation table (at page_dir).

extras/mini-os/arch/arm/arm32.S line 118

Quoted 10 lines

extras/mini-os/arch/arm/arm32.S new file +10 -0

+     @ Populate the whole pagedir with 1MB section descriptors.
+
+     mov     r1, r7                  @ r1 -> first section entry
+     add     r3, r1, #4*4*1024       @ limit (4 GB address space, 4 byte entries)
+     orr     r0, r8, r9              @ r0 = entry mapping section zero to start of physical RAM
+1:
+     str     r0, [r1],#4             @ write the section entry
+     add     r0, r0, #1 << 20        @ next physical page (wraps)
+     cmp     r1, r3
+     bne     1b

dsb here to make sure those writes have actually occurred before you come to rely on them.

If there is any chance of any of those writes going through a mapping which you are then changing at the same time then I worry that you might also need a dsb before each write.

I'm not 100% sure what would happen
in this case, I think you'd be better off creating a whole new set of PTs rather than pulling the rug out from under yourself. It's either that or cold towel on head time ;-)

Ian.

Quoted 8 lines · Ian Campbell

extras/mini-os/arch/arm/arm32.S line 82

Quoted 3 lines · Thomas Leonard

extras/mini-os/arch/arm/arm32.S new file +3 -0

+     @ Enable MMU / SCTLR
+     @ We leave caches off for now because we're going to be changing the
+     @ TLB a lot and this avoids having to flush the caches each time.

ARMv7 (or perhaps it was the virt extensions, either way you can rely on it here) makes the MMU cache coherent, so you don't need to flush the caches when doing a PTE write any more.

I tried enabling the D-cache here, but then it frequently (but not always) fails to boot, even with various dsb and isb instructions scattered around.

If I enable the D-cache immediately before the loop that writes the translation table entries, it fails. Adding a dsb after the loop doesn't help. But if I enable it after the loop then it's fine.

I don't know what's going on there - any suggestions? Here's my (failing) test-case (tested using Xen/stable-4.4 - 4.4.1-rc1-28-gee81dda):

https://github.com/talex5/xen/commit/adadfa7e483aa3f3a351c0aeca6a560614fcdead

Quoted 8 lines

extras/mini-os/arch/arm/arm32.S line 84

Quoted 5 lines

extras/mini-os/arch/arm/arm32.S new file +5 -0

+       @ Enable MMU / SCTLR
+       @ We leave caches off for now because we're going to be changing the
+       @ TLB a lot and this avoids having to flush the caches each time.
+       mrc     p15, 0, r1, c1, c0, 0   @ SCTLR
+       orr     r1, r1, #0x1            @ Enable MMU

I think you want a dsb here to ensure that any previous PTE write you did when setting up the PTs have actually occurred.

Presumably this isn't needed in the current code, since the caches are off?

Quoted 7 lines

Quoted 7 lines

extras/mini-os/arch/arm/arm32.S new file +7 -0

+     bx      r1
+
+@ Called once the MMU is enabled. The boot code and the page table are mapped,
+@ but nothing else is yet.
+@
+@ => r2 -> dtb (physical)
+@    r7 = virtual address of page table

Quoted 8 lines

Quoted 4 lines

+@    r8 = section entry template (flags)
+@    r9 = desired physical - virtual offset
+@    pc -> somewhere in newly-mapped virtual code section

+stage2:

I think you want another dsb around here somewhere to ensure that none of the PT updates you do in a moment float up and interfere with the mapping of the code between the write to SCTLR until the bx r1.

How does that happen? Presumably the isb below will block that.

Quoted 22 lines

Quoted 8 lines

extras/mini-os/arch/arm/arm32.S new file +8 -0

+     @ Invalidate TLB
+     mcr     p15, 0, r1, c8, c7, 0   @ TLBIALL
+     isb
+
+     @ The new mapping has now taken effect:
+     @ r7 -> page_dir
+
+     @ Fill in the whole top-level translation table (at page_dir).

extras/mini-os/arch/arm/arm32.S line 118

Quoted 10 lines

extras/mini-os/arch/arm/arm32.S new file +10 -0

+     @ Populate the whole pagedir with 1MB section descriptors.
+
+     mov     r1, r7                  @ r1 -> first section entry
+     add     r3, r1, #4*4*1024       @ limit (4 GB address space, 4 byte entries)
+     orr     r0, r8, r9              @ r0 = entry mapping section zero to start of physical RAM
+1:
+     str     r0, [r1],#4             @ write the section entry
+     add     r0, r0, #1 << 20        @ next physical page (wraps)
+     cmp     r1, r3
+     bne     1b

dsb here to make sure those writes have actually occurred before you come to rely on them.

Quoted 3 lines

If there is any chance of any of those writes going through a mapping which you are then changing at the same time then I worry that you might also need a dsb before each write.

The mapping for the translation table itself doesn't change (its entry will be written with the same value as previously).

Quoted 18 lines · Thomas Leonard

Quoted 8 lines · Ian Campbell

extras/mini-os/arch/arm/arm32.S line 82

Quoted 3 lines · Thomas Leonard

extras/mini-os/arch/arm/arm32.S new file +3 -0

+     @ Enable MMU / SCTLR
+     @ We leave caches off for now because we're going to be changing the
+     @ TLB a lot and this avoids having to flush the caches each time.

ARMv7 (or perhaps it was the virt extensions, either way you can rely on it here) makes the MMU cache coherent, so you don't need to flush the caches when doing a PTE write any more.

I tried enabling the D-cache here, but then it frequently (but not always) fails to boot, even with various dsb and isb instructions scattered around.

If I enable the D-cache immediately before the loop that writes the translation table entries, it fails. Adding a dsb after the loop doesn't help. But if I enable it after the loop then it's fine.

Quoted 5 lines

I don't know what's going on there - any suggestions? Here's my (failing) test-case (tested using Xen/stable-4.4 - 4.4.1-rc1-28-gee81dda):

https://github.com/talex5/xen/commit/adadfa7e483aa3f3a351c0aeca6a560614fcdead

My guess would be that it is related to the lack of dsb before the initial write to SCTLR which turns on M. Try:

        mrc     p15, 0, r1, c1, c0, 0   @ SCTLR
        orr     r1, r1, #0x1 @ Enable MMU
        dsb
        mcr     p15, 0, r1, c1, c0, 0   @ SCTLR
        isb

Otherwise I worry that the dsb() involved when you turn on caching are causing differences it what you are seeing at this point depending on where you place it.

Xen enables caches at the same time and MMU BTW.

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

extras/mini-os/arch/arm/arm32.S new file +5 -0

+       @ Enable MMU / SCTLR
+       @ We leave caches off for now because we're going to be changing the
+       @ TLB a lot and this avoids having to flush the caches each time.
+       mrc     p15, 0, r1, c1, c0, 0   @ SCTLR
+       orr     r1, r1, #0x1            @ Enable MMU

Quoted 4 lines

Quoted 2 lines

I think you want a dsb here to ensure that any previous PTE write you did when setting up the PTs have actually occurred.

Presumably this isn't needed in the current code, since the caches are off?

The processor can still reorder writes and things like that, so I think it is still needed.

Quoted 16 lines

Quoted 7 lines

Quoted 7 lines

extras/mini-os/arch/arm/arm32.S new file +7 -0

+     bx      r1
+
+@ Called once the MMU is enabled. The boot code and the page table are mapped,
+@ but nothing else is yet.
+@
+@ => r2 -> dtb (physical)
+@    r7 = virtual address of page table

Quoted 8 lines

Quoted 4 lines

+@    r8 = section entry template (flags)
+@    r9 = desired physical - virtual offset
+@    pc -> somewhere in newly-mapped virtual code section

+stage2:

I think you want another dsb around here somewhere to ensure that none of the PT updates you do in a moment float up and interfere with the mapping of the code between the write to SCTLR until the bx r1.

Quoted 1 line

How does that happen? Presumably the isb below will block that.

I don't think it will, it's effectively just a pipeline flush but that doesn't necessarily mean there isn't a write from below already in that pipeline.

(I think, I haven't actually gone back to the spec on this one...)

In Xen we do dsb+isb before and after the TLB flush.

Quoted 22 lines

Quoted 22 lines

Quoted 8 lines

extras/mini-os/arch/arm/arm32.S new file +8 -0

+     @ Invalidate TLB
+     mcr     p15, 0, r1, c8, c7, 0   @ TLBIALL
+     isb
+
+     @ The new mapping has now taken effect:
+     @ r7 -> page_dir
+
+     @ Fill in the whole top-level translation table (at page_dir).

extras/mini-os/arch/arm/arm32.S line 118

Quoted 10 lines

extras/mini-os/arch/arm/arm32.S new file +10 -0

+     @ Populate the whole pagedir with 1MB section descriptors.
+
+     mov     r1, r7                  @ r1 -> first section entry
+     add     r3, r1, #4*4*1024       @ limit (4 GB address space, 4 byte entries)
+     orr     r0, r8, r9              @ r0 = entry mapping section zero to start of physical RAM
+1:
+     str     r0, [r1],#4             @ write the section entry
+     add     r0, r0, #1 << 20        @ next physical page (wraps)
+     cmp     r1, r3
+     bne     1b

dsb here to make sure those writes have actually occurred before you come to rely on them.

Quoted 6 lines

Quoted 3 lines

If there is any chance of any of those writes going through a mapping which you are then changing at the same time then I worry that you might also need a dsb before each write.

The mapping for the translation table itself doesn't change (its entry will be written with the same value as previously).

Phew!

Quoted 2 lines

The processor can still reorder writes and things like that, so I think it is still needed.

Ah, I'd assumed it would default everything to Strongly Ordered with the MMU off, but I see it depends on how Xen sets the "Default cacheable" bit.

Quoted 6 lines

Quoted 1 line

How does that happen? Presumably the isb below will block that.

I don't think it will, it's effectively just a pipeline flush but that doesn't necessarily mean there isn't a write from below already in that pipeline.

Quoted 3 lines

(I think, I haven't actually gone back to the spec on this one...)

In Xen we do dsb+isb before and after the TLB flush.

I've added these in various places, but no improvement:

https://github.com/talex5/xen/commit/39199da8493ad6e235849d7ecd51a1415d7d60a7

Quoted 6 lines · Thomas Leonard

Quoted 2 lines

The processor can still reorder writes and things like that, so I think it is still needed.

Ah, I'd assumed it would default everything to Strongly Ordered with the MMU off, but I see it depends on how Xen sets the "Default cacheable" bit.

Even if memory accesses are strongly ordered that doesn't include co processor register accesses so they can still be reordered wrt loads/stores.

Quoted 6 lines

Quoted 6 lines

Quoted 1 line

How does that happen? Presumably the isb below will block that.

I don't think it will, it's effectively just a pipeline flush but that doesn't necessarily mean there isn't a write from below already in that pipeline.

Quoted 7 lines

Quoted 3 lines

(I think, I haven't actually gone back to the spec on this one...)

In Xen we do dsb+isb before and after the TLB flush.

I've added these in various places, but no improvement:

https://github.com/talex5/xen/commit/39199da8493ad6e235849d7ecd51a1415d7d60a7

Ah, you aren't setting the cacheability of the PT walks, I bet that's it. If you don't do that then you do need cache maintenance when writing PTEs (or to have caches disabled).

Xen uses LPAE mode and therefore the extended format of TTBCR (called HTCR for Xen), which is where the bits controlling the cacheability of page table walks are held for LPAE.

In non-LPAE mode it looks like the cacheability bits are in TTBR[01] in bits 0..6.

Quoted 31 lines · Ian Campbell

Quoted 6 lines · Thomas Leonard

Quoted 2 lines

The processor can still reorder writes and things like that, so I think it is still needed.

Ah, I'd assumed it would default everything to Strongly Ordered with the MMU off, but I see it depends on how Xen sets the "Default cacheable" bit.

Even if memory accesses are strongly ordered that doesn't include co processor register accesses so they can still be reordered wrt loads/stores.

Quoted 6 lines

Quoted 6 lines

Quoted 1 line

How does that happen? Presumably the isb below will block that.

I don't think it will, it's effectively just a pipeline flush but that doesn't necessarily mean there isn't a write from below already in that pipeline.

Quoted 7 lines

Quoted 3 lines

(I think, I haven't actually gone back to the spec on this one...)

In Xen we do dsb+isb before and after the TLB flush.

I've added these in various places, but no improvement:

https://github.com/talex5/xen/commit/39199da8493ad6e235849d7ecd51a1415d7d60a7

Ah, you aren't setting the cacheability of the PT walks, I bet that's it. If you don't do that then you do need cache maintenance when writing PTEs (or to have caches disabled).

Quoted 6 lines

Xen uses LPAE mode and therefore the extended format of TTBCR (called HTCR for Xen), which is where the bits controlling the cacheability of page table walks are held for LPAE.

In non-LPAE mode it looks like the cacheability bits are in TTBR[01] in bits 0..6.

You're right - that was it - thanks!

Based on an initial patch by Karim Raslan.

Signed-off-by Karim Allah Ahmed <[email protected]>

Signed-off-by Thomas Leonard <[email protected]>

---

Changes since v5:

- Require only minimum property lengths (requested by Ian Campbell).

Addressed Julien Grall's comments:
- Added comments explaining the lengths when checking FDT properties.
- Use paddr_t type to represent physical addresses.

---
 extras/mini-os/arch/arm/mm.c | 138 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 138 insertions(+)
 create mode 100644 extras/mini-os/arch/arm/mm.c

extras/mini-os/arch/arm/mm.c new file +0 -0

new file mode 100644
index 0000000..ec1f821

extras/mini-os/arch/arm/mm.c new file +9 -0

+
+unsigned long allocate_ondemand(unsigned long n, unsigned long alignment)
+{
+    // FIXME
+    BUG();
+}
+
+void arch_init_mm(unsigned long *start_pfn_p, unsigned long *max_pfn_p)
+{

Hi Thomas,

Quoted 19 lines · Thomas Leonard

Based on an initial patch by Karim Raslan.

Signed-off-by Karim Allah Ahmed <[email protected]>

Signed-off-by Thomas Leonard <[email protected]>

---

Changes since v5:

- Require only minimum property lengths (requested by Ian Campbell).

Addressed Julien Grall's comments:
- Added comments explaining the lengths when checking FDT properties.
- Use paddr_t type to represent physical addresses.

---
 extras/mini-os/arch/arm/mm.c | 138 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 138 insertions(+)
 create mode 100644 extras/mini-os/arch/arm/mm.c

Quoted 6 lines

extras/mini-os/arch/arm/mm.c new file +0 -0

new file mode 100644
index 0000000..ec1f821

extras/mini-os/arch/arm/mm.c line 8

Quoted 8 lines

extras/mini-os/arch/arm/mm.c new file +8 -0

@@ -0,0 +1,138 @@
+#include <mini-os/console.h>
+#include <xen/memory.h>
+#include <arch_mm.h>
+#include <mini-os/hypervisor.h>
+#include <libfdt.h>
+#include <lib.h>
+
+int physical_address_offset;

int64_t maybe?

Quoted 8 lines

extras/mini-os/arch/arm/mm.c new file +8 -0

+unsigned long allocate_ondemand(unsigned long n, unsigned long alignment)
+{
+    // FIXME
+    BUG();
+}
+
+void arch_init_mm(unsigned long *start_pfn_p, unsigned long *max_pfn_p)
+{

[..]

extras/mini-os/arch/arm/mm.c line 51

Quoted 3 lines

extras/mini-os/arch/arm/mm.c new file +3 -0

+    unsigned int end = (unsigned int) &_end;
+    paddr_t mem_base = fdt64_to_cpu(regs[0]);
+    unsigned int mem_size = fdt64_to_cpu(regs[1]);

mem_size should be a paddr_t. Even though, the first bank will always have the size fit in 32 bits.

extras/mini-os/arch/arm/mm.c line 56

Quoted 5 lines

extras/mini-os/arch/arm/mm.c new file +5 -0

+    printk("Found memory at %p (len 0x%x)\n", mem_base, mem_size);
+
+    BUG_ON(to_virt(mem_base) > (void *) &_text);          /* Our image isn't in our RAM! */
+    *start_pfn_p = PFN_UP(to_phys(end));
+    int heap_len = mem_size - (PFN_PHYS(*start_pfn_p) - mem_base);

You need to use at least an unsigned int here. And it will be better if you use paddr_t.

extras/mini-os/arch/arm/mm.c line 64

Quoted 8 lines

extras/mini-os/arch/arm/mm.c new file +8 -0

+    *max_pfn_p = *start_pfn_p + PFN_DOWN(heap_len);
+
+    printk("Using pages %d to %d as free space for heap.\n", *start_pfn_p, *max_pfn_p);
+
+    /* The device tree is probably in memory that we're about to hand over to the page
+     * allocator, so move it to the end and reserve that space.
+     */
+    int fdt_size = fdt_totalsize(device_tree);

uint32_t has the definition of the field totalsize in the fdt structure.

Regards,

--
Julien Grall

Based on an initial patch by Karim Raslan.

Signed-off-by Karim Allah Ahmed <[email protected]>

Signed-off-by Thomas Leonard <[email protected]>

---
 extras/mini-os/arch/arm/arm32.S | 22 ++++++++++++++++++++++
 extras/mini-os/arch/arm/sched.c | 38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+)
 create mode 100644 extras/mini-os/arch/arm/sched.c

extras/mini-os/arch/arm/arm32.S +3 -0

index 88efe24..56429b1 100644
@@ -204,8 +204,30 @@ irq_handler:
 IRQ_handler:
 	.long	0x0
 
+
+.globl __arch_switch_threads
+@ => r0 = &prev->sp

extras/mini-os/arch/arm/arm32.S +2 -0

+    /* Never arrive here! */
+}

Quoted 18 lines · Thomas Leonard

Based on an initial patch by Karim Raslan.

Signed-off-by Karim Allah Ahmed <[email protected]>

Signed-off-by Thomas Leonard <[email protected]>

---
 extras/mini-os/arch/arm/arm32.S | 22 ++++++++++++++++++++++
 extras/mini-os/arch/arm/sched.c | 38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+)
 create mode 100644 extras/mini-os/arch/arm/sched.c

extras/mini-os/arch/arm/arm32.S +0 -0

index 88efe24..56429b1 100644
@@ -204,8 +204,30 @@ irq_handler:
 IRQ_handler:
      .long   0x0
 

extras/mini-os/arch/arm/arm32.S irq_handler: · line 219

Quoted 13 lines

extras/mini-os/arch/arm/arm32.S +13 -0

+
+.globl __arch_switch_threads
+@ => r0 = &prev->sp
+@    r1 = &next->sp
+@ <= returns to next thread's saved return address
+__arch_switch_threads:
+     stmia   r0, {sp, lr}    @ Store current sp and ip to prev's struct thread
+     str     fp, [sp, #-4]   @ Store fp on the old stack
+
+     ldmia   r1, {sp, lr}    @ Load new sp, ip from next's struct thread
+     ldr     fp, [sp, #-4]   @ Restore fp from the stack
+
+     mov     pc, lr

I'm a bit confused by the lack of saving of the GPRs here. Shouldn't at least the caller saved registers (r4..r11) be saved/restored here?

extras/mini-os/arch/arm/arm32.S irq_handler: · line 222

Quoted 3 lines

extras/mini-os/arch/arm/arm32.S +3 -0

+void run_idle_thread(void)
+{
+    __asm__ __volatile__ ("mov sp, %0; mov pc, %1"::"r"(idle_thread->sp), "r"(idle_thread->ip));

Is bx preferred to mov pc these days? I don't know if it matters in this case...

Quoted 19 lines · Ian Campbell

Quoted 18 lines · Thomas Leonard

Based on an initial patch by Karim Raslan.

Signed-off-by Karim Allah Ahmed <[email protected]>

Signed-off-by Thomas Leonard <[email protected]>

---
 extras/mini-os/arch/arm/arm32.S | 22 ++++++++++++++++++++++
 extras/mini-os/arch/arm/sched.c | 38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+)
 create mode 100644 extras/mini-os/arch/arm/sched.c

extras/mini-os/arch/arm/arm32.S +0 -0

index 88efe24..56429b1 100644
@@ -204,8 +204,30 @@ irq_handler:
 IRQ_handler:
      .long   0x0
 

Quoted 16 lines

extras/mini-os/arch/arm/arm32.S irq_handler: · line 219

Quoted 13 lines

extras/mini-os/arch/arm/arm32.S +13 -0

+
+.globl __arch_switch_threads
+@ => r0 = &prev->sp
+@    r1 = &next->sp
+@ <= returns to next thread's saved return address
+__arch_switch_threads:
+     stmia   r0, {sp, lr}    @ Store current sp and ip to prev's struct thread
+     str     fp, [sp, #-4]   @ Store fp on the old stack
+
+     ldmia   r1, {sp, lr}    @ Load new sp, ip from next's struct thread
+     ldr     fp, [sp, #-4]   @ Restore fp from the stack
+
+     mov     pc, lr

I'm a bit confused by the lack of saving of the GPRs here. Shouldn't at least the caller saved registers (r4..r11) be saved/restored here?

If they were caller saved then sched.c would save them itself before calling switch_threads. But the AAPCS says:

"A subroutine must preserve the contents of the registers r4-r8, r10, r11 and SP (and r9 in PCS variants that designate r9 as v6)."

So I think these are callee-saved and we should save them here.

From: Karim Raslan <[email protected]>

Signed-off-by Karim Allah Ahmed <[email protected]>

Signed-off-by Thomas Leonard <[email protected]>

---
 extras/mini-os/arch/arm/events.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)
 create mode 100644 extras/mini-os/arch/arm/events.c

extras/mini-os/arch/arm/events.c new file +0 -0

new file mode 100644
index 0000000..441010d

extras/mini-os/arch/arm/time.c new file +21 -0

+/* These are peridically updated in shared_info, and then copied here. */
+struct shadow_time_info {
+    uint64_t tsc_timestamp;     /* TSC at last update of time vals.  */
+    uint64_t system_timestamp;  /* Time, in nanosecs, since boot.    */
+    uint32_t tsc_to_nsec_mul;
+    uint32_t tsc_to_usec_mul;
+    int tsc_shift;
+    uint32_t version;
+};
+static struct timespec shadow_ts;
+static uint32_t shadow_ts_version;
+
+static struct shadow_time_info shadow;
+
+static void get_time_values_from_xen(void)
+{
+    struct vcpu_time_info    *src = &HYPERVISOR_shared_info->vcpu_info[0].time;
+
+     do {
+        shadow.version = src->version;
+        rmb();

Hi Thomas,

Quoted 21 lines · Thomas Leonard

extras/mini-os/arch/arm/time.c new file +21 -0

+/* These are peridically updated in shared_info, and then copied here. */
+struct shadow_time_info {
+    uint64_t tsc_timestamp;     /* TSC at last update of time vals.  */
+    uint64_t system_timestamp;  /* Time, in nanosecs, since boot.    */
+    uint32_t tsc_to_nsec_mul;
+    uint32_t tsc_to_usec_mul;
+    int tsc_shift;
+    uint32_t version;
+};
+static struct timespec shadow_ts;
+static uint32_t shadow_ts_version;
+
+static struct shadow_time_info shadow;
+
+static void get_time_values_from_xen(void)
+{
+    struct vcpu_time_info    *src = &HYPERVISOR_shared_info->vcpu_info[0].time;
+
+     do {
+        shadow.version = src->version;
+        rmb();

extras/mini-os/arch/arm/time.c line 84

Quoted 10 lines

extras/mini-os/arch/arm/time.c new file +10 -0

+        shadow.tsc_timestamp     = src->tsc_timestamp;
+        shadow.system_timestamp  = src->system_time;
+        shadow.tsc_to_nsec_mul   = src->tsc_to_system_mul;
+        shadow.tsc_shift         = src->tsc_shift;
+        rmb();
+    }
+    while ((src->version & 1) | (shadow.version ^ src->version));
+
+    shadow.tsc_to_usec_mul = shadow.tsc_to_nsec_mul / 1000;
+}

It looks the code above is the same as x86. Did you think about sharing the code which used Xen time?

Looking closer to this patch, nothing is using the field of this shared structure, which is CPU time consuming and adding line for no improvement. So I think you can drop all the code related the PV timer for now.