[PATCH ARM v8 0/4] mini-os: initial ARM support

[PATCH ARM v8 0/4] mini-os: initial ARM support

[Thomas Leonard]

This series is based on Karim's ARM support commits, further broken up into
smaller patches.

Changes since v7:

- removed unnecessary isb
- removed some debug comments
- simplified some of the asm calls
- updated libfdt_env for new libfdt version

Thomas Leonard (4):
  mini-os: arm: time
  mini-os: arm: interrupt controller
  mini-os: arm: build system
  mini-os: arm: show registers, stack and exception vector on fault

 .gitignore                          |   3 +
 extras/mini-os/ARM-TODO.txt         |   4 +
 extras/mini-os/COPYING              |  27 ++++
 extras/mini-os/Config.mk            |   2 +
 extras/mini-os/Makefile             |  38 +++++-
 extras/mini-os/arch/arm/Makefile    |  32 +++++
 extras/mini-os/arch/arm/arch.mk     |   7 ++
 extras/mini-os/arch/arm/arm32.S     |  75 ++++++++++--
 extras/mini-os/arch/arm/gic.c       | 237 ++++++++++++++++++++++++++++++++++++
 extras/mini-os/arch/arm/panic.c     |  98 +++++++++++++++
 extras/mini-os/arch/arm/time.c      | 136 +++++++++++++++++++++
 extras/mini-os/include/lib.h        |   4 +-
 extras/mini-os/include/libfdt_env.h |  33 +++++
 extras/mini-os/lib/memmove.c        |  45 +++++++
 extras/mini-os/lib/string.c         |  12 ++
 15 files changed, 743 insertions(+), 10 deletions(-)
 create mode 100644 extras/mini-os/ARM-TODO.txt
 create mode 100755 extras/mini-os/arch/arm/Makefile
 create mode 100644 extras/mini-os/arch/arm/arch.mk
 create mode 100644 extras/mini-os/arch/arm/gic.c
 create mode 100644 extras/mini-os/arch/arm/panic.c
 create mode 100644 extras/mini-os/arch/arm/time.c
 create mode 100644 extras/mini-os/include/libfdt_env.h
 create mode 100644 extras/mini-os/lib/memmove.c

-- 
2.1.1

[PATCH ARM v8 1/4] mini-os: arm: time

[Thomas Leonard]

Based on an initial patch by Karim Raslan.

Signed-off-by: Karim Allah Ahmed <karim.allah.ahmed@gmail.com>
Signed-off-by: Thomas Leonard <talex5@gmail.com>

---

Changes since v7:

Addressed Ian Campbell's comments:

- removed unnecessary isb
- removed some debug comments
- simplified some of the asm calls
---
 extras/mini-os/arch/arm/time.c | 136 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 136 insertions(+)
 create mode 100644 extras/mini-os/arch/arm/time.c

diff --git a/extras/mini-os/arch/arm/time.c b/extras/mini-os/arch/arm/time.c
new file mode 100644
index 0000000..a088981
--- /dev/null
+++ b/extras/mini-os/arch/arm/time.c
@@ -0,0 +1,136 @@
+#include <mini-os/os.h>
+#include <mini-os/hypervisor.h>
+#include <mini-os/events.h>
+#include <mini-os/traps.h>
+#include <mini-os/types.h>
+#include <mini-os/time.h>
+#include <mini-os/lib.h>
+
+//#define VTIMER_DEBUG
+#ifdef VTIMER_DEBUG
+#define DEBUG(_f, _a...) \
+    printk("MINI_OS(file=vtimer.c, line=%d) " _f , __LINE__, ## _a)
+#else
+#define DEBUG(_f, _a...)    ((void)0)
+#endif
+
+/************************************************************************
+ * Time functions
+ *************************************************************************/
+
+static uint64_t cntvct_at_init;
+static uint32_t counter_freq;
+
+/* Compute with 96 bit intermediate result: (a*b)/c */
+uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
+{
+    union {
+        uint64_t ll;
+        struct {
+            uint32_t low, high;
+        } l;
+    } u, res;
+    uint64_t rl, rh;
+
+    u.ll = a;
+    rl = (uint64_t)u.l.low * (uint64_t)b;
+    rh = (uint64_t)u.l.high * (uint64_t)b;
+    rh += (rl >> 32);
+    res.l.high = rh / c;
+    res.l.low = (((rh % c) << 32) + (rl & 0xffffffff)) / c;
+    return res.ll;
+}
+
+static inline s_time_t ticks_to_ns(uint64_t ticks)
+{
+    return muldiv64(ticks, SECONDS(1), counter_freq);
+}
+
+static inline uint64_t ns_to_ticks(s_time_t ns)
+{
+    return muldiv64(ns, counter_freq, SECONDS(1));
+}
+
+/* Wall-clock time is not currently available on ARM, so this is always zero for now:
+ * http://wiki.xenproject.org/wiki/Xen_ARM_TODO#Expose_Wallclock_time_to_guests
+ */
+static struct timespec shadow_ts;
+
+static inline uint64_t read_virtual_count(void)
+{
+    uint32_t c_lo, c_hi;
+    __asm__ __volatile__("mrrc p15, 1, %0, %1, c14":"=r"(c_lo), "=r"(c_hi));
+    return (((uint64_t) c_hi) << 32) + c_lo;
+}
+
+/* monotonic_clock(): returns # of nanoseconds passed since time_init()
+ *        Note: This function is required to return accurate
+ *        time even in the absence of multiple timer ticks.
+ */
+uint64_t monotonic_clock(void)
+{
+    return ticks_to_ns(read_virtual_count() - cntvct_at_init);
+}
+
+int gettimeofday(struct timeval *tv, void *tz)
+{
+    uint64_t nsec = monotonic_clock();
+    nsec += shadow_ts.tv_nsec;
+
+    tv->tv_sec = shadow_ts.tv_sec;
+    tv->tv_sec += NSEC_TO_SEC(nsec);
+    tv->tv_usec = NSEC_TO_USEC(nsec % 1000000000UL);
+
+    return 0;
+}
+
+/* Set the timer and mask. */
+void write_timer_ctl(uint32_t value) {
+    __asm__ __volatile__(
+            "mcr p15, 0, %0, c14, c3, 1\n"
+            "isb"::"r"(value));
+}
+
+void set_vtimer_compare(uint64_t value) {
+    DEBUG("New CompareValue : %llx\n", value);
+
+    __asm__ __volatile__("mcrr p15, 3, %0, %H0, c14"
+            ::"r"(value));
+
+    /* Enable timer and unmask the output signal */
+    write_timer_ctl(1);
+}
+
+void unset_vtimer_compare(void) {
+    /* Disable timer and mask the output signal */
+    write_timer_ctl(2);
+}
+
+void block_domain(s_time_t until)
+{
+    uint64_t until_count = ns_to_ticks(until) + cntvct_at_init;
+    ASSERT(irqs_disabled());
+    if (read_virtual_count() < until_count)
+    {
+        set_vtimer_compare(until_count);
+        __asm__ __volatile__("wfi");
+        unset_vtimer_compare();
+
+        /* Give the IRQ handler a chance to handle whatever woke us up. */
+        local_irq_enable();
+        local_irq_disable();
+    }
+}
+
+void init_time(void)
+{
+    printk("Initialising timer interface\n");
+
+    __asm__ __volatile__("mrc p15, 0, %0, c14, c0, 0":"=r"(counter_freq));
+    cntvct_at_init = read_virtual_count();
+    printk("Virtual Count register is %llx, freq = %d Hz\n", cntvct_at_init, counter_freq);
+}
+
+void fini_time(void)
+{
+}
-- 
2.1.1

[PATCH ARM v8 2/4] mini-os: arm: interrupt controller

[Thomas Leonard]

Based on an initial patch by Karim Raslan.

Signed-off-by: Karim Allah Ahmed <karim.allah.ahmed@gmail.com>
Signed-off-by: Thomas Leonard <talex5@gmail.com>
---
 extras/mini-os/arch/arm/gic.c | 237 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 237 insertions(+)
 create mode 100644 extras/mini-os/arch/arm/gic.c

diff --git a/extras/mini-os/arch/arm/gic.c b/extras/mini-os/arch/arm/gic.c
new file mode 100644
index 0000000..6283ed5
--- /dev/null
+++ b/extras/mini-os/arch/arm/gic.c
@@ -0,0 +1,237 @@
+// ARM GIC implementation
+
+#include <mini-os/os.h>
+#include <mini-os/hypervisor.h>
+#include <mini-os/console.h>
+#include <libfdt.h>
+
+//#define VGIC_DEBUG
+#ifdef VGIC_DEBUG
+#define DEBUG(_f, _a...) \
+    printk("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;
+
+// Distributor Interface
+#define GICD_CTLR        0x0
+#define GICD_ISENABLER    0x100
+#define GICD_IPRIORITYR   0x400
+#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;
+}
+
+static inline void REG_WRITE32(volatile uint32_t *addr, unsigned int value)
+{
+    __asm__ __volatile__("str %0, [%1]"::"r"(value), "r"(addr));
+    wmb();
+}
+
+static void gic_set_priority(struct gic *gic, int irq_number, unsigned char priority)
+{
+    uint32_t value;
+    uint32_t *addr = REG(gicd(gic, GICD_IPRIORITYR)) + (irq_number >> 2);
+    value = REG_READ32(addr);
+    value &= ~(0xff << (8 * (irq_number & 0x3))); // clear old priority
+    value |= priority << (8 * (irq_number & 0x3)); // set new priority
+    REG_WRITE32(addr, value);
+}
+
+static void gic_route_interrupt(struct gic *gic, int irq_number, unsigned char cpu_set)
+{
+    uint32_t value;
+    uint32_t *addr = REG(gicd(gic, GICD_ITARGETSR)) + (irq_number >> 2);
+    value = REG_READ32(addr);
+    value &= ~(0xff << (8 * (irq_number & 0x3))); // clear old target
+    value |= cpu_set << (8 * (irq_number & 0x3)); // set new target
+    REG_WRITE32(addr, value);
+}
+
+/* When accessing the GIC registers, we can't use LDREX/STREX because it's not regular memory. */
+static __inline__ void clear_bit_non_atomic(int nr, volatile void *base)
+{
+    volatile uint32_t *tmp = base;
+    tmp[nr >> 5] &= (unsigned long)~(1 << (nr & 0x1f));
+}
+
+static __inline__ void set_bit_non_atomic(int nr, volatile void *base)
+{
+    volatile uint32_t *tmp = base;
+    tmp[nr >> 5] |= (1 << (nr & 0x1f));
+}
+
+/* Note: not thread safe (but we only support one CPU for now anyway) */
+static void gic_enable_interrupt(struct gic *gic, int irq_number,
+        unsigned char cpu_set, unsigned char level_sensitive, unsigned char ppi)
+{
+    void *set_enable_reg;
+    void *cfg_reg;
+
+    // set priority
+    gic_set_priority(gic, irq_number, 0x0);
+
+    // set target cpus for this interrupt
+    gic_route_interrupt(gic, irq_number, cpu_set);
+
+    // set level/edge triggered
+    cfg_reg = (void *)gicd(gic, GICD_ICFGR);
+    if (level_sensitive) {
+        clear_bit_non_atomic((irq_number * 2) + 1, cfg_reg);
+    } else {
+        set_bit_non_atomic((irq_number * 2) + 1, cfg_reg);
+    }
+    if (ppi)
+        clear_bit_non_atomic((irq_number * 2), cfg_reg);
+
+    wmb();
+
+    // enable forwarding interrupt from distributor to cpu interface
+    set_enable_reg = (void *)gicd(gic, GICD_ISENABLER);
+    set_bit_non_atomic(irq_number, set_enable_reg);
+    wmb();
+}
+
+static void gic_enable_interrupts(struct gic *gic)
+{
+    // Global enable forwarding interrupts from distributor to cpu interface
+    REG_WRITE32(REG(gicd(gic, GICD_CTLR)), 0x00000001);
+
+    // Global enable signalling of interrupt from the cpu interface
+    REG_WRITE32(REG(gicc(gic, GICC_CTLR)), 0x00000001);
+}
+
+static void gic_disable_interrupts(struct gic *gic)
+{
+    // Global disable signalling of interrupt from the cpu interface
+    REG_WRITE32(REG(gicc(gic, GICC_CTLR)), 0x00000000);
+
+    // Global disable forwarding interrupts from distributor to cpu interface
+    REG_WRITE32(REG(gicd(gic, GICD_CTLR)), 0x00000000);
+}
+
+static void gic_cpu_set_priority(struct gic *gic, char priority)
+{
+    REG_WRITE32(REG(gicc(gic, GICC_PMR)), priority & 0x000000FF);
+}
+
+static unsigned long gic_readiar(struct gic *gic) {
+    return REG_READ32(REG(gicc(gic, GICC_IAR))) & 0x000003FF; // Interrupt ID
+}
+
+static void gic_eoir(struct gic *gic, uint32_t irq) {
+    REG_WRITE32(REG(gicc(gic, GICC_EOIR)), irq & 0x000003FF);
+}
+
+//FIXME Get event_irq from dt
+#define EVENTS_IRQ 31
+#define VIRTUALTIMER_IRQ 27
+
+static void gic_handler(void) {
+    unsigned int irq = gic_readiar(&gic);
+
+    DEBUG("IRQ received : %i\n", irq);
+    switch(irq) {
+    case EVENTS_IRQ:
+        do_hypervisor_callback(NULL);
+        break;
+    case VIRTUALTIMER_IRQ:
+        /* We need to get this event to wake us up from block_domain,
+         * but we don't need to do anything special with it. */
+        break;
+    default:
+        DEBUG("Unhandled irq\n");
+        break;
+    }
+
+    DEBUG("EIRQ\n");
+
+    gic_eoir(&gic, irq);
+}
+
+void gic_init(void) {
+    gic.gicd_base = NULL;
+    int node = 0;
+    int depth = 0;
+    for (;;)
+    {
+        node = fdt_next_node(device_tree, node, &depth);
+        if (node <= 0 || depth < 0)
+            break;
+
+        if (fdt_getprop(device_tree, node, "interrupt-controller", NULL)) {
+            int len = 0;
+
+            if (fdt_node_check_compatible(device_tree, node, "arm,cortex-a15-gic") &&
+                fdt_node_check_compatible(device_tree, node, "arm,cortex-a7-gic")) {
+                printk("Skipping incompatible interrupt-controller node\n");
+                continue;
+            }
+
+            const uint64_t *reg = fdt_getprop(device_tree, node, "reg", &len);
+
+            /* We have two registers (GICC and GICD), each of which contains
+             * two parts (an address and a size), each of which is a 64-bit
+             * value (8 bytes), so we expect a length of 2 * 2 * 8 = 32.
+             * If any extra values are passed in future, we ignore them. */
+            if (reg == NULL || len < 32) {
+                printk("Bad 'reg' property: %p %d\n", reg, len);
+                continue;
+            }
+
+            gic.gicd_base = to_virt((long) fdt64_to_cpu(reg[0]));
+            gic.gicc_base = to_virt((long) fdt64_to_cpu(reg[2]));
+            printk("Found GIC: gicd_base = %p, gicc_base = %p\n", gic.gicd_base, gic.gicc_base);
+            break;
+        }
+    }
+    if (!gic.gicd_base) {
+        printk("GIC not found!\n");
+        BUG();
+    }
+    wmb();
+
+    /* Note: we could mark this as "device" memory here, but Xen will have already
+     * set it that way in the second stage translation table, so it's not necessary.
+     * See "Overlaying the memory type attribute" in the Architecture Reference Manual.
+     */
+
+    IRQ_handler = gic_handler;
+
+    gic_disable_interrupts(&gic);
+    gic_cpu_set_priority(&gic, 0xff);
+
+    /* Must call gic_enable_interrupts before enabling individual interrupts, otherwise our IRQ handler
+     * gets called endlessly with spurious interrupts. */
+    gic_enable_interrupts(&gic);
+
+    gic_enable_interrupt(&gic, EVENTS_IRQ /* interrupt number */, 0x1 /*cpu_set*/, 1 /*level_sensitive*/, 0 /* ppi */);
+    gic_enable_interrupt(&gic, VIRTUALTIMER_IRQ /* interrupt number */, 0x1 /*cpu_set*/, 1 /*level_sensitive*/, 1 /* ppi */);
+}
-- 
2.1.1

[PATCH ARM v8 3/4] mini-os: arm: build system

[Thomas Leonard]

Based on an initial patch by Karim Raslan.

This activates the ARM code added in the previous patches. 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

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

Signed-off-by: Karim Allah Ahmed <karim.allah.ahmed@gmail.com>
Signed-off-by: Thomas Leonard <talex5@gmail.com>

---

Changes since v7:

- updated libfdt_env for new libfdt version
---
 .gitignore                          |  3 +++
 extras/mini-os/ARM-TODO.txt         |  5 +++++
 extras/mini-os/COPYING              | 27 ++++++++++++++++++++++
 extras/mini-os/Config.mk            |  2 ++
 extras/mini-os/Makefile             | 38 +++++++++++++++++++++++++++++--
 extras/mini-os/arch/arm/Makefile    | 32 ++++++++++++++++++++++++++
 extras/mini-os/arch/arm/arch.mk     |  7 ++++++
 extras/mini-os/include/lib.h        |  4 +++-
 extras/mini-os/include/libfdt_env.h | 33 +++++++++++++++++++++++++++
 extras/mini-os/lib/memmove.c        | 45 +++++++++++++++++++++++++++++++++++++
 extras/mini-os/lib/string.c         | 12 ++++++++++
 11 files changed, 205 insertions(+), 3 deletions(-)
 create mode 100644 extras/mini-os/ARM-TODO.txt
 create mode 100755 extras/mini-os/arch/arm/Makefile
 create mode 100644 extras/mini-os/arch/arm/arch.mk
 create mode 100644 extras/mini-os/include/libfdt_env.h
 create mode 100644 extras/mini-os/lib/memmove.c

diff --git a/.gitignore b/.gitignore
index 67ed7cb..6e4182e 100644
--- a/.gitignore
+++ b/.gitignore
@@ -50,6 +50,9 @@ extras/mini-os/include/mini-os
 extras/mini-os/include/x86/mini-os
 extras/mini-os/include/xen
 extras/mini-os/include/list.h
+extras/mini-os/include/fdt.h
+extras/mini-os/include/libfdt.h
+extras/mini-os/libfdt
 extras/mini-os/mini-os*
 install/*
 linux-[^/]*-paravirt/*
diff --git a/extras/mini-os/ARM-TODO.txt b/extras/mini-os/ARM-TODO.txt
new file mode 100644
index 0000000..3226d39
--- /dev/null
+++ b/extras/mini-os/ARM-TODO.txt
@@ -0,0 +1,5 @@
+* support abort exception handling ( and others )
+* gic request_irq implementation, currently all IRQs all hardcoded in gic irq handler.
+* bind_*
+* add multiple cpu support (?)
+* map_frames
diff --git a/extras/mini-os/COPYING b/extras/mini-os/COPYING
index 1d9df6c..b676bb6 100644
--- a/extras/mini-os/COPYING
+++ b/extras/mini-os/COPYING
@@ -34,3 +34,30 @@ LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
 OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
 SUCH DAMAGE.
 
+
+Copyright (c) 2005,2006, NLnetLabs
+All rights reserved.
+
+Redistribution and use in source and binary forms, with or without
+modification, are permitted provided that the following conditions are met:
+
+    * Redistributions of source code must retain the above copyright notice,
+      this list of conditions and the following disclaimer.
+    * Redistributions in binary form must reproduce the above copyright
+      notice, this list of conditions and the following disclaimer in the
+      documentation and/or other materials provided with the distribution.
+    * Neither the name of NLnetLabs nor the names of its
+      contributors may be used to endorse or promote products derived from this
+      software without specific prior written permission.
+
+THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
+AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE
+LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
+POSSIBILITY OF SUCH DAMAGE.
diff --git a/extras/mini-os/Config.mk b/extras/mini-os/Config.mk
index 4852443..b3e8b90 100644
--- a/extras/mini-os/Config.mk
+++ b/extras/mini-os/Config.mk
@@ -12,6 +12,8 @@ export XEN_INTERFACE_VERSION
 # If not x86 then use $(XEN_TARGET_ARCH)
 ifeq ($(findstring x86_,$(XEN_TARGET_ARCH)),x86_)
 TARGET_ARCH_FAM = x86
+else ifeq ($(findstring arm,$(XEN_TARGET_ARCH)),arm)
+TARGET_ARCH_FAM = arm
 else
 TARGET_ARCH_FAM = $(XEN_TARGET_ARCH)
 endif
diff --git a/extras/mini-os/Makefile b/extras/mini-os/Makefile
index 6d6537e..77dc9aa 100644
--- a/extras/mini-os/Makefile
+++ b/extras/mini-os/Makefile
@@ -51,7 +51,7 @@ 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
+GENERATED_HEADERS := include/list.h $(ARCH_LINKS) include/mini-os include/xen include/$(TARGET_ARCH_FAM)/mini-os include/fdt.h include/libfdt.h
 
 EXTRA_DEPS += $(GENERATED_HEADERS)
 
@@ -75,7 +75,18 @@ EXTRA_OBJS =
 TARGET := mini-os
 
 # Subdirectories common to mini-os
-SUBDIRS := lib xenbus console
+SUBDIRS := lib xenbus console libfdt
+
+FDT_SRC :=
+ifeq ($(XEN_TARGET_ARCH),arm32)
+# Need libgcc.a for division helpers
+LDLIBS += `$(CC) -print-libgcc-file-name`
+
+# Device tree support
+FDT_SRC := libfdt/fdt.c libfdt/fdt_ro.c libfdt/fdt_strerror.c
+
+src-y += ${FDT_SRC}
+endif
 
 src-$(CONFIG_BLKFRONT) += blkfront.c
 src-$(CONFIG_TPMFRONT) += tpmfront.c
@@ -97,10 +108,13 @@ src-y += sched.c
 src-$(CONFIG_TEST) += test.c
 
 src-y += lib/ctype.c
+ifneq ($(XEN_TARGET_ARCH),arm32)
 src-y += lib/math.c
+endif
 src-y += lib/printf.c
 src-y += lib/stack_chk_fail.c
 src-y += lib/string.c
+src-y += lib/memmove.c
 src-y += lib/sys.c
 src-y += lib/xmalloc.c
 src-$(CONFIG_XENBUS) += lib/xs.c
@@ -125,6 +139,21 @@ $(ARCH_LINKS):
 	$(arch_links)
 endif
 
+include/fdt.h:
+	cp $(XEN_ROOT)/xen/include/xen/libfdt/fdt.h $@
+
+include/libfdt.h:
+	sed 's!xen/libfdt/!!' $(XEN_ROOT)/xen/include/xen/libfdt/libfdt.h > $@
+
+libfdt:
+	mkdir $@
+
+libfdt/libfdt_internal.h: libfdt
+	cp $(XEN_ROOT)/xen/common/libfdt/libfdt_internal.h $@
+
+libfdt/%.c: libfdt libfdt/libfdt_internal.h
+	cp $(XEN_ROOT)/xen/common/libfdt/$*.c $@
+
 include/list.h: $(XEN_ROOT)/tools/include/xen-external/bsd-sys-queue-h-seddery $(XEN_ROOT)/tools/include/xen-external/bsd-sys-queue.h
 	perl $^ --prefix=minios  >$@.new
 	$(call move-if-changed,$@.new,$@)
@@ -190,7 +219,11 @@ $(OBJ_DIR)/$(TARGET): $(OBJS) $(APP_O) arch_lib
 	$(LD) -r $(LDFLAGS) $(HEAD_OBJ) $(APP_O) $(OBJS) $(LDARCHLIB) $(LDLIBS) -o $@.o
 	$(OBJCOPY) -w -G $(GLOBAL_PREFIX)* -G _start $@.o $@.o
 	$(LD) $(LDFLAGS) $(LDFLAGS_FINAL) $@.o $(EXTRA_OBJS) -o $@
+ifeq ($(XEN_TARGET_ARCH),arm32)
+	$(OBJCOPY) -O binary $@ $@.img
+else
 	gzip -f -9 -c $@ >$@.gz
+endif
 
 .PHONY: clean arch_clean
 
@@ -202,6 +235,7 @@ clean:	arch_clean
 		rm -f $$dir/*.o; \
 	done
 	rm -f include/list.h
+	rm -f ${FDT_SRC} libfdt/libfdt_internal.h
 	rm -f $(OBJ_DIR)/*.o *~ $(OBJ_DIR)/core $(OBJ_DIR)/$(TARGET).elf $(OBJ_DIR)/$(TARGET).raw $(OBJ_DIR)/$(TARGET) $(OBJ_DIR)/$(TARGET).gz
 	find . $(OBJ_DIR) -type l | xargs rm -f
 	$(RM) $(OBJ_DIR)/lwip.a $(LWO)
diff --git a/extras/mini-os/arch/arm/Makefile b/extras/mini-os/arch/arm/Makefile
new file mode 100755
index 0000000..8b78651
--- /dev/null
+++ b/extras/mini-os/arch/arm/Makefile
@@ -0,0 +1,32 @@
+#
+# ARM architecture specific makefiles.
+#
+
+XEN_ROOT = $(CURDIR)/../../../..
+include $(XEN_ROOT)/Config.mk
+include ../../Config.mk
+
+# include arch.mk has to be before minios.mk!
+
+include arch.mk
+include ../../minios.mk
+
+# Sources here are all *.c (without $(XEN_TARGET_ARCH).S)
+# This is handled in $(HEAD_ARCH_OBJ)
+ARCH_SRCS := $(wildcard *.c)
+
+# The objects built from the sources.
+ARCH_OBJS := $(patsubst %.c,$(OBJ_DIR)/%.o,$(ARCH_SRCS))
+
+ARCH_OBJS += hypercalls32.o
+
+all: $(OBJ_DIR)/$(ARCH_LIB)
+
+# $(HEAD_ARCH_OBJ) is only built here, needed on linking
+# in ../../Makefile.
+$(OBJ_DIR)/$(ARCH_LIB): $(ARCH_OBJS) $(OBJ_DIR)/$(HEAD_ARCH_OBJ)
+	$(AR) rv $(OBJ_DIR)/$(ARCH_LIB) $(ARCH_OBJS)
+
+clean:
+	rm -f $(OBJ_DIR)/$(ARCH_LIB) $(ARCH_OBJS) $(OBJ_DIR)/$(HEAD_ARCH_OBJ)
+
diff --git a/extras/mini-os/arch/arm/arch.mk b/extras/mini-os/arch/arm/arch.mk
new file mode 100644
index 0000000..ad6e05f
--- /dev/null
+++ b/extras/mini-os/arch/arm/arch.mk
@@ -0,0 +1,7 @@
+ifeq ($(XEN_TARGET_ARCH),arm32)
+DEF_ASFLAGS += -march=armv7-a -mfpu=vfpv3
+ARCH_CFLAGS  := -march=armv7-a -marm -fms-extensions -D__arm__ -DXEN_HAVE_PV_GUEST_ENTRY #-DCPU_EXCLUSIVE_LDST
+EXTRA_INC += $(TARGET_ARCH_FAM)/$(XEN_TARGET_ARCH)
+EXTRA_SRC += arch/$(EXTRA_INC)
+endif
+
diff --git a/extras/mini-os/include/lib.h b/extras/mini-os/include/lib.h
index 62836c7..326e39f 100644
--- a/extras/mini-os/include/lib.h
+++ b/extras/mini-os/include/lib.h
@@ -95,6 +95,7 @@ char	*strncpy(char * __restrict, const char * __restrict, size_t);
 
 char	*strstr(const char *, const char *);
 
+void *memmove(void *, const void *, size_t);
 void *memset(void *, int, size_t);
 
 char *strchr(const char *p, int ch);
@@ -104,7 +105,8 @@ char *strrchr(const char *p, int ch);
  *	@(#)systm.h	8.7 (Berkeley) 3/29/95
  * $FreeBSD$
  */
-void	*memcpy(void *to, const void *from, size_t len);
+void *memcpy(void *to, const void *from, size_t len);
+void *memchr(const void *s, int c, size_t n);
 
 size_t strnlen(const char *, size_t);
 #endif
diff --git a/extras/mini-os/include/libfdt_env.h b/extras/mini-os/include/libfdt_env.h
new file mode 100644
index 0000000..722fac6
--- /dev/null
+++ b/extras/mini-os/include/libfdt_env.h
@@ -0,0 +1,33 @@
+#ifndef _LIBFDT_ENV_H
+#define _LIBFDT_ENV_H
+
+#include <stddef.h>
+#include <stdint.h>
+#include <lib.h>
+
+typedef uint16_t fdt16_t;
+typedef uint32_t fdt32_t;
+typedef uint64_t fdt64_t;
+
+#define EXTRACT_BYTE(n) ((unsigned long long)((uint8_t *)&x)[n])
+static inline uint16_t fdt16_to_cpu(uint16_t x)
+{
+    return (EXTRACT_BYTE(0) << 8) | EXTRACT_BYTE(1);
+}
+#define cpu_to_fdt16(x) fdt16_to_cpu(x)
+
+static inline uint32_t fdt32_to_cpu(uint32_t x)
+{
+    return (EXTRACT_BYTE(0) << 24) | (EXTRACT_BYTE(1) << 16) | (EXTRACT_BYTE(2) << 8) | EXTRACT_BYTE(3);
+}
+#define cpu_to_fdt32(x) fdt32_to_cpu(x)
+
+static inline uint64_t fdt64_to_cpu(uint64_t x)
+{
+    return (EXTRACT_BYTE(0) << 56) | (EXTRACT_BYTE(1) << 48) | (EXTRACT_BYTE(2) << 40) | (EXTRACT_BYTE(3) << 32)
+            | (EXTRACT_BYTE(4) << 24) | (EXTRACT_BYTE(5) << 16) | (EXTRACT_BYTE(6) << 8) | EXTRACT_BYTE(7);
+}
+#define cpu_to_fdt64(x) fdt64_to_cpu(x)
+#undef EXTRACT_BYTE
+
+#endif /* _LIBFDT_ENV_H */
diff --git a/extras/mini-os/lib/memmove.c b/extras/mini-os/lib/memmove.c
new file mode 100644
index 0000000..0298b7c
--- /dev/null
+++ b/extras/mini-os/lib/memmove.c
@@ -0,0 +1,45 @@
+/*
+ *	memmove.c: memmove compat implementation.
+ *
+ *	Copyright (c) 2001-2008, NLnet Labs. All rights reserved.
+ *
+ * See COPYING for the license.
+*/
+
+#include <os.h>
+#include <mini-os/lib.h>
+
+#ifndef HAVE_LIBC
+
+void *memmove(void *dest, const void *src, size_t n)
+{
+	uint8_t* from = (uint8_t*) src;
+	uint8_t* to = (uint8_t*) dest;
+
+	if (from == to || n == 0)
+		return dest;
+	if (to > from && to-from < (int)n) {
+		/* to overlaps with from */
+		/*  <from......>         */
+		/*         <to........>  */
+		/* copy in reverse, to avoid overwriting from */
+		int i;
+		for(i=n-1; i>=0; i--)
+			to[i] = from[i];
+		return dest;
+	}
+	if (from > to  && from-to < (int)n) {
+		/* to overlaps with from */
+		/*        <from......>   */
+		/*  <to........>         */
+		/* copy forwards, to avoid overwriting from */
+		size_t i;
+		for(i=0; i<n; i++)
+			to[i] = from[i];
+		return dest;
+	}
+	memcpy(dest, src, n);
+	return dest;
+}
+
+#endif
diff --git a/extras/mini-os/lib/string.c b/extras/mini-os/lib/string.c
index 8b24146..c96ca41 100644
--- a/extras/mini-os/lib/string.c
+++ b/extras/mini-os/lib/string.c
@@ -225,4 +225,16 @@ int ffs(int i)
    return 0;
 }
 
+void *memchr(const void *s, int c, size_t n)
+{
+    if (n != 0) {
+        const unsigned char *p = s;
+
+        do {
+            if (*p++ == (unsigned char)c)
+                return ((void *)(uintptr_t)(p - 1));
+        } while (--n != 0);
+    }
+    return (NULL);
+}
 #endif
-- 
2.1.1

[PATCH ARM v8 4/4] mini-os: arm: show registers, stack and exception vector on fault

[Thomas Leonard]

Signed-off-by: Thomas Leonard <talex5@gmail.com>
Acked-by: Ian Campbell <Ian.Campbell@citrix.com>
---
 extras/mini-os/ARM-TODO.txt     |  1 -
 extras/mini-os/arch/arm/arm32.S | 75 ++++++++++++++++++++++++++++---
 extras/mini-os/arch/arm/panic.c | 98 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 166 insertions(+), 8 deletions(-)
 create mode 100644 extras/mini-os/arch/arm/panic.c

diff --git a/extras/mini-os/ARM-TODO.txt b/extras/mini-os/ARM-TODO.txt
index 3226d39..1e40db1 100644
--- a/extras/mini-os/ARM-TODO.txt
+++ b/extras/mini-os/ARM-TODO.txt
@@ -1,4 +1,3 @@
-* support abort exception handling ( and others )
 * gic request_irq implementation, currently all IRQs all hardcoded in gic irq handler.
 * bind_*
 * add multiple cpu support (?)
diff --git a/extras/mini-os/arch/arm/arm32.S b/extras/mini-os/arch/arm/arm32.S
index 73223c8..a08a170 100644
--- a/extras/mini-os/arch/arm/arm32.S
+++ b/extras/mini-os/arch/arm/arm32.S
@@ -162,8 +162,69 @@ irqstack:
 	.fill (1024), 4, 0x0
 irqstack_end:
 
+fault_dump:
+	.fill 18, 4, 0x0		@ On fault, we save the registers + CPSR + handler address
+
 .popsection
 
+fault:
+	cpsid	aif			@ Disable interrupts
+
+	ldr	r13, =fault_dump
+	stmia	r13, {r0-r12}		@ Dump the non-banked registers directly (well, unless from FIQ mode)
+	str	r14, [r13, #15 << 2]	@ Our r14 is the faulting r15
+	mov	r0, r13
+
+	@ Save the caller's CPSR (our SPSR) too.
+	mrs	r1, SPSR
+	str	r1, [r13, #16 << 2]
+
+	@ Switch to the mode we came from to get r13 and r14.
+	@ If coming from user mode, use System mode instead so we're still
+	@ privileged.
+	and	r1, r1, #0x1f		@ r1 = SPSR mode
+	cmp	r1, #0x10		@ If from User mode
+	moveq	r1, #0x1f		@ Then use System mode instead
+
+	mrs	r3, CPSR		@ r3 = our CPSR
+	bic	r2, r3, #0x1f
+	orr	r2, r2, r1
+	msr	CPSR, r2		@ Change to mode r1
+
+	@ Save old mode's r13, r14
+	str	r13, [r0, #13 << 2]
+	str	r14, [r0, #14 << 2]
+
+	msr	CPSR, r3		@ Back to fault mode
+
+	ldr	r1, [r0, #17 << 2]
+	sub	r1, r1, #12		@ Fix to point at start of handler
+	str	r1, [r0, #17 << 2]
+
+	@ Call C code to format the register dump.
+	@ Clobbers the stack, but we're not going to return anyway.
+	ldr	sp, =_boot_stack_end
+	bl	dump_registers
+	b	do_exit
+
+@ We want to store a unique value to identify this handler, without corrupting
+@ any of the registers. So, we store r15 (which will point just after the branch).
+@ Later, we subtract 12 so the user gets pointed at the start of the exception
+@ handler.
+#define FAULT(name)			\
+.globl fault_##name;			\
+fault_##name:				\
+	ldr	r13, =fault_dump;	\
+	str	r15, [r13, #17 << 2];	\
+	b	fault
+
+FAULT(reset)
+FAULT(undefined_instruction)
+FAULT(svc)
+FAULT(prefetch_call)
+FAULT(prefetch_abort)
+FAULT(data_abort)
+
 @ exception base address
 .align 5
 .globl exception_vector_table
@@ -173,13 +234,13 @@ irqstack_end:
 @  instruction to clear an existing tag is required on context switches."
 @ -- ARM Cortex-A Series Programmer’s Guide (Version: 4.0)
 exception_vector_table:
-	b	. @ reset
-	b	. @ undefined instruction
-	b	. @ supervisor call
-	b	. @ prefetch call
-	b	. @ prefetch abort
-	b	. @ data abort
-	b	irq_handler @ irq
+	b	fault_reset
+	b	fault_undefined_instruction
+	b	fault_svc
+	b	fault_prefetch_call
+	b	fault_prefetch_abort
+	b	fault_data_abort
+	b	irq_handler @ IRQ
 	.word 0xe7f000f0    @ abort on FIQ
 
 @ Call fault_undefined_instruction in "Undefined mode"
diff --git a/extras/mini-os/arch/arm/panic.c b/extras/mini-os/arch/arm/panic.c
new file mode 100644
index 0000000..a049d05
--- /dev/null
+++ b/extras/mini-os/arch/arm/panic.c
@@ -0,0 +1,98 @@
+/******************************************************************************
+ * panic.c
+ *
+ * Displays a register dump and stack trace for debugging.
+ *
+ * Copyright (c) 2014, Thomas Leonard
+ * 
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
+ * deal in the Software without restriction, including without limitation the
+ * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ * 
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ * 
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR 
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, 
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE 
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER 
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER 
+ * DEALINGS IN THE SOFTWARE.
+ */
+
+#include <mini-os/os.h>
+#include <mini-os/console.h>
+#include <arch_mm.h>
+
+extern int irqstack[];
+extern int irqstack_end[];
+
+typedef void handler(void);
+
+extern handler fault_reset;
+extern handler fault_undefined_instruction;
+extern handler fault_svc;
+extern handler fault_prefetch_call;
+extern handler fault_prefetch_abort;
+extern handler fault_data_abort;
+
+void dump_registers(int *saved_registers) {
+    static int in_dump = 0;
+    int *sp, *stack_top, *x;
+    char *fault_name;
+    void *fault_handler;
+    int i;
+
+    if (in_dump)
+    {
+        printk("Crash while in dump_registers! Not generating a second report.\n");
+        return;
+    }
+
+    in_dump = 1;
+
+    fault_handler = (handler *) saved_registers[17];
+    if (fault_handler == fault_reset)
+        fault_name = "reset";
+    else if (fault_handler == fault_undefined_instruction)
+        fault_name = "undefined_instruction";
+    else if (fault_handler == fault_svc)
+        fault_name = "svc";
+    else if (fault_handler == fault_prefetch_call)
+        fault_name = "prefetch_call";
+    else if (fault_handler == fault_prefetch_abort)
+        fault_name = "prefetch_abort";
+    else if (fault_handler == fault_data_abort)
+        fault_name = "data_abort";
+    else
+        fault_name = "unknown fault type!";
+
+    printk("Fault handler at %p called (%s)\n", fault_handler, fault_name);
+
+    for (i = 0; i < 16; i++) {
+        printk("r%d = %x\n", i, saved_registers[i]);
+    }
+    printk("CPSR = %x\n", saved_registers[16]);
+
+    printk("Stack dump (innermost last)\n");
+    sp = (int *) saved_registers[13];
+
+    if (sp >= _boot_stack && sp <= _boot_stack_end)
+        stack_top = _boot_stack_end;                    /* The boot stack */
+    else if (sp >= irqstack && sp <= irqstack_end)
+        stack_top = irqstack_end;                       /* The IRQ stack */
+    else
+        stack_top = (int *) ((((unsigned long) sp) | (__STACK_SIZE-1)) + 1);        /* A normal thread stack */
+
+    for (x = stack_top - 1; x >= sp; x--)
+    {
+        printk("  [%8p] %8x\n", x, *x);
+    }
+    printk("End of stack\n");
+
+    in_dump = 0;
+}
-- 
2.1.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH ARM v8 1/4] mini-os: arm: time

[Ian Campbell]

On Fri, 2014-10-03 at 10:20 +0100, Thomas Leonard wrote:
> Based on an initial patch by Karim Raslan.
> 
> Signed-off-by: Karim Allah Ahmed <karim.allah.ahmed@gmail.com>
> Signed-off-by: Thomas Leonard <talex5@gmail.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

> +/* Wall-clock time is not currently available on ARM, so this is always zero for now:
> + * http://wiki.xenproject.org/wiki/Xen_ARM_TODO#Expose_Wallclock_time_to_guests

I have some slightly hacky patches for this, I really should dust them
off and submit them...

> +void block_domain(s_time_t until)
> +{
> +    uint64_t until_count = ns_to_ticks(until) + cntvct_at_init;
> +    ASSERT(irqs_disabled());
> +    if (read_virtual_count() < until_count)
> +    {
> +        set_vtimer_compare(until_count);
> +        __asm__ __volatile__("wfi");
> +        unset_vtimer_compare();
> +
> +        /* Give the IRQ handler a chance to handle whatever woke us up. */
> +        local_irq_enable();
> +        local_irq_disable();
> +    }

Just wondering, is this not roughly equivalent to a wfi loop with
interrupts enabled?

Re: [PATCH ARM v8 2/4] mini-os: arm: interrupt controller

[Ian Campbell]

On Fri, 2014-10-03 at 10:20 +0100, Thomas Leonard wrote:
> +static inline uint32_t REG_READ32(volatile uint32_t *addr)
> +{
> +    uint32_t value;
> +    __asm__ __volatile__("ldr %0, [%1]":"=&r"(value):"r"(addr));
> +    rmb();

I'm not 100% convinced that you need this rmb().

> +    return value;
> +}
> +
> +static inline void REG_WRITE32(volatile uint32_t *addr, unsigned int value)
> +{
> +    __asm__ __volatile__("str %0, [%1]"::"r"(value), "r"(addr));
> +    wmb();
> +}
> +
> +static void gic_set_priority(struct gic *gic, int irq_number, unsigned char priority)
> +{
> +    uint32_t value;
> +    uint32_t *addr = REG(gicd(gic, GICD_IPRIORITYR)) + (irq_number >> 2);
> +    value = REG_READ32(addr);
> +    value &= ~(0xff << (8 * (irq_number & 0x3))); // clear old priority
> +    value |= priority << (8 * (irq_number & 0x3)); // set new priority
> +    REG_WRITE32(addr, value);

I believe the IPRIORITYR registers are byte addressable, so you can just
do a strb of the new value without needing to read-modify-write.

> +static void gic_route_interrupt(struct gic *gic, int irq_number, unsigned char cpu_set)
> +{
> +    uint32_t value;
> +    uint32_t *addr = REG(gicd(gic, GICD_ITARGETSR)) + (irq_number >> 2);
> +    value = REG_READ32(addr);
> +    value &= ~(0xff << (8 * (irq_number & 0x3))); // clear old target
> +    value |= cpu_set << (8 * (irq_number & 0x3)); // set new target
> +    REG_WRITE32(addr, value);

Same for ITARGETSR.

> +/* Note: not thread safe (but we only support one CPU for now anyway) */
> +static void gic_enable_interrupt(struct gic *gic, int irq_number,
> +        unsigned char cpu_set, unsigned char level_sensitive, unsigned char ppi)
> +{
> +    void *set_enable_reg;
> +    void *cfg_reg;
> +
> +    // set priority
> +    gic_set_priority(gic, irq_number, 0x0);
> +
> +    // set target cpus for this interrupt
> +    gic_route_interrupt(gic, irq_number, cpu_set);
> +
> +    // set level/edge triggered
> +    cfg_reg = (void *)gicd(gic, GICD_ICFGR);
> +    if (level_sensitive) {
> +        clear_bit_non_atomic((irq_number * 2) + 1, cfg_reg);
> +    } else {
> +        set_bit_non_atomic((irq_number * 2) + 1, cfg_reg);
> +    }
> +    if (ppi)

missing else? Or should be folded in above as level_sensitive||ppi

> +        clear_bit_non_atomic((irq_number * 2), cfg_reg);
> +
> +    wmb();
> +
> +    // enable forwarding interrupt from distributor to cpu interface
> +    set_enable_reg = (void *)gicd(gic, GICD_ISENABLER);
> +    set_bit_non_atomic(irq_number, set_enable_reg);

I think the semantics of ISENABLER are that you write the bit you want
to enable as 1 and that zeroes are ignored. IOW you can just use a
normal write with the correct shift.

A clear is done via the separate ICENABLER, not by writing 0 to this
register.

> +//FIXME Get event_irq from dt
> +#define EVENTS_IRQ 31
> +#define VIRTUALTIMER_IRQ 27
> +
> +static void gic_handler(void) {
> +    unsigned int irq = gic_readiar(&gic);
> +
> +    DEBUG("IRQ received : %i\n", irq);
> +    switch(irq) {
> +    case EVENTS_IRQ:
> +        do_hypervisor_callback(NULL);
> +        break;
> +    case VIRTUALTIMER_IRQ:
> +        /* We need to get this event to wake us up from block_domain,
> +         * but we don't need to do anything special with it. */
> +        break;

There are a few magic values in the 1020..1023 range which you may see
in practice. In particular 1023 (I think) is a spurious interrupt, which
you should silently ignore.

> +    gic_enable_interrupt(&gic, EVENTS_IRQ /* interrupt number */, 0x1 /*cpu_set*/, 1 /*level_sensitive*/, 0 /* ppi */);
> +    gic_enable_interrupt(&gic, VIRTUALTIMER_IRQ /* interrupt number */, 0x1 /*cpu_set*/, 1 /*level_sensitive*/, 1 /* ppi */);

BTW, ppi or not is implicit in the irq number.


> +}

Re: [PATCH ARM v8 3/4] mini-os: arm: build system

[Ian Campbell]

On Fri, 2014-10-03 at 10:20 +0100, Thomas Leonard wrote:
> diff --git a/extras/mini-os/Makefile b/extras/mini-os/Makefile
> index 6d6537e..77dc9aa 100644
> --- a/extras/mini-os/Makefile
> +++ b/extras/mini-os/Makefile

WRT the freeze some of the changes here would appear to also affect x86
builds, which means they need more careful consideration. Konrad CCd.

> @@ -51,7 +51,7 @@ 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
> +GENERATED_HEADERS := include/list.h $(ARCH_LINKS) include/mini-os include/xen include/$(TARGET_ARCH_FAM)/mini-os include/fdt.h include/libfdt.h
>  
>  EXTRA_DEPS += $(GENERATED_HEADERS)
>  
> @@ -75,7 +75,18 @@ EXTRA_OBJS =
>  TARGET := mini-os
>  
>  # Subdirectories common to mini-os
> -SUBDIRS := lib xenbus console
> +SUBDIRS := lib xenbus console libfdt
> +
> +FDT_SRC :=
> +ifeq ($(XEN_TARGET_ARCH),arm32)
> +# Need libgcc.a for division helpers

Is this the __aeabi_* stuff?

Samuel, is this sort of thing OK in mini-os? Usually on the Xen/kernel
side we avoid libgcc.a and instead have our own copies of such helpers.
I don't recall why, I don't think it is licensing (libgcc has an
exception). Possibly just to give some control over what gets included?

> +LDLIBS += `$(CC) -print-libgcc-file-name`
> +
> +# Device tree support
> +FDT_SRC := libfdt/fdt.c libfdt/fdt_ro.c libfdt/fdt_strerror.c
> +
> +src-y += ${FDT_SRC}

We would normally use $(FDT_SRC) (TBH I'm not sure what the difference
is).

> +endif
>  
>  src-$(CONFIG_BLKFRONT) += blkfront.c
>  src-$(CONFIG_TPMFRONT) += tpmfront.c
> @@ -97,10 +108,13 @@ src-y += sched.c
>  src-$(CONFIG_TEST) += test.c
>  
>  src-y += lib/ctype.c
> +ifneq ($(XEN_TARGET_ARCH),arm32)
>  src-y += lib/math.c
> +endif
>  src-y += lib/printf.c
>  src-y += lib/stack_chk_fail.c
>  src-y += lib/string.c
> +src-y += lib/memmove.c
>  src-y += lib/sys.c
>  src-y += lib/xmalloc.c
>  src-$(CONFIG_XENBUS) += lib/xs.c
> @@ -125,6 +139,21 @@ $(ARCH_LINKS):
>  	$(arch_links)
>  endif
>  
> +include/fdt.h:
> +	cp $(XEN_ROOT)/xen/include/xen/libfdt/fdt.h $@
> +
> +include/libfdt.h:
> +	sed 's!xen/libfdt/!!' $(XEN_ROOT)/xen/include/xen/libfdt/libfdt.h > $@

Should both of these not depend on their input?

> +
> +libfdt:
> +	mkdir $@
> +
> +libfdt/libfdt_internal.h: libfdt
> +	cp $(XEN_ROOT)/xen/common/libfdt/libfdt_internal.h $@
> +
> +libfdt/%.c: libfdt libfdt/libfdt_internal.h
> +	cp $(XEN_ROOT)/xen/common/libfdt/$*.c $@

And these too. Would ln -s be better than cp? Or even VPATH tricks?

> @@ -202,6 +235,7 @@ clean:	arch_clean
>  		rm -f $$dir/*.o; \
>  	done
>  	rm -f include/list.h
> +	rm -f ${FDT_SRC} libfdt/libfdt_internal.h

$(FDT_SRC) again.

Ian.

[PATCH incomplete] xen: arm: wallclock support (incomplete, needs work/refactoring)

[Ian Campbell]

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

On Tue, 2014-10-21 at 11:50 +0100, Ian Campbell wrote:
> > +/* Wall-clock time is not currently available on ARM, so this is always zero for now:
> > + * http://secure-web.cisco.com/1riZQA377dZLKCo5tuoXAK4khWPPIvJePpjzLaQORhQEDB_kQWnA4fzAgRlX09RiW8tAnBtxXwom3oU37PxptJvdEgqAEvbJAHz9mB8G4h6XwnaDr9bcSh1o32Mldi1w6uBD_3Bf3fOL4m9UHxTUsdzkhF1wAMQEQej0ALt0lZHQ/http%3A%2F%2Fwiki.xenproject.org%2Fwiki%2FXen_ARM_TODO#Expose_Wallclock_time_to_guests
> 
> I have some slightly hacky patches for this, I really should dust them
> off and submit them...

Since that doesn't seem likely to happen soon rather than let them rot
in my tree here are the two patches (one for Xen, one for Linux). IMO
they need a fair bit of refactoring (e.g. to make useful bits of the x86
support common/generic) and other code cleanup etc.

Maybe someone would like to pick them up, or else I'll get to it
eventually.

Ian.

[-- Attachment #2: xen.patch --]
[-- Type: text/x-patch, Size: 6762 bytes --]

From 8290927257600951622d416c9e76ab9042c4d47d Mon Sep 17 00:00:00 2001
From: Ian Campbell <ian.campbell@citrix.com>
Date: Tue, 5 Aug 2014 11:10:54 +0100
Subject: [PATCH] xen: arm: support platform_hypercalls for XENPF_settime

Very WIP. Needs proper refactoring of x86 stuff etc.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/Makefile   |    1 +
 xen/arch/arm/domain.c   |    4 +++
 xen/arch/arm/time.c     |   63 ++++++++++++++++++++++++++++++++++++++++++++++-
 xen/arch/arm/traps.c    |    1 +
 xen/include/xsm/dummy.h |   12 ++++-----
 xen/include/xsm/xsm.h   |   13 +++++-----
 6 files changed, 81 insertions(+), 13 deletions(-)

diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index c13206f..f08187e 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -34,6 +34,7 @@ obj-y += hvm.o
 obj-y += device.o
 obj-y += decode.o
 obj-y += processor.o
+obj-y += platform_hypercall.o
 
 #obj-bin-y += ....o
 
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 2b53931..a48adfc 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -630,6 +630,7 @@ static int is_guest_pv64_psr(uint32_t psr)
 int arch_set_info_guest(
     struct vcpu *v, vcpu_guest_context_u c)
 {
+    struct domain *d = v->domain;
     struct vcpu_guest_context *ctxt = c.nat;
     struct vcpu_guest_core_regs *regs = &c.nat->user_regs;
 
@@ -667,6 +668,9 @@ int arch_set_info_guest(
     v->arch.ttbr1 = ctxt->ttbr1;
     v->arch.ttbcr = ctxt->ttbcr;
 
+    if ( v->vcpu_id == 0 )
+        update_domain_wallclock_time(d);
+
     v->is_initialised = 1;
 
     if ( ctxt->flags & VGCF_online )
diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
index a6436f1..e271530 100644
--- a/xen/arch/arm/time.c
+++ b/xen/arch/arm/time.c
@@ -35,6 +35,7 @@
 #include <asm/vgic.h>
 #include <asm/cpufeature.h>
 #include <asm/platform.h>
+#include <asm/div64.h>
 
 uint64_t __read_mostly boot_count;
 
@@ -217,13 +218,73 @@ void update_vcpu_system_time(struct vcpu *v)
 
 void domain_set_time_offset(struct domain *d, int32_t time_offset_seconds)
 {
+    printk("dom%d time offset %"PRId32"\n", d->domain_id, time_offset_seconds);
     d->time_offset_seconds = time_offset_seconds;
     /* XXX update guest visible wallclock time */
 }
 
+static u32 wc_sec, wc_nsec; /* UTC time at last 'time update'. */
+static DEFINE_SPINLOCK(wc_lock);
+
+/* Explicitly OR with 1 just in case version number gets out of sync. */
+#define version_update_begin(v) (((v)+1)|1)
+#define version_update_end(v)   ((v)+1)
+
+void update_domain_wallclock_time(struct domain *d)
+{
+    uint32_t *wc_version;
+
+    spin_lock(&wc_lock);
+
+    wc_version = &shared_info(d, wc_version);
+    *wc_version = version_update_begin(*wc_version);
+    wmb();
+
+    shared_info(d, wc_sec)  = wc_sec + d->time_offset_seconds;
+    shared_info(d, wc_nsec) = wc_nsec;
+
+    wmb();
+    *wc_version = version_update_end(*wc_version);
+
+    spin_unlock(&wc_lock);
+}
+
+/* Set clock to <secs,usecs> after 00:00:00 UTC, 1 January, 1970. */
+/* XXX refactor x86 */
+void do_settime(unsigned long secs, unsigned long nsecs, u64 system_time_base)
+{
+    u64 x;
+    u32 y;
+    struct domain *d;
+
+    x = SECONDS(secs) + (u64)nsecs - system_time_base;
+    y = do_div(x, 1000000000);
+
+    spin_lock(&wc_lock);
+    wc_sec  = x;
+    wc_nsec = y;
+    spin_unlock(&wc_lock);
+
+    rcu_read_lock(&domlist_read_lock);
+    for_each_domain ( d )
+        update_domain_wallclock_time(d);
+    rcu_read_unlock(&domlist_read_lock);
+}
+
 struct tm wallclock_time(uint64_t *ns)
 {
-    return (struct tm) { 0 };
+    uint64_t seconds, nsec;
+
+    if ( !wc_sec )
+        return (struct tm) { 0 };
+
+    seconds = NOW() + SECONDS(wc_sec) + wc_nsec;
+    nsec = do_div(seconds, 1000000000);
+
+    if ( ns )
+        *ns = nsec;
+
+    return gmtime(seconds);
 }
 
 /*
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 76a9586..227b4e2 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1035,6 +1035,7 @@ static arm_hypercall_t arm_hypercall_table[] = {
     HYPERCALL(grant_table_op, 3),
     HYPERCALL(multicall, 2),
     HYPERCALL_ARM(vcpu_op, 3),
+    HYPERCALL(platform_op, 1),
 };
 
 typedef int (*arm_psci_fn_t)(uint32_t, register_t);
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index c5aa316..2cc145c 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -507,6 +507,12 @@ static XSM_INLINE int xsm_hvm_param_nested(XSM_DEFAULT_ARG struct domain *d)
     return xsm_default_action(action, current->domain, d);
 }
 
+static XSM_INLINE int xsm_platform_op(XSM_DEFAULT_ARG uint32_t op)
+{
+    XSM_ASSERT_ACTION(XSM_PRIV);
+    return xsm_default_action(action, current->domain, NULL);
+}
+
 #ifdef CONFIG_X86
 static XSM_INLINE int xsm_do_mca(XSM_DEFAULT_VOID)
 {
@@ -574,12 +580,6 @@ static XSM_INLINE int xsm_apic(XSM_DEFAULT_ARG struct domain *d, int cmd)
     return xsm_default_action(action, d, NULL);
 }
 
-static XSM_INLINE int xsm_platform_op(XSM_DEFAULT_ARG uint32_t op)
-{
-    XSM_ASSERT_ACTION(XSM_PRIV);
-    return xsm_default_action(action, current->domain, NULL);
-}
-
 static XSM_INLINE int xsm_machine_memory_map(XSM_DEFAULT_VOID)
 {
     XSM_ASSERT_ACTION(XSM_PRIV);
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index a85045d..7752b37 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -140,6 +140,8 @@ struct xsm_operations {
     int (*hvm_control) (struct domain *d, unsigned long op);
     int (*hvm_param_nested) (struct domain *d);
 
+    int (*platform_op) (uint32_t cmd);
+
 #ifdef CONFIG_X86
     int (*do_mca) (void);
     int (*shadow_control) (struct domain *d, uint32_t op);
@@ -153,7 +155,6 @@ struct xsm_operations {
     int (*mem_sharing_op) (struct domain *d, struct domain *cd, int op);
     int (*apic) (struct domain *d, int cmd);
     int (*memtype) (uint32_t access);
-    int (*platform_op) (uint32_t cmd);
     int (*machine_memory_map) (void);
     int (*domain_memory_map) (struct domain *d);
 #define XSM_MMU_UPDATE_READ      1
@@ -534,6 +535,11 @@ static inline int xsm_hvm_param_nested (xsm_default_t def, struct domain *d)
     return xsm_ops->hvm_param_nested(d);
 }
 
+static inline int xsm_platform_op (xsm_default_t def, uint32_t op)
+{
+    return xsm_ops->platform_op(op);
+}
+
 #ifdef CONFIG_X86
 static inline int xsm_do_mca(xsm_default_t def)
 {
@@ -595,11 +601,6 @@ static inline int xsm_memtype (xsm_default_t def, uint32_t access)
     return xsm_ops->memtype(access);
 }
 
-static inline int xsm_platform_op (xsm_default_t def, uint32_t op)
-{
-    return xsm_ops->platform_op(op);
-}
-
 static inline int xsm_machine_memory_map(xsm_default_t def)
 {
     return xsm_ops->machine_memory_map();
-- 
1.7.10.4


[-- Attachment #3: linux.patch --]
[-- Type: text/x-patch, Size: 6848 bytes --]

From 1284ba2380b32bf62789b738dbac4d99da463ea1 Mon Sep 17 00:00:00 2001
From: Ian Campbell <ian.campbell@citrix.com>
Date: Tue, 5 Aug 2014 15:23:23 +0100
Subject: [PATCH] Wallclock time support. Needs much refactoring etc

---
 arch/arm/include/asm/xen/hypercall.h |    9 +++
 arch/arm/xen/enlighten.c             |  121 ++++++++++++++++++++++++++++++++++
 arch/arm/xen/hypercall.S             |    4 +-
 3 files changed, 133 insertions(+), 1 deletion(-)

diff --git a/arch/arm/include/asm/xen/hypercall.h b/arch/arm/include/asm/xen/hypercall.h
index 712b50e..055399a 100644
--- a/arch/arm/include/asm/xen/hypercall.h
+++ b/arch/arm/include/asm/xen/hypercall.h
@@ -35,6 +35,7 @@
 
 #include <xen/interface/xen.h>
 #include <xen/interface/sched.h>
+#include <xen/interface/platform.h>
 
 long privcmd_call(unsigned call, unsigned long a1,
 		unsigned long a2, unsigned long a3,
@@ -49,6 +50,14 @@ int HYPERVISOR_memory_op(unsigned int cmd, void *arg);
 int HYPERVISOR_physdev_op(int cmd, void *arg);
 int HYPERVISOR_vcpu_op(int cmd, int vcpuid, void *extra_args);
 int HYPERVISOR_tmem_op(void *arg);
+int HYPERVISOR_dom0_op_raw(struct xen_platform_op *platform_op);
+static inline int
+HYPERVISOR_dom0_op(struct xen_platform_op *platform_op)
+{
+        platform_op->interface_version = XENPF_INTERFACE_VERSION;
+        return HYPERVISOR_dom0_op_raw(platform_op);
+}
+
 int HYPERVISOR_multicall(struct multicall_entry *calls, uint32_t nr);
 
 static inline int
diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index 1e63243..0364987 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -11,6 +11,7 @@
 #include <xen/xenbus.h>
 #include <xen/page.h>
 #include <xen/interface/sched.h>
+#include <xen/interface/platform.h>
 #include <xen/xen-ops.h>
 #include <asm/xen/hypervisor.h>
 #include <asm/xen/hypercall.h>
@@ -24,6 +25,7 @@
 #include <linux/cpuidle.h>
 #include <linux/cpufreq.h>
 #include <linux/cpu.h>
+#include <linux/pvclock_gtod.h>
 
 #include <linux/mm.h>
 
@@ -219,6 +221,122 @@ static irqreturn_t xen_arm_callback(int irq, void *arg)
 	return IRQ_HANDLED;
 }
 
+unsigned long xen_clocksource_read(void)
+{
+	unsigned long val = 0;
+	/* XXX arch timer? Error handling etc etc */
+	if (read_current_timer(&val) < 0)
+		printk("read_current_timer failed?\n");
+	return val;
+}
+
+void pvclock_read_wallclock(struct pvclock_wall_clock *wall_clock,
+                            struct pvclock_vcpu_time_info *vcpu_time,
+                            struct timespec *ts)
+{
+        u32 version;
+        u64 delta;
+        struct timespec now;
+
+        /* get wallclock at system boot */
+        do {
+                version = wall_clock->version;
+                rmb();          /* fetch version before time */
+                now.tv_sec  = wall_clock->sec;
+                now.tv_nsec = wall_clock->nsec;
+                rmb();          /* fetch time before checking version */
+        } while ((wall_clock->version & 1) || (version != wall_clock->version));
+
+        //delta = pvclock_clocksource_read(vcpu_time);    /* time since system boot */
+	delta = xen_clocksource_read();
+        delta += now.tv_sec * (u64)NSEC_PER_SEC + now.tv_nsec;
+
+        now.tv_nsec = do_div(delta, NSEC_PER_SEC);
+        now.tv_sec = delta;
+
+        set_normalized_timespec(ts, now.tv_sec, now.tv_nsec);
+}
+
+static void xen_read_wallclock(struct timespec *ts)
+{
+	struct shared_info *s = HYPERVISOR_shared_info;
+	struct pvclock_wall_clock *wall_clock = &(s->wc);
+        struct pvclock_vcpu_time_info *vcpu_time;
+
+	vcpu_time = &get_cpu_var(xen_vcpu)->time;
+	pvclock_read_wallclock(wall_clock, vcpu_time, ts);
+	put_cpu_var(xen_vcpu);
+}
+
+/* Parts could be common with x86? */
+static int xen_pvclock_gtod_notify(struct notifier_block *nb,
+				   unsigned long was_set, void *priv)
+{
+	/* Protected by the calling core code serialization */
+	static struct timespec next_sync;
+
+	struct xen_platform_op op;
+	struct timespec now;
+
+	now = __current_kernel_time();
+
+	/*
+	 * We only take the expensive HV call when the clock was set
+	 * or when the 11 minutes RTC synchronization time elapsed.
+	 */
+	if (!was_set && timespec_compare(&now, &next_sync) < 0)
+		return NOTIFY_OK;
+
+	op.cmd = XENPF_settime;
+	op.u.settime.secs = now.tv_sec;
+	op.u.settime.nsecs = now.tv_nsec;
+	op.u.settime.system_time = xen_clocksource_read();
+	printk("GTOD: Setting to %ld.%ld at %lld\n",
+	       (long)op.u.settime.secs,
+	       (long)op.u.settime.nsecs,
+	       (long long)op.u.settime.system_time);
+	(void)HYPERVISOR_dom0_op(&op);
+
+	/*
+	 * Move the next drift compensation time 11 minutes
+	 * ahead. That's emulating the sync_cmos_clock() update for
+	 * the hardware RTC.
+	 */
+	next_sync = now;
+	next_sync.tv_sec += 11 * 60;
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block xen_pvclock_gtod_notifier = {
+	.notifier_call = xen_pvclock_gtod_notify,
+};
+
+static void __init xen_time_init(void)
+{
+	//int cpu = smp_processor_id();
+	struct timespec tp;
+
+	/* Set initial system time with full resolution */
+	xen_read_wallclock(&tp);
+	do_settimeofday(&tp);
+
+	printk("INITIAL TIME %ld.%ld\n",
+	       (long)tp.tv_sec,
+	       (long)tp.tv_nsec);
+	{
+		struct shared_info *s = HYPERVISOR_shared_info;
+		struct pvclock_wall_clock *wall_clock = &(s->wc);
+		struct pvclock_vcpu_time_info *vcpu_time;
+		printk("SHARED INFO TIME %ld.%ld\n",
+		       (long)wall_clock->sec,
+		       (long)wall_clock->nsec);
+	}
+
+	if (xen_initial_domain())
+		pvclock_gtod_register_notifier(&xen_pvclock_gtod_notifier);
+}
+
 /*
  * see Documentation/devicetree/bindings/arm/xen.txt for the
  * documentation of the Xen Device Tree format.
@@ -323,6 +441,8 @@ static int __init xen_guest_init(void)
 
 	register_cpu_notifier(&xen_cpu_notifier);
 
+	xen_time_init();
+
 	return 0;
 }
 early_initcall(xen_guest_init);
@@ -359,4 +479,5 @@ EXPORT_SYMBOL_GPL(HYPERVISOR_physdev_op);
 EXPORT_SYMBOL_GPL(HYPERVISOR_vcpu_op);
 EXPORT_SYMBOL_GPL(HYPERVISOR_tmem_op);
 EXPORT_SYMBOL_GPL(HYPERVISOR_multicall);
+EXPORT_SYMBOL_GPL(HYPERVISOR_dom0_op_raw);
 EXPORT_SYMBOL_GPL(privcmd_call);
diff --git a/arch/arm/xen/hypercall.S b/arch/arm/xen/hypercall.S
index 44e3a5f..d3bf35e 100644
--- a/arch/arm/xen/hypercall.S
+++ b/arch/arm/xen/hypercall.S
@@ -78,7 +78,6 @@ ENTRY(HYPERVISOR_##hypercall)			\
 ENDPROC(HYPERVISOR_##hypercall)
 
                 .text
-
 HYPERCALL2(xen_version);
 HYPERCALL3(console_io);
 HYPERCALL3(grant_table_op);
@@ -91,6 +90,9 @@ HYPERCALL3(vcpu_op);
 HYPERCALL1(tmem_op);
 HYPERCALL2(multicall);
 
+#define __HYPERVISOR_dom0_op_raw __HYPERVISOR_dom0_op /* Hack to allow setting interface version from C wrapper */
+HYPERCALL1(dom0_op_raw);/*XXX legacy name for what is now platform_op, used by x86 too*/
+
 ENTRY(privcmd_call)
 	stmdb sp!, {r4}
 	mov r12, r0
-- 
1.7.10.4


[-- Attachment #4: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH ARM v8 2/4] mini-os: arm: interrupt controller

[Julien Grall]

Hi Ian and Thomas,

On 10/21/2014 12:00 PM, Ian Campbell wrote:
> On Fri, 2014-10-03 at 10:20 +0100, Thomas Leonard wrote:
>> +static void gic_set_priority(struct gic *gic, int irq_number, unsigned char priority)
>> +{
>> +    uint32_t value;
>> +    uint32_t *addr = REG(gicd(gic, GICD_IPRIORITYR)) + (irq_number >> 2);
>> +    value = REG_READ32(addr);
>> +    value &= ~(0xff << (8 * (irq_number & 0x3))); // clear old priority
>> +    value |= priority << (8 * (irq_number & 0x3)); // set new priority
>> +    REG_WRITE32(addr, value);
> 
> I believe the IPRIORITYR registers are byte addressable, so you can just
> do a strb of the new value without needing to read-modify-write.
> 
>> +static void gic_route_interrupt(struct gic *gic, int irq_number, unsigned char cpu_set)
>> +{
>> +    uint32_t value;
>> +    uint32_t *addr = REG(gicd(gic, GICD_ITARGETSR)) + (irq_number >> 2);
>> +    value = REG_READ32(addr);
>> +    value &= ~(0xff << (8 * (irq_number & 0x3))); // clear old target
>> +    value |= cpu_set << (8 * (irq_number & 0x3)); // set new target
>> +    REG_WRITE32(addr, value);
> 
> Same for ITARGETSR.

Our implementation of ITARGETSR doesn't handle correctly read/write per
byte. If the register is RO (such as for the SGIs and PPIs), our write
ignore is checking that the guest is writing a word.

Even though we need to fix it in Xen (I could send a patch for it). We
need to keep this implementation if we want mini-os to run on Xen 4.4.x.

>> +/* Note: not thread safe (but we only support one CPU for now anyway) */
>> +static void gic_enable_interrupt(struct gic *gic, int irq_number,
>> +        unsigned char cpu_set, unsigned char level_sensitive, unsigned char ppi)
>> +{
>> +    void *set_enable_reg;
>> +    void *cfg_reg;
>> +
>> +    // set priority
>> +    gic_set_priority(gic, irq_number, 0x0);
>> +
>> +    // set target cpus for this interrupt
>> +    gic_route_interrupt(gic, irq_number, cpu_set);
>> +
>> +    // set level/edge triggered
>> +    cfg_reg = (void *)gicd(gic, GICD_ICFGR);
>> +    if (level_sensitive) {
>> +        clear_bit_non_atomic((irq_number * 2) + 1, cfg_reg);
>> +    } else {
>> +        set_bit_non_atomic((irq_number * 2) + 1, cfg_reg);
>> +    }
>> +    if (ppi)
> 
> missing else? Or should be folded in above as level_sensitive||ppi

I would even drop this check. I don't think, we need to have specific
configuration for PPIs.

[..]

>> +    gic_enable_interrupt(&gic, EVENTS_IRQ /* interrupt number */, 0x1 /*cpu_set*/, 1 /*level_sensitive*/, 0 /* ppi */);
>> +    gic_enable_interrupt(&gic, VIRTUALTIMER_IRQ /* interrupt number */, 0x1 /*cpu_set*/, 1 /*level_sensitive*/, 1 /* ppi */);
> 
> BTW, ppi or not is implicit in the irq number.

At the same time, both interrupt are PPIs, why the former as ppi = 0 and
the latter 1?

Regards,

-- 
Julien Grall

Re: [PATCH ARM v8 2/4] mini-os: arm: interrupt controller

[Ian Campbell]

On Tue, 2014-10-21 at 15:26 +0100, Julien Grall wrote:
> Our implementation of ITARGETSR doesn't handle correctly read/write per
> byte. If the register is RO (such as for the SGIs and PPIs), our write
> ignore is checking that the guest is writing a word.

That's a bug.

> Even though we need to fix it in Xen (I could send a patch for it). We
> need to keep this implementation if we want mini-os to run on Xen 4.4.x.

We should fix bugs, not workaround them in the guest.

Re: [PATCH ARM v8 2/4] mini-os: arm: interrupt controller

[Julien Grall]

On 10/21/2014 04:16 PM, Ian Campbell wrote:
> On Tue, 2014-10-21 at 15:26 +0100, Julien Grall wrote:
>> Our implementation of ITARGETSR doesn't handle correctly read/write per
>> byte. If the register is RO (such as for the SGIs and PPIs), our write
>> ignore is checking that the guest is writing a word.
> 
> That's a bug.

Agree

>> Even though we need to fix it in Xen (I could send a patch for it). We
>> need to keep this implementation if we want mini-os to run on Xen 4.4.x.
> 
> We should fix bugs, not workaround them in the guest.

Writing a 32 bits value into those registers is perfectly value and not
a workaround.

In this case, it would make mini-os working directly on Xen 4.4.

-- 
Julien Grall

Re: [PATCH ARM v8 2/4] mini-os: arm: interrupt controller

[Ian Campbell]

On Tue, 2014-10-21 at 16:22 +0100, Julien Grall wrote:
> On 10/21/2014 04:16 PM, Ian Campbell wrote:
> > On Tue, 2014-10-21 at 15:26 +0100, Julien Grall wrote:
> >> Our implementation of ITARGETSR doesn't handle correctly read/write per
> >> byte. If the register is RO (such as for the SGIs and PPIs), our write
> >> ignore is checking that the guest is writing a word.
> > 
> > That's a bug.
> 
> Agree
> 
> >> Even though we need to fix it in Xen (I could send a patch for it). We
> >> need to keep this implementation if we want mini-os to run on Xen 4.4.x.
> > 
> > We should fix bugs, not workaround them in the guest.
> 
> Writing a 32 bits value into those registers is perfectly value and not
> a workaround.

If you would normally write 1 byte (which makes perfect sense for this
register) but can't because of a Xen bug then avoiding that by
read/modify/write of 4 bytes because of a Xen bug *is* a workaround.

> In this case, it would make mini-os working directly on Xen 4.4.

and how long should mini-os carry this hack for then?

Ian.

Re: [PATCH ARM v8 2/4] mini-os: arm: interrupt controller

[Julien Grall]

On 10/21/2014 04:35 PM, Ian Campbell wrote:
> On Tue, 2014-10-21 at 16:22 +0100, Julien Grall wrote:
>> On 10/21/2014 04:16 PM, Ian Campbell wrote:
>>> On Tue, 2014-10-21 at 15:26 +0100, Julien Grall wrote:
>>>> Our implementation of ITARGETSR doesn't handle correctly read/write per
>>>> byte. If the register is RO (such as for the SGIs and PPIs), our write
>>>> ignore is checking that the guest is writing a word.
>>>
>>> That's a bug.
>>
>> Agree
>>
>>>> Even though we need to fix it in Xen (I could send a patch for it). We
>>>> need to keep this implementation if we want mini-os to run on Xen 4.4.x.
>>>
>>> We should fix bugs, not workaround them in the guest.
>>
>> Writing a 32 bits value into those registers is perfectly value and not
>> a workaround.
> If you would normally write 1 byte (which makes perfect sense for this
> register) but can't because of a Xen bug then avoiding that by
> read/modify/write of 4 bytes because of a Xen bug *is* a workaround.

This case will only happen when the guest is writing in ITARGET for
SPI/PPIs which is RO for PPIs.

The guest may want to write another value than the actual one. Therefore
this could also be considered as a bug from the guest POV.

So we also avoid to avoid writing in ITARGET when it's not necessary.

>> In this case, it would make mini-os working directly on Xen 4.4.

> and how long should mini-os carry this hack for then?

The current issue could be considered as an hardware bug (even though
it's an emulation). Time to time guest has to deal with hardware bug. As
long as it's very simple to fix it and doesn't impact performance, I
guess it's better to have it forever.
In this case, interrupts will be configured only once during mini-os
initialization.

If nobody care about running mini-os on current version of current Xen
4.4.x, then fine.

Regards,

-- 
Julien Grall

Re: [PATCH ARM v8 2/4] mini-os: arm: interrupt controller

[Anil Madhavapeddy]

On 21 Oct 2014, at 17:03, Julien Grall <julien.grall@linaro.org> wrote:
> 
>>> In this case, it would make mini-os working directly on Xen 4.4.
> 
>> and how long should mini-os carry this hack for then?
> 
> The current issue could be considered as an hardware bug (even though
> it's an emulation). Time to time guest has to deal with hardware bug. As
> long as it's very simple to fix it and doesn't impact performance, I
> guess it's better to have it forever.
> In this case, interrupts will be configured only once during mini-os
> initialization.
> 
> If nobody care about running mini-os on current version of current Xen
> 4.4.x, then fine.

Isn't this what XEN_INTERFACE_VERSION checks are for?  It's certainly
convenient to have MiniOS support the last stable version of the Xen
ABI unless it's a huge diff, and this should just be a localised #ifdef.

-anil

Re: [PATCH ARM v8 2/4] mini-os: arm: interrupt controller

[Ian Campbell]

On Tue, 2014-10-21 at 19:14 +0100, Anil Madhavapeddy wrote:
> On 21 Oct 2014, at 17:03, Julien Grall <julien.grall@linaro.org> wrote:
> > 
> >>> In this case, it would make mini-os working directly on Xen 4.4.
> > 
> >> and how long should mini-os carry this hack for then?
> > 
> > The current issue could be considered as an hardware bug (even though
> > it's an emulation). Time to time guest has to deal with hardware bug. As
> > long as it's very simple to fix it and doesn't impact performance, I
> > guess it's better to have it forever.
> > In this case, interrupts will be configured only once during mini-os
> > initialization.
> > 
> > If nobody care about running mini-os on current version of current Xen
> > 4.4.x, then fine.

Anybody who cares should fix the bug in Xen.  It would have taken less
typing than this thread.

> Isn't this what XEN_INTERFACE_VERSION checks are for?  It's certainly
> convenient to have MiniOS support the last stable version of the Xen
> ABI unless it's a huge diff, and this should just be a localised #ifdef.

This isn't an ABI issue, it's a violation of a hardware spec which we
are supposed to be emulating.

In any case XEN_INTERFACE_VERSION is for API compatiblity (for users of
the Xen headers), not ABI compatibility.

Ian.

Re: [PATCH ARM v8 3/4] mini-os: arm: build system

[Samuel Thibault]

Ian Campbell, le Tue 21 Oct 2014 12:44:27 +0100, a écrit :
> > +ifeq ($(XEN_TARGET_ARCH),arm32)
> > +# Need libgcc.a for division helpers
> 
> Is this the __aeabi_* stuff?
> 
> Samuel, is this sort of thing OK in mini-os? Usually on the Xen/kernel
> side we avoid libgcc.a and instead have our own copies of such helpers.
> I don't recall why, I don't think it is licensing (libgcc has an
> exception). Possibly just to give some control over what gets included?

IIRC we would tend to get additional things brought in, with special
linker features and all kinds of things we don't want to have to
support.  Since you never know what compiler would be used, it was saner
to just provide an implementation.

Samuel

Re: [PATCH ARM v8 2/4] mini-os: arm: interrupt controller

[Samuel Thibault]

Ian Campbell, le Tue 21 Oct 2014 12:00:18 +0100, a écrit :
> On Fri, 2014-10-03 at 10:20 +0100, Thomas Leonard wrote:
> > +static inline uint32_t REG_READ32(volatile uint32_t *addr)
> > +{
> > +    uint32_t value;
> > +    __asm__ __volatile__("ldr %0, [%1]":"=&r"(value):"r"(addr));
> > +    rmb();
> 
> I'm not 100% convinced that you need this rmb().
> 
> > +    return value;
> > +}
> > +
> > +static inline void REG_WRITE32(volatile uint32_t *addr, unsigned int value)
> > +{
> > +    __asm__ __volatile__("str %0, [%1]"::"r"(value), "r"(addr));
> > +    wmb();
> > +}

I don't really see why such barriers are needed indeed. Are they needed
to actually push the values out? Or are they needed to synchronize with
others reads & writes in the source code (in which case REG_WRITE32 is
bogus, the barrier should be before the str...). Barriers should usually
be put in the code which needs them, not hidden in operations and thus
getting the thing working by luck only.

Samuel

Re: [PATCH ARM v8 3/4] mini-os: arm: build system

[Ian Campbell]

On Tue, 2014-10-21 at 23:50 +0200, Samuel Thibault wrote:
> Ian Campbell, le Tue 21 Oct 2014 12:44:27 +0100, a écrit :
> > > +ifeq ($(XEN_TARGET_ARCH),arm32)
> > > +# Need libgcc.a for division helpers
> > 
> > Is this the __aeabi_* stuff?
> > 
> > Samuel, is this sort of thing OK in mini-os? Usually on the Xen/kernel
> > side we avoid libgcc.a and instead have our own copies of such helpers.
> > I don't recall why, I don't think it is licensing (libgcc has an
> > exception). Possibly just to give some control over what gets included?
> 
> IIRC we would tend to get additional things brought in, with special
> linker features and all kinds of things we don't want to have to
> support.  Since you never know what compiler would be used, it was saner
> to just provide an implementation.

So it sounds like we'd be better off finding a BSD licensed
implementation of the require __aeabi_* and including those in mini-os?

Ian.

Re: [PATCH ARM v8 2/4] mini-os: arm: interrupt controller

[Ian Campbell]

On Tue, 2014-10-21 at 23:54 +0200, Samuel Thibault wrote:
> Ian Campbell, le Tue 21 Oct 2014 12:00:18 +0100, a écrit :
> > On Fri, 2014-10-03 at 10:20 +0100, Thomas Leonard wrote:
> > > +static inline uint32_t REG_READ32(volatile uint32_t *addr)
> > > +{
> > > +    uint32_t value;
> > > +    __asm__ __volatile__("ldr %0, [%1]":"=&r"(value):"r"(addr));
> > > +    rmb();
> > 
> > I'm not 100% convinced that you need this rmb().
> > 
> > > +    return value;
> > > +}
> > > +
> > > +static inline void REG_WRITE32(volatile uint32_t *addr, unsigned int value)
> > > +{
> > > +    __asm__ __volatile__("str %0, [%1]"::"r"(value), "r"(addr));
> > > +    wmb();
> > > +}
> 
> I don't really see why such barriers are needed indeed. Are they needed
> to actually push the values out?

That would, I think, require an isb() (instruction barrier) whereas
wmb() turns into a dsb() (data barrier). I expect you are write and
these rmb/wmb are not needed, but an isb may be needed in the caller if
they want to rely on the affect of a write (e.g. enabling the
controller)

Ian.

Re: [PATCH ARM v8 3/4] mini-os: arm: build system

[Samuel Thibault]

Ian Campbell, le Wed 22 Oct 2014 10:01:51 +0100, a écrit :
> On Tue, 2014-10-21 at 23:50 +0200, Samuel Thibault wrote:
> > Ian Campbell, le Tue 21 Oct 2014 12:44:27 +0100, a écrit :
> > > > +ifeq ($(XEN_TARGET_ARCH),arm32)
> > > > +# Need libgcc.a for division helpers
> > > 
> > > Is this the __aeabi_* stuff?
> > > 
> > > Samuel, is this sort of thing OK in mini-os? Usually on the Xen/kernel
> > > side we avoid libgcc.a and instead have our own copies of such helpers.
> > > I don't recall why, I don't think it is licensing (libgcc has an
> > > exception). Possibly just to give some control over what gets included?
> > 
> > IIRC we would tend to get additional things brought in, with special
> > linker features and all kinds of things we don't want to have to
> > support.  Since you never know what compiler would be used, it was saner
> > to just provide an implementation.
> 
> So it sounds like we'd be better off finding a BSD licensed
> implementation of the require __aeabi_* and including those in mini-os?

Yes.

Samuel

Re: [PATCH ARM v8 2/4] mini-os: arm: interrupt controller

[Julien Grall]

On 10/22/2014 10:03 AM, Ian Campbell wrote:
> On Tue, 2014-10-21 at 23:54 +0200, Samuel Thibault wrote:
>> Ian Campbell, le Tue 21 Oct 2014 12:00:18 +0100, a écrit :
>>> On Fri, 2014-10-03 at 10:20 +0100, Thomas Leonard wrote:
>>>> +static inline uint32_t REG_READ32(volatile uint32_t *addr)
>>>> +{
>>>> +    uint32_t value;
>>>> +    __asm__ __volatile__("ldr %0, [%1]":"=&r"(value):"r"(addr));
>>>> +    rmb();
>>>
>>> I'm not 100% convinced that you need this rmb().

Most the GIC code doesn't require read barrier but...

>>>
>>>> +    return value;
>>>> +}
>>>> +
>>>> +static inline void REG_WRITE32(volatile uint32_t *addr, unsigned int value)
>>>> +{
>>>> +    __asm__ __volatile__("str %0, [%1]"::"r"(value), "r"(addr));
>>>> +    wmb();
>>>> +}

write barrier may be necessary on some, where we need to wait that all
write has been done before doing this one (such as enable the GIC ...).

So this function is buggy. It should be:

wmb();
__asm__ __volatile__(....).

>>
>> I don't really see why such barriers are needed indeed. Are they needed
>> to actually push the values out?
> 
> That would, I think, require an isb() (instruction barrier) whereas
> wmb() turns into a dsb() (data barrier). I expect you are write and
> these rmb/wmb are not needed, but an isb may be needed in the caller if
> they want to rely on the affect of a write (e.g. enabling the
> controller)

isb is useful for cache and system control registers. We don't use such
things in the GIC code, because the GIC register is memory mapped. We
only need to ensure that the write are ordered when it's necessary (only
few places).

Regards,

-- 
Julien Grall

Re: [PATCH ARM v8 2/4] mini-os: arm: interrupt controller

[Samuel Thibault]

Julien Grall, le Wed 22 Oct 2014 14:06:44 +0100, a écrit :
> >>>> +    return value;
> >>>> +}
> >>>> +
> >>>> +static inline void REG_WRITE32(volatile uint32_t *addr, unsigned int value)
> >>>> +{
> >>>> +    __asm__ __volatile__("str %0, [%1]"::"r"(value), "r"(addr));
> >>>> +    wmb();
> >>>> +}
> 
> write barrier may be necessary on some, where we need to wait that all
> write has been done before doing this one (such as enable the GIC ...).
> 
> So this function is buggy. It should be:
> 
> wmb();
> __asm__ __volatile__(....).

Well, I'd say it's the caller of REG_WRITE32 which should explicit it.

Samuel

Re: [PATCH ARM v8 3/4] mini-os: arm: build system

[Thomas Leonard]

On 21 October 2014 22:50, Samuel Thibault <samuel.thibault@ens-lyon.org> wrote:
> Ian Campbell, le Tue 21 Oct 2014 12:44:27 +0100, a écrit :
>> > +ifeq ($(XEN_TARGET_ARCH),arm32)
>> > +# Need libgcc.a for division helpers
>>
>> Is this the __aeabi_* stuff?
>>
>> Samuel, is this sort of thing OK in mini-os? Usually on the Xen/kernel
>> side we avoid libgcc.a and instead have our own copies of such helpers.
>> I don't recall why, I don't think it is licensing (libgcc has an
>> exception). Possibly just to give some control over what gets included?
>
> IIRC we would tend to get additional things brought in, with special
> linker features and all kinds of things we don't want to have to
> support.  Since you never know what compiler would be used, it was saner
> to just provide an implementation.

As I understand it, libgcc provides required support functions for the
code that gcc generates. If you use a different compiler, you'll
presumably want different support functions too.

Could you say a bit more about the linker problems you had? I haven't
noticed any problems myself, and I would assume it would just link in
the functions it needs and do nothing else. I'd like to know what I'm
trying to fix before reimplementing anything...

Thanks,


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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH ARM v8 1/4] mini-os: arm: time

[Thomas Leonard]

On 21 October 2014 11:50, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Fri, 2014-10-03 at 10:20 +0100, Thomas Leonard wrote:
>> Based on an initial patch by Karim Raslan.
>>
>> Signed-off-by: Karim Allah Ahmed <karim.allah.ahmed@gmail.com>
>> Signed-off-by: Thomas Leonard <talex5@gmail.com>
>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>
>> +/* Wall-clock time is not currently available on ARM, so this is always zero for now:
>> + * http://wiki.xenproject.org/wiki/Xen_ARM_TODO#Expose_Wallclock_time_to_guests
>
> I have some slightly hacky patches for this, I really should dust them
> off and submit them...
>
>> +void block_domain(s_time_t until)
>> +{
>> +    uint64_t until_count = ns_to_ticks(until) + cntvct_at_init;
>> +    ASSERT(irqs_disabled());
>> +    if (read_virtual_count() < until_count)
>> +    {
>> +        set_vtimer_compare(until_count);
>> +        __asm__ __volatile__("wfi");
>> +        unset_vtimer_compare();
>> +
>> +        /* Give the IRQ handler a chance to handle whatever woke us up. */
>> +        local_irq_enable();
>> +        local_irq_disable();
>> +    }
>
> Just wondering, is this not roughly equivalent to a wfi loop with
> interrupts enabled?

I'm not quite sure what you mean.

If we enable interrupts before the wfi then I think the following could occur:

1. Application checks for work, finds none and calls block_domain.
2. block_domain enables interrupts.
3. An interrupt occurs.
4. The interrupt handler sets a flag indicating work to do.
5. wfi is called, putting the domain to sleep, even though there is work to do.

Enabling IRQs after block_domain ensures we can't sleep while we have
work to do.


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

Re: [PATCH ARM v8 3/4] mini-os: arm: build system

[Samuel Thibault]

Thomas Leonard, le Sun 26 Oct 2014 09:46:09 +0000, a écrit :
> Could you say a bit more about the linker problems you had?

Really digging in the archives this time :)

ia64-specific:
http://lists.xen.org/archives/html/xen-ia64-devel/2008-03/msg00126.html
http://lists.xen.org/archives/html/xen-ia64-devel/2008-12/msg00070.html
x86_64-specific (missing red zone support)
http://lists.xen.org/archives/html/xen-ia64-devel/2008-02/msg00251.html

So I guess it could be OK on arm, but you have to make sure that Mini-OS
implements the whole ABI that gcc will use. Testing is not enough, I got
hit by the red zone for instance.

Samuel

Re: [PATCH ARM v8 3/4] mini-os: arm: build system

[Thomas Leonard]

On 26 October 2014 09:55, Samuel Thibault <samuel.thibault@ens-lyon.org> wrote:
> Thomas Leonard, le Sun 26 Oct 2014 09:46:09 +0000, a écrit :
>> Could you say a bit more about the linker problems you had?
>
> Really digging in the archives this time :)
>
> ia64-specific:
> http://lists.xen.org/archives/html/xen-ia64-devel/2008-03/msg00126.html
> http://lists.xen.org/archives/html/xen-ia64-devel/2008-12/msg00070.html
> x86_64-specific (missing red zone support)
> http://lists.xen.org/archives/html/xen-ia64-devel/2008-02/msg00251.html
>
> So I guess it could be OK on arm, but you have to make sure that Mini-OS
> implements the whole ABI that gcc will use. Testing is not enough, I got
> hit by the red zone for instance.

On ARM, we have a separate stack for the IRQ handler, so the red zone
at least shouldn't be a problem.


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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH ARM v8 1/4] mini-os: arm: time

[Ian Campbell]

On Sun, 2014-10-26 at 09:51 +0000, Thomas Leonard wrote:
> On 21 October 2014 11:50, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > On Fri, 2014-10-03 at 10:20 +0100, Thomas Leonard wrote:
> >> Based on an initial patch by Karim Raslan.
> >>
> >> Signed-off-by: Karim Allah Ahmed <karim.allah.ahmed@gmail.com>
> >> Signed-off-by: Thomas Leonard <talex5@gmail.com>
> >
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> >
> >> +/* Wall-clock time is not currently available on ARM, so this is always zero for now:
> >> + * http://wiki.xenproject.org/wiki/Xen_ARM_TODO#Expose_Wallclock_time_to_guests
> >
> > I have some slightly hacky patches for this, I really should dust them
> > off and submit them...
> >
> >> +void block_domain(s_time_t until)
> >> +{
> >> +    uint64_t until_count = ns_to_ticks(until) + cntvct_at_init;
> >> +    ASSERT(irqs_disabled());
> >> +    if (read_virtual_count() < until_count)
> >> +    {
> >> +        set_vtimer_compare(until_count);
> >> +        __asm__ __volatile__("wfi");
> >> +        unset_vtimer_compare();
> >> +
> >> +        /* Give the IRQ handler a chance to handle whatever woke us up. */
> >> +        local_irq_enable();
> >> +        local_irq_disable();
> >> +    }
> >
> > Just wondering, is this not roughly equivalent to a wfi loop with
> > interrupts enabled?
> 
> I'm not quite sure what you mean.
> 
> If we enable interrupts before the wfi then I think the following could occur:
> 
> 1. Application checks for work, finds none and calls block_domain.
> 2. block_domain enables interrupts.
> 3. An interrupt occurs.
> 4. The interrupt handler sets a flag indicating work to do.
> 5. wfi is called, putting the domain to sleep, even though there is work to do.
> 
> Enabling IRQs after block_domain ensures we can't sleep while we have
> work to do.

Ah, yes.

Ian.

Re: [PATCH ARM v8 2/4] mini-os: arm: interrupt controller

[Thomas Leonard]

On 22 October 2014 14:06, Julien Grall <julien.grall@linaro.org> wrote:
> On 10/22/2014 10:03 AM, Ian Campbell wrote:
>> On Tue, 2014-10-21 at 23:54 +0200, Samuel Thibault wrote:
>>> Ian Campbell, le Tue 21 Oct 2014 12:00:18 +0100, a écrit :
>>>> On Fri, 2014-10-03 at 10:20 +0100, Thomas Leonard wrote:
>>>>> +static inline uint32_t REG_READ32(volatile uint32_t *addr)
>>>>> +{
>>>>> +    uint32_t value;
>>>>> +    __asm__ __volatile__("ldr %0, [%1]":"=&r"(value):"r"(addr));
>>>>> +    rmb();
>>>>
>>>> I'm not 100% convinced that you need this rmb().
>
> Most the GIC code doesn't require read barrier but...
>
>>>>
>>>>> +    return value;
>>>>> +}
>>>>> +
>>>>> +static inline void REG_WRITE32(volatile uint32_t *addr, unsigned int value)
>>>>> +{
>>>>> +    __asm__ __volatile__("str %0, [%1]"::"r"(value), "r"(addr));
>>>>> +    wmb();
>>>>> +}
>
> write barrier may be necessary on some, where we need to wait that all
> write has been done before doing this one (such as enable the GIC ...).
>
> So this function is buggy. It should be:
>
> wmb();
> __asm__ __volatile__(....).

gic_init does an explicit wmb() before enabling the GIC anyway,
although I'm not really sure why it's needed (these barriers are from
Karim's original code, so I don't know the original reason for them).
Xen will have marked the GIC memory as device memory, so I guess we're
protected from many effects ("The number, order and sizes of the
accesses are maintained.").

Maybe none of these barriers is necessary.

>>> I don't really see why such barriers are needed indeed. Are they needed
>>> to actually push the values out?
>>
>> That would, I think, require an isb() (instruction barrier) whereas
>> wmb() turns into a dsb() (data barrier). I expect you are write and
>> these rmb/wmb are not needed, but an isb may be needed in the caller if
>> they want to rely on the affect of a write (e.g. enabling the
>> controller)
>
> isb is useful for cache and system control registers. We don't use such
> things in the GIC code, because the GIC register is memory mapped. We
> only need to ensure that the write are ordered when it's necessary (only
> few places).
>
> Regards,
>
> --
> Julien Grall



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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH ARM v8 2/4] mini-os: arm: interrupt controller

[Julien Grall]

On 10/28/2014 03:15 PM, Thomas Leonard wrote:
> On 22 October 2014 14:06, Julien Grall <julien.grall@linaro.org> wrote:
>> On 10/22/2014 10:03 AM, Ian Campbell wrote:
>>> On Tue, 2014-10-21 at 23:54 +0200, Samuel Thibault wrote:
>>>> Ian Campbell, le Tue 21 Oct 2014 12:00:18 +0100, a écrit :
>>>>> On Fri, 2014-10-03 at 10:20 +0100, Thomas Leonard wrote:
>>>>>> +static inline uint32_t REG_READ32(volatile uint32_t *addr)
>>>>>> +{
>>>>>> +    uint32_t value;
>>>>>> +    __asm__ __volatile__("ldr %0, [%1]":"=&r"(value):"r"(addr));
>>>>>> +    rmb();
>>>>>
>>>>> I'm not 100% convinced that you need this rmb().
>>
>> Most the GIC code doesn't require read barrier but...
>>
>>>>>
>>>>>> +    return value;
>>>>>> +}
>>>>>> +
>>>>>> +static inline void REG_WRITE32(volatile uint32_t *addr, unsigned int value)
>>>>>> +{
>>>>>> +    __asm__ __volatile__("str %0, [%1]"::"r"(value), "r"(addr));
>>>>>> +    wmb();
>>>>>> +}
>>
>> write barrier may be necessary on some, where we need to wait that all
>> write has been done before doing this one (such as enable the GIC ...).
>>
>> So this function is buggy. It should be:
>>
>> wmb();
>> __asm__ __volatile__(....).
> 
> gic_init does an explicit wmb() before enabling the GIC anyway,
> although I'm not really sure why it's needed (these barriers are from
> Karim's original code, so I don't know the original reason for them).
> Xen will have marked the GIC memory as device memory, so I guess we're
> protected from many effects ("The number, order and sizes of the
> accesses are maintained.").

Device memory doesn't mean the barrier are not necessary... The barriers
are there for the whole memory, not only the GIC memory.

A common use case is sending an SGI. You need to ensure that every
read/write before the SGI will be seen by the other processors.
Otherwise they may not see correctly the data.

> Maybe none of these barriers is necessary.
>>>> I don't really see why such barriers are needed indeed. Are they needed
<>>>> to actually push the values out?
>>>
>>> That would, I think, require an isb() (instruction barrier) whereas
>>> wmb() turns into a dsb() (data barrier). I expect you are write and
>>> these rmb/wmb are not needed, but an isb may be needed in the caller if
>>> they want to rely on the affect of a write (e.g. enabling the
>>> controller)
>>
>> isb is useful for cache and system control registers. We don't use such
>> things in the GIC code, because the GIC register is memory mapped. We
>> only need to ensure that the write are ordered when it's necessary (only
>> few places).
>>
>> Regards,
>>
>> --
>> Julien Grall
> 
> 
> 


-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH ARM v8 2/4] mini-os: arm: interrupt controller

[Thomas Leonard]

On 28 October 2014 15:25, Julien Grall <julien.grall@linaro.org> wrote:
> On 10/28/2014 03:15 PM, Thomas Leonard wrote:
>> On 22 October 2014 14:06, Julien Grall <julien.grall@linaro.org> wrote:
>>> On 10/22/2014 10:03 AM, Ian Campbell wrote:
>>>> On Tue, 2014-10-21 at 23:54 +0200, Samuel Thibault wrote:
>>>>> Ian Campbell, le Tue 21 Oct 2014 12:00:18 +0100, a écrit :
>>>>>> On Fri, 2014-10-03 at 10:20 +0100, Thomas Leonard wrote:
>>>>>>> +static inline uint32_t REG_READ32(volatile uint32_t *addr)
>>>>>>> +{
>>>>>>> +    uint32_t value;
>>>>>>> +    __asm__ __volatile__("ldr %0, [%1]":"=&r"(value):"r"(addr));
>>>>>>> +    rmb();
>>>>>>
>>>>>> I'm not 100% convinced that you need this rmb().
>>>
>>> Most the GIC code doesn't require read barrier but...
>>>
>>>>>>
>>>>>>> +    return value;
>>>>>>> +}
>>>>>>> +
>>>>>>> +static inline void REG_WRITE32(volatile uint32_t *addr, unsigned int value)
>>>>>>> +{
>>>>>>> +    __asm__ __volatile__("str %0, [%1]"::"r"(value), "r"(addr));
>>>>>>> +    wmb();
>>>>>>> +}
>>>
>>> write barrier may be necessary on some, where we need to wait that all
>>> write has been done before doing this one (such as enable the GIC ...).
>>>
>>> So this function is buggy. It should be:
>>>
>>> wmb();
>>> __asm__ __volatile__(....).
>>
>> gic_init does an explicit wmb() before enabling the GIC anyway,
>> although I'm not really sure why it's needed (these barriers are from
>> Karim's original code, so I don't know the original reason for them).
>> Xen will have marked the GIC memory as device memory, so I guess we're
>> protected from many effects ("The number, order and sizes of the
>> accesses are maintained.").
>
> Device memory doesn't mean the barrier are not necessary... The barriers
> are there for the whole memory, not only the GIC memory.
>
> A common use case is sending an SGI. You need to ensure that every
> read/write before the SGI will be seen by the other processors.
> Otherwise they may not see correctly the data.

Right, but I mean in the context of this code. The only things we're doing are:

- enabling interrupts (in gic_init)
- reading and acking an interrupt (in gic_handler)

I don't see that other processors are involved here,

If enabling interrupts is delayed slightly, it shouldn't have any
effect (even if we get to block_domain, wfi will flush the writes).

We might need a dsb after acking the interrupt (the wmb call currently
does this, but it could be more explicit that we need a flush).


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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH ARM v8 2/4] mini-os: arm: interrupt controller

[Julien Grall]

On 10/28/2014 03:43 PM, Thomas Leonard wrote:
> On 28 October 2014 15:25, Julien Grall <julien.grall@linaro.org> wrote:
>> On 10/28/2014 03:15 PM, Thomas Leonard wrote:
>>> On 22 October 2014 14:06, Julien Grall <julien.grall@linaro.org> wrote:
>>>> On 10/22/2014 10:03 AM, Ian Campbell wrote:
>>>>> On Tue, 2014-10-21 at 23:54 +0200, Samuel Thibault wrote:
>>>>>> Ian Campbell, le Tue 21 Oct 2014 12:00:18 +0100, a écrit :
>>>>>>> On Fri, 2014-10-03 at 10:20 +0100, Thomas Leonard wrote:
>>>>>>>> +static inline uint32_t REG_READ32(volatile uint32_t *addr)
>>>>>>>> +{
>>>>>>>> +    uint32_t value;
>>>>>>>> +    __asm__ __volatile__("ldr %0, [%1]":"=&r"(value):"r"(addr));
>>>>>>>> +    rmb();
>>>>>>>
>>>>>>> I'm not 100% convinced that you need this rmb().
>>>>
>>>> Most the GIC code doesn't require read barrier but...
>>>>
>>>>>>>
>>>>>>>> +    return value;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static inline void REG_WRITE32(volatile uint32_t *addr, unsigned int value)
>>>>>>>> +{
>>>>>>>> +    __asm__ __volatile__("str %0, [%1]"::"r"(value), "r"(addr));
>>>>>>>> +    wmb();
>>>>>>>> +}
>>>>
>>>> write barrier may be necessary on some, where we need to wait that all
>>>> write has been done before doing this one (such as enable the GIC ...).
>>>>
>>>> So this function is buggy. It should be:
>>>>
>>>> wmb();
>>>> __asm__ __volatile__(....).
>>>
>>> gic_init does an explicit wmb() before enabling the GIC anyway,
>>> although I'm not really sure why it's needed (these barriers are from
>>> Karim's original code, so I don't know the original reason for them).
>>> Xen will have marked the GIC memory as device memory, so I guess we're
>>> protected from many effects ("The number, order and sizes of the
>>> accesses are maintained.").
>>
>> Device memory doesn't mean the barrier are not necessary... The barriers
>> are there for the whole memory, not only the GIC memory.
>>
>> A common use case is sending an SGI. You need to ensure that every
>> read/write before the SGI will be seen by the other processors.
>> Otherwise they may not see correctly the data.
> 
> Right, but I mean in the context of this code. The only things we're doing are:
> 
> - enabling interrupts (in gic_init)

You need to take care of the barrier/lock for enabling interrupts. Other
processor may be present at that time.

Though IIRC, mini-os for ARM is not yet SMP.

> - reading and acking an interrupt (in gic_handler)

You don't care for this part as it's per CPU.

[..]

> If enabling interrupts is delayed slightly, it shouldn't have any
> effect (even if we get to block_domain, wfi will flush the writes).

Relying on a later flush that is currently existing is not the right
thing to do. You don't know how mini-os will evolve.

You have to check if barriers are needed everywhere.

Regards,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH ARM v8 1/4] mini-os: arm: time

[Thomas Leonard]

On 27 October 2014 10:34, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Sun, 2014-10-26 at 09:51 +0000, Thomas Leonard wrote:
>> On 21 October 2014 11:50, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>> > On Fri, 2014-10-03 at 10:20 +0100, Thomas Leonard wrote:
>> >> Based on an initial patch by Karim Raslan.
>> >>
>> >> Signed-off-by: Karim Allah Ahmed <karim.allah.ahmed@gmail.com>
>> >> Signed-off-by: Thomas Leonard <talex5@gmail.com>
>> >
>> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
>> >
>> >> +/* Wall-clock time is not currently available on ARM, so this is always zero for now:
>> >> + * http://wiki.xenproject.org/wiki/Xen_ARM_TODO#Expose_Wallclock_time_to_guests
>> >
>> > I have some slightly hacky patches for this, I really should dust them
>> > off and submit them...
>> >
>> >> +void block_domain(s_time_t until)
>> >> +{
>> >> +    uint64_t until_count = ns_to_ticks(until) + cntvct_at_init;
>> >> +    ASSERT(irqs_disabled());
>> >> +    if (read_virtual_count() < until_count)
>> >> +    {
>> >> +        set_vtimer_compare(until_count);
>> >> +        __asm__ __volatile__("wfi");
>> >> +        unset_vtimer_compare();
>> >> +
>> >> +        /* Give the IRQ handler a chance to handle whatever woke us up. */
>> >> +        local_irq_enable();
>> >> +        local_irq_disable();
>> >> +    }
>> >
>> > Just wondering, is this not roughly equivalent to a wfi loop with
>> > interrupts enabled?
>>
>> I'm not quite sure what you mean.
>>
>> If we enable interrupts before the wfi then I think the following could occur:
>>
>> 1. Application checks for work, finds none and calls block_domain.
>> 2. block_domain enables interrupts.
>> 3. An interrupt occurs.
>> 4. The interrupt handler sets a flag indicating work to do.
>> 5. wfi is called, putting the domain to sleep, even though there is work to do.
>>
>> Enabling IRQs after block_domain ensures we can't sleep while we have
>> work to do.
>
> Ah, yes.

So, can this patch be applied as-is now?


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

Re: [PATCH ARM v8 2/4] mini-os: arm: interrupt controller

[Thomas Leonard]

On 28 October 2014 15:51, Julien Grall <julien.grall@linaro.org> wrote:
> On 10/28/2014 03:43 PM, Thomas Leonard wrote:
>> On 28 October 2014 15:25, Julien Grall <julien.grall@linaro.org> wrote:
>>> On 10/28/2014 03:15 PM, Thomas Leonard wrote:
>>>> On 22 October 2014 14:06, Julien Grall <julien.grall@linaro.org> wrote:
>>>>> On 10/22/2014 10:03 AM, Ian Campbell wrote:
>>>>>> On Tue, 2014-10-21 at 23:54 +0200, Samuel Thibault wrote:
>>>>>>> Ian Campbell, le Tue 21 Oct 2014 12:00:18 +0100, a écrit :
>>>>>>>> On Fri, 2014-10-03 at 10:20 +0100, Thomas Leonard wrote:
>>>>>>>>> +static inline uint32_t REG_READ32(volatile uint32_t *addr)
>>>>>>>>> +{
>>>>>>>>> +    uint32_t value;
>>>>>>>>> +    __asm__ __volatile__("ldr %0, [%1]":"=&r"(value):"r"(addr));
>>>>>>>>> +    rmb();
>>>>>>>>
>>>>>>>> I'm not 100% convinced that you need this rmb().
>>>>>
>>>>> Most the GIC code doesn't require read barrier but...
>>>>>
>>>>>>>>
>>>>>>>>> +    return value;
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> +static inline void REG_WRITE32(volatile uint32_t *addr, unsigned int value)
>>>>>>>>> +{
>>>>>>>>> +    __asm__ __volatile__("str %0, [%1]"::"r"(value), "r"(addr));
>>>>>>>>> +    wmb();
>>>>>>>>> +}
>>>>>
>>>>> write barrier may be necessary on some, where we need to wait that all
>>>>> write has been done before doing this one (such as enable the GIC ...).
>>>>>
>>>>> So this function is buggy. It should be:
>>>>>
>>>>> wmb();
>>>>> __asm__ __volatile__(....).
>>>>
>>>> gic_init does an explicit wmb() before enabling the GIC anyway,
>>>> although I'm not really sure why it's needed (these barriers are from
>>>> Karim's original code, so I don't know the original reason for them).
>>>> Xen will have marked the GIC memory as device memory, so I guess we're
>>>> protected from many effects ("The number, order and sizes of the
>>>> accesses are maintained.").
>>>
>>> Device memory doesn't mean the barrier are not necessary... The barriers
>>> are there for the whole memory, not only the GIC memory.
>>>
>>> A common use case is sending an SGI. You need to ensure that every
>>> read/write before the SGI will be seen by the other processors.
>>> Otherwise they may not see correctly the data.
>>
>> Right, but I mean in the context of this code. The only things we're doing are:
>>
>> - enabling interrupts (in gic_init)
>
> You need to take care of the barrier/lock for enabling interrupts. Other
> processor may be present at that time.
>
> Though IIRC, mini-os for ARM is not yet SMP.

It's difficult to see what needs to be done to support code that
doesn't exist yet. For example, would gic_init be called once, or once
per processor? Before or after the other processors are started?

>> - reading and acking an interrupt (in gic_handler)
>
> You don't care for this part as it's per CPU.

Just to confirm: you mean that writing to the GIC registers happens
immediately, without buffering? i.e. calling gic_eoir() and then
reenabling interrupts won't trigger again because the ack hasn't
reached the GIC yet?

> [..]
>
>> If enabling interrupts is delayed slightly, it shouldn't have any
>> effect (even if we get to block_domain, wfi will flush the writes).
>
> Relying on a later flush that is currently existing is not the right
> thing to do. You don't know how mini-os will evolve.
>
> You have to check if barriers are needed everywhere.

Could you give a concrete example of where not having a barrier would
cause a problem? As far as I can see:

- If the caller of gic_init requires something to reach RAM, disk or
whatever before interrupts are enabled, then that caller should add
the barrier. gic.c can't know what needs flushing first.

- Operations performed by gic.c are synchronous, because they write to
device memory (so the processor can't reorder them) and use __asm__
__volatile__ (preventing the compiler from reordering them). So there
should be no need for a barrier afterwards, or in between operations
either.


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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH ARM v8 1/4] mini-os: arm: time

[Ian Campbell]

On Thu, 2014-11-13 at 16:29 +0000, Thomas Leonard wrote:
> On 27 October 2014 10:34, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > On Sun, 2014-10-26 at 09:51 +0000, Thomas Leonard wrote:
> >> On 21 October 2014 11:50, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> >> > On Fri, 2014-10-03 at 10:20 +0100, Thomas Leonard wrote:
> >> >> Based on an initial patch by Karim Raslan.
> >> >>
> >> >> Signed-off-by: Karim Allah Ahmed <karim.allah.ahmed@gmail.com>
> >> >> Signed-off-by: Thomas Leonard <talex5@gmail.com>
> >> >
> >> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> >> >
> >> >> +/* Wall-clock time is not currently available on ARM, so this is always zero for now:
> >> >> + * http://wiki.xenproject.org/wiki/Xen_ARM_TODO#Expose_Wallclock_time_to_guests
> >> >
> >> > I have some slightly hacky patches for this, I really should dust them
> >> > off and submit them...
> >> >
> >> >> +void block_domain(s_time_t until)
> >> >> +{
> >> >> +    uint64_t until_count = ns_to_ticks(until) + cntvct_at_init;
> >> >> +    ASSERT(irqs_disabled());
> >> >> +    if (read_virtual_count() < until_count)
> >> >> +    {
> >> >> +        set_vtimer_compare(until_count);
> >> >> +        __asm__ __volatile__("wfi");
> >> >> +        unset_vtimer_compare();
> >> >> +
> >> >> +        /* Give the IRQ handler a chance to handle whatever woke us up. */
> >> >> +        local_irq_enable();
> >> >> +        local_irq_disable();
> >> >> +    }
> >> >
> >> > Just wondering, is this not roughly equivalent to a wfi loop with
> >> > interrupts enabled?
> >>
> >> I'm not quite sure what you mean.
> >>
> >> If we enable interrupts before the wfi then I think the following could occur:
> >>
> >> 1. Application checks for work, finds none and calls block_domain.
> >> 2. block_domain enables interrupts.
> >> 3. An interrupt occurs.
> >> 4. The interrupt handler sets a flag indicating work to do.
> >> 5. wfi is called, putting the domain to sleep, even though there is work to do.
> >>
> >> Enabling IRQs after block_domain ensures we can't sleep while we have
> >> work to do.
> >
> > Ah, yes.
> 
> So, can this patch be applied as-is now?

We are now post-rc2 in the 4.5.0 release process, so the answer would be
"needs a release exception, but it's a feature so probably not" (and it
would have been a bit dubious towards the end of October too, which was
post rc1, and feature freeze was the end of September in any case).

However this is part of a new mini-os port which isn't even hooked into
the main build system yet (AFAICT), so in that sense it is utterly
harmless to apply. On the other hand there is a bunch more patches to
come which are needed to make the mini-os port actually useful, and I'm
not sure those are all utterly harmless e.g. to common or x86 code (as
in I've not gone looked at the diffstat for the remaining patches), so
in that sense there's no harm waiting for 4.6 development to open.

I defer to the release manager (Konrad, CCd) on this...

Re: [PATCH ARM v8 2/4] mini-os: arm: interrupt controller

[Julien Grall]

Hi Thomas,

On 14/11/2014 10:22, Thomas Leonard wrote:
> On 28 October 2014 15:51, Julien Grall <julien.grall@linaro.org> wrote:
>> On 10/28/2014 03:43 PM, Thomas Leonard wrote:
>>> On 28 October 2014 15:25, Julien Grall <julien.grall@linaro.org> wrote:
>>>> On 10/28/2014 03:15 PM, Thomas Leonard wrote:
>>>>> On 22 October 2014 14:06, Julien Grall <julien.grall@linaro.org> wrote:
>>>>>> On 10/22/2014 10:03 AM, Ian Campbell wrote:
>>>>>>> On Tue, 2014-10-21 at 23:54 +0200, Samuel Thibault wrote:
>>>>>>>> Ian Campbell, le Tue 21 Oct 2014 12:00:18 +0100, a écrit :
>>>>>>>>> On Fri, 2014-10-03 at 10:20 +0100, Thomas Leonard wrote:
>>>>>>>>>> +static inline uint32_t REG_READ32(volatile uint32_t *addr)
>>>>>>>>>> +{
>>>>>>>>>> +    uint32_t value;
>>>>>>>>>> +    __asm__ __volatile__("ldr %0, [%1]":"=&r"(value):"r"(addr));
>>>>>>>>>> +    rmb();
>>>>>>>>>
>>>>>>>>> I'm not 100% convinced that you need this rmb().
>>>>>>
>>>>>> Most the GIC code doesn't require read barrier but...
>>>>>>
>>>>>>>>>
>>>>>>>>>> +    return value;
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +static inline void REG_WRITE32(volatile uint32_t *addr, unsigned int value)
>>>>>>>>>> +{
>>>>>>>>>> +    __asm__ __volatile__("str %0, [%1]"::"r"(value), "r"(addr));
>>>>>>>>>> +    wmb();
>>>>>>>>>> +}
>>>>>>
>>>>>> write barrier may be necessary on some, where we need to wait that all
>>>>>> write has been done before doing this one (such as enable the GIC ...).
>>>>>>
>>>>>> So this function is buggy. It should be:
>>>>>>
>>>>>> wmb();
>>>>>> __asm__ __volatile__(....).
>>>>>
>>>>> gic_init does an explicit wmb() before enabling the GIC anyway,
>>>>> although I'm not really sure why it's needed (these barriers are from
>>>>> Karim's original code, so I don't know the original reason for them).
>>>>> Xen will have marked the GIC memory as device memory, so I guess we're
>>>>> protected from many effects ("The number, order and sizes of the
>>>>> accesses are maintained.").
>>>>
>>>> Device memory doesn't mean the barrier are not necessary... The barriers
>>>> are there for the whole memory, not only the GIC memory.
>>>>
>>>> A common use case is sending an SGI. You need to ensure that every
>>>> read/write before the SGI will be seen by the other processors.
>>>> Otherwise they may not see correctly the data.
>>>
>>> Right, but I mean in the context of this code. The only things we're doing are:
>>>
>>> - enabling interrupts (in gic_init)
>>
>> You need to take care of the barrier/lock for enabling interrupts. Other
>> processor may be present at that time.
>>
>> Though IIRC, mini-os for ARM is not yet SMP.
>
> It's difficult to see what needs to be done to support code that
> doesn't exist yet. For example, would gic_init be called once, or once
> per processor? Before or after the other processors are started?

Some part of the code maybe necessary: setting PPIs/SGIs... You can give 
a look to Linux/Xen GIC drivers to have an idea on what is necessary.

>>> - reading and acking an interrupt (in gic_handler)
>>
>> You don't care for this part as it's per CPU.
>
> Just to confirm: you mean that writing to the GIC registers happens
> immediately, without buffering? i.e. calling gic_eoir() and then
> reenabling interrupts won't trigger again because the ack hasn't
> reached the GIC yet?

You don't care if the EOI takes few miliseconds more to reach the GICC 
interface. The same interrupt won't trigger until the EOI has been handling.

>> [..]
>>
>>> If enabling interrupts is delayed slightly, it shouldn't have any
>>> effect (even if we get to block_domain, wfi will flush the writes).
>>
>> Relying on a later flush that is currently existing is not the right
>> thing to do. You don't know how mini-os will evolve.
>>
>> You have to check if barriers are needed everywhere.
>
> Could you give a concrete example of where not having a barrier would
> cause a problem? As far as I can see:
>
> - If the caller of gic_init requires something to reach RAM, disk or
> whatever before interrupts are enabled, then that caller should add
> the barrier. gic.c can't know what needs flushing first.

What do you mean by flushing? Cache?

gic.c may require to put data in RAM too... a barrier in the IRQ 
enabling code will make sure that the data is seen by the other processor.

> - Operations performed by gic.c are synchronous, because they write to
> device memory (so the processor can't reorder them) and use __asm__
> __volatile__ (preventing the compiler from reordering them). So there
> should be no need for a barrier afterwards, or in between operations
> either.

The GICv2 is divided in 2 parts: GICC and GICD. The former is per-cpu 
while the latter is shared.

I agree that Device memory is synchronous. But with SMP you never know 
which processor will write/read first on this region. So you have to 
take lock to order them.

Regards,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH ARM v8 2/4] mini-os: arm: interrupt controller

[Ian Campbell]

On Fri, 2014-11-14 at 11:33 +0000, Julien Grall wrote:
> >> Though IIRC, mini-os for ARM is not yet SMP.
> >
> > It's difficult to see what needs to be done to support code that
> > doesn't exist yet. For example, would gic_init be called once, or once
> > per processor? Before or after the other processors are started?
> 
> Some part of the code maybe necessary: setting PPIs/SGIs... You can give 
> a look to Linux/Xen GIC drivers to have an idea on what is necessary.

This would be necessary as part of the port of mini-os to support SMP. I
don't see why it should be necessary now, going to SMP is surely going
to require a bunch of refactoring in other places too (Xen itself
certainly did).

Ian.

Re: [PATCH ARM v8 2/4] mini-os: arm: interrupt controller

[Julien Grall]

On 14/11/2014 11:42, Ian Campbell wrote:
> On Fri, 2014-11-14 at 11:33 +0000, Julien Grall wrote:
>>>> Though IIRC, mini-os for ARM is not yet SMP.
>>>
>>> It's difficult to see what needs to be done to support code that
>>> doesn't exist yet. For example, would gic_init be called once, or once
>>> per processor? Before or after the other processors are started?
>>
>> Some part of the code maybe necessary: setting PPIs/SGIs... You can give
>> a look to Linux/Xen GIC drivers to have an idea on what is necessary.
>
> This would be necessary as part of the port of mini-os to support SMP. I
> don't see why it should be necessary now, going to SMP is surely going
> to require a bunch of refactoring in other places too (Xen itself
> certainly did).

I just answered to his question. I didn't asked for changing this code 
right now.

-- 
Julien Grall

Re: [PATCH ARM v8 2/4] mini-os: arm: interrupt controller

[Ian Campbell]

On Fri, 2014-11-14 at 11:48 +0000, Julien Grall wrote:
> 
> On 14/11/2014 11:42, Ian Campbell wrote:
> > On Fri, 2014-11-14 at 11:33 +0000, Julien Grall wrote:
> >>>> Though IIRC, mini-os for ARM is not yet SMP.
> >>>
> >>> It's difficult to see what needs to be done to support code that
> >>> doesn't exist yet. For example, would gic_init be called once, or once
> >>> per processor? Before or after the other processors are started?
> >>
> >> Some part of the code maybe necessary: setting PPIs/SGIs... You can give
> >> a look to Linux/Xen GIC drivers to have an idea on what is necessary.
> >
> > This would be necessary as part of the port of mini-os to support SMP. I
> > don't see why it should be necessary now, going to SMP is surely going
> > to require a bunch of refactoring in other places too (Xen itself
> > certainly did).
> 
> I just answered to his question. I didn't asked for changing this code 
> right now.

I'm afraid you haven't made that at all clear until now.

Ian.

Re: [PATCH ARM v8 3/4] mini-os: arm: build system

[Thomas Leonard]

On 26 October 2014 10:25, Thomas Leonard <talex5@gmail.com> wrote:
> On 26 October 2014 09:55, Samuel Thibault <samuel.thibault@ens-lyon.org> wrote:
>> Thomas Leonard, le Sun 26 Oct 2014 09:46:09 +0000, a écrit :
>>> Could you say a bit more about the linker problems you had?
>>
>> Really digging in the archives this time :)
>>
>> ia64-specific:
>> http://lists.xen.org/archives/html/xen-ia64-devel/2008-03/msg00126.html
>> http://lists.xen.org/archives/html/xen-ia64-devel/2008-12/msg00070.html
>> x86_64-specific (missing red zone support)
>> http://lists.xen.org/archives/html/xen-ia64-devel/2008-02/msg00251.html
>>
>> So I guess it could be OK on arm, but you have to make sure that Mini-OS
>> implements the whole ABI that gcc will use. Testing is not enough, I got
>> hit by the red zone for instance.
>
> On ARM, we have a separate stack for the IRQ handler, so the red zone
> at least shouldn't be a problem.

Incidentally, why doesn't Mini-OS/x86 use a red zone? I assume there's
a worthwhile performance benefit to it, and it would prevent subtle
bugs when software is accidentally compiled for the normal ABI.


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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH ARM v8 3/4] mini-os: arm: build system

[Ian Campbell]

On Mon, 2014-11-17 at 11:42 +0000, Thomas Leonard wrote:
> On 26 October 2014 10:25, Thomas Leonard <talex5@gmail.com> wrote:
> > On 26 October 2014 09:55, Samuel Thibault <samuel.thibault@ens-lyon.org> wrote:
> >> Thomas Leonard, le Sun 26 Oct 2014 09:46:09 +0000, a écrit :
> >>> Could you say a bit more about the linker problems you had?
> >>
> >> Really digging in the archives this time :)
> >>
> >> ia64-specific:
> >> http://lists.xen.org/archives/html/xen-ia64-devel/2008-03/msg00126.html
> >> http://lists.xen.org/archives/html/xen-ia64-devel/2008-12/msg00070.html
> >> x86_64-specific (missing red zone support)
> >> http://lists.xen.org/archives/html/xen-ia64-devel/2008-02/msg00251.html
> >>
> >> So I guess it could be OK on arm, but you have to make sure that Mini-OS
> >> implements the whole ABI that gcc will use. Testing is not enough, I got
> >> hit by the red zone for instance.
> >
> > On ARM, we have a separate stack for the IRQ handler, so the red zone
> > at least shouldn't be a problem.
> 
> Incidentally, why doesn't Mini-OS/x86 use a red zone? I assume there's
> a worthwhile performance benefit to it, and it would prevent subtle
> bugs when software is accidentally compiled for the normal ABI.

Redzones are incompatible with taking interrupts at the same privilege
level (which implies no stack switch, hence clobbering), and mini-os
runs everything at kernel privilege level.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH ARM v8 3/4] mini-os: arm: build system

[Andrew Cooper]

On 17/11/14 11:42, Thomas Leonard wrote:
> On 26 October 2014 10:25, Thomas Leonard <talex5@gmail.com> wrote:
>> On 26 October 2014 09:55, Samuel Thibault <samuel.thibault@ens-lyon.org> wrote:
>>> Thomas Leonard, le Sun 26 Oct 2014 09:46:09 +0000, a écrit :
>>>> Could you say a bit more about the linker problems you had?
>>> Really digging in the archives this time :)
>>>
>>> ia64-specific:
>>> http://lists.xen.org/archives/html/xen-ia64-devel/2008-03/msg00126.html
>>> http://lists.xen.org/archives/html/xen-ia64-devel/2008-12/msg00070.html
>>> x86_64-specific (missing red zone support)
>>> http://lists.xen.org/archives/html/xen-ia64-devel/2008-02/msg00251.html
>>>
>>> So I guess it could be OK on arm, but you have to make sure that Mini-OS
>>> implements the whole ABI that gcc will use. Testing is not enough, I got
>>> hit by the red zone for instance.
>> On ARM, we have a separate stack for the IRQ handler, so the red zone
>> at least shouldn't be a problem.
> Incidentally, why doesn't Mini-OS/x86 use a red zone? I assume there's
> a worthwhile performance benefit to it, and it would prevent subtle
> bugs when software is accidentally compiled for the normal ABI.
>
>

Kernel code (intended for ring0) cannot use red zones as a stack switch
doesn't occur when an exception/interrupt occurs.  Compiling any kernel
code with red zone support leads to subtle clobbering of state.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH ARM v8 3/4] mini-os: arm: build system

[Samuel Thibault]

Thomas Leonard, le Mon 17 Nov 2014 11:42:38 +0000, a écrit :
> On 26 October 2014 10:25, Thomas Leonard <talex5@gmail.com> wrote:
> > On 26 October 2014 09:55, Samuel Thibault <samuel.thibault@ens-lyon.org> wrote:
> >> Thomas Leonard, le Sun 26 Oct 2014 09:46:09 +0000, a écrit :
> >>> Could you say a bit more about the linker problems you had?
> >>
> >> Really digging in the archives this time :)
> >>
> >> ia64-specific:
> >> http://lists.xen.org/archives/html/xen-ia64-devel/2008-03/msg00126.html
> >> http://lists.xen.org/archives/html/xen-ia64-devel/2008-12/msg00070.html
> >> x86_64-specific (missing red zone support)
> >> http://lists.xen.org/archives/html/xen-ia64-devel/2008-02/msg00251.html
> >>
> >> So I guess it could be OK on arm, but you have to make sure that Mini-OS
> >> implements the whole ABI that gcc will use. Testing is not enough, I got
> >> hit by the red zone for instance.
> >
> > On ARM, we have a separate stack for the IRQ handler, so the red zone
> > at least shouldn't be a problem.
> 
> Incidentally, why doesn't Mini-OS/x86 use a red zone?

AIUI, we have nothing in x86 to make interrupts use another stack, so we
can't prevent them from overwriting the red zone even just a bit.

Samuel

Re: [PATCH ARM v8 1/4] mini-os: arm: time

[Konrad Rzeszutek Wilk]

On Fri, Nov 14, 2014 at 10:29:26AM +0000, Ian Campbell wrote:
> On Thu, 2014-11-13 at 16:29 +0000, Thomas Leonard wrote:
> > On 27 October 2014 10:34, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > > On Sun, 2014-10-26 at 09:51 +0000, Thomas Leonard wrote:
> > >> On 21 October 2014 11:50, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > >> > On Fri, 2014-10-03 at 10:20 +0100, Thomas Leonard wrote:
> > >> >> Based on an initial patch by Karim Raslan.
> > >> >>
> > >> >> Signed-off-by: Karim Allah Ahmed <karim.allah.ahmed@gmail.com>
> > >> >> Signed-off-by: Thomas Leonard <talex5@gmail.com>
> > >> >
> > >> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > >> >
> > >> >> +/* Wall-clock time is not currently available on ARM, so this is always zero for now:
> > >> >> + * http://wiki.xenproject.org/wiki/Xen_ARM_TODO#Expose_Wallclock_time_to_guests
> > >> >
> > >> > I have some slightly hacky patches for this, I really should dust them
> > >> > off and submit them...
> > >> >
> > >> >> +void block_domain(s_time_t until)
> > >> >> +{
> > >> >> +    uint64_t until_count = ns_to_ticks(until) + cntvct_at_init;
> > >> >> +    ASSERT(irqs_disabled());
> > >> >> +    if (read_virtual_count() < until_count)
> > >> >> +    {
> > >> >> +        set_vtimer_compare(until_count);
> > >> >> +        __asm__ __volatile__("wfi");
> > >> >> +        unset_vtimer_compare();
> > >> >> +
> > >> >> +        /* Give the IRQ handler a chance to handle whatever woke us up. */
> > >> >> +        local_irq_enable();
> > >> >> +        local_irq_disable();
> > >> >> +    }
> > >> >
> > >> > Just wondering, is this not roughly equivalent to a wfi loop with
> > >> > interrupts enabled?
> > >>
> > >> I'm not quite sure what you mean.
> > >>
> > >> If we enable interrupts before the wfi then I think the following could occur:
> > >>
> > >> 1. Application checks for work, finds none and calls block_domain.
> > >> 2. block_domain enables interrupts.
> > >> 3. An interrupt occurs.
> > >> 4. The interrupt handler sets a flag indicating work to do.
> > >> 5. wfi is called, putting the domain to sleep, even though there is work to do.
> > >>
> > >> Enabling IRQs after block_domain ensures we can't sleep while we have
> > >> work to do.
> > >
> > > Ah, yes.
> > 
> > So, can this patch be applied as-is now?
> 
> We are now post-rc2 in the 4.5.0 release process, so the answer would be
> "needs a release exception, but it's a feature so probably not" (and it
> would have been a bit dubious towards the end of October too, which was
> post rc1, and feature freeze was the end of September in any case).
> 
> However this is part of a new mini-os port which isn't even hooked into
> the main build system yet (AFAICT), so in that sense it is utterly
> harmless to apply. On the other hand there is a bunch more patches to
> come which are needed to make the mini-os port actually useful, and I'm
> not sure those are all utterly harmless e.g. to common or x86 code (as
> in I've not gone looked at the diffstat for the remaining patches), so
> in that sense there's no harm waiting for 4.6 development to open.
> 
> I defer to the release manager (Konrad, CCd) on this...

I would prefer to defer this to Xen 4.6 to keep the amount of patches
going in staging to be bug-fixes.

Thank you.
> 
> 

Re: [PATCH ARM v8 1/4] mini-os: arm: time

[Ian Campbell]

On Tue, 2014-10-21 at 11:50 +0100, Ian Campbell wrote:
> On Fri, 2014-10-03 at 10:20 +0100, Thomas Leonard wrote:
> > Based on an initial patch by Karim Raslan.
> > 
> > Signed-off-by: Karim Allah Ahmed <karim.allah.ahmed@gmail.com>
> > Signed-off-by: Thomas Leonard <talex5@gmail.com>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

Applied (for 4.6), thanks.

#2 had comments and #3 didn't make sense without it, I think, and had
comments of its own.

It looks like #4 could go in independently of those two, except for the
change to extras/mini-os/ARM-TODO.txt (added by #3). I could drop that
hunk as I apply -- thoughts?

Re: [PATCH ARM v8 1/4] mini-os: arm: time

[Thomas Leonard]

On 8 January 2015 at 15:52, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Tue, 2014-10-21 at 11:50 +0100, Ian Campbell wrote:
>> On Fri, 2014-10-03 at 10:20 +0100, Thomas Leonard wrote:
>> > Based on an initial patch by Karim Raslan.
>> >
>> > Signed-off-by: Karim Allah Ahmed <karim.allah.ahmed@gmail.com>
>> > Signed-off-by: Thomas Leonard <talex5@gmail.com>
>>
>> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>
> Applied (for 4.6), thanks.
>
> #2 had comments and #3 didn't make sense without it, I think, and had
> comments of its own.
>
> It looks like #4 could go in independently of those two, except for the
> change to extras/mini-os/ARM-TODO.txt (added by #3). I could drop that
> hunk as I apply -- thoughts?

Yes, just drop it if applying #4 first. Thanks!


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

Re: [PATCH ARM v8 1/4] mini-os: arm: time

[Ian Campbell]

On Thu, 2015-01-08 at 15:58 +0000, Thomas Leonard wrote:
> On 8 January 2015 at 15:52, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > On Tue, 2014-10-21 at 11:50 +0100, Ian Campbell wrote:
> >> On Fri, 2014-10-03 at 10:20 +0100, Thomas Leonard wrote:
> >> > Based on an initial patch by Karim Raslan.
> >> >
> >> > Signed-off-by: Karim Allah Ahmed <karim.allah.ahmed@gmail.com>
> >> > Signed-off-by: Thomas Leonard <talex5@gmail.com>
> >>
> >> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> >
> > Applied (for 4.6), thanks.
> >
> > #2 had comments and #3 didn't make sense without it, I think, and had
> > comments of its own.
> >
> > It looks like #4 could go in independently of those two, except for the
> > change to extras/mini-os/ARM-TODO.txt (added by #3). I could drop that
> > hunk as I apply -- thoughts?
> 
> Yes, just drop it if applying #4 first. Thanks!

Done, thanks.