On 28/6/2022 1:14 pm, Kinsey Moore wrote:
> 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.
I will take a look once the hardware becomes available to me.
I could be mistaken with that piece of the change but I seem to remember both
enable writes causing an issue and no writes resulting in no interrupts.
Chris
_______________________________________________
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
