[RFC,v4,7/8] arm_gic: Add GICC_APRn state to the GICState

The GICC_APRn registers are not currently supported by the ARM GIC v2.0
emulation.  This patch adds the missing state.

Note that we also change the number of APRs to use a define GIC_NR_APRS
based on the maximum number of preemption levels.  This patch also adds
RAZ/WI accessors for the four registers on the emulated CPU interface.

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
Changes [v3 -> v4]:
 - Fixed grammatical error and use qemu_log_mask for print.

 hw/intc/arm_gic.c                |  5 +++++
 hw/intc/arm_gic_common.c         |  5 +++--
 include/hw/intc/arm_gic_common.h | 11 +++++++++++
 3 files changed, 19 insertions(+), 2 deletions(-)

On 21 December 2013 06:09, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> The GICC_APRn registers are not currently supported by the ARM GIC v2.0
> emulation.  This patch adds the missing state.
>
> Note that we also change the number of APRs to use a define GIC_NR_APRS
> based on the maximum number of preemption levels.  This patch also adds
> RAZ/WI accessors for the four registers on the emulated CPU interface.
>
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
> Changes [v3 -> v4]:
>  - Fixed grammatical error and use qemu_log_mask for print.
>
>  hw/intc/arm_gic.c                |  5 +++++
>  hw/intc/arm_gic_common.c         |  5 +++--
>  include/hw/intc/arm_gic_common.h | 11 +++++++++++
>  3 files changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index 7ea869a..5973bdb 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -649,6 +649,8 @@ static uint32_t gic_cpu_read(GICState *s, int cpu, int offset)
>          return s->current_pending[cpu];
>      case 0x1c: /* Aliased Binary Point */
>          return s->abpr[cpu];
> +    case 0xd0: case 0xd4: case 0xd8: case 0xdc:
> +        return s->apr[(offset - 0xd0) / 4][cpu];
>      default:
>          qemu_log_mask(LOG_GUEST_ERROR,
>                        "gic_cpu_read: Bad offset %x\n", (int)offset);
> @@ -676,6 +678,9 @@ static void gic_cpu_write(GICState *s, int cpu, int offset, uint32_t value)
>              s->abpr[cpu] = (value & 0x7);
>          }
>          break;
> +    case 0xd0: case 0xd4: case 0xd8: case 0xdc:
> +        qemu_log_mask(LOG_UNIMP, "Writing APR not implemented\n");
> +        break;
>      default:
>          qemu_log_mask(LOG_GUEST_ERROR,
>                        "gic_cpu_write: Bad offset %x\n", (int)offset);
> diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
> index ca12f06..f9a7a0a 100644
> --- a/hw/intc/arm_gic_common.c
> +++ b/hw/intc/arm_gic_common.c
> @@ -59,8 +59,8 @@ static const VMStateDescription vmstate_gic_irq_state = {
>
>  static const VMStateDescription vmstate_gic = {
>      .name = "arm_gic",
> -    .version_id = 6,
> -    .minimum_version_id = 6,
> +    .version_id = 7,
> +    .minimum_version_id = 7,
>      .pre_save = gic_pre_save,
>      .post_load = gic_post_load,
>      .fields = (VMStateField[]) {
> @@ -79,6 +79,7 @@ static const VMStateDescription vmstate_gic = {
>          VMSTATE_UINT16_ARRAY(current_pending, GICState, GIC_NCPU),
>          VMSTATE_UINT8_ARRAY(bpr, GICState, GIC_NCPU),
>          VMSTATE_UINT8_ARRAY(abpr, GICState, GIC_NCPU),
> +        VMSTATE_UINT32_2DARRAY(apr, GICState, GIC_NR_APRS, GIC_NCPU),
>          VMSTATE_END_OF_LIST()
>      }
>  };
> diff --git a/include/hw/intc/arm_gic_common.h b/include/hw/intc/arm_gic_common.h
> index f37bf69..1c8bb2a 100644
> --- a/include/hw/intc/arm_gic_common.h
> +++ b/include/hw/intc/arm_gic_common.h
> @@ -31,6 +31,9 @@
>  /* Maximum number of possible CPU interfaces, determined by GIC architecture */
>  #define GIC_NCPU 8
>
> +#define MAX_NR_GROUP_PRIO 128
> +#define GIC_NR_APRS (MAX_NR_GROUP_PRIO / 32)
> +
>  typedef struct gic_irq_state {
>      /* The enable bits are only banked for per-cpu interrupts.  */
>      uint8_t enabled;
> @@ -76,6 +79,14 @@ typedef struct GICState {
>      uint8_t  bpr[GIC_NCPU];
>      uint8_t  abpr[GIC_NCPU];
>
> +    /* The APR is implementation defined, so we choose a layout identical to
> +     * the KVM ABI layout for QEMU's implementation of the gic:
> +     * If an interrupt for preemption level X is active, then
> +     *   APRn[X mod 32] == 0b1,  where n = X / 32
> +     * otherwise the bit is clear.

If you add a note to this comment:
    * TODO: rewrite the interrupt acknowlege/complete routines to use
    * the APR registers to track the necessary information to update
    * s->running_priority[] on interrupt completion (ie completely remove
    * last_active[][] and running_irq[]). This will be necessary if we ever
    * want to support TCG<->KVM migration, or TCG guests which can
    * do power management involving powering down and restarting
    * the GIC.

then you can add
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

(Both last_active[][] and running_irq[] are not guest visible, and
the only thing we use them for in the TCG GIC is so we can set
the running_priority (which is guest visible). I think that doing this
via the APRn instead is actually pretty straightforward: as per
the GICv2 spec section on GICC_EOIR, gic_complete_irq just
needs to clear the highest-priority bit in the relevant APRn and
set the running priority to correspond to whatever the next
highest set bit is. We might also need to implement priority groups
and the binary point stuff too to get this to work correctly.
Obviously there are cases where this behaves differently from
the current implementation (especially if the guest messes with
the interrupt priority registers while an interrupt is active) but
I think they fall into the realm of IMPDEF behaviour. In any
case, we don't need to try to do this right now...)

thanks
-- PMM