On Fri, Sep 06, 2013 at 04:57:05PM +0200, Paolo Bonzini wrote:
Il 25/08/2013 17:47, Alexander Graf ha scritto:
On 23.08.2013, at 21:10, Christoffer Dall wrote:
Save and restore the ARM KVM VGIC state from the kernel. We rely on QEMU to marshal the GICState data structure and therefore simply synchronize the kernel state with the QEMU emulated state in both directions.
We take some care on the restore path to check the VGIC has been configured with enough IRQs and CPU interfaces that we can properly restore the state, and for separate set/clear registers we first fully clear the registers and then set the required bits.
Signed-off-by: Christoffer Dall christoffer.dall@linaro.org
hw/intc/arm_gic_common.c | 2 + hw/intc/arm_gic_kvm.c | 418 ++++++++++++++++++++++++++++++++++++++++++- hw/intc/gic_internal.h | 1 + include/migration/vmstate.h | 6 + 4 files changed, 425 insertions(+), 2 deletions(-)
diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c index a50ded2..f39bdc1 100644 --- a/hw/intc/arm_gic_common.c +++ b/hw/intc/arm_gic_common.c @@ -77,6 +77,8 @@ static const VMStateDescription vmstate_gic = { VMSTATE_UINT16_ARRAY(running_priority, GICState, NCPU), VMSTATE_UINT16_ARRAY(current_pending, GICState, NCPU), VMSTATE_UINT8_2DARRAY(binary_point, GICState, 2, NCPU),
VMSTATE_UINT32_2DARRAY(active_prio, GICState, 4, NCPU),VMSTATE_UINT32(num_irq, GICState), VMSTATE_END_OF_LIST()}; diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c index 9f0a852..9785281 100644 --- a/hw/intc/arm_gic_kvm.c +++ b/hw/intc/arm_gic_kvm.c @@ -23,6 +23,15 @@ #include "kvm_arm.h" #include "gic_internal.h"
+//#define DEBUG_GIC_KVM
+#ifdef DEBUG_GIC_KVM +#define DPRINTF(fmt, ...) \ +do { fprintf(stderr, "arm_gic: " fmt , ## __VA_ARGS__); } while (0) +#else +#define DPRINTF(fmt, ...) do {} while(0)
You really want to make DPRINTF still be format checked by the compiler. Check out hw/intc/openpic.c for an example how to get there.
+#endif
#define TYPE_KVM_ARM_GIC "kvm-arm-gic" #define KVM_ARM_GIC(obj) \ OBJECT_CHECK(GICState, (obj), TYPE_KVM_ARM_GIC) @@ -73,14 +82,419 @@ 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) +{
- KVMARMGICClass *kgc = KVM_ARM_GIC_GET_CLASS(s);
- return kgc->dev_fd >= 0;
+}
+static void kvm_arm_gic_reg_access(GICState *s, int group, int offset,
int cpu, uint32_t *val, bool write)+{
- KVMARMGICClass *kgc = KVM_ARM_GIC_GET_CLASS(s);
- struct kvm_device_attr attr;
- int type;
- 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 = (uint64_t)(long)val;
uintptr_t?
- if (write) {
type = KVM_SET_DEVICE_ATTR;- } else {
type = KVM_GET_DEVICE_ATTR;- }
- if (kvm_device_ioctl(kgc->dev_fd, type, &attr) < 0) {
fprintf(stderr, "KVM_{SET/GET}_DEVICE_ATTR failed: %s\n",strerror(errno));abort();- }
+}
+static void kvm_arm_gic_dist_reg_access(GICState *s, int offset, int cpu,
uint32_t *val, bool write)+{
- kvm_arm_gic_reg_access(s, KVM_DEV_ARM_VGIC_GRP_DIST_REGS,
offset, cpu, val, write);+}
+static void kvm_arm_gic_cpu_reg_access(GICState *s, int offset, int cpu,
uint32_t *val, bool write)+{
- kvm_arm_gic_reg_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++)
+/*
- Translate from the in-kernel field for an IRQ value to/from the qemu
- representation.
- */
+typedef void (*vgic_translate_fn)(GICState *s, int irq, int cpu,
uint32_t *field, bool to_kernel);+/* synthetic translate function used for clear/set registers to completely
- clear a setting using a clear-register before setting the remaing bits
- using a set-register */
+static void translate_clear(GICState *s, int irq, int cpu,
uint32_t *field, bool to_kernel)+{
- if (to_kernel) {
*field = ~0;- } else {
/* does not make sense: qemu model doesn't use set/clear regs */abort();- }
I don't understand the to_kernel bits. I thought we're in a device file that only works with KVM?
The opposite of "to_kernel" is "from_kernel".
IIUC this is for GIC registers that are modelled as a pair of MMIO registers, one with write-1-sets and one with write-1-clears semantics. The patch is first writing all ones to the w1c register, and then writing the actual value to the w1s register.
The way we solved this for x86 is that the set-registers, when called from userspace, also clear the zero bits. See the "host_initiated" parts in arch/x86/kvm/x86.c. However, this would be an ABI change for ARM, so Christoffer's solution is also fine.
I prefer sticking to the ARM ABI so we don't have to keep a documentation of exceptions to that around to the widest extend possible. See my comment on Alex's e-mail for a more thorough response to this as well.
Sorry for the long response time on this.
One comment on the function names:
kvm_arm_gic_dist_readr kvm_arm_gic_dist_writer
Why not get_reg/set_reg (I was quite surprised to see readr instead of reader :) and it took me a while to understand the convention)? Or if the name is too long, s/readr/get/ and s/writer/set/ would be enough to match the ioctls.
r for register, read register, write register...
I thought I'd seen this convention elsewhere, but I may be wrong. If you feel really strongly about it, I can rename.
-Christoffer