[PATCH] irq/arm-gicv3.h: Enable interrupt groups 0 and 1

[Chris Johns]

On 24/6/2022 7:43 pm, Sebastian Huber wrote:
> The GICv3 support is used by AArch32 (indicated by the ARM_MULTILIB_ARCH_V4
> define) and AArch64 targets.  Use the existing WRITE_SR() abstraction to access
> the interrupt group 0 and 1 enable registers.  This fixes the build for the
> AArch32 target.

It needs to be tested on hardware before I am OK with it. It also needs EL3
firmware, ie TF-A, to correctly initialise a system.

I would be OK with qemu if it can be shown it honours the security level
correctly. I however have no time to determine this as I have Versal hardware
that did not like the changes.

> ---
>  bsps/include/dev/irq/arm-gicv3.h | 23 ++++++-----------------
>  1 file changed, 6 insertions(+), 17 deletions(-)
> 
> diff --git a/bsps/include/dev/irq/arm-gicv3.h b/bsps/include/dev/irq/arm-gicv3.h
> index a79368ebdf..7db7bad034 100644
> --- a/bsps/include/dev/irq/arm-gicv3.h
> +++ b/bsps/include/dev/irq/arm-gicv3.h
> @@ -116,9 +116,11 @@ extern "C" {
>  #else /* ARM_MULTILIB_ARCH_V4 */
>  
>  /* AArch64 GICv3 registers are not named in GCC */

The FreeBSD would suggest this is not entirely true? May be it is for aarch32?

> -#define ICC_IGRPEN0 "S3_0_C12_C12_6, %0"
> -#define ICC_IGRPEN1 "S3_0_C12_C12_7, %0"
> +#define ICC_IGRPEN0_EL1 "S3_0_C12_C12_6, %0"
> +#define ICC_IGRPEN1_EL1 "S3_0_C12_C12_7, %0"

This looks like it is only a label change and so it is the same opcode. Is that
correct?

>  #define ICC_IGRPEN1_EL3 "S3_6_C12_C12_7, %0"
> +#define ICC_IGRPEN0 ICC_IGRPEN0_EL1
> +#define ICC_IGRPEN1 ICC_IGRPEN1_EL1
>  #define ICC_PMR     "S3_0_C4_C6_0, %0"
>  #define ICC_EOIR1   "S3_0_C12_C12_1, %0"
>  #define ICC_SRE     "S3_0_C12_C12_5, %0"
> @@ -300,20 +302,6 @@ static void gicv3_init_dist(volatile gic_dist *dist)
>    }
>  }
>  
> -/*
> - * A better way to access these registers than special opcodes
> - */
> -#define isb() __asm __volatile("isb" : : : "memory")
> -
> -#define  WRITE_SPECIALREG(reg, _val)                                    \
> -  __asm __volatile("msr  " __STRING(reg) ", %0" : : "r"((uint64_t)_val))
> -
> -#define  gic_icc_write(reg, val)            \
> -do {                                        \
> -  WRITE_SPECIALREG(icc_ ##reg ##_el1, val); \
> -  isb();                                    \
> -} while (0)
> -
>  static void gicv3_init_cpu_interface(uint32_t cpu_index)
>  {
>    uint32_t sre_value = 0x7;
> @@ -335,7 +323,8 @@ static void gicv3_init_cpu_interface(uint32_t cpu_index)
>    }
>  
>    /* Enable interrupt groups 0 and 1 */
> -  gic_icc_write(IGRPEN1, 1);'

This has been tested and works on a Versal.

> +  WRITE_SR(ICC_IGRPEN0, 0x1);

This crashed in EL1 on a Versal with 2021.2 TF-A.

Why do you need to set this here?

> +  WRITE_SR(ICC_IGRPEN1, 0x1);

This instruction also generated an exception. It has been a while but I am
pretty sure I had to comment this one and when I did no interrupts happened. The
code I ported from FreeBSD worked.

I also think the FreeBSD calls are easier to review and so maintain. I find
those ARM type registers difficult to find and check.

Chris