mini-os: initial ARM support

Make .o rules depend on the includes. Before, only the final link step depended on setting up the includes directory, making parallel builds unreliable.

Make symlinks use explicit make rules instead of using a phony target. Avoids unnecessary rebuilds.

[[email protected]: bring back "make links", for stubdom]

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

---
 extras/mini-os/Makefile | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

extras/mini-os/Makefile +17 -6

index 50d038b..6d6537e 100644
@@ -50,6 +50,11 @@ flags-$(CONFIG_XENBUS) += -DCONFIG_XENBUS
 
 DEF_CFLAGS += $(flags-y)
 
+# Symlinks and headers that must be created before building the C files
+GENERATED_HEADERS := include/list.h $(ARCH_LINKS) include/mini-os include/xen include/$(TARGET_ARCH_FAM)/mini-os
+
+EXTRA_DEPS += $(GENERATED_HEADERS)
+
 # Include common mini-os makerules.
 include minios.mk
 

@@ -124,11 +129,18 @@ include/list.h: $(XEN_ROOT)/tools/include/xen-external/bsd-sys-queue-h-seddery $
 	perl $^ --prefix=minios  >[email protected]
 	$(call move-if-changed,[email protected],$@)
 
+# Used by stubdom's Makefile
 .PHONY: links
-links: include/list.h $(ARCH_LINKS)
-	[ -e include/xen ] || ln -sf ../../../xen/include/public include/xen
-	[ -e include/mini-os ] || ln -sf . include/mini-os
-	[ -e include/$(TARGET_ARCH_FAM)/mini-os ] || ln -sf . include/$(TARGET_ARCH_FAM)/mini-os
+links: $(GENERATED_HEADERS)
+
+include/xen:
+	ln -sf ../../../xen/include/public $@
+
+include/mini-os:
+	ln -sf . $@
+
+include/$(TARGET_ARCH_FAM)/mini-os:
+	ln -sf . $@
 
 .PHONY: arch_lib
 arch_lib:

@@ -174,7 +186,7 @@ ifneq ($(APP_OBJS),)
 APP_O=$(OBJ_DIR)/$(TARGET)_app.o 
 endif
 
-$(OBJ_DIR)/$(TARGET): links include/list.h $(OBJS) $(APP_O) arch_lib
+$(OBJ_DIR)/$(TARGET): $(OBJS) $(APP_O) arch_lib
 	$(LD) -r $(LDFLAGS) $(HEAD_OBJ) $(APP_O) $(OBJS) $(LDARCHLIB) $(LDLIBS) -o [email protected]
 	$(OBJCOPY) -w -G $(GLOBAL_PREFIX)* -G _start [email protected] [email protected]
 	$(LD) $(LDFLAGS) $(LDFLAGS_FINAL) [email protected] $(EXTRA_OBJS) -o $@

@@ -212,4 +224,3 @@ tags:
 .PHONY: TAGS
 TAGS:
 	$(all_sources) | xargs etags
-

Make .o rules depend on the includes. Before, only the final link step depended on setting up the includes directory, making parallel builds unreliable.

Make symlinks use explicit make rules instead of using a phony target. Avoids unnecessary rebuilds.

[[email protected]: bring back "make links", for stubdom]

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

---
 extras/mini-os/Makefile | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

extras/mini-os/Makefile +17 -6

index 50d038b..6d6537e 100644
@@ -50,6 +50,11 @@ flags-$(CONFIG_XENBUS) += -DCONFIG_XENBUS
 
 DEF_CFLAGS += $(flags-y)
 
+# Symlinks and headers that must be created before building the C files
+GENERATED_HEADERS := include/list.h $(ARCH_LINKS) include/mini-os include/xen include/$(TARGET_ARCH_FAM)/mini-os
+
+EXTRA_DEPS += $(GENERATED_HEADERS)
+
 # Include common mini-os makerules.
 include minios.mk
 

@@ -124,11 +129,18 @@ include/list.h: $(XEN_ROOT)/tools/include/xen-external/bsd-sys-queue-h-seddery $
 	perl $^ --prefix=minios  >[email protected]
 	$(call move-if-changed,[email protected],$@)
 
+# Used by stubdom's Makefile
 .PHONY: links
-links: include/list.h $(ARCH_LINKS)
-	[ -e include/xen ] || ln -sf ../../../xen/include/public include/xen
-	[ -e include/mini-os ] || ln -sf . include/mini-os
-	[ -e include/$(TARGET_ARCH_FAM)/mini-os ] || ln -sf . include/$(TARGET_ARCH_FAM)/mini-os
+links: $(GENERATED_HEADERS)
+
+include/xen:
+	ln -sf ../../../xen/include/public $@
+
+include/mini-os:
+	ln -sf . $@
+
+include/$(TARGET_ARCH_FAM)/mini-os:
+	ln -sf . $@
 
 .PHONY: arch_lib
 arch_lib:

@@ -174,7 +186,7 @@ ifneq ($(APP_OBJS),)
 APP_O=$(OBJ_DIR)/$(TARGET)_app.o 
 endif
 
-$(OBJ_DIR)/$(TARGET): links include/list.h $(OBJS) $(APP_O) arch_lib
+$(OBJ_DIR)/$(TARGET): $(OBJS) $(APP_O) arch_lib
 	$(LD) -r $(LDFLAGS) $(HEAD_OBJ) $(APP_O) $(OBJS) $(LDARCHLIB) $(LDLIBS) -o [email protected]
 	$(OBJCOPY) -w -G $(GLOBAL_PREFIX)* -G _start [email protected] [email protected]
 	$(LD) $(LDFLAGS) $(LDFLAGS_FINAL) [email protected] $(EXTRA_OBJS) -o $@

@@ -212,4 +224,3 @@ tags:
 .PHONY: TAGS
 TAGS:
 	$(all_sources) | xargs etags
-

Signature · 1 line

2.0.0

Would crash if HYPERVISOR_event_channel_op returned an error code. The other changes in this commit are just fixing indentation.

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

Would crash if HYPERVISOR_event_channel_op returned an error code. The other changes in this commit are just fixing indentation.

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

This marks the channel as closed, in case someone tries to use it again.

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

This marks the channel as closed, in case someone tries to use it again.

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

POSIX requires this.

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

POSIX requires this.

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

From: Karim Raslan <[email protected]>

On ARM, Mini-OS will boot and display some output on the console. Tested with:

make XEN_TARGET_ARCH=arm32 CROSS_COMPILE=arm-linux-gnueabihf- \
	CONFIG_TEST=y CONFIG_START_NETWORK=n CONFIG_BLKFRONT=n \
	CONFIG_NETFRONT=n CONFIG_FBFRONT=n CONFIG_KBDFRONT=n \
	CONFIG_CONSFRONT=n CONFIG_XC=n -j4

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

[[email protected]: made x86_64 support work again]

[[email protected]: split into multiple patches]

[[email protected]: re-enabled force_evtchn_callback]

[[email protected]: enable regular console]

[[email protected]: fixed initialisation code:
- Configure write-back caching in page table. This is needed for reliable hypercalls to Xen (thanks to Julien Grall).
- Use "client mode" for access control (domains are deprecated, according to ARM Cortex-A Series Programmer’s Guide version 4.0, section 9.6.4).
- Enable more SCTLR features (icache, branch prediction)]

[[email protected]: use Virtual Count register for monotonic time]

[[email protected]: fixed HYPERVISOR_shutdown]

[[email protected]: get xenstore details from hypervisor]

[[email protected]: use GCC implementation of division]

[[email protected]: include hypervisor.h from os.h, as on x86]

[[email protected]: cleaned up interrupt handlers and threading]

[[email protected]: call exit_thread when a thread returns]

[[email protected]: implemented block_domain for ARM]

[[email protected]: fixed hang when enabling interrupts]

[[email protected]: added -march=armv7-a to flags]

[[email protected]: implemented bitops for ARM]

[[email protected]: CLREX after handling IRQs]

[[email protected]: unbind debug port at shutdown]

[[email protected]: allow unaligned accesses]

[[email protected]: added arch_endian.h for ARM]

[[email protected]: fix zImage header for XSA-95]

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

extras/mini-os/drivers/gic.c new file +16 -0

new file mode 100644
index 0000000..3141830
@@ -0,0 +1,187 @@
+// ARM GIC implementation
+
+#include <mini-os/os.h>
+#include <mini-os/hypervisor.h>
+
+//#define VGIC_DEBUG
+#ifdef VGIC_DEBUG
+#define DEBUG(_f, _a...) \
+    DEBUG("MINI_OS(file=vgic.c, line=%d) " _f , __LINE__, ## _a)
+#else
+#define DEBUG(_f, _a...)    ((void)0)
+#endif
+
+extern void (*IRQ_handler)(void);
+
+struct gic {

extras/mini-os/drivers/gic.c new file +20 -0

+#define GICD_ITARGETSR    0x800
+#define GICD_ICFGR        0xC00
+
+// CPU Interface
+#define GICC_CTLR    0x0
+#define GICC_PMR    0x4
+#define GICC_IAR    0xc
+#define GICC_EOIR    0x10
+#define GICC_HPPIR    0x18
+
+#define gicd(gic, offset) ((gic)->gicd_base + (offset))
+#define gicc(gic, offset) ((gic)->gicc_base + (offset))
+
+#define REG(addr) ((uint32_t *)(addr))
+
+static inline uint32_t REG_READ32(volatile uint32_t *addr)
+{
+    uint32_t value;
+    __asm__ __volatile__("ldr %0, [%1]":"=&r"(value):"r"(addr));
+    rmb();

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

index 3c92d82..780c74a 100644
@@ -179,6 +179,7 @@ void init_events(void)
 void fini_events(void)
 {
     /* Dealloc all events */
+    arch_unbind_ports();
     unbind_all_ports();
     arch_fini_events();
 }

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

     uint32_t sid;
     struct xen_flask_op op;
     op.cmd = FLASK_GET_PEER_SID;

@@ -253,6 +255,7 @@ int evtchn_get_peercontext(evtchn_port_t local_port, char *ctx, int size)
     op.u.sid_context.size = size;
     set_xen_guest_handle(op.u.sid_context.context, ctx);
     rc = _hypercall1(int, xsm_op, &op);
+#endif
     return rc;
 }
 

extras/mini-os/hypervisor.c +0 -0

index b4688a0..1b61d9b 100644

extras/mini-os/hypervisor.c +8 -0

         }
     }
 

@@ -73,18 +73,26 @@ void do_hypervisor_callback(struct pt_regs *regs)
 
 void force_evtchn_callback(void)
 {
+#ifdef XEN_HAVE_PV_UPCALL_MASK
     int save;
+#endif
     vcpu_info_t *vcpu;
     vcpu = &HYPERVISOR_shared_info->vcpu_info[smp_processor_id()];
+#ifdef XEN_HAVE_PV_UPCALL_MASK
     save = vcpu->evtchn_upcall_mask;
+#endif
 
     while (vcpu->evtchn_upcall_pending) {
+#ifdef XEN_HAVE_PV_UPCALL_MASK
         vcpu->evtchn_upcall_mask = 1;
+#endif
         barrier();
         do_hypervisor_callback(NULL);
         barrier();
+#ifdef XEN_HAVE_PV_UPCALL_MASK
         vcpu->evtchn_upcall_mask = save;
         barrier();
+#endif
     };
 }
 

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

new file mode 100755
index 0000000..d57f150
@@ -0,0 +1,49 @@
+
+
+#ifndef __ARCH_ASM_SPINLOCK_H
+#define __ARCH_ASM_SPINLOCK_H
+
+#include "os.h"
+
+
+#define ARCH_SPIN_LOCK_UNLOCKED { 1 }
+
+/*
+ * Simple spin lock operations.  There are two variants, one clears IRQ's
+ * on the local processor, one does not.
+ *
+ * We make no fairness assumptions. They have a cost.

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

+
+static inline void _raw_spin_unlock(spinlock_t *lock)
+{
+    xchg(&lock->slock, 1);
+}
+
+static inline int _raw_spin_trylock(spinlock_t *lock)
+{
+    return xchg(&lock->slock, 0) != 0 ? 1 : 0;
+}
+

+#endif /* __HYPERCALL_X86_64_H__ */

int do_event(evtchn_port_t port, struct pt_regs *regs);

extras/mini-os/include/gic.h new file +1 -0

new file mode 100644
index 0000000..cead2e5
@@ -0,0 +1 @@
+void gic_init(void);

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

index a62cb78..052f4f8 100644

extras/mini-os/kernel.c +4 -0

index 9a30550..7b2b8fc 100644
@@ -47,6 +47,10 @@
 #include <xen/features.h>
 #include <xen/version.h>
 
+#ifdef __arm__
+#include <mini-os/gic.h>
+#endif
+
 uint8_t xen_features[XENFEAT_NR_SUBMAPS * 32];
 
 void setup_xen_features(void)

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

     /* Call (possibly overridden) app_main() */
     app_main(&start_info);
 

extras/mini-os/sched.c +0 -0

index 174945e..b99d7dc 100644

extras/mini-os/sched.c +5 -11

     /* Not runable, not exited, not sleeping */
     thread->flags = 0;
     thread->wakeup_time = 0LL;

@@ -165,28 +168,28 @@ struct _reent *__getreent(void)
     struct _reent *_reent;
 
     if (!threads_started)
-	_reent = _impure_ptr;
+        _reent = _impure_ptr;
     else if (in_callback)
-	_reent = &callback_reent;
+        _reent = &callback_reent;
     else
-	_reent = &get_current()->reent;
+        _reent = &get_current()->reent;
 #ifndef NDEBUG
 #if defined(__x86_64__) || defined(__x86__)
     {
 #ifdef __x86_64__
-	register unsigned long sp asm ("rsp");
+        register unsigned long sp asm ("rsp");
 #else
-	register unsigned long sp asm ("esp");
+        register unsigned long sp asm ("esp");
 #endif
-	if ((sp & (STACK_SIZE-1)) < STACK_SIZE / 16) {
-	    static int overflowing;
-	    if (!overflowing) {
-		overflowing = 1;
-		printk("stack overflow\n");
-		BUG();

Hi Thomas,

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

Quoted 9 lines

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

new file mode 100644
index 0000000..4f953ec
@@ -0,0 +1,155 @@
+#define PHYS_START (0x80008000)

This address is wrong with the latest xen unstable. Any reason to make mini-os tight to a specific physical address?

[..]

extras/mini-os/arch/arm/hypercalls32.S

Quoted 6 lines

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

new file mode 100644
index 0000000..e2f21c4

This code is based on the one in Linux (arch/arm/xen/hypercall.S), right? IIRC it's a BSD license but you have to keep it.

[..]

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

Quoted 11 lines

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

new file mode 100644
index 0000000..bb6aa0e
@@ -0,0 +1,44 @@
+#include <console.h>
+#include <arch_mm.h>
+
+#define PHYS_START (0x80008000 + (1000 * 4 * 1024))
+#define PHYS_SIZE (40*1024*1024)

Same remark as earlier in arm/arm32.S. But I see you don't use them anymore after patch #10. So please drop the both defines in #10.

[..]

Quoted 9 lines

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

+void set_vtimer_compare(uint64_t value) {
+    uint32_t x, y;
+
+    DEBUG("New CompareValue : %llx\n", value);
+    x = 0xFFFFFFFFULL & value;
+    y = (value >> 32) & 0xFFFFFFFF;
+
+    __asm__ __volatile__("mcrr p15, 3, %0, %1, c14\n"
+            "isb"::"r"(x), "r"(y));

Quoted 5 lines

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

+
+    __asm__ __volatile__("mov %0, #0x1\n"
+            "mcr p15, 0, %0, c14, c3, 1\n" /* Enable timer and unmask the

output signal */
+ "isb":"=r"(x));

I don't think you need to add an isb per instruction. One should be enough.

[..]

Quoted 28 lines

extras/mini-os/drivers/gic.c new file +22 -0

new file mode 100644
index 0000000..3141830
@@ -0,0 +1,187 @@
+// ARM GIC implementation
+
+#include <mini-os/os.h>
+#include <mini-os/hypervisor.h>
+
+//#define VGIC_DEBUG
+#ifdef VGIC_DEBUG
+#define DEBUG(_f, _a...) \
+    DEBUG("MINI_OS(file=vgic.c, line=%d) " _f , __LINE__, ## _a)
+#else
+#define DEBUG(_f, _a...)    ((void)0)
+#endif
+
+extern void (*IRQ_handler)(void);
+
+struct gic {
+    volatile char *gicd_base;
+    volatile char *gicc_base;
+};
+
+static struct gic gic;
+

extras/mini-os/drivers/gic.c line 26

Quoted 4 lines

extras/mini-os/drivers/gic.c new file +4 -0

+// Distributor Interface
+#define GICD_CTLR        0x0
+#define GICD_ISENABLER    0x100
+#define GICD_PRIORITY    0x400

You use the right name for every macro except this one. I would rename it to GICD_IPRIORITYR to stay consistent.

Quoted 22 lines

extras/mini-os/drivers/gic.c new file +22 -0

+#define GICD_ITARGETSR    0x800
+#define GICD_ICFGR        0xC00
+
+// CPU Interface
+#define GICC_CTLR    0x0
+#define GICC_PMR    0x4
+#define GICC_IAR    0xc
+#define GICC_EOIR    0x10
+#define GICC_HPPIR    0x18
+
+#define gicd(gic, offset) ((gic)->gicd_base + (offset))
+#define gicc(gic, offset) ((gic)->gicc_base + (offset))
+
+#define REG(addr) ((uint32_t *)(addr))
+
+static inline uint32_t REG_READ32(volatile uint32_t *addr)
+{
+    uint32_t value;
+    __asm__ __volatile__("ldr %0, [%1]":"=&r"(value):"r"(addr));
+    rmb();
+    return value;
+}

extras/mini-os/drivers/gic.c line 55

Quoted 8 lines

extras/mini-os/drivers/gic.c new file +2 -0

+
+static inline void REG_WRITE32(volatile uint32_t *addr, unsigned int

value)

extras/mini-os/drivers/gic.c new file +5 -0

+{
+    __asm__ __volatile__("str %0, [%1]"::"r"(value), "r"(addr));
+    wmb();
+}
+

Quoted 2 lines

+static void gic_set_priority(struct gic *gic, unsigned char irq_number, unsigned char priority)

hmmmm why do you use unsigned char to describe the interrupt?

extras/mini-os/drivers/gic.c line 59

Quoted 3 lines

extras/mini-os/drivers/gic.c new file +3 -0

+{
+    uint32_t value;
+    value = REG_READ32(REG(gicd(gic, GICD_PRIORITY)) + irq_number);

There is 4 interrupts describe per register, this should be (irq_number & ~3).

extras/mini-os/drivers/gic.c line 61

Quoted 2 lines

extras/mini-os/drivers/gic.c new file +2 -0

+    value &= ~(0xff << (8 * (irq_number & 0x3))); // set priority to '0'
+    value |= priority << (8 * (irq_number & 0x3)); // add our priority

It looks strange that you correctly handle the shift here.

Quoted 1 line

+ REG_WRITE32(REG(gicd(gic, GICD_PRIORITY)) + irq_number, value);

irq_number & ~3

extras/mini-os/drivers/gic.c line 68

Quoted 7 lines

extras/mini-os/drivers/gic.c new file +3 -0

+}
+
+static void gic_route_interrupt(struct gic *gic, unsigned char

irq_number, unsigned char cpu_set)

extras/mini-os/drivers/gic.c new file +3 -0

+{
+    uint32_t value;
+    value = REG_READ32(REG(gicd(gic, GICD_ITARGETSR)) + irq_number);

Same remark as gic_set_priority here.

extras/mini-os/drivers/gic.c

Quoted 2 lines

extras/mini-os/drivers/gic.c new file +2 -0

+    value &= ~(0xff << (8 * (irq_number & 0x3))); // set priority to '0'
+    value |= cpu_set << (8 * (irq_number & 0x3)); // add our priority

The comments are wrong in the 2 lines above.

Quoted 1 line

+ REG_WRITE32(REG(gicd(gic, GICD_ITARGETSR)) + irq_number, value);

irq_number & ~3;

extras/mini-os/drivers/gic.c line 77

Quoted 7 lines

extras/mini-os/drivers/gic.c new file +3 -0

+}
+
+/* When accessing the GIC registers, we can't use LDREX/STREX because

it's not regular memory. */

extras/mini-os/drivers/gic.c new file +3 -0

+static __inline__ void clear_bit_non_atomic(int nr, volatile void *base)
+{
+    uint32_t *tmp = (uint32_t *)base;

You don't need to cast here and I suspect your variable need to be volatile otherwise the compiler may do some assumption.

extras/mini-os/drivers/gic.c line 83

Quoted 6 lines

extras/mini-os/drivers/gic.c new file +6 -0

+    tmp[nr >> 5] &= (unsigned long)~(1 << (nr & 0x1f));
+}
+
+static __inline__ void set_bit_non_atomic(int nr, volatile void *base)
+{
+    uint32_t *tmp = (uint32_t *)base;

Same remark here.

extras/mini-os/drivers/gic.c line 173

Quoted 4 lines

extras/mini-os/drivers/gic.c new file +4 -0

+void gic_init(void) {
+    // FIXME Get from dt!
+    gic.gicd_base = (char *)0x2c001000ULL;
+    gic.gicc_base = (char *)0x2c002000ULL;

Those values are wrong.

Quoted 15 lines

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

index 3c92d82..780c74a 100644
@@ -179,6 +179,7 @@ void init_events(void)
 void fini_events(void)
 {
     /* Dealloc all events */
+    arch_unbind_ports();
     unbind_all_ports();
     arch_fini_events();
 }

@@ -238,7 +239,8 @@ int evtchn_bind_interdomain(domid_t pal, evtchn_port_t
 remote_port,
 

extras/mini-os/events.c int evtchn_bind_interdomain(domid_t pal, evtchn_port_t remote_port, · line 243

Quoted 5 lines

extras/mini-os/events.c +2 -1

 int evtchn_get_peercontext(evtchn_port_t local_port, char *ctx, int size)
 {
-    int rc;
+    int rc = 0;
+#ifndef __arm__                        /* TODO */

Do you still need this TODO & ifdef? XSM is working correctly on ARM. And even if it was not implemented the compilation should not failed.

Quoted 19 lines

extras/mini-os/events.c +0 -0

     uint32_t sid;
     struct xen_flask_op op;

     op.cmd = FLASK_GET_PEER_SID;

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

@@ -253,6 +255,7 @@ int evtchn_get_peercontext(evtchn_port_t local_port,
 char *ctx, int size)
     op.u.sid_context.size = size;
     set_xen_guest_handle(op.u.sid_context.context, ctx);
     rc = _hypercall1(int, xsm_op, &op);
+#endif
     return rc;
 }

extras/mini-os/hypervisor.c +0 -0

index b4688a0..1b61d9b 100644
@@ -64,7 +64,7 @@ void do_hypervisor_callback(struct pt_regs *regs)
             l2 &= ~(1UL << l2i);
 

extras/mini-os/hypervisor.c void do_hypervisor_callback(struct pt_regs *regs) · line 67

Quoted 3 lines

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

             port = (l1i * (sizeof(unsigned long) * 8)) + l2i;
-                       do_event(port, regs);
+            do_event(port, regs);

This patch is long and difficult to read. Please avoid indentation change in non-modified code at the same time.

Quoted 30 lines

extras/mini-os/hypervisor.c +8 -0

         }
     }
 

@@ -73,18 +73,26 @@ void do_hypervisor_callback(struct pt_regs *regs)
 
 void force_evtchn_callback(void)
 {
+#ifdef XEN_HAVE_PV_UPCALL_MASK
     int save;
+#endif
     vcpu_info_t *vcpu;
     vcpu = &HYPERVISOR_shared_info->vcpu_info[smp_processor_id()];
+#ifdef XEN_HAVE_PV_UPCALL_MASK
     save = vcpu->evtchn_upcall_mask;
+#endif
 
     while (vcpu->evtchn_upcall_pending) {
+#ifdef XEN_HAVE_PV_UPCALL_MASK
         vcpu->evtchn_upcall_mask = 1;
+#endif
         barrier();
         do_hypervisor_callback(NULL);
         barrier();
+#ifdef XEN_HAVE_PV_UPCALL_MASK
         vcpu->evtchn_upcall_mask = save;
         barrier();
+#endif
     };
 }
 

extras/mini-os/hypervisor.c inline void unmask_evtchn(uint32_t port) · line 126

Quoted 10 lines

extras/mini-os/hypervisor.c +2 -0

@@ -110,7 +118,9 @@ inline void unmask_evtchn(uint32_t port)
               &vcpu_info->evtchn_pending_sel) )
     {
         vcpu_info->evtchn_upcall_pending = 1;
+#ifdef XEN_HAVE_PV_UPCALL_MASK
         if ( !vcpu_info->evtchn_upcall_mask )
+#endif
             force_evtchn_callback();
     }
 }

I would have move those change in a separate patch.

[..]

Quoted 29 lines

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

new file mode 100755
index 0000000..d57f150
@@ -0,0 +1,49 @@
+
+
+#ifndef __ARCH_ASM_SPINLOCK_H
+#define __ARCH_ASM_SPINLOCK_H
+
+#include "os.h"
+
+
+#define ARCH_SPIN_LOCK_UNLOCKED { 1 }
+
+/*
+ * Simple spin lock operations.  There are two variants, one clears IRQ's
+ * on the local processor, one does not.
+ *
+ * We make no fairness assumptions. They have a cost.
+ */
+
+#define arch_spin_is_locked(x)    (*(volatile signed char *)(&(x)->slock)

<= 0)
+#define arch_spin_unlock_wait(x) do { barrier(); } while(spin_is_locked(x))
+

Quoted 5 lines

+/*
+ * This works. Despite all the confusion.
+ * (except on PPro SMP or if we are using OOSTORE)
+ * (PPro errata 66, 92)
+ */

This comment is x86... It should not have been copied here.

Quoted 19 lines

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

+
+static inline void _raw_spin_unlock(spinlock_t *lock)
+{
+    xchg(&lock->slock, 1);
+}
+
+static inline int _raw_spin_trylock(spinlock_t *lock)
+{
+    return xchg(&lock->slock, 0) != 0 ? 1 : 0;
+}
+
+static inline void _raw_spin_lock(spinlock_t *lock)
+{
+    volatile int was_locked;
+    do {
+        was_locked = xchg(&lock->slock, 0) == 0 ? 1 : 0;
+    } while(was_locked);
+}
+

Quoted 3 lines

+static inline void _raw_spin_lock_flags (spinlock_t *lock, unsigned long flags)
+{

You didn't implement this function. It looks like it's not used in mini-os, right? If so, please add a BUG_ON just in case that someone decide to use it later.

extras/mini-os/include/arm/hypercall-arm32.h

Quoted 7 lines

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

new file mode 100644
index 0000000..0fc1c03

Actually this file can be name hypercall.h. On ARM64 the set of hypercall is exactly the same. The only different that you will have between both architecture is the assembly code and the system registers.

[..]

extras/mini-os/include/arm/hypercall-arm32.h line 43

Quoted 3 lines

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

+inline int
+HYPERVISOR_mmu_update(
+    mmu_update_t *req, int count, int *success_count, domid_t domid);

Why do you use inline in all external functions in this include?

Hence most of this hypercall doesn't exist on ARM. Please define only the necessary one.

[..]

Quoted 1 line

+#endif /* __HYPERCALL_X86_64_H__ */

__HYPERCALL_ARM_H__

[..]

extras/mini-os/include/arm/hypercall-arm32.h

Quoted 2 lines

extras/mini-os/include/arm/hypercall-arm32.h new file +2 -0

+// disable interrupts
+static inline __cli(void) {

I see that you use __cli and __sti only
__cli and __sti are only in 2 defines below. These name are only x86 specific. Please rename them or merge this code in local_irq_{disable,enable}

extras/mini-os/include/arm/hypercall-arm32.h

Quoted 2 lines

extras/mini-os/include/arm/hypercall-arm32.h new file +2 -0

+    int x;
+    __asm__ __volatile__("mrs %0, cpsr;cpsid i":"=r"(x)::"memory");

Why do you need to read cpsr. cpsid i is enough here.

Quoted 4 lines

extras/mini-os/include/arm/hypercall-arm32.h new file +4 -0

+}
+
+// enable interrupts
+static inline __sti(void) {

extras/mini-os/include/arm/hypercall-arm32.h

Quoted 5 lines

extras/mini-os/include/arm/hypercall-arm32.h new file +5 -0

+    int x;
+    __asm__ __volatile__("mrs %0, cpsr\n"
+                        "bic %0, %0, #0x80\n"
+                        "msr cpsr_c, %0"
+                        :"=r"(x)::"memory");

This can simply be done by cpsie i

extras/mini-os/include/arm/hypercall-arm32.h

Quoted 6 lines

extras/mini-os/include/arm/hypercall-arm32.h new file +6 -0

+}
+
+static inline int irqs_disabled() {
+    int x;
+    __asm__ __volatile__("mrs %0, cpsr\n":"=r"(x)::"memory");
+    return (x & 0x80);

You can use local_save_flags directly.

extras/mini-os/include/arm/hypercall-arm32.h

Quoted 10 lines

extras/mini-os/include/arm/hypercall-arm32.h new file +4 -0

+}
+
+#define local_irq_save(x) { \
+    __asm__ __volatile__("mrs %0, cpsr;cpsid i; and %0, %0,

#0x80":"=r"(x)::"memory"); \

extras/mini-os/include/arm/hypercall-arm32.h new file +5 -0

+}
+
+#define local_irq_restore(x) {    \
+    __asm__ __volatile__("msr cpsr_c, %0"::"r"(x):"memory");    \
+}

If you mask the result in local_irq_save, and use this value directly is local_irq_restore, you will disable by mistake the Asynchronous Abort exception....

You have to remove the and %0... at the end.

Quoted 4 lines

extras/mini-os/include/arm/hypercall-arm32.h new file +2 -0

+#define local_save_flags(x)    { \
+    __asm__ __volatile__("mrs %0, cpsr; and %0, %0,

0x80":"=r"(x)::"memory"); \
+}

Same remark here.

extras/mini-os/include/arm/hypercall-arm32.h

Quoted 7 lines

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

+#define local_irq_disable()    __cli()
+#define local_irq_enable() __sti()
+
+#if defined(__arm__)
+#define mb() __asm__("dmb");
+#define rmb() __asm__("dmb");
+#define wmb() __asm__("dmb");

I suspect you want to dsb here. You want to make sure that every memory access has finished avoid executing new instruction until this is done.

dmb only ensure the memory ordering.

Also, you want to clobber the memory to avoid the compiler:
1) reordering the instructions
2) prefetch data from the memory

extras/mini-os/include/arm/hypercall-arm32.h

Quoted 4 lines

extras/mini-os/include/arm/hypercall-arm32.h new file +4 -0

+#elif defined(__aarch64__)
+#define mb()
+#define rmb()
+#define wmb()

I'm confused with this piece of code... memory barrier are also required on aarch64. Hence dmb/isb also exist on this architecture

You don't need the if ... elif ... else

extras/mini-os/include/arm/hypercall-arm32.h

Quoted 2 lines

extras/mini-os/include/arm/hypercall-arm32.h new file +2 -0

+#define unlikely(x)  __builtin_expect((x),0)
+#define likely(x)  __builtin_expect((x),1)

Can't it be common with x86?

[..]

Quoted 14 lines

int do_event(evtchn_port_t port, struct pt_regs *regs);

extras/mini-os/include/gic.h new file +1 -0

new file mode 100644
index 0000000..cead2e5
@@ -0,0 +1 @@
+void gic_init(void);

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

index a62cb78..052f4f8 100644

extras/mini-os/include/hypervisor.h line 24

Quoted 7 lines

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

@@ -18,6 +18,10 @@
 #include <hypercall-x86_32.h>
 #elif defined(__x86_64__)
 #include <hypercall-x86_64.h>
+#elif defined(__arm__)
+#include <hypercall-arm32.h>
+#elif defined(__aarch64__)
+#include <hypercall-arm64.h>

Hmmm, the filehypercall-arm64.h doesn't exists in this series...

Futhermore, as I said ealier the set of hypercall is exactly the same on arm64.

[..]

Quoted 8 lines

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

+#if defined(__i386__) || defined(__arm__)
 typedef long long           quad_t;
 typedef unsigned long long  u_quad_t;

@@ -40,7 +40,7 @@ typedef unsigned long       u_quad_t;
 typedef struct { unsigned long pte; } pte_t;
 #endif /* __i386__ || __x86_64__ */
 

extras/mini-os/include/hypervisor.h typedef unsigned long u_quad_t; · line 44

Quoted 3 lines

extras/mini-os/include/hypervisor.h +1 -1

-#ifdef __x86_64__
+#if defined(__x86_64__) || defined(__aarch64__)
 #define __pte(x) ((pte_t) { (x) } )

This looks wrong to me. The pte layout is exactly the same on arm32 and arm64. Hence, this is only used in the x86 code. Please either move pte_t in x86 part or define it correctly on ARM...

[..]

Quoted 19 lines

extras/mini-os/kernel.c +4 -0

index 9a30550..7b2b8fc 100644
@@ -47,6 +47,10 @@
 #include <xen/features.h>
 #include <xen/version.h>
 
+#ifdef __arm__
+#include <mini-os/gic.h>
+#endif
+
 uint8_t xen_features[XENFEAT_NR_SUBMAPS * 32];
 
 void setup_xen_features(void)

@@ -147,6 +151,10 @@ void start_kernel(void)
     create_thread("shutdown", shutdown_thread, NULL);
 #endif
 

extras/mini-os/kernel.c void start_kernel(void) · line 157

Quoted 4 lines

extras/mini-os/kernel.c +4 -0

+#ifdef __arm__
+    gic_init();
+#endif
+

I would define a function arch_init. This will avoid the #ifdef __arm__ in the code common.

Hence, it looks strange that sometime you have #if __arm__ && __arch64__ and sometimes not...

Quoted 3 lines

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

     /* Call (possibly overridden) app_main() */
     app_main(&start_info);
 

extras/mini-os/sched.c struct thread* create_thread(char *name, void · line 151

Quoted 12 lines

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

index 174945e..b99d7dc 100644
@@ -145,6 +145,9 @@ struct thread* create_thread(char *name, void
 (*function)(void *), void *data)
     unsigned long flags;
     /* Call architecture specific setup. */
     thread = arch_create_thread(name, function, data);
+    if(!thread)
+        BUG(); //For now, FIXME should just return NULL
+

This change is in the common code. I think this should go in a separate patch.

Hence looking again to arch_create_thread for ARM you:
1) never return NULL
2) never check the xmalloc/alloc_pages return.

Quoted 36 lines

extras/mini-os/sched.c +0 -0

     /* Not runable, not exited, not sleeping */
     thread->flags = 0;

     thread->wakeup_time = 0LL;

extras/mini-os/sched.c +8 -13

@@ -165,28 +168,28 @@ struct _reent *__getreent(void)
     struct _reent *_reent;
 
     if (!threads_started)
-       _reent = _impure_ptr;
+        _reent = _impure_ptr;
     else if (in_callback)
-       _reent = &callback_reent;
+        _reent = &callback_reent;
     else
-       _reent = &get_current()->reent;
+        _reent = &get_current()->reent;
 #ifndef NDEBUG
 #if defined(__x86_64__) || defined(__x86__)
     {
 #ifdef __x86_64__
-       register unsigned long sp asm ("rsp");
+        register unsigned long sp asm ("rsp");
 #else
-       register unsigned long sp asm ("esp");
+        register unsigned long sp asm ("esp");
 #endif
-       if ((sp & (STACK_SIZE-1)) < STACK_SIZE / 16) {
-           static int overflowing;
-           if (!overflowing) {
-               overflowing = 1;
-               printk("stack overflow\n");
-               BUG();
-           }
-       }
+        if ((sp & (STACK_SIZE-1)) < STACK_SIZE / 16) {
+            static int overflowing;
+            if (!overflowing) {

extras/mini-os/sched.c struct _reent *__getreent(void) · line 192

Quoted 5 lines

extras/mini-os/sched.c +5 -0

+                overflowing = 1;
+                printk("stack overflow\n");
+                BUG();
+            }
+        }

all this chunk: spurious changes?

On 18 June 2014 23:40, Julien Grall <[email protected]> wrote:
Hi Julien,

Quoted 11 lines

Quoted 11 lines

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

+void set_vtimer_compare(uint64_t value) {
+    uint32_t x, y;
+
+    DEBUG("New CompareValue : %llx\n", value);
+    x = 0xFFFFFFFFULL & value;
+    y = (value >> 32) & 0xFFFFFFFF;
+
+    __asm__ __volatile__("mcrr p15, 3, %0, %1, c14\n"
+            "isb"::"r"(x), "r"(y));

Quoted 8 lines

Quoted 5 lines

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

+
+    __asm__ __volatile__("mov %0, #0x1\n"
+            "mcr p15, 0, %0, c14, c3, 1\n" /* Enable timer and unmask the

output signal */
+ "isb":"=r"(x));

I don't think you need to add an isb per instruction. One should be enough.

Actually, do we need one at all? Setting the timer shouldn't affect the instruction pipline, I think.

Quoted 31 lines

[..]

Quoted 28 lines

extras/mini-os/drivers/gic.c new file +22 -0

new file mode 100644
index 0000000..3141830
@@ -0,0 +1,187 @@
+// ARM GIC implementation
+
+#include <mini-os/os.h>
+#include <mini-os/hypervisor.h>
+
+//#define VGIC_DEBUG
+#ifdef VGIC_DEBUG
+#define DEBUG(_f, _a...) \
+    DEBUG("MINI_OS(file=vgic.c, line=%d) " _f , __LINE__, ## _a)
+#else
+#define DEBUG(_f, _a...)    ((void)0)
+#endif
+
+extern void (*IRQ_handler)(void);
+
+struct gic {
+    volatile char *gicd_base;
+    volatile char *gicc_base;
+};
+
+static struct gic gic;
+

Quoted 8 lines

extras/mini-os/drivers/gic.c line 26

Quoted 4 lines

extras/mini-os/drivers/gic.c new file +4 -0

+// Distributor Interface
+#define GICD_CTLR        0x0
+#define GICD_ISENABLER    0x100
+#define GICD_PRIORITY    0x400

You use the right name for every macro except this one. I would rename it to GICD_IPRIORITYR to stay consistent.

Done.

Quoted 7 lines

extras/mini-os/drivers/gic.c line 55

Quoted 7 lines

extras/mini-os/drivers/gic.c new file +1 -0

+static inline void REG_WRITE32(volatile uint32_t *addr, unsigned int

value)

extras/mini-os/drivers/gic.c new file +5 -0

+{
+    __asm__ __volatile__("str %0, [%1]"::"r"(value), "r"(addr));
+    wmb();
+}
+

Do we need inline assembler to write these values? I'd imagine that a plain C store to a volatile 32-bit pointer would always do the same thing.

Quoted 7 lines

extras/mini-os/drivers/gic.c line 59

Quoted 3 lines

extras/mini-os/drivers/gic.c new file +3 -0

+{
+    uint32_t value;
+    value = REG_READ32(REG(gicd(gic, GICD_PRIORITY)) + irq_number);

There is 4 interrupts describe per register, this should be (irq_number & ~3).

Or should it be (irq_number >> 2)? REG casts to uint32_t.

Quoted 16 lines

extras/mini-os/drivers/gic.c line 68

Quoted 7 lines

extras/mini-os/drivers/gic.c new file +3 -0

+}
+
+static void gic_route_interrupt(struct gic *gic, unsigned char

irq_number, unsigned char cpu_set)

extras/mini-os/drivers/gic.c new file +3 -0

+{
+    uint32_t value;
+    value = REG_READ32(REG(gicd(gic, GICD_ITARGETSR)) + irq_number);

Same remark as gic_set_priority here.

extras/mini-os/drivers/gic.c

Quoted 2 lines

extras/mini-os/drivers/gic.c new file +2 -0

+    value &= ~(0xff << (8 * (irq_number & 0x3))); // set priority to '0'
+    value |= cpu_set << (8 * (irq_number & 0x3)); // add our priority

Quoted 7 lines

The comments are wrong in the 2 lines above.

Quoted 1 line

+ REG_WRITE32(REG(gicd(gic, GICD_ITARGETSR)) + irq_number, value);

irq_number & ~3;

The ARM Generic Interrupt Controller Architecture Specification says "These registers are byte-accessible.". So we should be able to write the byte directly. I thought this might work:

static void gic_route_interrupt(struct gic *gic, int irq_number, unsigned char cpu_set)

{
    gicd(gic, GICD_ITARGETSR)[irq_number] = cpu_set;
    wmb();
}

But it doesn't, because Xen's write_ignore handler (vgic.c:656 on the 4.4 branch) does:

write_ignore:
if ( dabt.size != 2 ) goto bad_width;

Bug?

Quoted 16 lines

extras/mini-os/drivers/gic.c

Quoted 10 lines

extras/mini-os/drivers/gic.c new file +4 -0

+}
+
+#define local_irq_save(x) { \
+    __asm__ __volatile__("mrs %0, cpsr;cpsid i; and %0, %0,

#0x80":"=r"(x)::"memory"); \

extras/mini-os/drivers/gic.c new file +5 -0

+}
+
+#define local_irq_restore(x) {    \
+    __asm__ __volatile__("msr cpsr_c, %0"::"r"(x):"memory");    \
+}

If you mask the result in local_irq_save, and use this value directly is local_irq_restore, you will disable by mistake the Asynchronous Abort exception....

Quoted 1 line

You have to remove the and %0... at the end.

This makes schedule() fail, because it checks whether the result is zero. Perhaps we should continue to mask it, but only apply the IRQ bit in local_irq_restore?

Quoted 11 lines

extras/mini-os/drivers/gic.c

Quoted 7 lines

extras/mini-os/drivers/gic.c new file +7 -0

+#define local_irq_disable()    __cli()
+#define local_irq_enable() __sti()
+
+#if defined(__arm__)
+#define mb() __asm__("dmb");
+#define rmb() __asm__("dmb");
+#define wmb() __asm__("dmb");

I suspect you want to dsb here. You want to make sure that every memory access has finished avoid executing new instruction until this is done.

Is this necessary? As I understand it, using Xen's ring system requires a strict ordering (dmb), but we don't need to stall the processor waiting for each write to complete before going on to the next one.

Quoted 5 lines

extras/mini-os/drivers/gic.c

Quoted 2 lines

extras/mini-os/drivers/gic.c new file +2 -0

+#define unlikely(x)  __builtin_expect((x),0)
+#define likely(x)  __builtin_expect((x),1)

Can't it be common with x86?

Which header file should it go in?

Quoted 11 lines

[..]

Quoted 8 lines

extras/mini-os/drivers/gic.c new file +1 -0

+#if defined(__i386__) || defined(__arm__)
 typedef long long           quad_t;
 typedef unsigned long long  u_quad_t;

@@ -40,7 +40,7 @@ typedef unsigned long       u_quad_t;
 typedef struct { unsigned long pte; } pte_t;
 #endif /* __i386__ || __x86_64__ */
 

Quoted 8 lines

extras/mini-os/drivers/gic.c typedef unsigned long u_quad_t; · line 44

Quoted 3 lines

extras/mini-os/drivers/gic.c new file +1 -1

-#ifdef __x86_64__
+#if defined(__x86_64__) || defined(__aarch64__)
 #define __pte(x) ((pte_t) { (x) } )

This looks wrong to me. The pte layout is exactly the same on arm32 and arm64. Hence, this is only used in the x86 code. Please either move pte_t in x86 part or define it correctly on ARM...

Moved to x86. Actually, this simplifies things further, because it can go in the separate hypercall-*.h files and lose the #ifdef completely.

Quoted 7 lines

Quoted 5 lines

extras/mini-os/include/arm/os.h line 70

Quoted 2 lines

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

+#define unlikely(x)  __builtin_expect((x),0)
+#define likely(x)  __builtin_expect((x),1)

Can't it be common with x86?

Which header file should it go in?

I don't know much mini-os, but I would add either in lib.h or create a new header compiler.h.

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

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