Re: [boot-wrapper PATCH 3/5] GICv3: initialize without RMW

[Andre Przywara <andre.przywara@arm.com>]

On 8/24/21 2:48 PM, Mark Rutland wrote:
> There's no need to perform an RMW sequence to initialize ICC_SRE_EL3, as
> there are no bits that we need to preserve, and generally we should
> reset registers to specific values such that RESx bits aren't configured
> to UNKNOWN values that could be problematic in future architecture
> versions.
> 
> Instead, let's initialize ICC_SRE_EL3 with a constant value. Since the
> `DIB` and `DFB` fields are RAO/WI in some configurations and we have no
> reason to initialize these to 0, we always initialize these to 1, in
> addition to `SRE` and `SRE_Enable`.

Indeed, actually the architectural reset value is 0 (bypass enabled), 
and we seem to be just saved by the fact that the model implements them 
as RAO/WI. So forcing a value is the right thing to do.

> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Alexandru Elisei <alexandru.elisei@arm.com>
> Cc: Andre Przywara <andre.przywara@arm.com>

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

> ---
>   arch/aarch32/include/asm/gic-v3.h | 7 -------
>   arch/aarch64/include/asm/gic-v3.h | 7 -------
>   common/gic-v3.c                   | 8 +++-----
>   3 files changed, 3 insertions(+), 19 deletions(-)
> 
> diff --git a/arch/aarch32/include/asm/gic-v3.h b/arch/aarch32/include/asm/gic-v3.h
> index ec9a327..65f38de 100644
> --- a/arch/aarch32/include/asm/gic-v3.h
> +++ b/arch/aarch32/include/asm/gic-v3.h
> @@ -9,13 +9,6 @@
>   #ifndef __ASM_AARCH32_GICV3_H
>   #define __ASM_AARCH32_GICV3_H
>   
> -static inline uint32_t gic_read_icc_sre(void)
> -{
> -	uint32_t val;
> -	asm volatile ("mrc p15, 6, %0, c12, c12, 5" : "=r" (val));
> -	return val;
> -}
> -
>   static inline void gic_write_icc_sre(uint32_t val)
>   {
>   	asm volatile ("mcr p15, 6, %0, c12, c12, 5" : : "r" (val));
> diff --git a/arch/aarch64/include/asm/gic-v3.h b/arch/aarch64/include/asm/gic-v3.h
> index e743c02..5b32380 100644
> --- a/arch/aarch64/include/asm/gic-v3.h
> +++ b/arch/aarch64/include/asm/gic-v3.h
> @@ -15,13 +15,6 @@
>   #define ICC_CTLR_EL3	"S3_6_C12_C12_4"
>   #define ICC_PMR_EL1	"S3_0_C4_C6_0"
>   
> -static inline uint32_t gic_read_icc_sre(void)
> -{
> -	uint32_t val;
> -	asm volatile ("mrs %0, " ICC_SRE_EL3 : "=r" (val));
> -	return val;
> -}
> -
>   static inline void gic_write_icc_sre(uint32_t val)
>   {
>   	asm volatile ("msr " ICC_SRE_EL3 ", %0" : : "r" (val));
> diff --git a/common/gic-v3.c b/common/gic-v3.c
> index 62f9676..6207007 100644
> --- a/common/gic-v3.c
> +++ b/common/gic-v3.c
> @@ -42,6 +42,8 @@
>   #define GICR_TYPER_Last			(1 << 4)
>   
>   #define ICC_SRE_SRE			(1 << 0)
> +#define ICC_SRE_DFB			(1 << 1)
> +#define ICC_SRE_DIB			(1 << 2)
>   #define ICC_SRE_Enable			(1 << 3)
>   
>   void gic_secure_init_primary(void)
> @@ -101,8 +103,6 @@ void gic_secure_init_primary(void)
>   
>   void gic_secure_init(void)
>   {
> -	uint32_t sre;
> -
>   	/*
>   	 * If GICv3 is not available, skip initialisation. The OS will probably
>   	 * fail with a warning, but this should be easier to debug than a
> @@ -114,9 +114,7 @@ void gic_secure_init(void)
>   	if (this_cpu_logical_id() == 0)
>   		gic_secure_init_primary();
>   
> -	sre = gic_read_icc_sre();
> -	sre |= ICC_SRE_Enable | ICC_SRE_SRE;
> -	gic_write_icc_sre(sre);
> +	gic_write_icc_sre(ICC_SRE_Enable | ICC_SRE_DIB | ICC_SRE_DFB | ICC_SRE_SRE);
>   	isb();
>   
>   	gic_write_icc_ctlr(0);
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel