[v10,2/5] intc/gic: 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 | 42 +++++++++++++++++----------------------
 hw/intc/vgic_common.h | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 73 insertions(+), 24 deletions(-)
 create mode 100644 hw/intc/vgic_common.h

Hi Pavel,
On 08/18/2015 03:33 PM, Pavel Fedin wrote:
> These functions are useful also for vGICv3 implementation. Make them accessible
> from within other modules.

I think it would be worth justifying the changes in signature:
removal of GICState* due to the introduction of  GICV3State and also
justify replacement of uint32_t *val into void*.
> 
> 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.
I would put the above paragraph below "---"
> 
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> ---
>  hw/intc/arm_gic_kvm.c | 42 +++++++++++++++++----------------------
>  hw/intc/vgic_common.h | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 73 insertions(+), 24 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..0c71ef8 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,6 +87,13 @@ 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)
inline?
> +{
> +    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;
> @@ -107,8 +114,8 @@ static bool kvm_gic_supports_attr(GICState *s, int group, int attrnum)
>      return kvm_device_ioctl(s->dev_fd, KVM_HAS_DEVICE_ATTR, &attr) == 0;
>  }
>  
> -static void kvm_gic_access(GICState *s, int group, int offset,
> -                                   int cpu, uint32_t *val, bool write)
> +void kvm_gic_access(int dev_fd, int group, int offset,
> +                    int cpu, void *val, bool write)
>  {
>      struct kvm_device_attr attr;
>      int type;
> @@ -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++)
>  
> @@ -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);
> @@ -578,13 +571,14 @@ static void kvm_arm_gic_realize(DeviceState *dev, Error **errp)
>  
>      if (kvm_gic_supports_attr(s, 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,
>                                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..130ea64
> --- /dev/null
> +++ b/hw/intc/vgic_common.h
> @@ -0,0 +1,55 @@
> +/*
> + * 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
> +
> +/**
> + * kvm_arm_gic_set_irq - Send an IRQ to the in-kernel vGIC
> + * @num_irq: Total number of IRQs configured for the GIC instance
> + * @irq: qemu internal IRQ line number:
> + *  [0..N-1] : external interrupts
> + *  [N..N+31] : PPI (internal) interrupts for CPU 0
> + *  [N+32..N+63] : PPI (internal interrupts for CPU 1
> + * @level: level of the IRQ line.
> + */
> +void kvm_arm_gic_set_irq(uint32_t num_irq, int irq, int level);
> +
> +/**
> + * kvm_gic_access - Read or write vGIC memory-mapped register
> + * @dev_fd: fd of the device to act on
> + * @group: ID of the memory-mapped region
> + * @offset: offset within the region
> + * @cpu: vCPU number
> + * @val: pointer to the storage area for the data
> + * @write - true for writing and false for reading
> + */
> +void kvm_gic_access(int dev_fd, int group, int offset, int cpu,
> +                    void *val, bool write);
> +
> +#define kvm_gicd_access(s, offset, cpu, val, write) \
> +    kvm_gic_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_DIST_REGS, \
> +                   offset, cpu, val, write)
> +
> +#define kvm_gicc_access(s, offset, cpu, val, write) \
> +    kvm_gic_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CPU_REGS, \
> +                   offset, cpu, val, write)

what is the point of moving kvm_gicd_access and kvm_gicc_access here? If
I am not mistaken, they only are used in arm_gic_kvm.c? I think they can
stay static in arm_gic_kvm.c?

Best Regards

Eric
> +
> +#endif
>

Hello!

> I think it would be worth justifying the changes in signature:
> removal of GICState* due to the introduction of  GICV3State and also
> justify replacement of uint32_t *val into void*.

 I described it in the cover letter. Right now you don't see usage for it, but here i started
prototyping live migration, and in some cases 'val' is going to be uint64_t. This is because
GICD_IROUTER registers are 64 bits wide. kvm_gic_access() by itself does not dereference the
pointer, just passes it to the kernel, therefore i decided to make it type-agnostic.

> what is the point of moving kvm_gicd_access and kvm_gicc_access here? If
> I am not mistaken, they only are used in arm_gic_kvm.c? I think they can
> stay static in arm_gic_kvm.c?

 They will be used in future for live migration, at least kvm_gicd_access. kvm_gicc_access just
accompanies it to keep a single style for both.

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

Hi Pavel,

On 08/19/2015 08:36 AM, Pavel Fedin wrote:
>  Hello!
> 
>> I think it would be worth justifying the changes in signature:
>> removal of GICState* due to the introduction of  GICV3State and also
>> justify replacement of uint32_t *val into void*.
> 
>  I described it in the cover letter. 

OK thanks for the explanations. I think they are useful to understand
the content of the patch and I would advise you to put them in the
commit message since the cover letter then is discarded when committing
the series.
Right now you don't see usage for it, but here i started
> prototyping live migration, and in some cases 'val' is going to be uint64_t. This is because
> GICD_IROUTER registers are 64 bits wide. kvm_gic_access() by itself does not dereference the
> pointer, just passes it to the kernel, therefore i decided to make it type-agnostic.
> 
>> what is the point of moving kvm_gicd_access and kvm_gicc_access here? If
>> I am not mistaken, they only are used in arm_gic_kvm.c? I think they can
>> stay static in arm_gic_kvm.c?
> 
>  They will be used in future for live migration, at least kvm_gicd_access. kvm_gicc_access just
> accompanies it to keep a single style for both.
OK. Then either explain it in the commit msg or move them later when
live migration gets implemented.

Best Regards

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