[PATCH 3/3] aarch64/gicv3: Remove accesses to secure registers

[Kinsey Moore]

On 6/15/2022 09:51, Gedare Bloom wrote:
> On Tue, Jun 14, 2022 at 6:56 PM Chris Johns <chrisj at rtems.org> wrote:
>> On 14/6/2022 11:44 pm, Gedare Bloom wrote:
>>> On Mon, Jun 13, 2022 at 7:39 PM <chrisj at rtems.org> wrote:
>>>> From: Chris Johns <chrisj at rtems.org>
>>>>
>>>> ---
>>>>   bsps/include/dev/irq/arm-gicv3.h | 18 +++++++++++++++---
>>>>   1 file changed, 15 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/bsps/include/dev/irq/arm-gicv3.h b/bsps/include/dev/irq/arm-gicv3.h
>>>> index 0d3ef9a1c1..a79368ebdf 100644
>>>> --- a/bsps/include/dev/irq/arm-gicv3.h
>>>> +++ b/bsps/include/dev/irq/arm-gicv3.h
>>>> @@ -300,12 +300,25 @@ 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;
>>>>     WRITE_SR(ICC_SRE, sre_value);
>>>>     WRITE_SR(ICC_PMR, GIC_CPUIF_ICCPMR_PRIORITY(0xff));
>>>> -  WRITE_SR(ICC_BPR0, GIC_CPUIF_ICCBPR_BINARY_POINT(0x0));
>>> This appears unrelated?
>> The binary pointer effects the secure interrupts and so cannot be touched on
>> EL1. It traps into EL3. It makes sense to me it is protected. There is no code
>> in Petalinux or FreeBSD writing to this register.
>>
>>>>     volatile gic_redist *redist = gicv3_get_redist(cpu_index);
>>>>     uint32_t waker = redist->icrwaker;
>>>> @@ -322,8 +335,7 @@ static void gicv3_init_cpu_interface(uint32_t cpu_index)
>>>>     }
>>>>
>>>>     /* Enable interrupt groups 0 and 1 */
>>>> -  WRITE_SR(ICC_IGRPEN0, 0x1);
>>>> -  WRITE_SR(ICC_IGRPEN1, 0x1);
>>>> +  gic_icc_write(IGRPEN1, 1);
>>> Removed the write to IGRPEN0?
>> The write I replaced is touching a secure register and so traps into EL3 when
>> your TF-A has enabled a secure mode.
>>
>> The enable is replaced with a write to the EL1 accessible register. This is how
>> FreeBSD does it so I copied that method rather than the binary opcode approach
>> which I found complicated.
>>
>> You need a suitably configured TF-A to run on aarch64. I would be questioning
>> any TF-A that lets this code run without these changes. The Xilinx 2020.2
>> vck-190 build of TF-A lets our code run without this patch however 2021.2 had
>> tightened things. Xilinx and I looked into the history of their TF-A source and
>> how they build it and came to the conclusion the change in the secure mode has
>> come from ARM and their TF-A code.
>>
> OK, thanks for the explanation. This looks ok to me.
>
This set of changes makes sense given our previous discussion and the above.


Kinsey