On 22 August 2014 05:29, Fabian Aggeler <[email protected]> wrote:
> This register is banked in GICs with Security Extensions. Storing the
> non-secure copy of BPR in the abpr, which is an alias to the non-secure
> copy for secure access. ABPR itself is only accessible from secure state
> if the GIC implements Security Extensions.
>
> Signed-off-by: Fabian Aggeler <[email protected]>
> ---
> hw/intc/arm_gic.c | 25 +++++++++++++++++++++----
> include/hw/intc/arm_gic_common.h | 8 +++++---
> 2 files changed, 26 insertions(+), 7 deletions(-)
>
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index 7f7fac3..57021fd 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -792,7 +792,12 @@ static uint32_t gic_cpu_read(GICState *s, int cpu,
> int offset)
> case 0x04: /* Priority mask */
> return s->priority_mask[cpu];
> case 0x08: /* Binary Point */
> - return s->bpr[cpu];
> + if (s->security_extn && ns_access()) {
> + /* BPR is banked. Non-secure copy stored in ABPR. */
> + return s->abpr[cpu];
> + } else {
> + return s->bpr[cpu];
> + }
> case 0x0c: /* Acknowledge */
> return gic_acknowledge_irq(s, cpu);
> case 0x14: /* Running Priority */
> @@ -800,7 +805,14 @@ static uint32_t gic_cpu_read(GICState *s, int cpu,
> int offset)
> case 0x18: /* Highest Pending Interrupt */
> return s->current_pending[cpu];
> case 0x1c: /* Aliased Binary Point */
> - return s->abpr[cpu];
> + if ((s->security_extn && ns_access())) {
> + /* If Security Extensions are present ABPR is a secure
> register,
> + * only accessible from secure state.
> + */
> + return 0;
> + } else {
> + return s->abpr[cpu];
> + }
> case 0xd0: case 0xd4: case 0xd8: case 0xdc:
> return s->apr[(offset - 0xd0) / 4][cpu];
> default:
> @@ -819,12 +831,17 @@ static void gic_cpu_write(GICState *s, int cpu, int
> offset, uint32_t value)
> s->priority_mask[cpu] = (value & 0xff);
> break;
> case 0x08: /* Binary Point */
> - s->bpr[cpu] = (value & 0x7);
> + if (s->security_extn && ns_access()) {
> + /* BPR is banked. Non-secure copy stored in ABPR. */
> + s->abpr[cpu] = (value & 0x7);
> + } else {
> + s->bpr[cpu] = (value & 0x7);
> + }
> break;
> case 0x10: /* End Of Interrupt */
> return gic_complete_irq(s, cpu, value & 0x3ff);
> case 0x1c: /* Aliased Binary Point */
> - if (s->revision >= 2) {
> + if (s->revision >= 2 && !(s->security_extn && ns_access())) {
>
According to to the v2 spec, this register is present in GICv1 if the
security extensions are present but always in GICv2 (reason for the
previous condition). I think this needs to be rewritten to be :
if ((s->revision >= 2 && !s->security_extn) || (s->security_extn &&
!ns_access()) {
s->abpr[cpu] = (value & 0x7);
}
> s->abpr[cpu] = (value & 0x7);
> }
> break;
> diff --git a/include/hw/intc/arm_gic_common.h
> b/include/hw/intc/arm_gic_common.h
> index a912972..c547418 100644
> --- a/include/hw/intc/arm_gic_common.h
> +++ b/include/hw/intc/arm_gic_common.h
> @@ -78,9 +78,11 @@ typedef struct GICState {
> uint16_t running_priority[GIC_NCPU];
> uint16_t current_pending[GIC_NCPU];
>
> - /* We present the GICv2 without security extensions to a guest and
> - * therefore the guest can configure the GICC_CTLR to configure group
> 1
> - * binary point in the abpr.
> + /* If we present the GICv2 without security extensions to a guest,
> + * the guest can configure the GICC_CTLR to configure group 1 binary
> point
> + * in the abpr.
> + * For a GIC with Security Extensions we use use bpr for the
> + * secure copy and abpr as storage for the non-secure copy of the
> register.
> */
> uint8_t bpr[GIC_NCPU];
> uint8_t abpr[GIC_NCPU];
> --
> 1.8.3.2
>
>
Not sure if it occurs elsewhere, but these changes don't account for the
GICC_CTLR.CBPR settings which affects the read value.
