Re: [Qemu-devel] [PATCH 4/5] hw: arm_gic: Support setting/getting binary point reg

[Peter Maydell]

On 23 August 2013 21:10, Christoffer Dall <address@hidden> wrote:
> Add a binary_point field to the gic emulation structure and support
> setting/getting this register now when we have it.  We don't actually
> support interrupt grouping yet, oh well.
>
> Signed-off-by: Christoffer Dall <address@hidden>
> ---
>  hw/intc/arm_gic.c        |    5 ++---
>  hw/intc/arm_gic_common.c |    1 +
>  hw/intc/gic_internal.h   |    3 +++
>  3 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index 4da534f..cb2004d 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -568,8 +568,7 @@ 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 */
> -        /* ??? Not implemented.  */
> -        return 0;
> +        return s->binary_point[0][cpu];
>      case 0x0c: /* Acknowledge */
>          value = gic_acknowledge_irq(s, cpu);
>          value |= (GIC_SGI_SRC(value, cpu) & 0x7) << 10;
> @@ -596,7 +595,7 @@ 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 */
> -        /* ??? Not implemented.  */
> +        s->binary_point[0][cpu] = (value & 0x7);
>          break;
>      case 0x10: /* End Of Interrupt */
>          return gic_complete_irq(s, cpu, value & 0x3ff);
> diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
> index 7a1c9e5..a50ded2 100644
> --- a/hw/intc/arm_gic_common.c
> +++ b/hw/intc/arm_gic_common.c
> @@ -76,6 +76,7 @@ static const VMStateDescription vmstate_gic = {
>          VMSTATE_UINT16_ARRAY(running_irq, GICState, NCPU),
>          VMSTATE_UINT16_ARRAY(running_priority, GICState, NCPU),
>          VMSTATE_UINT16_ARRAY(current_pending, GICState, NCPU),
> +        VMSTATE_UINT8_2DARRAY(binary_point, GICState, 2, NCPU),
>          VMSTATE_END_OF_LIST()
>      }
>  };
> diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
> index 6f04885..424a41f 100644
> --- a/hw/intc/gic_internal.h
> +++ b/hw/intc/gic_internal.h
> @@ -98,6 +98,9 @@ typedef struct GICState {
>      uint16_t running_priority[NCPU];
>      uint16_t current_pending[NCPU];
>
> +    /* these registers are mainly used for save/restore of KVM state */
> +    uint8_t  binary_point[2][NCPU]; /* [0]: group 0, [1]: group 1 */

This looks a bit fishy. I think fundamentally we need
to be clear about what kind of GIC we (and KVM) are
exposing to the guest. I think that that is supposed
to be "a v2 GIC without the Security Extensions", yes?
The other possible answer would be "a v2 GIC with the
security extensions, but you're stuck in the NS world".
The distinction is important, because it affects what
state the guest can see. For v2-GIC-no-TZ, there are
two registers' worth of state per CPU, accessible as
GICC_BPR and GICC_ABPR. For v2-GIC-TZ, a guest stuck in
the NS world only gets to see one register worth of
state, ie the NS banked version of GICC_BPR (and
GICC_ABPR is an alias of it). There is another register
worth of state in the S banked GICC_BPR, but it's
totally inaccessible to the guest.

The TCG QEMU GIC model is currently adopting the
"GIC without Security Extensions" model, which implies
that we should be implementing GIC_ABPR too. What
model does KVM's in-kernel vGIC use? (ie what state
does it put into binary_point[0] and [1] on save/load)?

thanks
-- PMM