[v3,2/5] Extract some reusable vGIC code

These functions are useful also for vGICv3 implementation. Make them accessible
from within other modules.

Actually kvm_dist_get() and kvm_dist_put() could also be made reusable, but
they would require two extra parameters (s->dev_fd and s->num_cpu) as well as
lots of typecasts of 's' to DeviceState * and back to GICState *. This makes
the code very ugly so i decided to stop at this point. I tried also an
approach with making a base class for all possible GICs, but it would contain
only three variables (dev_fd, cpu_num and irq_num), and accessing them through
the rest of the code would be again tedious (either ugly casts or qemu-style
separate object pointer). So i disliked it too.

Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
---
 hw/intc/arm_gic_kvm.c | 82 ++++++++++++++++++++++++---------------------------
 hw/intc/vgic_common.h | 43 +++++++++++++++++++++++++++
 2 files changed, 81 insertions(+), 44 deletions(-)
 create mode 100644 hw/intc/vgic_common.h

Pavel,
On 07/07/2015 03:54 PM, Pavel Fedin wrote:
> These functions are useful also for vGICv3 implementation. Make them accessible
> from within other modules.
> 
> Actually kvm_dist_get() and kvm_dist_put() could also be made reusable, but
> they would require two extra parameters (s->dev_fd and s->num_cpu) as well as
> lots of typecasts of 's' to DeviceState * and back to GICState *. This makes
> the code very ugly so i decided to stop at this point. I tried also an
> approach with making a base class for all possible GICs, but it would contain
> only three variables (dev_fd, cpu_num and irq_num), and accessing them through
> the rest of the code would be again tedious (either ugly casts or qemu-style
> separate object pointer). So i disliked it too.
> 
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> ---
>  hw/intc/arm_gic_kvm.c | 82 ++++++++++++++++++++++++---------------------------
>  hw/intc/vgic_common.h | 43 +++++++++++++++++++++++++++
>  2 files changed, 81 insertions(+), 44 deletions(-)
>  create mode 100644 hw/intc/vgic_common.h
> 
> diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c
> index f56bff1..8821559 100644
> --- a/hw/intc/arm_gic_kvm.c
> +++ b/hw/intc/arm_gic_kvm.c
> @@ -23,6 +23,7 @@
>  #include "sysemu/kvm.h"
>  #include "kvm_arm.h"
>  #include "gic_internal.h"
> +#include "vgic_common.h"
>  
>  //#define DEBUG_GIC_KVM
>  
> @@ -52,7 +53,7 @@ typedef struct KVMARMGICClass {
>      void (*parent_reset)(DeviceState *dev);
>  } KVMARMGICClass;
>  
> -static void kvm_arm_gic_set_irq(void *opaque, int irq, int level)
> +void kvm_arm_gic_set_irq(uint32_t num_irq, int irq, int level)
>  {
>      /* Meaning of the 'irq' parameter:
>       *  [0..N-1] : external interrupts
> @@ -63,10 +64,9 @@ static void kvm_arm_gic_set_irq(void *opaque, int irq, int level)
>       * has separate fields in the irq number for type,
>       * CPU number and interrupt number.
>       */
> -    GICState *s = (GICState *)opaque;
>      int kvm_irq, irqtype, cpu;
>  
> -    if (irq < (s->num_irq - GIC_INTERNAL)) {
> +    if (irq < (num_irq - GIC_INTERNAL)) {
>          /* External interrupt. The kernel numbers these like the GIC
>           * hardware, with external interrupt IDs starting after the
>           * internal ones.
> @@ -77,7 +77,7 @@ static void kvm_arm_gic_set_irq(void *opaque, int irq, int level)
>      } else {
>          /* Internal interrupt: decode into (cpu, interrupt id) */
>          irqtype = KVM_ARM_IRQ_TYPE_PPI;
> -        irq -= (s->num_irq - GIC_INTERNAL);
> +        irq -= (num_irq - GIC_INTERNAL);
>          cpu = irq / GIC_INTERNAL;
>          irq %= GIC_INTERNAL;
>      }
> @@ -87,12 +87,19 @@ static void kvm_arm_gic_set_irq(void *opaque, int irq, int level)
>      kvm_set_irq(kvm_state, kvm_irq, !!level);
>  }
>  
> +static void kvm_arm_gicv2_set_irq(void *opaque, int irq, int level)
> +{
> +    GICState *s = (GICState *)opaque;
> +
> +    kvm_arm_gic_set_irq(s->num_irq, irq, level);
> +}
> +
>  static bool kvm_arm_gic_can_save_restore(GICState *s)
>  {
>      return s->dev_fd >= 0;
>  }
>  
> -static bool kvm_gic_supports_attr(GICState *s, int group, int attrnum)
> +bool kvm_gic_supports_attr(int dev_fd, int group, int attrnum)
>  {
>      struct kvm_device_attr attr = {
>          .group = group,
> @@ -100,14 +107,14 @@ static bool kvm_gic_supports_attr(GICState *s, int group, int attrnum)
>          .flags = 0,
>      };
>  
> -    if (s->dev_fd == -1) {
> +    if (dev_fd == -1) {
>          return false;
>      }
>  
> -    return kvm_device_ioctl(s->dev_fd, KVM_HAS_DEVICE_ATTR, &attr) == 0;
> +    return kvm_device_ioctl(dev_fd, KVM_HAS_DEVICE_ATTR, &attr) == 0;
>  }
>  
> -static void kvm_gic_access(GICState *s, int group, int offset,
> +void kvm_gic_access(int dev_fd, int group, int offset,
>                                     int cpu, uint32_t *val, bool write)
>  {
>      struct kvm_device_attr attr;
> @@ -130,7 +137,7 @@ static void kvm_gic_access(GICState *s, int group, int offset,
>          type = KVM_GET_DEVICE_ATTR;
>      }
>  
> -    err = kvm_device_ioctl(s->dev_fd, type, &attr);
> +    err = kvm_device_ioctl(dev_fd, type, &attr);
>      if (err < 0) {
>          fprintf(stderr, "KVM_{SET/GET}_DEVICE_ATTR failed: %s\n",
>                  strerror(-err));
> @@ -138,20 +145,6 @@ static void kvm_gic_access(GICState *s, int group, int offset,
>      }
>  }
>  
> -static void kvm_gicd_access(GICState *s, int offset, int cpu,
> -                            uint32_t *val, bool write)
> -{
> -    kvm_gic_access(s, KVM_DEV_ARM_VGIC_GRP_DIST_REGS,
> -                   offset, cpu, val, write);
> -}
> -
> -static void kvm_gicc_access(GICState *s, int offset, int cpu,
> -                            uint32_t *val, bool write)
> -{
> -    kvm_gic_access(s, KVM_DEV_ARM_VGIC_GRP_CPU_REGS,
> -                   offset, cpu, val, write);
> -}
> -
>  #define for_each_irq_reg(_ctr, _max_irq, _field_width) \
>      for (_ctr = 0; _ctr < ((_max_irq) / (32 / (_field_width))); _ctr++)
>  
> @@ -291,7 +284,7 @@ static void kvm_dist_get(GICState *s, uint32_t offset, int width,
>          irq = i * regsz;
>          cpu = 0;
>          while ((cpu < s->num_cpu && irq < GIC_INTERNAL) || cpu == 0) {
> -            kvm_gicd_access(s, offset, cpu, &reg, false);
> +            kvm_gicd_access(s->dev_fd, offset, cpu, &reg, false);
>              for (j = 0; j < regsz; j++) {
>                  field = extract32(reg, j * width, width);
>                  translate_fn(s, irq + j, cpu, &field, false);
> @@ -324,7 +317,7 @@ static void kvm_dist_put(GICState *s, uint32_t offset, int width,
>                  translate_fn(s, irq + j, cpu, &field, true);
>                  reg = deposit32(reg, j * width, width, field);
>              }
> -            kvm_gicd_access(s, offset, cpu, &reg, true);
> +            kvm_gicd_access(s->dev_fd, offset, cpu, &reg, true);
>  
>              cpu++;
>          }
> @@ -355,10 +348,10 @@ static void kvm_arm_gic_put(GICState *s)
>  
>      /* s->ctlr -> GICD_CTLR */
>      reg = s->ctlr;
> -    kvm_gicd_access(s, 0x0, 0, &reg, true);
> +    kvm_gicd_access(s->dev_fd, 0x0, 0, &reg, true);
>  
>      /* Sanity checking on GICD_TYPER and s->num_irq, s->num_cpu */
> -    kvm_gicd_access(s, 0x4, 0, &reg, false);
> +    kvm_gicd_access(s->dev_fd, 0x4, 0, &reg, false);
>      num_irq = ((reg & 0x1f) + 1) * 32;
>      num_cpu = ((reg & 0xe0) >> 5) + 1;
>  
> @@ -416,24 +409,24 @@ static void kvm_arm_gic_put(GICState *s)
>      for (cpu = 0; cpu < s->num_cpu; cpu++) {
>          /* s->cpu_ctlr[cpu] -> GICC_CTLR */
>          reg = s->cpu_ctlr[cpu];
> -        kvm_gicc_access(s, 0x00, cpu, &reg, true);
> +        kvm_gicc_access(s->dev_fd, 0x00, cpu, &reg, true);
>  
>          /* s->priority_mask[cpu] -> GICC_PMR */
>          reg = (s->priority_mask[cpu] & 0xff);
> -        kvm_gicc_access(s, 0x04, cpu, &reg, true);
> +        kvm_gicc_access(s->dev_fd, 0x04, cpu, &reg, true);
>  
>          /* s->bpr[cpu] -> GICC_BPR */
>          reg = (s->bpr[cpu] & 0x7);
> -        kvm_gicc_access(s, 0x08, cpu, &reg, true);
> +        kvm_gicc_access(s->dev_fd, 0x08, cpu, &reg, true);
>  
>          /* s->abpr[cpu] -> GICC_ABPR */
>          reg = (s->abpr[cpu] & 0x7);
> -        kvm_gicc_access(s, 0x1c, cpu, &reg, true);
> +        kvm_gicc_access(s->dev_fd, 0x1c, cpu, &reg, true);
>  
>          /* s->apr[n][cpu] -> GICC_APRn */
>          for (i = 0; i < 4; i++) {
>              reg = s->apr[i][cpu];
> -            kvm_gicc_access(s, 0xd0 + i * 4, cpu, &reg, true);
> +            kvm_gicc_access(s->dev_fd, 0xd0 + i * 4, cpu, &reg, true);
>          }
>      }
>  }
> @@ -454,11 +447,11 @@ static void kvm_arm_gic_get(GICState *s)
>       */
>  
>      /* GICD_CTLR -> s->ctlr */
> -    kvm_gicd_access(s, 0x0, 0, &reg, false);
> +    kvm_gicd_access(s->dev_fd, 0x0, 0, &reg, false);
>      s->ctlr = reg;
>  
>      /* Sanity checking on GICD_TYPER -> s->num_irq, s->num_cpu */
> -    kvm_gicd_access(s, 0x4, 0, &reg, false);
> +    kvm_gicd_access(s->dev_fd, 0x4, 0, &reg, false);
>      s->num_irq = ((reg & 0x1f) + 1) * 32;
>      s->num_cpu = ((reg & 0xe0) >> 5) + 1;
>  
> @@ -469,7 +462,7 @@ static void kvm_arm_gic_get(GICState *s)
>      }
>  
>      /* GICD_IIDR -> ? */
> -    kvm_gicd_access(s, 0x8, 0, &reg, false);
> +    kvm_gicd_access(s->dev_fd, 0x8, 0, &reg, false);
>  
>      /* Clear all the IRQ settings */
>      for (i = 0; i < s->num_irq; i++) {
> @@ -507,24 +500,24 @@ static void kvm_arm_gic_get(GICState *s)
>  
>      for (cpu = 0; cpu < s->num_cpu; cpu++) {
>          /* GICC_CTLR -> s->cpu_ctlr[cpu] */
> -        kvm_gicc_access(s, 0x00, cpu, &reg, false);
> +        kvm_gicc_access(s->dev_fd, 0x00, cpu, &reg, false);
>          s->cpu_ctlr[cpu] = reg;
>  
>          /* GICC_PMR -> s->priority_mask[cpu] */
> -        kvm_gicc_access(s, 0x04, cpu, &reg, false);
> +        kvm_gicc_access(s->dev_fd, 0x04, cpu, &reg, false);
>          s->priority_mask[cpu] = (reg & 0xff);
>  
>          /* GICC_BPR -> s->bpr[cpu] */
> -        kvm_gicc_access(s, 0x08, cpu, &reg, false);
> +        kvm_gicc_access(s->dev_fd, 0x08, cpu, &reg, false);
>          s->bpr[cpu] = (reg & 0x7);
>  
>          /* GICC_ABPR -> s->abpr[cpu] */
> -        kvm_gicc_access(s, 0x1c, cpu, &reg, false);
> +        kvm_gicc_access(s->dev_fd, 0x1c, cpu, &reg, false);
>          s->abpr[cpu] = (reg & 0x7);
>  
>          /* GICC_APRn -> s->apr[n][cpu] */
>          for (i = 0; i < 4; i++) {
> -            kvm_gicc_access(s, 0xd0 + i * 4, cpu, &reg, false);
> +            kvm_gicc_access(s->dev_fd, 0xd0 + i * 4, cpu, &reg, false);
>              s->apr[i][cpu] = reg;
>          }
>      }
> @@ -596,15 +589,16 @@ static void kvm_arm_gic_realize(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> -    if (kvm_gic_supports_attr(s, KVM_DEV_ARM_VGIC_GRP_NR_IRQS, 0)) {
> +    if (kvm_gic_supports_attr(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_NR_IRQS, 0)) {
>          uint32_t numirqs = s->num_irq;
> -        kvm_gic_access(s, KVM_DEV_ARM_VGIC_GRP_NR_IRQS, 0, 0, &numirqs, 1);
> +        kvm_gic_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_NR_IRQS, 0, 0,
> +                       &numirqs, 1);
>      }
>  
>      /* Tell the kernel to complete VGIC initialization now */
> -    if (kvm_gic_supports_attr(s, KVM_DEV_ARM_VGIC_GRP_CTRL,
> +    if (kvm_gic_supports_attr(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
>                                KVM_DEV_ARM_VGIC_CTRL_INIT)) {
> -        kvm_gic_access(s, KVM_DEV_ARM_VGIC_GRP_CTRL,
> +        kvm_gic_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
>                            KVM_DEV_ARM_VGIC_CTRL_INIT, 0, 0, 1);
>      }

this patch file does not compile without next patches in the series:
/local/augere/LINARO/QEMU/qemu/hw/intc/arm_gic_kvm.c:565:5: error:
passing argument 2 of 'qdev_init_gpio_in' from incompatible pointer type
[-Werror]
     qdev_init_gpio_in(dev, kvm_arm_gic_set_irq, i);

also

/local/augere/LINARO/QEMU/qemu/hw/intc/arm_gic_kvm.c:90:13: error:
'kvm_arm_gicv2_set_irq' defined but not used [-Werror=unused-function]
 static void kvm_arm_gicv2_set_irq(void *opaque, int irq, int level)

its structure must at least be reconsidered to fix those compilation
issues I think.

Eric

>  
> diff --git a/hw/intc/vgic_common.h b/hw/intc/vgic_common.h
> new file mode 100644
> index 0000000..67241d3
> --- /dev/null
> +++ b/hw/intc/vgic_common.h
> @@ -0,0 +1,43 @@
> +/*
> + * ARM KVM vGIC utility functions
> + *
> + * Copyright (c) 2015 Samsung Electronics
> + * Written by Pavel Fedin
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef QEMU_ARM_VGIC_COMMON_H
> +#define QEMU_ARM_VGIC_COMMON_H
> +
> +void kvm_arm_gic_set_irq(uint32_t num_irq, int irq, int level);
> +bool kvm_gic_supports_attr(int dev_fd, int group, int attrnum);
> +void kvm_gic_access(int dev_fd, int group, int offset, int cpu,
> +                    uint32_t *val, bool write);
> +
> +static inline void kvm_gicd_access(int dev_fd, int offset, int cpu,
> +                                   uint32_t *val, bool write)
> +{
> +    kvm_gic_access(dev_fd, KVM_DEV_ARM_VGIC_GRP_DIST_REGS,
> +                   offset, cpu, val, write);
> +}
> +
> +static inline void kvm_gicc_access(int dev_fd, int offset, int cpu,
> +                                   uint32_t *val, bool write)
> +{
> +    kvm_gic_access(dev_fd, KVM_DEV_ARM_VGIC_GRP_CPU_REGS,
> +                   offset, cpu, val, write);
> +}
> +
> +#endif
>

Hi!

> /local/augere/LINARO/QEMU/qemu/hw/intc/arm_gic_kvm.c:565:5: error:
> passing argument 2 of 'qdev_init_gpio_in' from incompatible pointer type
> [-Werror]
>      qdev_init_gpio_in(dev, kvm_arm_gic_set_irq, i);
> 
> also
> 
> /local/augere/LINARO/QEMU/qemu/hw/intc/arm_gic_kvm.c:90:13: error:
> 'kvm_arm_gicv2_set_irq' defined but not used [-Werror=unused-function]
>  static void kvm_arm_gicv2_set_irq(void *opaque, int irq, int level)

 Ooops, i have occasionally missed this during rebase and conflict fixing. Actually it's a big bug,
and the code would not even work. :( Thank you for spotting this. In my master this issue is not
present. Fixing and will respin today.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia