Re: [Qemu-devel] [PATCH v8 2/5] Extract some reusable vGIC code

[Peter Maydell]

On 10 August 2015 at 13:06, Pavel Fedin <address@hidden> 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 <address@hidden>
> ---
>  hw/intc/arm_gic_kvm.c | 84 
> ++++++++++++++++++++++++---------------------------
>  hw/intc/vgic_common.h | 43 ++++++++++++++++++++++++++
>  2 files changed, 82 insertions(+), 45 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 e5d0f67..6ce539b 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)

The need to add num_irq to this prototype reveals the ugliness
of it as an interface. It would be much nicer to convert the
GIC to using named GPIO arrays for its incoming interrupt
lines, so that you can handle the different cases of SPIs
and PPIs naturally rather than exposing the awkward mapping
from multiple different sets of interrupts into a single
integer space.

I don't insist on that being done for the GICv3 work, but it
does rather affect the shape of these functions you're trying
to factor out, so it's worth considering.

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

If you want to make this public it would be better to call
it kvm_device_supports_attr() and put it in kvm-all.c, because
it's not really GIC specific.

>  {
>      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);
> -}

I don't think we gain much by making these two functions common,
and we do get a lot of churn in the existing code below.

> -
>  #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;
>          }
>      }
> @@ -559,7 +552,7 @@ static void kvm_arm_gic_realize(DeviceState *dev, Error 
> **errp)
>          return;
>      }
>
> -    gic_init_irqs_and_mmio(s, kvm_arm_gic_set_irq, NULL);
> +    gic_init_irqs_and_mmio(s, kvm_arm_gicv2_set_irq, NULL);
>
>      for (i = 0; i < s->num_irq - GIC_INTERNAL; i++) {
>          qemu_irq irq = qdev_get_gpio_in(dev, i);
> @@ -576,15 +569,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);
>      }
>
> 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);

Any new functions declared in header files should have documentation
comments in the usual format.

thanks
-- PMM