Re: [XEN v3 11/12] xen/Arm: GICv3: Define macros to read/write 64 bit

[Xenia Ragiadakou <burzalodowa@xxxxxxxxx>]

Hi Ayan,

On 11/11/22 16:17, Ayan Kumar Halder wrote:
> On AArch32, ldrd/strd instructions are not atomic when used to access MMIO.
> Furthermore, ldrd/strd instructions are not decoded by Arm when running as
> a guest to access emulated MMIO region.
> Thus, we have defined readq_relaxed_non_atomic()/writeq_relaxed_non_atomic()
> which in turn calls readl_relaxed()/writel_relaxed() for the lower and upper
> 32 bits.
> As GICv3 registers (GICD_IROUTER, GICR_TYPER) can be accessed in a non atomic
> fashion, so we have used {read/write}q_relaxed_non_atomic() on Arm32.
>
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
> ---
>
> Changes from :-
> v1 - 1. Use ldrd/strd for readq_relaxed()/writeq_relaxed().
> 2. No need to use le64_to_cpu() as the returned byte order is already in cpu
> endianess.
>
> v2 - 1. Replace {read/write}q_relaxed with {read/write}q_relaxed_non_atomic().
>
>   xen/arch/arm/gic-v3.c               | 12 ++++++++++++
>   xen/arch/arm/include/asm/arm32/io.h |  9 +++++++++
>   2 files changed, 21 insertions(+)
>
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 6457e7033c..a5bc549765 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -651,7 +651,11 @@ static void __init gicv3_dist_init(void)
>       affinity &= ~GICD_IROUTER_SPI_MODE_ANY;
>   
>       for ( i = NR_GIC_LOCAL_IRQS; i < nr_lines; i++ )
> +#ifdef CONFIG_ARM_32
> +        writeq_relaxed_non_atomic(affinity, GICD + GICD_IROUTER + i * 8);
> +#else
>           writeq_relaxed(affinity, GICD + GICD_IROUTER + i * 8);
> +#endif
>   }
>   
>   static int gicv3_enable_redist(void)
> @@ -745,7 +749,11 @@ static int __init gicv3_populate_rdist(void)
>           }
>   
>           do {
> +#ifdef CONFIG_ARM_32
> +            typer = readq_relaxed_non_atomic(ptr + GICR_TYPER);
> +#else
>               typer = readq_relaxed(ptr + GICR_TYPER);
> +#endif
>   
>               if ( (typer >> 32) == aff )
>               {
> @@ -1265,7 +1273,11 @@ static void gicv3_irq_set_affinity(struct irq_desc 
> *desc, const cpumask_t *mask)
>       affinity &= ~GICD_IROUTER_SPI_MODE_ANY;
>   
>       if ( desc->irq >= NR_GIC_LOCAL_IRQS )
> +#ifdef CONFIG_ARM_32
> +        writeq_relaxed_non_atomic(affinity, (GICD + GICD_IROUTER + desc->irq * 
> 8));
> +#else
>           writeq_relaxed(affinity, (GICD + GICD_IROUTER + desc->irq * 8));
> +#endif
>   
>       spin_unlock(&gicv3.lock);
>   }
> diff --git a/xen/arch/arm/include/asm/arm32/io.h 
> b/xen/arch/arm/include/asm/arm32/io.h
> index 73a879e9fb..4ddfbea5c2 100644
> --- a/xen/arch/arm/include/asm/arm32/io.h
> +++ b/xen/arch/arm/include/asm/arm32/io.h
> @@ -80,17 +80,26 @@ 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_non_atomic(c) \
> +                         ({ u64 __r = (((u64)readl_relaxed((c) + 4)) << 32) | \
> +                                             readl_relaxed(c); __r; })
As Julien pointed out, the expression c will be evaluated twice and if 
it produces side effects they will be performed twice.
To prevent this, you can either assign the expression to a local 
variable and pass this one to readl_relaxed() or use a static inline 
function instead of a macro, for implementing readq_relaxed_non_atomic().
The latter is the MISRA C recommended (not strictly required) approach 
according to Dir 4.9 "A function should be used in preference to a 
function-like macro where
 they are interchangeable".
...

>   
>   #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_non_atomic(v,c) \
> +                                ({ writel_relaxed((u32)v, c); \
> +                                   writel_relaxed((u32)((v) >> 32), (c) + 4); 
> })
... same here.

>   
>   #define readb(c)                ({ u8  __v = readb_relaxed(c); __iormb(); __v; })
>   #define readw(c)                ({ u16 __v = readw_relaxed(c); __iormb(); 
> __v; })
>   #define readl(c)                ({ u32 __v = readl_relaxed(c); __iormb(); 
> __v; })
> +#define readq(c)                ({ u64 __v = readq_relaxed_non_atomic(c); \
> +                                             __iormb(); __v; })
I think that, here also, the macro identifier needs to inform that the 
access is non-atomic.
...

>   
>   #define writeb(v,c)             ({ __iowmb(); writeb_relaxed(v,c); })
>   #define writew(v,c)             ({ __iowmb(); writew_relaxed(v,c); })
>   #define writel(v,c)             ({ __iowmb(); writel_relaxed(v,c); })
> +#define writeq(v,c)             ({ __iowmb(); writeq_relaxed_non_atomic(v,c); 
> })
... same here.

>   
>   #endif /* _ARM_ARM32_IO_H */
--
Xenia