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

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

On 11/11/2022 16:17, Xenia Ragiadakou wrote:
> Hi Ayan,
Hi Xenia,
> 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() 
Just to make sure I understand you correctly, you are suggesting this :-

#define readq_relaxed_non_atomic(c) \

                        ({ void _iomem *__addr = (c); \

                            u64 __r = (((u64)readl_relaxed(__addr + 4)) 
<< 32) | \
readl_relaxed(__addr); __r; })

#define writeq_relaxed_non_atomic(v,c) \

                       (( u64 __v = (v); \

                          void _iomem *__addr = (c); \

                          writel_relaxed((u32)__v, __addr); \

                          writel_relaxed((u32)((__v) >> 32), (__addr + 
4); })
Is this correct understanding ?

> 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".
I have mixed opinion about this.

On one hand, there will be a performance penalty when invoking a 
function (compared to macro).
On the other hand {readq/writeq}_relaxed_non_atomic() are called during 
init (gicv3 initialization, setting up the interrupt handlers), so the 
impact will not be bad.
I am fine with whatever you and any maintainer suggest.

Also now I realize that I had missed another point raised by Julien (a 
code comment explaining why ldrd/strd cannot be used) :(.
I will address this in my next version

> ...
>
> >     #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.
Ack
> >     #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.
I will remove this as we will be using readq_relaxed_non_atomic() in the 
code.
> ...
>
> >     #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.
I will remove this as we will be using writeq_relaxed_non_atomic()  in 
the code.
- Ayan

> >     #endif /* _ARM_ARM32_IO_H */