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

[Julien Grall <julien@xxxxxxx>]

Hi,

On 28/11/2022 15:56, 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.
> For AArch64, readq_relaxed_non_atomic()/writeq_relaxed_non_atomic() invokes
> readq_relaxed()/writeq_relaxed() respectively.
> As GICv3 registers (GICD_IROUTER, GICR_TYPER) can be accessed in a non atomic
> manner, so we have used readq_relaxed_non_atomic()/writeq_relaxed_non_atomic().
I had another look at the code and I think there needs some 
clarification necessary because the accesses to IROUTER is non-obvious.
> 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().
>
> v3 - 1. Use inline function definitions for {read/write}q_relaxed_non_atomic().
> 2. For AArch64, {read/write}q_relaxed_non_atomic() should invoke 
> {read/write}q_relaxed().
> Thus, we can avoid any ifdef in gic-v3.c.
>
>   xen/arch/arm/gic-v3.c               |  6 +++---
>   xen/arch/arm/include/asm/arm32/io.h | 20 ++++++++++++++++++++
>   xen/arch/arm/include/asm/arm64/io.h |  2 ++
>   3 files changed, 25 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 6457e7033c..3c5b88148c 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -651,7 +651,7 @@ static void __init gicv3_dist_init(void)
>       affinity &= ~GICD_IROUTER_SPI_MODE_ANY;
>   
>       for ( i = NR_GIC_LOCAL_IRQS; i < nr_lines; i++ )
> -        writeq_relaxed(affinity, GICD + GICD_IROUTER + i * 8);
> +        writeq_relaxed_non_atomic(affinity, GICD + GICD_IROUTER + i * 8);
Using non atomic here makes sense because AFAIU the interrupt will be 
disabled. Therefore the GIC should not use the register.
>   }
>   
>   static int gicv3_enable_redist(void)
> @@ -745,7 +745,7 @@ static int __init gicv3_populate_rdist(void)
>           }
>   
>           do {
> -            typer = readq_relaxed(ptr + GICR_TYPER);
> +            typer = readq_relaxed_non_atomic(ptr + GICR_TYPER);
This non-atomic read is OK because GICR_TYPER is read-only.

>   
>               if ( (typer >> 32) == aff )
>               {
> @@ -1265,7 +1265,7 @@ 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 )
> -        writeq_relaxed(affinity, (GICD + GICD_IROUTER + desc->irq * 8));
> +        writeq_relaxed_non_atomic(affinity, (GICD + GICD_IROUTER + desc->irq * 
> 8));
This can be called with interrupt enabled. So a non-atomic access means 
the GIC will see a transient value when only one of two 32-bit will be 
updated.
In practice this is fine because only Aff3 is so far defined in the top 
32-bits. So effectively, they will be RESS0 and never change.
Cheers,

--
Julien Grall