Re: [Minios-devel] [PATCH v3 12/43] arm64: add the basic helpers for arm64

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

Most of my comments from 
<cc143169-08da-18b2-060f-8823909abe6e@xxxxxxxxxx> has not been 
addressed. Some of them are probably have been repeated 3-4 times.... So 
please address them.
On 16/04/2018 07:31, Huang Shijie wrote:
> This patch adds the basic helpers in headers for arm64:
>      1.) mb/rmb/wmb
>      2.) local_irq_disable/local_irq_enable
>      3.) local_irq_save/local_irq_restore/local_save_flags
>      4.) simplify the __ffs
>      5.) add BUG().
You also drop arm32 code making that patch quite confusing to read.

> This patch refers to Chen Baozi's patch:
Do you mean "based on" rather than "refers to"?

>       "Initial codes for arm64"
> And this patch also refers to FreeBSD code:
Ditto

>       sys/arm64/include/atomic.h
You very likely have to retain the copyright from those files.

> Signed-off-by: Huang Shijie <shijie.huang@xxxxxxx>
> ---
>   include/arm/arm64/os.h | 28 +++++++++++++++++++
>   include/arm/os.h       | 75 ++++++++++++++++----------------------------------
>   2 files changed, 52 insertions(+), 51 deletions(-)
>   create mode 100644 include/arm/arm64/os.h
>
> diff --git a/include/arm/arm64/os.h b/include/arm/arm64/os.h
> new file mode 100644
> index 0000000..3d4aada
> --- /dev/null
> +++ b/include/arm/arm64/os.h
> @@ -0,0 +1,28 @@
> +#ifndef _ARM64_OS_H_
> +#define _ARM64_OS_H_
> +
> +#define BUG()           __asm__ __volatile("wfi" ::: "memory")
I don't understand that implementation. WFI means wait for interrupt and 
will continue as soon as an interrupt came up. What you want here is 
dumping the registers and exit.
So most likely you want to use "brk" that will provoke a trap.

> +
> +static inline void local_irq_disable(void)
> +{
> +    __asm__ __volatile__("msr daifset, #2":::"memory");
Again, does the __ is really needed? Same for everywhere in that patch.
You said you will check it and never came back.

> +}
> +
> +static inline void local_irq_enable(void)
> +{
> +    __asm__ __volatile__("msr daifclr, #2":::"memory");
> +}
> +
> +#define local_irq_save(x) { \
> +    __asm__ __volatile__("mrs %0, daif; msr daifset, #2":"=r"(x)::"memory"); \
> +}
> +
> +#define local_irq_restore(x) { \
> +    __asm__ __volatile__("msr daif, %0"::"r"(x):"memory"); \
> +}
> +
> +#define local_save_flags(x) { \
> +    __asm__ __volatile__("mrs %0, daif":"=r"(x)::"memory"); \
> +}
> +
> +#endif
> diff --git a/include/arm/os.h b/include/arm/os.h
> index 6a1cc37..b10557a 100644
> --- a/include/arm/os.h
> +++ b/include/arm/os.h
> @@ -8,41 +8,42 @@
>   #include <mini-os/compiler.h>
>   #include <mini-os/kernel.h>
>   #include <xen/xen.h>
> +#include <arm64/os.h>
Same remark as for traps.h. You want to #ifdef that.

>   
>   void arch_fini(void);
>   void timer_handler(evtchn_port_t port, struct pt_regs *regs, void *ign);
>   
>   extern void *device_tree;
>   
> -#define BUG() while(1){asm volatile (".word 0xe7f000f0\n");} /* Undefined instruction; will call our fault handler. */
> -
>   #define smp_processor_id() 0
>   
>   #define barrier() __asm__ __volatile__("": : :"memory")
>   
>   extern shared_info_t *HYPERVISOR_shared_info;
>   
> -// disable interrupts
> -static inline void local_irq_disable(void) {
> -    __asm__ __volatile__("cpsid i":::"memory");
> -}
> -
> -// enable interrupts
> -static inline void local_irq_enable(void) {
> -    __asm__ __volatile__("cpsie i":::"memory");
> -}
> -
> -#define local_irq_save(x) { \
> -    __asm__ __volatile__("mrs %0, cpsr;cpsid i":"=r"(x)::"memory");    \
> -}
> -
> -#define local_irq_restore(x) {    \
> -    __asm__ __volatile__("msr cpsr_c, %0"::"r"(x):"memory");    \
> -}
> +#define isb()           __asm__ __volatile("isb" ::: "memory")
> +
> +/*
> + * Options for DMB and DSB:
> + *     oshld   Outer Shareable, load
> + *     oshst   Outer Shareable, store
> + *     osh     Outer Shareable, all
> + *     nshld   Non-shareable, load
> + *     nshst   Non-shareable, store
> + *     nsh     Non-shareable, all
> + *     ishld   Inner Shareable, load
> + *     ishst   Inner Shareable, store
> + *     ish     Inner Shareable, all
> + *     ld      Full system, load
> + *     st      Full system, store
> + *     sy      Full system, all
> + */
> +#define dmb(opt)        __asm__ __volatile("dmb " #opt ::: "memory")
> +#define dsb(opt)        __asm__ __volatile("dsb " #opt ::: "memory")
>   
> -#define local_save_flags(x)    { \
> -    __asm__ __volatile__("mrs %0, cpsr":"=r"(x)::"memory");    \
> -}
> +#define mb()            dmb(sy) /* Full system memory barrier all */
> +#define wmb()           dmb(st) /* Full system memory barrier store */
> +#define rmb()           dmb(ld) /* Full system memory barrier load */
ld does not exist for arm32. So you want to ifdef that.

But again, can you explain why you use dmb(...) and not dsb(...) here.

>   
>   static inline int irqs_disabled(void) {
>       int x;
> @@ -50,14 +51,8 @@ static inline int irqs_disabled(void) {
>       return x & 0x80;
>   }
>   
> -/* We probably only need "dmb" here, but we'll start by being paranoid. */
> -#define mb() __asm__("dsb":::"memory");
> -#define rmb() __asm__("dsb":::"memory");
> -#define wmb() __asm__("dsb":::"memory");
> -
>   /************************** arm *******************************/
>   #ifdef __INSIDE_MINIOS__
> -#if defined (__arm__)
>   #define xchg(ptr,v) __atomic_exchange_n(ptr, v, __ATOMIC_SEQ_CST)
>   
>   /**
> @@ -129,31 +124,9 @@ static __inline__ void clear_bit(int nr, volatile unsigned 
> long *addr)
>    */
>   static __inline__ unsigned long __ffs(unsigned long word)
>   {
> -    int clz;
> -
> -    /* xxxxx10000 = word
> -     * xxxxx01111 = word - 1
> -     * 0000011111 = word ^ (word - 1)
> -     *      4     = 31 - clz(word ^ (word - 1))
> -     */
> -
> -    __asm__ (
> -        "sub r0, %[word], #1\n"
> -        "eor r0, r0, %[word]\n"
> -        "clz %[clz], r0\n":
> -        /* Outputs: */
> -        [clz] "=r"(clz):
> -        /* Inputs: */
> -        [word] "r"(word):
> -        /* Clobbers: */
> -        "r0");
> -
> -    return 31 - clz;
> +    return __builtin_ctzl(word);
Again, are you sure __builtin_ctzl is defined everywhere? I asked you to 
tell use whether all compilers (e.g GCC and Clang) supports the builtin 
and from which version.
>   }
>   
> -#else /* ifdef __arm__ */
> -#error "Unsupported architecture"
> -#endif
>   #endif /* ifdef __INSIDE_MINIOS */
>   
>   /********************* common arm32 and arm64  ****************************/
Cheers,

--
Julien Grall

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel