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

[Kinsey Moore]

On 6/26/2022 22:37, Chris Johns wrote:
> 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.
I, unfortunately, have no way to test this on hardware but I would 
agree. I would not be inclined to trust qemu in this regard.
>> ---
>>   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?
IIRC, a select few were named and usable in GCC, but the vast majority 
were not. This may have gotten better with more recent GCC releases 
since this comment was written more than 2 years ago.
>
>> -#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?
According to the ARMv8 TRM, this is the full proper name for it.
>
>>   #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.
The opcode S3_0_C12_C12_7 should be identical in behavior and assembly 
with the name ICC_IGRPEN1_EL1. It's spelled out in the ARMv8 TRM on 
D12-3006.

Kinsey