Re: [PATCH] irq/arm-gicv3.h: Enable interrupt groups 0 and 1
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
Re: [PATCH] irq/arm-gicv3.h: Enable interrupt groups 0 and 1
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
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
Re: [PATCH] irq/arm-gicv3.h: Enable interrupt groups 0 and 1
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
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
