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

[Julien Grall <julien@xxxxxxx>]

Hi,

On 01/11/2022 07:08, Xenia Ragiadakou wrote:
> On 10/31/22 17:13, Ayan Kumar Halder wrote:
> > Defined readq_relaxed()/writeq_relaxed() to read and write 64 bit regs.
> > This uses ldrd/strd instructions.
> >
> > Signed-off-by: Ayan Kumar Halder <ayankuma@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.
> >
> >   xen/arch/arm/include/asm/arm32/io.h | 21 +++++++++++++++++++++
> >   1 file changed, 21 insertions(+)
> >
> > diff --git a/xen/arch/arm/include/asm/arm32/io.h 
> > b/xen/arch/arm/include/asm/arm32/io.h
> > index 73a879e9fb..d9d19ad764 100644
> > --- a/xen/arch/arm/include/asm/arm32/io.h
> > +++ b/xen/arch/arm/include/asm/arm32/io.h
> > @@ -72,6 +72,22 @@ static inline u32 __raw_readl(const volatile void 
> > __iomem *addr)
> >           return val;
> >   }
> > +static inline u64 __raw_readq(const volatile void __iomem *addr)
> > +{
> > +        u64 val;
> > +        asm volatile("ldrd %Q1, %R1, %0"
> > +                     : "+Qo" (*(volatile u64 __force *)addr),
> > +                       "=r" (val));
> > +        return val;
> > +}
> > +
> > +static inline void __raw_writeq(u64 val, const volatile void __iomem 
> > *addr)
> > +{
> > +    asm volatile("strd %Q1, %R1, %0"
> > +                 : "+Q" (*(volatile u64 __force *)addr)
> > +                 : "r" (val));
> > +}
> > +
> >   #define __iormb()               rmb()
> >   #define __iowmb()               wmb()
> > @@ -80,17 +96,22 @@ 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((__force __le64) \
> > +                                        __raw_readq(c)); __r; })
> >   #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)     __raw_writeq((__force u64) 
> > cpu_to_le64(v),c)
> >   #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(c); 
> > __iormb(); __v; })
> >   #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(v,c); })
> >   #endif /* _ARM_ARM32_IO_H */
> AFAIU, ldrd/strd accesses to MMIO are not guaranteed to be 64-bit 
> single-copy atomic. So, as Julien suggested, you still need to use a 
> different name to reflect this.
> Also, having nested virtualization in mind, since these instructions 
> can't be virtualized, maybe it would be better to avoid using them for 
> MMIO accesses.
+1. The previous version was actually using 32-bit access and it is not 
clear to me why the new version is using 64-bit access.
Cheers,

--
Julien Grall