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

[Peter Maydell]

On 9 September 2015 at 08:49, Pavel Fedin  wrote:
> Some functions previously used only by vGICv2 are useful also for vGICv3
> implementation. Untie them from GICState and make accessible from within
> other modules:
> - kvm_arm_gic_set_irq()
> - kvm_gic_supports_attr() - moved to common code and renamed to
>   kvm_device_check_attr()
> - kvm_gic_access() - turned into GIC-independent kvm_device_access().
>   Data pointer changed to void * because some GICv3 registers are
>   64-bit wide
>
> Some of these changes are not used right now, but they will be helpful for
> implementing live migration.
>
> 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 
> Tested-by: Ashok kumar 

Reviewed-by: Peter Maydell 

thanks
-- PMM

[Qemu-devel] [PATCH v14 2/5] intc/gic: Extract some reusable vGIC code

[Pavel Fedin]

Some functions previously used only by vGICv2 are useful also for vGICv3
implementation. Untie them from GICState and make accessible from within
other modules:
- kvm_arm_gic_set_irq()
- kvm_gic_supports_attr() - moved to common code and renamed to
  kvm_device_check_attr()
- kvm_gic_access() - turned into GIC-independent kvm_device_access().
  Data pointer changed to void * because some GICv3 registers are
  64-bit wide

Some of these changes are not used right now, but they will be helpful for
implementing live migration.

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 
Tested-by: Ashok kumar 
---
 hw/intc/arm_gic_kvm.c | 98 +--
 hw/intc/vgic_common.h | 35 ++
 include/sysemu/kvm.h  | 26 ++
 kvm-all.c | 34 ++
 4 files changed, 128 insertions(+), 65 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..e8b2386 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,69 +87,36 @@ static void kvm_arm_gic_set_irq(void *opaque, int irq, int 
level)
 kvm_set_irq(kvm_state, kvm_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)
+static void kvm_arm_gicv2_set_irq(void *opaque, int irq, int level)
 {
-struct kvm_device_attr attr = {
-.group = group,
-.attr = attrnum,
-.flags = 0,
-};
-
-if (s->dev_fd == -1) {
-return false;
-}
+GICState *s = (GICState *)opaque;
 
-return kvm_device_ioctl(s->dev_fd, KVM_HAS_DEVICE_ATTR, ) == 0;
+kvm_arm_gic_set_irq(s->num_irq, irq, level);
 }
 
-static void kvm_gic_access(GICState *s, int group, int offset,
-   int cpu, uint32_t *val, bool write)
+static bool kvm_arm_gic_can_save_restore(GICState *s)
 {
-struct kvm_device_attr attr;
-int type;
-int err;
-
-cpu = cpu & 0xff;
-
-attr.flags = 0;
-attr.group = group;
-attr.attr = (((uint64_t)cpu << KVM_DEV_ARM_VGIC_CPUID_SHIFT) &
- KVM_DEV_ARM_VGIC_CPUID_MASK) |
-(((uint64_t)offset << KVM_DEV_ARM_VGIC_OFFSET_SHIFT) &
- KVM_DEV_ARM_VGIC_OFFSET_MASK);
-attr.addr = (uintptr_t)val;
-
-if (write) {
-type = KVM_SET_DEVICE_ATTR;
-} else {
-type = KVM_GET_DEVICE_ATTR;
-}
-
-err = kvm_device_ioctl(s->dev_fd, type, );
-if (err < 0) {
-fprintf(stderr, "KVM_{SET/GET}_DEVICE_ATTR failed: %s\n",
-strerror(-err));
-abort();
-}
+return s->dev_fd >= 0;
 }
 
+#define KVM_VGIC_ATTR(offset, cpu) \
+uint64_t)(cpu) << KVM_DEV_ARM_VGIC_CPUID_SHIFT) & \
+  KVM_DEV_ARM_VGIC_CPUID_MASK) | \
+ (((uint64_t)(offset) << KVM_DEV_ARM_VGIC_OFFSET_SHIFT) & \
+  KVM_DEV_ARM_VGIC_OFFSET_MASK))
+
 static void kvm_gicd_access(GICState *s, int offset, int