Re: [Qemu-devel] [PATCH v2 09/16] hw/intc/arm_gic: Make ICCBPR/GICC_BPR banked

[Peter Maydell]

On 30 October 2014 at 22:12, Greg Bellows <address@hidden> wrote:
> From: Fabian Aggeler <address@hidden>
>
> 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 <address@hidden>
>
> ---
>
> v1 -> v2
> - Fix ABPR read handling when security extensions are not present
> - Fix BPR write to take into consideration the minimum value written to ABPR
>   and restrict BPR->ABPR mirroring to GICv2 and up.
> - Fix ABPR write to take into consideration the minumum value written
> - Fix ABPR write condition break-down to include mirroring of ABPR writes to
>   BPR.
> ---
>  hw/intc/arm_gic.c                | 54 
> ++++++++++++++++++++++++++++++++++++----
>  include/hw/intc/arm_gic_common.h | 11 +++++---
>  2 files changed, 57 insertions(+), 8 deletions(-)
>
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index 3c0414f..3761d12 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -840,7 +840,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 */
> @@ -848,7 +853,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 || (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:
> @@ -867,13 +879,45 @@ 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. */
> +            /* The non-secure (ABPR) must not be below an implementation
> +             * defined minimum value between 1-4.
> +             * NOTE: BPR_MIN is currently set to 0, which is always true 
> given
> +             *       the value is unsigned, so no check is necessary.
> +             */
> +            s->abpr[cpu] = (GIC_MIN_ABPR <= (value & 0x7))
> +                                ? (value & 0x7) : GIC_MIN_ABPR;
> +        } else {
> +            s->bpr[cpu] = (value & 0x7);
> +            if (s->revision >= 2) {
> +                /* On GICv2 without sec ext, GICC_ABPR is an alias of 
> GICC_BPR
> +                 * so mirror the write.
> +                 */
> +                 s->abpr[cpu] = s->bpr[cpu];

My reading of the spec says that GICv2 without Security extensions
should not alias these two registers.

-- PMM