[Top] [All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH ARM v5 19/20] mini-os: initial ARM support



On 26/06/14 12:28, Thomas Leonard wrote:

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

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

Signed-off-by: Karim Allah Ahmed <karim.allah.ahmed@xxxxxxxxx>
[talex5@xxxxxxxxx: made x86_64 support work again]
[talex5@xxxxxxxxx: split into multiple patches]
[talex5@xxxxxxxxx: re-enabled force_evtchn_callback]
[talex5@xxxxxxxxx: enable regular console]
[talex5@xxxxxxxxx: fixed initialisation code:
- Configure write-back caching in page table. This is needed for
   reliable hypercalls to Xen (thanks to Julien Grall).
- Use "client mode" for access control (domains are deprecated,
   according to ARM Cortex-A Series Programmerâs Guide version 4.0,
   section 9.6.4).
- Enable more SCTLR features (icache, branch prediction)]
[talex5@xxxxxxxxx: use Virtual Count register for monotonic time]
[talex5@xxxxxxxxx: fixed HYPERVISOR_shutdown]
[talex5@xxxxxxxxx: get xenstore details from hypervisor]
[talex5@xxxxxxxxx: use GCC implementation of division]
[talex5@xxxxxxxxx: cleaned up interrupt handlers and threading]
[talex5@xxxxxxxxx: call exit_thread when a thread returns]
[talex5@xxxxxxxxx: implemented block_domain for ARM]
[talex5@xxxxxxxxx: fixed hang when enabling interrupts]
[talex5@xxxxxxxxx: added -march=armv7-a to flags]
[talex5@xxxxxxxxx: CLREX after handling IRQs]
[talex5@xxxxxxxxx: unbind debug port at shutdown]
[talex5@xxxxxxxxx: allow unaligned accesses]
[talex5@xxxxxxxxx: fix zImage header for XSA-95]
[talex5@xxxxxxxxx: get RAM base and size from the FDT]
[talex5@xxxxxxxxx: get GIC addresses from FDT]
[talex5@xxxxxxxxx: added ARM grant table initialisation]
[talex5@xxxxxxxxx: added missing copyright header to hypercalls32.S]
[talex5@xxxxxxxxx: moved GIC driver to arm directory]
[talex5@xxxxxxxxx: fixes suggested by Julien Grall:
- Removed unnecessary isb.
- Renamed GICD_PRIORITY to GICD_IPRIORITYR.
- Change IRQ number type from unsigned char to int.
- Added volatile to {set,clear}_bit_non_atomic.
- Fixed some comments.
- Check compatible properties in DTB.]
[talex5@xxxxxxxxx: made image relocatable]
[talex5@xxxxxxxxx: added mfn_to_pfn and pfn_to_mfn]
IHMO all these changes doesn't belong to the commit message, i.e should not appear in the commit message when Ian will apply your commit message.
I would move them in a changelog. You can separate the commit message and the changelog by --- and a newline. Everything after the --- will be removed by git am. 


Signed-off-by: Thomas Leonard <talex5@xxxxxxxxx>
---
  extras/mini-os/ARM-TODO.txt              |   6 +
  extras/mini-os/Config.mk                 |   2 +
  extras/mini-os/Makefile                  |   9 ++
  extras/mini-os/arch/arm/Makefile         |  32 ++++
  extras/mini-os/arch/arm/arch.mk          |   7 +
  extras/mini-os/arch/arm/arm32.S          | 266 +++++++++++++++++++++++++++++++
  extras/mini-os/arch/arm/events.c         |  30 ++++
  extras/mini-os/arch/arm/gic.c            | 222 ++++++++++++++++++++++++++
  extras/mini-os/arch/arm/hypercalls32.S   |  75 +++++++++
  extras/mini-os/arch/arm/minios-arm32.lds |  75 +++++++++
  extras/mini-os/arch/arm/mm.c             | 134 ++++++++++++++++
  extras/mini-os/arch/arm/sched.c          |  37 +++++
  extras/mini-os/arch/arm/setup.c          | 116 ++++++++++++++
  extras/mini-os/arch/arm/time.c           | 202 +++++++++++++++++++++++
  extras/mini-os/kernel.c                  |   2 +-
  15 files changed, 1214 insertions(+), 1 deletion(-)
  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/arm32.S
  create mode 100644 extras/mini-os/arch/arm/events.c
  create mode 100644 extras/mini-os/arch/arm/gic.c
  create mode 100644 extras/mini-os/arch/arm/hypercalls32.S
  create mode 100755 extras/mini-os/arch/arm/minios-arm32.lds
  create mode 100644 extras/mini-os/arch/arm/mm.c
  create mode 100644 extras/mini-os/arch/arm/sched.c
  create mode 100644 extras/mini-os/arch/arm/setup.c
  create mode 100644 extras/mini-os/arch/arm/time.c

diff --git a/extras/mini-os/Config.mk b/extras/mini-os/Config.mk
index d61877b..4ecde54 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 931cd05..01d8af0 100644
--- a/extras/mini-os/Makefile
+++ b/extras/mini-os/Makefile
@@ -78,6 +78,9 @@ TARGET := mini-os
  SUBDIRS := lib xenbus console

  ifeq ($(XEN_TARGET_ARCH),arm32)
+# Need libgcc.a for division helpers
+LDLIBS += `$(CC) -print-libgcc-file-name`


OOI, how much code does add libgcc for the division helpers?


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..ab20d99
--- /dev/null
+++ b/extras/mini-os/arch/arm/arch.mk
@@ -0,0 +1,7 @@
+ifeq ($(XEN_TARGET_ARCH),arm32)
+DEF_ASFLAGS += -march=armv7-a
+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/arch/arm/arm32.S b/extras/mini-os/arch/arm/arm32.S
new file mode 100644
index 0000000..de74ed9
--- /dev/null
+++ b/extras/mini-os/arch/arm/arm32.S
@@ -0,0 +1,266 @@
+@ Virtual address of the start of RAM (any value will do, but it must be
+@ section-aligned). Update the lds script if changed.
+#define VIRT_BASE 0x400000
+
+@ Offset of the kernel within the RAM. This is a zImage convention which we
+@ rely on.
+#define ZIMAGE_KERNEL_OFFSET 0x8000
Hmmm... this is not a zImage convention... IIRC Linux is using this offset to have enough space to create startup page table during boot.
The Linux zImage is able to relocate itself in the memory to respect this convention. But the zImage itself can be loaded anywhere in the memory.
By looking to your code below your are relying that the kernel will be loaded at 0xXXXX8000 which is incorrect. This offset is odd and make other kernel (such as FreeBSD) requiring the same trick which is not part of the Linux boot protocol.
I plan to send a patch to require the start address to be 2MB aligned, so kernels will be able to use 2MB (for LPAE) and 1MB section for there early page table.
If the kernel wants another alignment, then you will have to relocate yourself. Even though in your case, you don't need this odd 0xXXXX8000. 

[..]


+       @ Fill in the whole top-level translation table (at page_dir).
+       @ Populate the whole pagedir with 1MB section descriptors.
+
+       mov     r1, r7                  @ r1 -> first section entry
+       add     r3, r1, #4*4*1024       @ limit (4 GB address space, 4 byte 
entries)
+       orr     r0, r8, r9              @ r0 = entry mapping section zero to 
start of physical RAM
+1:
+       str     r0, [r1],#4             @ write the section entry
+       add     r0, r0, #1 << 20  @ next physical page (wraps)
+       cmp     r1, r3
+       bne     1b
I would document a bit more this part. It took me a bit of time to understand that you mapping the whole address space in an odd manner.
While it's fine for a first version of Mini-OS support for ARM. It think at long term you want to map only necessary bank rank. It will be easier to catch programming error and avoid to trap in the hypervisor because the physical address doesn't exist. 

Futhermore, the data abort sent by the hypervisor is odd (debug smth).


+.pushsection .data
+_data:
+.align 14
+.globl page_dir
+@ Each word maps one 1MB virtual section to a physical section
+page_dir:
+       .fill (4*1024), 4, 0x0
+
+.align 12
+.globl shared_info_page
+shared_info_page:
+       .fill (1024), 4, 0x0
+
+.align 3
+.globl stack
+stack:
+       .fill (4*1024), 4, 0x0
+stack_end:
+
+.align 3
+irqstack:
+       .fill (1024), 4, 0x0
+irqstack_end:
+
+.globl physical_address_offset
+physical_address_offset:
+       .long   0
+
+.popsection


Any reason to define theses variables in ASM rather than 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..5641eb0
--- /dev/null
+++ b/extras/mini-os/arch/arm/gic.c
@@ -0,0 +1,222 @@
+// ARM GIC implementation
+
+#include <mini-os/os.h>
+#include <mini-os/hypervisor.h>
+#include <libfdt.h>
+
+//#define VGIC_DEBUG
+#ifdef VGIC_DEBUG
+#define DEBUG(_f, _a...) \
+    DEBUG("MINI_OS(file=vgic.c, line=%d) " _f , __LINE__, ## _a)


Did you intend to use printk here?

[..]


+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-a9-gic") 
&&
I don't think this compatible is necessary. Cortex A9 doesn't support virtualisation. 


+                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);
+            if (reg == NULL || len != 32) {
As asked on the previous version, if you plan to assume specific range size for the time-being, please explain the 32. 


+                /* TODO: support other formats */
+                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]));
AFAIU, your are mapping the GIC region with normal attribute. With this attribute, the processor may reorder the write in the device memory, preload memory, caching.... Which is completely wrong in this case. I'm sa bit surprised that it works correctly. You have to map thoses regions as device memory. 

[..]


diff --git a/extras/mini-os/arch/arm/hypercalls32.S 
b/extras/mini-os/arch/arm/hypercalls32.S
new file mode 100644
index 0000000..0d7662d
--- /dev/null
+++ b/extras/mini-os/arch/arm/hypercalls32.S


[..]


+#define __HYPERVISOR_memory_op            12
+#define __HYPERVISOR_xen_version          17
+#define __HYPERVISOR_console_io           18
+#define __HYPERVISOR_grant_table_op       20
+#define __HYPERVISOR_vcpu_op              24
+#define __HYPERVISOR_xsm_op               27
+#define __HYPERVISOR_sched_op             29
+#define __HYPERVISOR_event_channel_op     32
+#define __HYPERVISOR_physdev_op           33
+#define __HYPERVISOR_hvm_op               34
+#define __HYPERVISOR_sysctl               35
+#define __HYPERVISOR_domctl               36
Hmmm... why do you hardcode those numbers here? Can't you include the correct header? 

[..]


diff --git a/extras/mini-os/arch/arm/minios-arm32.lds 
b/extras/mini-os/arch/arm/minios-arm32.lds
new file mode 100755
index 0000000..b18ca55
--- /dev/null
+++ b/extras/mini-os/arch/arm/minios-arm32.lds
@@ -0,0 +1,75 @@
+OUTPUT_ARCH(arm)
+ENTRY(_start)
+SECTIONS
+{
+  . = 0x408000;
I would use the preprocessor to avoid hardcoding address here. It will help later if someone wants to change the virtual address of the kernel. 

[..]


diff --git a/extras/mini-os/arch/arm/mm.c b/extras/mini-os/arch/arm/mm.c



[..]


+void arch_init_mm(unsigned long *start_pfn_p, unsigned long *max_pfn_p)


[..]


+
+    /* Xen will always provide us at least one bank of memory.
+     * Mini-OS will use the first bank for the time-being. */
+    regs = fdt_getprop(device_tree, memory, "reg", &prop_len);
+    if (regs == NULL || prop_len != 16) {


Same remark as in gic_init. Please explain where does come from the 16.

[..]


+        /* TODO: support other formats */
+        printk("Bad 'reg' property: %p %d\n", regs, prop_len);
+        BUG();
+    }
+
+    unsigned int end = (unsigned int) &_end;
+    unsigned int mem_base = fdt64_to_cpu(regs[0]);
+    unsigned int mem_size = fdt64_to_cpu(regs[1]);
In general if you have to cast/create a variable to contain a physical address. Please make create a type paddr_t and use it.
When someone will implement arm64 support it will have less trouble because we cast correctly cast the value. 

IIRC, there is few other place where it's the same problem.


+/* Get Xen's suggested physical page assignments for the grant table. */
+static unsigned long get_gnttab_base(void)
+{
+    int hypervisor;
+    int len = 0;
+    const uint64_t *regs;
+    unsigned int gnttab_base;
+
+    hypervisor = fdt_node_offset_by_compatible(device_tree, -1, "xen,xen");
+    BUG_ON(hypervisor < 0);
+
+    regs = fdt_getprop(device_tree, hypervisor, "reg", &len);
+    if (regs == NULL || len != 16) {
+        /* TODO: support other formats */


Please explain why the 16.


+        printk("Bad 'reg' property: %p %d\n", regs, len);
+        BUG();
+    }
+
+    gnttab_base = fdt64_to_cpu(regs[0]);
This look odd... you are using unsigned int for the variable but the function is return unsigned long. 


+    printk("FDT suggests grant table base %lx\n", gnttab_base);
+
+    return gnttab_base;
+}


[..]

Regards,

--
Julien Grall

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

 

©2013 Xen Project, A Linux Foundation Collaborative Project. All Rights Reserved.
Linux Foundation is a registered trademark of The Linux Foundation.
Xen Project is a trademark of The Linux Foundation.

Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.