Re: [Qemu-devel] [PATCH v8 2/5] Extract some reusable vGIC code
Hello! The need to add num_irq to this prototype reveals the ugliness of it as an interface. 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. You know, i was also thinking about it. And actually there is a way to resolve this: make some common structure holding num_irqs, num_cpus and dev_fd, and share it between GICv2 and GICv3 code. But wouldn't this be ugly too? So, i decided to make some compromise and leave it this way, at least for now. Regarding the latter two functions, i thought that they are useful for future live migration, aren't they? 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. No no, not now. I believe this would require lots if changes in all machne models, i'm not ready for this... Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia
Re: [Qemu-devel] [PATCH v8 2/5] Extract some reusable vGIC code
On 10 August 2015 at 13:06, Pavel Fedin p.fe...@samsung.com 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.fe...@samsung.com
---
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,
[Qemu-devel] [PATCH v8 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.fe...@samsung.com
---
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)
{
/* 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,
