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 <[email protected]>
---
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 */
