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

Re: [Minios-devel] [PATCH v3 34/43] arm64: gic: implement the REG_WRITE32/REG_READ32



Hi,

On 16/04/18 07:32, Huang Shijie wrote:
Implement the REG_WRITE32/REG_READ32 for arm64, and add
a new header io.h for them.

It looks like to me you want to follow what x86 does for reading MMIO. Have a look at iorw.h.


This patch also removes the REG_WRITE32/REG_READ32 for arm32. >
Signed-off-by: Huang Shijie <shijie.huang@xxxxxxx>
---
  arch/arm/gic.c         | 15 +--------------
  include/arm/arm64/io.h | 18 ++++++++++++++++++
  2 files changed, 19 insertions(+), 14 deletions(-)
  create mode 100644 include/arm/arm64/io.h

diff --git a/arch/arm/gic.c b/arch/arm/gic.c
index 687f242..8be1285 100644
--- a/arch/arm/gic.c
+++ b/arch/arm/gic.c
@@ -4,6 +4,7 @@
  #include <mini-os/hypervisor.h>
  #include <mini-os/console.h>
  #include <libfdt.h>
+#include <mini-os/arm64/io.h>
//#define VGIC_DEBUG
  #ifdef VGIC_DEBUG
@@ -41,20 +42,6 @@ static struct gic gic;
#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;
diff --git a/include/arm/arm64/io.h b/include/arm/arm64/io.h
new file mode 100644
index 0000000..f4c087c
--- /dev/null
+++ b/include/arm/arm64/io.h
@@ -0,0 +1,18 @@
+#ifndef __ARM64_IO_H__
+#define __ARM64_IO_H__
+
+static inline uint32_t REG_READ32(volatile uint32_t *addr)
+{
+    uint32_t value;
+
+    __asm__ __volatile__("ldr %w0, [%1]":"=&r"(value):"r"(addr));
+    rmb();

Having a barrier here seems to be a bit over the top for me. What are you trying to prevent with that barrier here? You will have the read ordered but not the write...

+    return value;
+}
+
+static inline void REG_WRITE32(volatile uint32_t *addr, unsigned int value)
+{
+    __asm__ __volatile__("str %w0, [%1]"::"r" (value), "r"(addr));
+    wmb();

Same here. But the barrier seems wrong at the first place, wmb() is used to get all the write before the barrier executed before the write after the barrier. This is not going to prevent a load to be executed before.

So what are you trying to prevent?

+}
+#endif


Cheers,

--
Julien Grall

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

 


Rackspace

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