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

[Huang Shijie <shijie.huang@xxxxxxx>]

On Wed, Apr 18, 2018 at 09:27:12PM +0100, Julien Grall wrote:
> 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.
sorry, I did reply the old email.  I will address them here..

> 
> 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.
yes. I can remove it to #2 patch.
> 
> >
> >This patch refers to Chen Baozi's patch:
> 
> Do you mean "based on" rather than "refers to"?
okay.

> 
> >      "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.
do you mean I should copy the copyright here?
I think both the code is FreeBSD license, so I did not do that.

> 
> >
> >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.
okay, I will use "brk" in next version..
> 
> >+
> >+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.
We (Steve and I) ever checked the GCC document about this.

https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html

“The asm keyword is a GNU extension. When writing code that can be compiled 
with -ansi and
the various -std options, use __asm__ instead of asm (see Alternate Keywords).”

> 
> >+}
> >+
> >+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.
do you mean I should add #ifdef here?

thanks
> 
> >  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.
You mean I should add #ifdef here?

> 
> But again, can you explain why you use dmb(...) and not dsb(...) here.
dsb() is more strict then the dmb().

"This enforces the same ordering as the Data Memory Barrier, but has the 
additional
effect of blocking execution of any further instructions, not just loads or 
stores,
or both, until synchronization is complete."       


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

It is a gcc builtin in all recent gcc verion:
https://gcc.gnu.org/onlinedocs/gcc-3.4.6/gcc/Other-Builtins.html

…

https://gcc.gnu.org/onlinedocs/gcc-7.2.0/gcc/Other-Builtins.html

so I think it is okay here..


thanks
Huang Shijie

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