Re: [Minios-devel] [PATCH 09/40] arm64: add the basic helpers for arm64

[Huang Shijie <shijie.huang@xxxxxxx>]

On Fri, Nov 03, 2017 at 04:58:52PM +0000, Julien Grall wrote:
> Hi Shijie,
> 
> On 03/11/17 03:11, Huang Shijie wrote:
> > This patch adds the basic helpers in header os.h for arm32 and arm64:
> 
> This is slightly untrue. arm32 helpers already exists. You only move them in
> separate headers. But they should really be moved in a separate patch.
okay, I can change the comment. I do not want to add another patch for
the moving.
> 
> >      0.) mb/rmb/wmb
> >      1.) local_irq_disable/local_irq_enable
> >      2.) local_irq_save/local_irq_restore/local_save_flags
> >      3.) __ffs
> > 
> > This patch refers to Chen Baozi's patch:
> >       "Initial codes for arm64"
> > 
> > Change-Id: Id9025f016968a815029dfba7ee275d9f757974f0
> > Jira: ENTOS-247
> > Signed-off-by: Huang Shijie <shijie.huang@xxxxxxx>
> > ---
> >   include/arm/arm32/os.h | 29 ++++++++++++++++++++++
> >   include/arm/arm64/os.h | 35 +++++++++++++++++++++++++++
> >   include/arm/os.h       | 65 
> > ++++++--------------------------------------------
> >   3 files changed, 71 insertions(+), 58 deletions(-)
> >   create mode 100644 include/arm/arm32/os.h
> >   create mode 100644 include/arm/arm64/os.h
> > 
> > diff --git a/include/arm/arm32/os.h b/include/arm/arm32/os.h
> > new file mode 100644
> > index 0000000..6da604f
> > --- /dev/null
> > +++ b/include/arm/arm32/os.h
> > @@ -0,0 +1,29 @@
> > +#ifndef _ARM32_OS_H
> > +#define _ARM32_OS_H
> > +
> > +static inline void local_irq_disable(void) {
> > +    __asm__ __volatile__("cpsid i":::"memory");
> 
> Does the __ is really needed? Same for everywhere in that patch.
I am not sure, I can try to remove the __, and compile the code for
arm32.
> 
> > +}
> > +
> > +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 local_save_flags(x)    { \
> > +    __asm__ __volatile__("mrs %0, cpsr":"=r"(x)::"memory");    \
> > +}
> > +
> > +/* 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");
> > +
> > +#endif
> 
> Let's try to follow good practice on new files. E.g:
> 
> #endif /* _ARM32_OS_H */
> 
> /*
>  * Local variables:
>  * mode: C
>  * c-file-style: "BSD"
>  * c-basic-offset: 4
>  * tab-width: 4
>  * indent-tabs-mode: nil
>  * End:
>  */
okay.
> 
> > diff --git a/include/arm/arm64/os.h b/include/arm/arm64/os.h
> > new file mode 100644
> > index 0000000..cb2ab14
> > --- /dev/null
> > +++ b/include/arm/arm64/os.h
> > @@ -0,0 +1,35 @@
> > +#ifndef _ARM64_OS_H_
> > +#define _ARM64_OS_H_
> > +
> > +static inline void local_irq_disable(void)
> > +{
> > +    __asm__ __volatile__("msr daifset, #2": : :"memory");
> > +}
> > +
> > +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"); \
> 
> Can you try to be consistent. Either you put space between : or you don't.
> 
> Personally, I would prefer the ::: version.
okay. I can remove the spaces in the next version.
> 
> > +}
> > +
> > +#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"); \
> > +}
> > +
> > +#define isb()           __asm__ __volatile("isb" : : : "memory")
> > +
> > +#define dmb(opt)        __asm__ __volatile("dmb " #opt : : : "memory")
> > +#define dsb(opt)        __asm__ __volatile("dsb " #opt : : : "memory")
> 
> isb, dmb, dsb are the same mnemonic on Arm32 and Arm64. Please move them in
> common header.
okay.

> 
> > +
> > +#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 */
> 
> Can you explain the rationale behind using dmb here and not dsb?
> 
> Note that I will ask the same, if you move back to dsb.
From the linux kernel code, it should be dsb here, I will read the Spec
later :).

> 
> > +
> > +#endif
> > diff --git a/include/arm/os.h b/include/arm/os.h
> > index 6a1cc37..37fb007 100644
> > --- a/include/arm/os.h
> > +++ b/include/arm/os.h
> > @@ -22,27 +22,11 @@ extern void *device_tree;
> >   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 local_save_flags(x)    { \
> > -    __asm__ __volatile__("mrs %0, cpsr":"=r"(x)::"memory");    \
> > -}
> > +#if defined(__arm__)
> > +#include <arm32/os.h>
> > +#elif defined(__aarch64__)
> > +#include <arm64/os.h>
> > +#endif
> >   static inline int irqs_disabled(void) {
> >       int x;
> > @@ -50,14 +34,7 @@ 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)
> >   /**
> > @@ -121,39 +98,11 @@ static __inline__ void clear_bit(int nr, volatile 
> > unsigned long *addr)
> >       test_and_clear_bit(nr, addr);
> >   }
> > -/**
> > - * __ffs - find first (lowest) set bit in word.
> > - * @word: The word to search
> > - *
> > - * Undefined if no bit exists, so code should check against 0 first.
> > - */
> 
> Why did you drop the comment?
okay.
> 
> > -static __inline__ unsigned long __ffs(unsigned long word)
> > +static inline unsigned long __ffs(unsigned long word)
> 
> Why the prototype has been changed?
okay.
> 
> >   {
> > -    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);
> 
> Are you sure __builtin_ctzl is defined everywhere? Furthermore, this does
Does the arm32-gcc not provide the __builtin_ctzl()?

Thanks
Huang Shijie

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