Re: [RFC PATCH v1 11/12] Arm: GICv3: Define macros to read/write 64 bit

[Julien Grall <julien@xxxxxxx>]

Hi Ayan,

On 21/10/2022 16:31, Ayan Kumar Halder wrote:
> Defined readq_relaxed()/writeq_relaxed() to read and write 64 bit regs.
> This in turn calls readl_relaxed()/writel_relaxed() twice for the lower
> and upper 32 bits.
This needs an explanation why we can't use "strd/ldrd". And the same 
would have to be duplicated in the code below.
> Signed-off-by: Ayan Kumar Halder <ayankuma@xxxxxxx>
> ---
>   xen/arch/arm/include/asm/arm32/io.h | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/xen/arch/arm/include/asm/arm32/io.h 
> b/xen/arch/arm/include/asm/arm32/io.h
> index 73a879e9fb..6a5f563fbc 100644
> --- a/xen/arch/arm/include/asm/arm32/io.h
> +++ b/xen/arch/arm/include/asm/arm32/io.h
> @@ -80,10 +80,14 @@ static inline u32 __raw_readl(const volatile void __iomem 
> *addr)
>                                           __raw_readw(c)); __r; })
>   #define readl_relaxed(c) ({ u32 __r = le32_to_cpu((__force __le32) \
>                                           __raw_readl(c)); __r; })
> +#define readq_relaxed(c) ({ u64 __r = (le64_to_cpu(readl_relaxed(c+4)) << 32) 
> | \
> +                                        readl_relaxed(c); __r; })
All the read*_relaxed are provide atomic read. This is not guaranteed by 
your new helper. The name should be different (maybe 
readq_relaxed_non_atomic()) to make clear of the difference.
I also don't quite understand the implementation. The value returned by 
readl_relaxed() is already in the CPU endianess. So why do you call 
le64_to_cpu() on top?
>   
>   #define writeb_relaxed(v,c)     __raw_writeb(v,c)
>   #define writew_relaxed(v,c)     __raw_writew((__force u16) cpu_to_le16(v),c)
>   #define writel_relaxed(v,c)     __raw_writel((__force u32) cpu_to_le32(v),c)
> +#define writeq_relaxed(v,c)     writel_relaxed(((uint64_t)v&0xffffffff), c); \
> +                                    writel_relaxed((((uint64_t)v)>>32), (c+4));
This needs to be surrounded with do { } while (0), otherwise the 
following would not properly work:
if ( foo )
  writeq_relaxed(v, c);

Similarly, if 'v' is a call, then it will end up to be called twice.

>   
>   #define readb(c)                ({ u8  __v = readb_relaxed(c); __iormb(); __v; })
>   #define readw(c)                ({ u16 __v = readw_relaxed(c); __iormb(); 
> __v; })
Cheers,

--
Julien Grall