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

["Andreas Färber" <afaerber@suse.de>]

"arm_gic:" is sufficient, "hw/arm_gic:" if you see the need.

Am 23.08.2013 22:10, schrieb Christoffer Dall:
> 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 <christoffer.dall@linaro.org>
> ---
>  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()
>      }
>  };

You can't just add VMState fields without bumping the version_id and
marking the field as starting in that version. That breaks
loadvm/migration format compatibility.

> 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 */

binary_point[1] is never used, but changing the VMState field from
length 1 to length 2 is probably more difficult than having a few extra
bytes here.

Regards,
Andreas

> +
>      uint32_t num_cpu;
>  
>      MemoryRegion iomem; /* Distributor */

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg