[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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.

     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.

+}
+
+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:
 */

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.

+}
+
+#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.

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

+
+#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?

-static __inline__ unsigned long __ffs(unsigned long word)
+static inline unsigned long __ffs(unsigned long word)

Why the prototype has been changed?

  {
-    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 not belong to this patch.

  }
-#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/cgi-bin/mailman/listinfo/minios-devel

 


Rackspace

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