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
