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

[Julien Grall <julien.grall@xxxxxxx>]

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