Re: [PATCH 3/3] aarch64/gicv3: Remove accesses to secure registers
On 6/15/2022 09:51, Gedare Bloom wrote: On Tue, Jun 14, 2022 at 6:56 PM Chris Johns wrote: On 14/6/2022 11:44 pm, Gedare Bloom wrote: On Mon, Jun 13, 2022 at 7:39 PM wrote: From: Chris Johns --- 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 ___ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel
Re: [PATCH 3/3] aarch64/gicv3: Remove accesses to secure registers
On Tue, Jun 14, 2022 at 6:56 PM Chris Johns wrote: > > On 14/6/2022 11:44 pm, Gedare Bloom wrote: > > On Mon, Jun 13, 2022 at 7:39 PM wrote: > >> > >> From: Chris Johns > >> > >> --- > >> 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. > Chris ___ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel
Re: [PATCH 3/3] aarch64/gicv3: Remove accesses to secure registers
On 14/6/2022 11:44 pm, Gedare Bloom wrote: > On Mon, Jun 13, 2022 at 7:39 PM wrote: >> >> From: Chris Johns >> >> --- >> 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. Chris ___ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel
Re: [PATCH 3/3] aarch64/gicv3: Remove accesses to secure registers
On Mon, Jun 13, 2022 at 7:39 PM wrote: > > From: Chris Johns > > --- > 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? > >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? >WRITE_SR(ICC_CTLR, 0x0); > } > > -- > 2.19.1 > > ___ > devel mailing list > devel@rtems.org > http://lists.rtems.org/mailman/listinfo/devel ___ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel
[PATCH 3/3] aarch64/gicv3: Remove accesses to secure registers
From: Chris Johns --- 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)); 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); WRITE_SR(ICC_CTLR, 0x0); } -- 2.19.1 ___ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel