mini-os: initial ARM support
Subsystems (2)
v8 cover mini-os: initial ARM support
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
Signature · 1 line
2.1.1
PATCH v8 1/4 mini-os: arm: time
Based on an initial patch by Karim Raslan.
---
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
extras/mini-os/arch/arm/time.c new file +136 -0
@@ -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 ) +{ +}
Signature · 1 line
2.1.1
Re: [PATCH ARM v8 1/4] mini-os: arm: time
Quoted 4 lines · Thomas Leonard
Based on an initial patch by Karim Raslan.
Signed-off-by Karim Allah Ahmed <[email protected]>Signed-off-by Thomas Leonard <[email protected]>
Quoted 2 lines
+/* 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...
Quoted 4 lines
extras/mini-os/arch/arm/time.c new file +4
-0+void block_domain (s_time_t until) +{ +uint64_t until_count =ns_to_ticks (until) + cntvct_at_init; +ASSERT (irqs_disabled ());
extras/mini-os/arch/arm/time.c line 122
Quoted 10 lines
extras/mini-os/arch/arm/time.c new file +10
-0+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?
PATCH 1/1 xen: arm: wallclock support (incomplete, needs work/refactoring)
Quoted 5 lines · Ian Campbell
Quoted 2 lines
+/* 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_guestsI 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.
From 8290927257600951622d416c9e76ab9042c4d47d Mon Sep 17 00:00:00 2001
From: Ian Campbell <[email protected]>
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.
--- 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(-)
xen/arch/arm/Makefile +1 -0
@@ -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
xen/arch/arm/domain.c +4 -0
@@ -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 )
xen/arch/arm/time.c +62 -1
81 lines not on this page · raw message
xen/arch/arm/traps.c +1 -0
@@ -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 );
xen/include/xsm/dummy.h +6 -6
@@ -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 );
xen/include/xsm/xsm.h +7 -6
@@ -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 ();
Signature · 1 line
1.7.10.4
From 1284ba2380b32bf62789b738dbac4d99da463ea1 Mon Sep 17 00:00:00 2001
From: Ian Campbell <[email protected]>
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(-)
arch/arm/include/asm/xen/hypercall.h +9 -0
@@ -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
arch/arm/xen/enlighten.c +121 -0
149 lines not on this page · raw message
arch/arm/xen/hypercall.S +3 -1
@@ -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
Signature · 1 line
1.7.10.4
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel
Re: [PATCH ARM v8 1/4] mini-os: arm: time
In reply to Ian CampbellQuoted 23 lines · Ian Campbell
Quoted 4 lines · Thomas Leonard
Based on an initial patch by Karim Raslan.
Signed-off-by Karim Allah Ahmed <[email protected]>Signed-off-by Thomas Leonard <[email protected]>Acked-by Ian Campbell <[email protected]>Quoted 2 lines
+/* 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_guestsI have some slightly hacky patches for this, I really should dust them off and submit them...
Quoted 9 lines
extras/mini-os/arch/arm/time.c new file +9
-0+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 ();
Quoted 8 lines
extras/mini-os/arch/arm/time.c line 122
Quoted 5 lines
extras/mini-os/arch/arm/time.c new file +5
-0+ +/* 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.
Signature · 3 lines
Dr Thomas Leonard http://0install.net/ GPG: 9242 9807 C985 3C07 44A6 8B9A AE07 8280 59A5 3CC1 GPG: DA98 25AE CAD0 8975 7CDA BD8E 0713 3F96 CA74 D8BA
Re: [PATCH ARM v8 1/4] mini-os: arm: time
Quoted 37 lines · Thomas Leonard
Quoted 23 lines · Ian Campbell
Quoted 4 lines · Thomas Leonard
Based on an initial patch by Karim Raslan.
Signed-off-by Karim Allah Ahmed <[email protected]>Signed-off-by Thomas Leonard <[email protected]>Acked-by Ian Campbell <[email protected]>Quoted 2 lines
+/* 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_guestsI have some slightly hacky patches for this, I really should dust them off and submit them...
Quoted 9 lines
extras/mini-os/arch/arm/time.c new file +9
-0+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 ();Quoted 8 lines
extras/mini-os/arch/arm/time.c line 122
Quoted 5 lines
extras/mini-os/arch/arm/time.c new file +5
-0+ +/* 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:
Quoted 8 lines
1. Application checks for work, finds none and calls
block_domain.
2.block_domainenables 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_domainensures we can't sleep while we have work to do.
Ah, yes.
Ian.
Re: [PATCH ARM v8 1/4] mini-os: arm: time
Quoted 44 lines · Ian Campbell
Quoted 43 lines · Thomas Leonard
Quoted 23 lines · Ian Campbell
Quoted 4 lines · Thomas Leonard
Based on an initial patch by Karim Raslan.
Signed-off-by Karim Allah Ahmed <[email protected]>Signed-off-by Thomas Leonard <[email protected]>Acked-by Ian Campbell <[email protected]>Quoted 2 lines
+/* 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_guestsI have some slightly hacky patches for this, I really should dust them off and submit them...
Quoted 9 lines
extras/mini-os/arch/arm/time.c new file +9
-0+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 ();Quoted 8 lines
extras/mini-os/arch/arm/time.c line 122
Quoted 5 lines
extras/mini-os/arch/arm/time.c new file +5
-0+ +/* 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_domainenables 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.
Quoted 4 lines
Quoted 2 lines
Enabling IRQs after
block_domainensures we can't sleep while we have work to do.Ah, yes.
So, can this patch be applied as-is now?
Signature · 3 lines
Dr Thomas Leonard http://0install.net/ GPG: 9242 9807 C985 3C07 44A6 8B9A AE07 8280 59A5 3CC1 GPG: DA98 25AE CAD0 8975 7CDA BD8E 0713 3F96 CA74 D8BA
Re: [PATCH ARM v8 1/4] mini-os: arm: time
Quoted 45 lines · Thomas Leonard
Quoted 44 lines · Ian Campbell
Quoted 43 lines · Thomas Leonard
Quoted 23 lines · Ian Campbell
Quoted 4 lines · Thomas Leonard
Based on an initial patch by Karim Raslan.
Signed-off-by Karim Allah Ahmed <[email protected]>Signed-off-by Thomas Leonard <[email protected]>Acked-by Ian Campbell <[email protected]>Quoted 2 lines
+/* 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_guestsI have some slightly hacky patches for this, I really should dust them off and submit them...
Quoted 9 lines
extras/mini-os/arch/arm/time.c new file +9
-0+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 ();Quoted 8 lines
extras/mini-os/arch/arm/time.c line 122
Quoted 5 lines
extras/mini-os/arch/arm/time.c new file +5
-0+ +/* 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_domainenables 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.
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
Enabling IRQs after
block_domainensures 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 1/4] mini-os: arm: time
Quoted 66 lines · Ian Campbell
Quoted 45 lines · Thomas Leonard
Quoted 44 lines · Ian Campbell
Quoted 43 lines · Thomas Leonard
Quoted 23 lines · Ian Campbell
Quoted 4 lines · Thomas Leonard
Based on an initial patch by Karim Raslan.
Signed-off-by Karim Allah Ahmed <[email protected]>Signed-off-by Thomas Leonard <[email protected]>Acked-by Ian Campbell <[email protected]>Quoted 2 lines
+/* 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_guestsI have some slightly hacky patches for this, I really should dust them off and submit them...
Quoted 9 lines
extras/mini-os/arch/arm/time.c new file +9
-0+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 ();Quoted 8 lines
extras/mini-os/arch/arm/time.c line 122
Quoted 5 lines
extras/mini-os/arch/arm/time.c new file +5
-0+ +/* 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_domainenables 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.Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
Enabling IRQs after
block_domainensures 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.
Quoted 1 line
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.
Quoted 2 lines
Re: [PATCH ARM v8 1/4] mini-os: arm: time
In reply to Ian CampbellQuoted 7 lines · Ian Campbell
Quoted 4 lines · Thomas Leonard
Based on an initial patch by Karim Raslan.
Signed-off-by Karim Allah Ahmed <[email protected]>Signed-off-by Thomas Leonard <[email protected]>Acked-by Ian Campbell <[email protected]>
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
Quoted 9 lines · Ian Campbell
Quoted 7 lines · Ian Campbell
Quoted 4 lines · Thomas Leonard
Based on an initial patch by Karim Raslan.
Signed-off-by Karim Allah Ahmed <[email protected]>Signed-off-by Thomas Leonard <[email protected]>Acked-by Ian Campbell <[email protected]>
Quoted 8 lines
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!
Signature · 3 lines
Dr Thomas Leonard http://0install.net/ GPG: 9242 9807 C985 3C07 44A6 8B9A AE07 8280 59A5 3CC1 GPG: DA98 25AE CAD0 8975 7CDA BD8E 0713 3F96 CA74 D8BA
Re: [PATCH ARM v8 1/4] mini-os: arm: time
Quoted 12 lines · Thomas Leonard
Quoted 11 lines · Ian Campbell
Quoted 7 lines · Ian Campbell
Quoted 4 lines · Thomas Leonard
Based on an initial patch by Karim Raslan.
Signed-off-by Karim Allah Ahmed <[email protected]>Signed-off-by Thomas Leonard <[email protected]>Acked-by Ian Campbell <[email protected]>Applied (for 4.6), thanks.
Quoted 8 lines
Quoted 6 lines
#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.
PATCH v8 2/4 mini-os: arm: interrupt controller
In reply to Thomas LeonardBased on an initial patch by Karim Raslan.
--- extras/mini-os/arch/arm/gic.c | 237 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 237 insertions(+) create mode 100644 extras/mini-os/arch/arm/gic.c
extras/mini-os/arch/arm/gic.c new file +237 -0
@@ -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 */ ); +}
Signature · 1 line
2.1.1
Re: [PATCH ARM v8 2/4] mini-os: arm: interrupt controller
extras/mini-os/arch/arm/gic.c line 48
Quoted 5 lines · Thomas Leonard
extras/mini-os/arch/arm/gic.c new file +5
-0+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().
Quoted 7 lines
extras/mini-os/arch/arm/gic.c new file +7
-0+return value; +} + +static inline void REG_WRITE32 (volatile uint32_t *addr,unsigned int value) +{ + __asm__ __volatile__("str %0, [%1]" ::"r" (value),"r" (addr)); +wmb ();
extras/mini-os/arch/arm/gic.c line 65
Quoted 10 lines
extras/mini-os/arch/arm/gic.c new file +10
-0+} + +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.
extras/mini-os/arch/arm/gic.c line 75
Quoted 8 lines
extras/mini-os/arch/arm/gic.c new file +8
-0+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.
Quoted 11 lines
extras/mini-os/arch/arm/gic.c new file +11
-0+/* 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
extras/mini-os/arch/arm/gic.c line 111
Quoted 10 lines
extras/mini-os/arch/arm/gic.c new file +10
-0+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
extras/mini-os/arch/arm/gic.c line 118
Quoted 7 lines
extras/mini-os/arch/arm/gic.c new file +7
-0+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.
Quoted 6 lines
extras/mini-os/arch/arm/gic.c new file +6
-0+//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);
extras/mini-os/arch/arm/gic.c line 168
Quoted 10 lines
extras/mini-os/arch/arm/gic.c new file +10
-0+ +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.
extras/mini-os/arch/arm/gic.c line 236
Quoted 2 lines
extras/mini-os/arch/arm/gic.c new file +2
-0+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.
Quoted 1 line
+}
Re: [PATCH ARM v8 2/4] mini-os: arm: interrupt controller
Hi Ian and Thomas,
Quoted 13 lines · Ian Campbell
extras/mini-os/arch/arm/gic.c line 65
Quoted 8 lines · Thomas Leonard
extras/mini-os/arch/arm/gic.c new file +8
-0+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.
Quoted 10 lines
extras/mini-os/arch/arm/gic.c line 75
Quoted 8 lines
extras/mini-os/arch/arm/gic.c new file +8
-0+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.
Quoted 15 lines
Quoted 15 lines
extras/mini-os/arch/arm/gic.c new file +15
-0+/* 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 );
Quoted 8 lines
extras/mini-os/arch/arm/gic.c line 111
Quoted 6 lines
extras/mini-os/arch/arm/gic.c new file +6
-0+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.
[..]
Quoted 4 lines
extras/mini-os/arch/arm/gic.c line 236
Quoted 2 lines
extras/mini-os/arch/arm/gic.c new file +2
-0+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,
Signature · 1 line
Julien Grall
Re: [PATCH ARM v8 2/4] mini-os: arm: interrupt controller
Quoted 3 lines · Julien Grall
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.
Quoted 2 lines
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
Quoted 6 lines · Ian Campbell
Quoted 3 lines · Julien Grall
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
Quoted 4 lines
Quoted 2 lines
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.
Signature · 1 line
Julien Grall
Re: [PATCH ARM v8 2/4] mini-os: arm: interrupt controller
Quoted 10 lines · Julien Grall
Quoted 6 lines · Ian Campbell
Quoted 3 lines · Julien Grall
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
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
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.
Quoted 1 line
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
Quoted 14 lines · Ian Campbell
Quoted 13 lines · Julien Grall
Quoted 6 lines · Ian Campbell
Quoted 3 lines · Julien Grall
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
Quoted 3 lines
Quoted 2 lines
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.
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
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.
Quoted 3 lines
Quoted 1 line
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,
Signature · 1 line
Julien Grall
Re: [PATCH ARM v8 2/4] mini-os: arm: interrupt controller
Quoted 12 lines · Julien Grall
Quoted 4 lines
Quoted 2 lines
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.
Quoted 2 lines
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
Quoted 13 lines · Anil Madhavapeddy
Quoted 12 lines · Julien Grall
Quoted 4 lines
Quoted 2 lines
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.
Quoted 2 lines
Quoted 2 lines
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.
Quoted 3 lines
Isn't this what
XEN_INTERFACE_VERSIONchecks 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 2/4] mini-os: arm: interrupt controller
In reply to Ian CampbellQuoted 9 lines · Ian Campbell, le Tue 21 Oct 2014 12:00:18 +0100, a écrit :
extras/mini-os/arch/arm/gic.c line 48
Quoted 5 lines · Thomas Leonard
extras/mini-os/arch/arm/gic.c new file +5
-0+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().
Quoted 8 lines
extras/mini-os/arch/arm/gic.c line 56
Quoted 8 lines
extras/mini-os/arch/arm/gic.c new file +8
-0+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 2/4] mini-os: arm: interrupt controller
Quoted 10 lines · Samuel Thibault
Quoted 9 lines · Ian Campbell, le Tue 21 Oct 2014 12:00:18 +0100, a écrit :
extras/mini-os/arch/arm/gic.c line 48
Quoted 5 lines · Thomas Leonard
extras/mini-os/arch/arm/gic.c new file +5
-0+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().
Quoted 11 lines
Quoted 8 lines
extras/mini-os/arch/arm/gic.c line 56
Quoted 8 lines
extras/mini-os/arch/arm/gic.c new file +8
-0+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 2/4] mini-os: arm: interrupt controller
Quoted 10 lines · Ian Campbell
Quoted 9 lines · Samuel Thibault
Quoted 8 lines · Ian Campbell, le Tue 21 Oct 2014 12:00:18 +0100, a écrit :
extras/mini-os/arch/arm/gic.c line 48
Quoted 5 lines · Thomas Leonard
extras/mini-os/arch/arm/gic.c new file +5
-0+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...
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
extras/mini-os/arch/arm/gic.c line 56
Quoted 9 lines
extras/mini-os/arch/arm/gic.c new file +8
-0+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__(....).
Quoted 9 lines
Quoted 3 lines
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) whereaswmb()turns into adsb()(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,
Signature · 1 line
Julien Grall
Re: [PATCH ARM v8 2/4] mini-os: arm: interrupt controller
Quoted 12 lines · Julien Grall, le Wed 22 Oct 2014 14:06:44 +0100, a écrit :
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
extras/mini-os/arch/arm/gic.c line 56
Quoted 8 lines
extras/mini-os/arch/arm/gic.c new file +8
-0+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 ...).
Quoted 4 lines
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 2/4] mini-os: arm: interrupt controller
In reply to Julien GrallQuoted 27 lines · Julien Grall
Quoted 10 lines · Ian Campbell
Quoted 9 lines · Samuel Thibault
Quoted 8 lines · Ian Campbell, le Tue 21 Oct 2014 12:00:18 +0100, a écrit :
extras/mini-os/arch/arm/gic.c line 48
Quoted 5 lines · Thomas Leonard
extras/mini-os/arch/arm/gic.c new file +5
-0+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...
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
extras/mini-os/arch/arm/gic.c line 56
Quoted 9 lines
extras/mini-os/arch/arm/gic.c new file +8
-0+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 ...).
Quoted 4 lines
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.
Quoted 18 lines
Quoted 8 lines
Quoted 2 lines
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) whereaswmb()turns into adsb()(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
Signature · 8 lines
Dr Thomas Leonard http://0install.net/ GPG: 9242 9807 C985 3C07 44A6 8B9A AE07 8280 59A5 3CC1 GPG: DA98 25AE CAD0 8975 7CDA BD8E 0713 3F96 CA74 D8BA _______________________________________________ Xen-devel mailing list [email protected] http://lists.xen.org/xen-devel
Re: [PATCH ARM v8 2/4] mini-os: arm: interrupt controller
Quoted 33 lines · Thomas Leonard
Quoted 27 lines · Julien Grall
Quoted 10 lines · Ian Campbell
Quoted 9 lines · Samuel Thibault
Quoted 8 lines · Ian Campbell, le Tue 21 Oct 2014 12:00:18 +0100, a écrit :
extras/mini-os/arch/arm/gic.c line 48
Quoted 5 lines · Thomas Leonard
extras/mini-os/arch/arm/gic.c new file +5
-0+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...
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
extras/mini-os/arch/arm/gic.c line 56
Quoted 9 lines
extras/mini-os/arch/arm/gic.c new file +8
-0+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 ...).
Quoted 4 lines
So this function is buggy. It should be:
wmb();__asm____volatile__(....).
Quoted 6 lines
gic_initdoes an explicitwmb()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.
Quoted 2 lines
Maybe none of these barriers is necessary.
Quoted 1 line
Quoted 1 line
Quoted 1 line
I don't really see why such barriers are needed indeed. Are they needed
<>>>> to actually push the values out?
Quoted 19 lines
Quoted 16 lines
Quoted 6 lines
That would, I think, require an
isb()(instruction barrier) whereaswmb()turns into adsb()(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
Signature · 6 lines
Julien Grall _______________________________________________ Xen-devel mailing list [email protected] http://lists.xen.org/xen-devel
Re: [PATCH ARM v8 2/4] mini-os: arm: interrupt controller
Quoted 41 lines · Julien Grall
Quoted 33 lines · Thomas Leonard
Quoted 27 lines · Julien Grall
Quoted 10 lines · Ian Campbell
Quoted 9 lines · Samuel Thibault
Quoted 8 lines · Ian Campbell, le Tue 21 Oct 2014 12:00:18 +0100, a écrit :
Review
Quoted 5 lines · Thomas Leonard
(quoted diff) +5
-0+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...
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Review
Quoted 9 lines
(quoted diff) +8
-0+ 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 ...).
Quoted 4 lines
So this function is buggy. It should be:
wmb();__asm____volatile__(....).Quoted 6 lines
gic_initdoes an explicitwmb()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.").
Quoted 6 lines
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).
Signature · 8 lines
Dr Thomas Leonard http://0install.net/ GPG: 9242 9807 C985 3C07 44A6 8B9A AE07 8280 59A5 3CC1 GPG: DA98 25AE CAD0 8975 7CDA BD8E 0713 3F96 CA74 D8BA _______________________________________________ Xen-devel mailing list [email protected] http://lists.xen.org/xen-devel
Re: [PATCH ARM v8 2/4] mini-os: arm: interrupt controller
Quoted 45 lines · Thomas Leonard
Quoted 44 lines · Julien Grall
Quoted 33 lines · Thomas Leonard
Quoted 27 lines · Julien Grall
Quoted 10 lines · Ian Campbell
Quoted 9 lines · Samuel Thibault
Quoted 8 lines · Ian Campbell, le Tue 21 Oct 2014 12:00:18 +0100, a écrit :
Review
Quoted 5 lines · Thomas Leonard
(quoted diff) +5
-0+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...
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Review
Quoted 9 lines
(quoted diff) +8
-0+ 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 ...).
Quoted 4 lines
So this function is buggy. It should be:
wmb();__asm____volatile__(....).Quoted 6 lines
gic_initdoes an explicitwmb()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.
Quoted 7 lines
Quoted 3 lines
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.
Quoted 1 line
- reading and acking an interrupt (in
gic_handler)
You don't care for this part as it's per CPU.
[..]
Quoted 2 lines
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,
Signature · 6 lines
Julien Grall _______________________________________________ Xen-devel mailing list [email protected] http://lists.xen.org/xen-devel
Re: [PATCH ARM v8 2/4] mini-os: arm: interrupt controller
Quoted 50 lines · Julien Grall
Quoted 49 lines · Thomas Leonard
Quoted 47 lines · Julien Grall
Quoted 33 lines · Thomas Leonard
Quoted 27 lines · Julien Grall
Quoted 10 lines · Ian Campbell
Quoted 9 lines · Samuel Thibault
Quoted 8 lines · Ian Campbell, le Tue 21 Oct 2014 12:00:18 +0100, a écrit :
Review
Quoted 5 lines · Thomas Leonard
(quoted diff) +5
-0+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...
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Review
Quoted 9 lines
(quoted diff) +8
-0+ 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 ...).
Quoted 4 lines
So this function is buggy. It should be:
wmb();__asm____volatile__(....).Quoted 6 lines
gic_initdoes an explicitwmb()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.
Quoted 8 lines
Quoted 3 lines
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?
Quoted 3 lines
Quoted 1 line
- 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?
Quoted 9 lines
[..]
Quoted 2 lines
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.
Signature · 8 lines
Dr Thomas Leonard http://0install.net/ GPG: 9242 9807 C985 3C07 44A6 8B9A AE07 8280 59A5 3CC1 GPG: DA98 25AE CAD0 8975 7CDA BD8E 0713 3F96 CA74 D8BA _______________________________________________ Xen-devel mailing list [email protected] http://lists.xen.org/xen-devel
Re: [PATCH ARM v8 2/4] mini-os: arm: interrupt controller
Hi Thomas,
Quoted 55 lines · Thomas Leonard
Quoted 54 lines · Julien Grall
Quoted 52 lines · Thomas Leonard
Quoted 41 lines · Julien Grall
Quoted 33 lines · Thomas Leonard
Quoted 27 lines · Julien Grall
Quoted 10 lines · Ian Campbell
Quoted 9 lines · Samuel Thibault
Quoted 8 lines · Ian Campbell, le Tue 21 Oct 2014 12:00:18 +0100, a écrit :
Review
Quoted 5 lines · Thomas Leonard
(quoted diff) +5
-0+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...
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Review
Quoted 9 lines
(quoted diff) +8
-0+ 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 ...).
Quoted 4 lines
So this function is buggy. It should be:
wmb();__asm____volatile__(....).Quoted 6 lines
gic_initdoes an explicitwmb()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.").Quoted 6 lines
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)
Quoted 8 lines
Quoted 4 lines
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_initbe 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.
Quoted 8 lines
Quoted 3 lines
Quoted 1 line
- 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.
Quoted 8 lines
Quoted 8 lines
[..]
Quoted 2 lines
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.
Quoted 8 lines
Quoted 1 line
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_initrequires something to reach RAM, disk or whatever before interrupts are enabled, then that caller should add the barrier.gic.ccan'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.
Quoted 5 lines
- Operations performed by
gic.care 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,
Signature · 6 lines
Julien Grall _______________________________________________ Xen-devel mailing list [email protected] http://lists.xen.org/xen-devel
Re: [PATCH ARM v8 2/4] mini-os: arm: interrupt controller
Quoted 8 lines · Julien Grall
Quoted 5 lines
Quoted 1 line
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_initbe 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
Quoted 7 lines · Ian Campbell
Quoted 6 lines · Julien Grall
Quoted 5 lines
Quoted 1 line
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_initbe called once, or once per processor? Before or after the other processors are started?
Quoted 7 lines
Quoted 2 lines
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.
Signature · 1 line
Julien Grall
Re: [PATCH ARM v8 2/4] mini-os: arm: interrupt controller
Quoted 12 lines · Julien Grall
Quoted 10 lines · Ian Campbell
Quoted 8 lines · Julien Grall
Quoted 5 lines
Quoted 1 line
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_initbe 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.
Quoted 7 lines
Quoted 4 lines
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.
PATCH v8 3/4 mini-os: arm: build system
In reply to Thomas LeonardBased 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'scontrib/ldns/compat/memmove.c (r246827).
---
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
.gitignore +3 -0
@@ -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/*
extras/mini-os/ARM-TODO.txt new file +5 -0
@@ -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
extras/mini-os/COPYING +27 -0
@@ -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.
extras/mini-os/Config.mk +2 -0
@@ -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
extras/mini-os/Makefile +36 -2
@@ -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 >[email protected] $(call move-if-changed,[email protected],$@)
@@ -190,7 +219,11 @@ $(OBJ_DIR)/$(TARGET): $(OBJS) $(APP_O) arch_lib $(LD) -r $(LDFLAGS) $(HEAD_OBJ) $(APP_O) $(OBJS) $(LDARCHLIB) $(LDLIBS) -o [email protected] $(OBJCOPY) -w -G $(GLOBAL_PREFIX)* -G _start [email protected] [email protected] $(LD) $(LDFLAGS) $(LDFLAGS_FINAL) [email protected] $(EXTRA_OBJS) -o $@ +ifeq ($(XEN_TARGET_ARCH),arm32) + $(OBJCOPY) -O binary $@ [email protected] +else gzip -f -9 -c $@ >[email protected] +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)
extras/mini-os/arch/arm/Makefile new file +32 -0
@@ -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) +
extras/mini-os/arch/arm/arch.mk new file +7 -0
@@ -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 +
extras/mini-os/include/lib.h +3 -1
@@ -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
extras/mini-os/include/libfdt_env.h new file +33 -0
@@ -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 */
extras/mini-os/lib/memmove.c new file +45 -0
45 lines not on this page · raw message
extras/mini-os/lib/string.c +12 -0
@@ -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
Signature · 1 line
2.1.1
Re: [PATCH ARM v8 3/4] mini-os: arm: build system
extras/mini-os/Makefile
Quoted 4 lines · Thomas Leonard
extras/mini-os/Makefile +0
-0
WRT the freeze some of the changes here would appear to also affect x86 builds, which means they need more careful consideration. Konrad CCd.
Quoted 10 lines
extras/mini-os/Makefile +1
-1@@ -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)
extras/mini-os/Makefile EXTRA_OBJS = · line 82
Quoted 9 lines
extras/mini-os/Makefile +5
-1@@ -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?
extras/mini-os/Makefile EXTRA_OBJS = · line 88
Quoted 6 lines
extras/mini-os/Makefile +6
-0+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).
Quoted 19 lines
extras/mini-os/Makefile +4
-0+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
extras/mini-os/Makefile $(ARCH_LINKS): · line 146
Quoted 8 lines
extras/mini-os/Makefile +5
-0@@ -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?
extras/mini-os/Makefile $(ARCH_LINKS): · line 155
Quoted 9 lines
extras/mini-os/Makefile +9
-0+ +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?
extras/mini-os/Makefile clean: arch_clean · line 238
Quoted 5 lines
extras/mini-os/Makefile +1
-0@@ -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.
Re: [PATCH ARM v8 3/4] mini-os: arm: build system
Quoted 9 lines · Ian Campbell, le Tue 21 Oct 2014 12:44:27 +0100, a écrit :
extras/mini-os/Makefile EXTRA_OBJS = · line 82
Quoted 2 lines
extras/mini-os/Makefile +2
-0+ifeq ($(XEN_TARGET_ARCH),arm32) +# Need libgcc.a for division helpersIs 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 3/4] mini-os: arm: build system
Quoted 11 lines · Samuel Thibault
Quoted 9 lines · Ian Campbell, le Tue 21 Oct 2014 12:44:27 +0100, a écrit :
extras/mini-os/Makefile EXTRA_OBJS = · line 82
Quoted 2 lines
extras/mini-os/Makefile +2
-0+ifeq ($(XEN_TARGET_ARCH),arm32) +# Need libgcc.a for division helpersIs 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?
Quoted 4 lines
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 3/4] mini-os: arm: build system
Quoted 12 lines · Ian Campbell, le Wed 22 Oct 2014 10:01:51 +0100, a écrit :
Quoted 11 lines · Samuel Thibault
Quoted 9 lines · Ian Campbell, le Tue 21 Oct 2014 12:44:27 +0100, a écrit :
extras/mini-os/Makefile EXTRA_OBJS = · line 82
Quoted 2 lines
extras/mini-os/Makefile +2
-0+ifeq ($(XEN_TARGET_ARCH),arm32) +# Need libgcc.a for division helpersIs 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?
Quoted 7 lines
Quoted 4 lines
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 3/4] mini-os: arm: build system
In reply to Samuel ThibaultQuoted 11 lines · Samuel Thibault
Quoted 9 lines · Ian Campbell, le Tue 21 Oct 2014 12:44:27 +0100, a écrit :
extras/mini-os/Makefile EXTRA_OBJS = · line 82
Quoted 2 lines
extras/mini-os/Makefile +2
-0+ifeq ($(XEN_TARGET_ARCH),arm32) +# Need libgcc.a for division helpersIs 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?
Quoted 4 lines
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,
Signature · 8 lines
Dr Thomas Leonard http://0install.net/ GPG: 9242 9807 C985 3C07 44A6 8B9A AE07 8280 59A5 3CC1 GPG: DA98 25AE CAD0 8975 7CDA BD8E 0713 3F96 CA74 D8BA _______________________________________________ Xen-devel mailing list [email protected] http://lists.xen.org/xen-devel
Re: [PATCH ARM v8 3/4] mini-os: arm: build system
Quoted 1 line · 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
Quoted 11 lines · Samuel Thibault
Quoted 1 line · 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
Quoted 3 lines
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.
Signature · 8 lines
Dr Thomas Leonard http://0install.net/ GPG: 9242 9807 C985 3C07 44A6 8B9A AE07 8280 59A5 3CC1 GPG: DA98 25AE CAD0 8975 7CDA BD8E 0713 3F96 CA74 D8BA _______________________________________________ Xen-devel mailing list [email protected] http://lists.xen.org/xen-devel
Re: [PATCH ARM v8 3/4] mini-os: arm: build system
Quoted 12 lines · Thomas Leonard
Quoted 11 lines · Samuel Thibault
Quoted 1 line · 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
Quoted 6 lines
Quoted 3 lines
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.
Signature · 8 lines
Dr Thomas Leonard http://0install.net/ GPG: 9242 9807 C985 3C07 44A6 8B9A AE07 8280 59A5 3CC1 GPG: DA98 25AE CAD0 8975 7CDA BD8E 0713 3F96 CA74 D8BA _______________________________________________ Xen-devel mailing list [email protected] http://lists.xen.org/xen-devel
Re: [PATCH ARM v8 3/4] mini-os: arm: build system
Quoted 17 lines · Thomas Leonard
Quoted 16 lines · Thomas Leonard
Quoted 14 lines · Samuel Thibault
Quoted 1 line · 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.htmlSo 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.
Quoted 6 lines
Quoted 2 lines
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
[email protected]
http://lists.xen.org/xen-devel
Re: [PATCH ARM v8 3/4] mini-os: arm: build system
In reply to Thomas LeonardQuoted 13 lines · Thomas Leonard, le Mon 17 Nov 2014 11:42:38 +0000, a écrit :
Quoted 12 lines · Thomas Leonard
Quoted 11 lines · Samuel Thibault
Quoted 1 line · 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
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
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 3/4] mini-os: arm: build system
In reply to Thomas LeonardQuoted 12 lines · Thomas Leonard
Quoted 11 lines · Thomas Leonard
Quoted 10 lines · Samuel Thibault
Quoted 1 line · 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
Quoted 10 lines
Quoted 5 lines
Quoted 3 lines
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
[email protected]
http://lists.xen.org/xen-devel
PATCH v8 4/4 mini-os: arm: show registers, stack and exception vector on fault
In reply to Thomas Leonard--- 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
extras/mini-os/ARM-TODO.txt +0 -1
@@ -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 (?)
extras/mini-os/arch/arm/arm32.S +68 -7
89 lines not on this page · raw message
extras/mini-os/arch/arm/panic.c new file +98 -0
98 lines not on this page · raw message
Signature · 7 lines
2.1.1 _______________________________________________ Xen-devel mailing list [email protected] http://lists.xen.org/xen-devel