From 60ff4e63e2ea4738f114cbaf1f17e6e0184fc09c Mon Sep 17 00:00:00 2001 From: Stefan Weil Date: Wed, 1 Apr 2015 17:57:29 +0100 Subject: [PATCH 1/8] hw/arm/highbank: Fix resource leak and wrong image loading Coverity reports a resource leak for sysboot_filename which is allocated by qemu_find_file. In addition, that name is used to get the size of the image, but a different image name was used to load it. In addition, instead of passing the maximum allowed image size the actual image size was passed to load_image_targphys. Fix all three issues. Signed-off-by: Stefan Weil Message-id: 1426326781-2488-1-git-send-email-sw@weilnetz.de Signed-off-by: Peter Maydell --- hw/arm/highbank.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c index ddd10dc26f..07cb4e057b 100644 --- a/hw/arm/highbank.c +++ b/hw/arm/highbank.c @@ -278,8 +278,7 @@ static void calxeda_init(MachineState *machine, enum cxmachines machine_id) if (bios_name != NULL) { sysboot_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name); if (sysboot_filename != NULL) { - uint32_t filesize = get_image_size(sysboot_filename); - if (load_image_targphys("sysram.bin", 0xfff88000, filesize) < 0) { + if (load_image_targphys(sysboot_filename, 0xfff88000, 0x8000) < 0) { hw_error("Unable to load %s\n", bios_name); } g_free(sysboot_filename); From db25a15817f98c46c5f0eea4f414249d8fbb96b1 Mon Sep 17 00:00:00 2001 From: Stefan Weil Date: Wed, 1 Apr 2015 17:57:29 +0100 Subject: [PATCH 2/8] hw/arm/vexpress: Fix memory leak reported by Coverity As the conditional statement had to be split anyway, we can also add a better error report message. Signed-off-by: Stefan Weil Message-id: 1426877963-3556-1-git-send-email-sw@weilnetz.de Signed-off-by: Peter Maydell --- hw/arm/vexpress.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c index e9a7cede64..dd045271b8 100644 --- a/hw/arm/vexpress.c +++ b/hw/arm/vexpress.c @@ -563,6 +563,7 @@ static void vexpress_common_init(MachineState *machine) */ if (bios_name) { char *fn; + int image_size; if (drive_get(IF_PFLASH, 0, 0)) { error_report("The contents of the first flash device may be " @@ -571,8 +572,14 @@ static void vexpress_common_init(MachineState *machine) exit(1); } fn = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name); - if (!fn || load_image_targphys(fn, map[VE_NORFLASH0], - VEXPRESS_FLASH_SIZE) < 0) { + if (!fn) { + error_report("Could not find ROM image '%s'", bios_name); + exit(1); + } + image_size = load_image_targphys(fn, map[VE_NORFLASH0], + VEXPRESS_FLASH_SIZE); + g_free(fn); + if (image_size < 0) { error_report("Could not load ROM image '%s'", bios_name); exit(1); } From 4de9a883be653f02f8c1d5dcd1066f614d9606b6 Mon Sep 17 00:00:00 2001 From: Stefan Weil Date: Wed, 1 Apr 2015 17:57:29 +0100 Subject: [PATCH 3/8] hw/arm/virt: Fix memory leak reported by Coverity As the conditional statement had to be split anyway, we can also add a better error report message. Signed-off-by: Stefan Weil Message-id: 1426877982-3603-1-git-send-email-sw@weilnetz.de Signed-off-by: Peter Maydell --- hw/arm/virt.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/hw/arm/virt.c b/hw/arm/virt.c index b652b07ced..7d082e233c 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -553,6 +553,7 @@ static void create_flash(const VirtBoardInfo *vbi) if (bios_name) { char *fn; + int image_size; if (drive_get(IF_PFLASH, 0, 0)) { error_report("The contents of the first flash device may be " @@ -561,7 +562,13 @@ static void create_flash(const VirtBoardInfo *vbi) exit(1); } fn = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name); - if (!fn || load_image_targphys(fn, flashbase, flashsize) < 0) { + if (!fn) { + error_report("Could not find ROM image '%s'", bios_name); + exit(1); + } + image_size = load_image_targphys(fn, flashbase, flashsize); + g_free(fn); + if (image_size < 0) { error_report("Could not load ROM image '%s'", bios_name); exit(1); } From 7847f9ea9fce15a9ecfb62ab72c1e84ff516b0db Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Wed, 1 Apr 2015 17:57:29 +0100 Subject: [PATCH 4/8] target-arm: Store SPSR_EL1 state in banked_spsr[1] (SPSR_svc) The AArch64 SPSR_EL1 register is architecturally mandated to be mapped to the AArch32 SPSR_svc register. This means its state should live in QEMU's env->banked_spsr[1] field. Correct the various places in the code that incorrectly put it in banked_spsr[0]. Signed-off-by: Peter Maydell --- target-arm/helper-a64.c | 2 +- target-arm/helper.c | 2 +- target-arm/internals.h | 5 ++++- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c index 7e0d038563..861f6fa69c 100644 --- a/target-arm/helper-a64.c +++ b/target-arm/helper-a64.c @@ -523,7 +523,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs) aarch64_save_sp(env, arm_current_el(env)); env->elr_el[new_el] = env->pc; } else { - env->banked_spsr[0] = cpsr_read(env); + env->banked_spsr[aarch64_banked_spsr_index(new_el)] = cpsr_read(env); if (!env->thumb) { env->cp15.esr_el[new_el] |= 1 << 25; } diff --git a/target-arm/helper.c b/target-arm/helper.c index 10886c5281..d77c6de40c 100644 --- a/target-arm/helper.c +++ b/target-arm/helper.c @@ -2438,7 +2438,7 @@ static const ARMCPRegInfo v8_cp_reginfo[] = { { .name = "SPSR_EL1", .state = ARM_CP_STATE_AA64, .type = ARM_CP_ALIAS, .opc0 = 3, .opc1 = 0, .crn = 4, .crm = 0, .opc2 = 0, - .access = PL1_RW, .fieldoffset = offsetof(CPUARMState, banked_spsr[0]) }, + .access = PL1_RW, .fieldoffset = offsetof(CPUARMState, banked_spsr[1]) }, /* We rely on the access checks not allowing the guest to write to the * state field when SPSel indicates that it's being used as the stack * pointer. diff --git a/target-arm/internals.h b/target-arm/internals.h index bb171a73bd..2cc301762c 100644 --- a/target-arm/internals.h +++ b/target-arm/internals.h @@ -82,11 +82,14 @@ static inline void arm_log_exception(int idx) /* * For AArch64, map a given EL to an index in the banked_spsr array. + * Note that this mapping and the AArch32 mapping defined in bank_number() + * must agree such that the AArch64<->AArch32 SPSRs have the architecturally + * mandated mapping between each other. */ static inline unsigned int aarch64_banked_spsr_index(unsigned int el) { static const unsigned int map[4] = { - [1] = 0, /* EL1. */ + [1] = 1, /* EL1. */ [2] = 6, /* EL2. */ [3] = 7, /* EL3. */ }; From 1a1753f747544d20b999d466b1017721668bfb82 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20Benn=C3=A9e?= Date: Wed, 1 Apr 2015 17:57:30 +0100 Subject: [PATCH 5/8] target-arm: kvm: save/restore mp state MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This adds the saving and restore of the current Multi-Processing state of the machine. While the KVM_GET/SET_MP_STATE API exposes a number of potential states for x86 we only use two for ARM. Either the process is running or not. We then save this state into the cpu_powered TCG state to avoid changing the serialisation format. Signed-off-by: Alex Bennée Signed-off-by: Peter Maydell --- target-arm/kvm.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ target-arm/kvm32.c | 4 ++++ target-arm/kvm64.c | 4 ++++ target-arm/kvm_arm.h | 17 +++++++++++++++++ 4 files changed, 69 insertions(+) diff --git a/target-arm/kvm.c b/target-arm/kvm.c index 72c1fa1e64..fdd9ba3f1d 100644 --- a/target-arm/kvm.c +++ b/target-arm/kvm.c @@ -28,6 +28,8 @@ const KVMCapabilityInfo kvm_arch_required_capabilities[] = { KVM_CAP_LAST_INFO }; +static bool cap_has_mp_state; + int kvm_arm_vcpu_init(CPUState *cs) { ARMCPU *cpu = ARM_CPU(cs); @@ -157,6 +159,8 @@ int kvm_arch_init(MachineState *ms, KVMState *s) */ kvm_async_interrupts_allowed = true; + cap_has_mp_state = kvm_check_extension(s, KVM_CAP_MP_STATE); + type_register_static(&host_arm_cpu_type_info); return 0; @@ -458,6 +462,46 @@ void kvm_arm_reset_vcpu(ARMCPU *cpu) } } +/* + * Update KVM's MP_STATE based on what QEMU thinks it is + */ +int kvm_arm_sync_mpstate_to_kvm(ARMCPU *cpu) +{ + if (cap_has_mp_state) { + struct kvm_mp_state mp_state = { + .mp_state = + cpu->powered_off ? KVM_MP_STATE_STOPPED : KVM_MP_STATE_RUNNABLE + }; + int ret = kvm_vcpu_ioctl(CPU(cpu), KVM_SET_MP_STATE, &mp_state); + if (ret) { + fprintf(stderr, "%s: failed to set MP_STATE %d/%s\n", + __func__, ret, strerror(-ret)); + return -1; + } + } + + return 0; +} + +/* + * Sync the KVM MP_STATE into QEMU + */ +int kvm_arm_sync_mpstate_to_qemu(ARMCPU *cpu) +{ + if (cap_has_mp_state) { + struct kvm_mp_state mp_state; + int ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_MP_STATE, &mp_state); + if (ret) { + fprintf(stderr, "%s: failed to get MP_STATE %d/%s\n", + __func__, ret, strerror(-ret)); + abort(); + } + cpu->powered_off = (mp_state.mp_state == KVM_MP_STATE_STOPPED); + } + + return 0; +} + void kvm_arch_pre_run(CPUState *cs, struct kvm_run *run) { } diff --git a/target-arm/kvm32.c b/target-arm/kvm32.c index 94030d1acb..49b6babc05 100644 --- a/target-arm/kvm32.c +++ b/target-arm/kvm32.c @@ -356,6 +356,8 @@ int kvm_arch_put_registers(CPUState *cs, int level) return EINVAL; } + kvm_arm_sync_mpstate_to_kvm(cpu); + return ret; } @@ -427,5 +429,7 @@ int kvm_arch_get_registers(CPUState *cs) */ write_list_to_cpustate(cpu); + kvm_arm_sync_mpstate_to_qemu(cpu); + return 0; } diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c index 8cf3a627ec..fed03f2ae6 100644 --- a/target-arm/kvm64.c +++ b/target-arm/kvm64.c @@ -211,6 +211,8 @@ int kvm_arch_put_registers(CPUState *cs, int level) return EINVAL; } + kvm_arm_sync_mpstate_to_kvm(cpu); + /* TODO: * FP state */ @@ -310,6 +312,8 @@ int kvm_arch_get_registers(CPUState *cs) */ write_list_to_cpustate(cpu); + kvm_arm_sync_mpstate_to_qemu(cpu); + /* TODO: other registers */ return ret; } diff --git a/target-arm/kvm_arm.h b/target-arm/kvm_arm.h index 455dea3f3f..5abd5916d1 100644 --- a/target-arm/kvm_arm.h +++ b/target-arm/kvm_arm.h @@ -162,6 +162,23 @@ typedef struct ARMHostCPUClass { */ bool kvm_arm_get_host_cpu_features(ARMHostCPUClass *ahcc); + +/** + * kvm_arm_sync_mpstate_to_kvm + * @cpu: ARMCPU + * + * If supported set the KVM MP_STATE based on QEMU's model. + */ +int kvm_arm_sync_mpstate_to_kvm(ARMCPU *cpu); + +/** + * kvm_arm_sync_mpstate_to_qemu + * @cpu: ARMCPU + * + * If supported get the MP_STATE from KVM and store in QEMU's model. + */ +int kvm_arm_sync_mpstate_to_qemu(ARMCPU *cpu); + #endif #endif From 74fdb781c19ef4b781cb6fda48f1f9ebd11257fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20Benn=C3=A9e?= Date: Wed, 1 Apr 2015 17:57:30 +0100 Subject: [PATCH 6/8] hw/intc: arm_gic_kvm.c restore config first MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As there is logic to deal with the difference between edge and level triggered interrupts in the kernel we must ensure it knows the configuration of the IRQs before we restore the pending state. Signed-off-by: Alex Bennée Acked-by: Christoffer Dall Signed-off-by: Peter Maydell --- hw/intc/arm_gic_kvm.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c index 0d207508a0..e1952ad974 100644 --- a/hw/intc/arm_gic_kvm.c +++ b/hw/intc/arm_gic_kvm.c @@ -370,6 +370,11 @@ static void kvm_arm_gic_put(GICState *s) * the appropriate CPU interfaces in the kernel) */ kvm_dist_put(s, 0x800, 8, s->num_irq, translate_targets); + /* irq_state[n].trigger -> GICD_ICFGRn + * (restore configuration registers before pending IRQs so we treat + * level/edge correctly) */ + kvm_dist_put(s, 0xc00, 2, s->num_irq, translate_trigger); + /* irq_state[n].pending + irq_state[n].level -> GICD_ISPENDRn */ kvm_dist_put(s, 0x280, 1, s->num_irq, translate_clear); kvm_dist_put(s, 0x200, 1, s->num_irq, translate_pending); @@ -378,8 +383,6 @@ static void kvm_arm_gic_put(GICState *s) kvm_dist_put(s, 0x380, 1, s->num_irq, translate_clear); kvm_dist_put(s, 0x300, 1, s->num_irq, translate_active); - /* irq_state[n].trigger -> GICD_ICFRn */ - kvm_dist_put(s, 0xc00, 2, s->num_irq, translate_trigger); /* s->priorityX[irq] -> ICD_IPRIORITYRn */ kvm_dist_put(s, 0x400, 8, s->num_irq, translate_priority); From 0e4b586932768107448f342ae4a314beedfa8f11 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20Benn=C3=A9e?= Date: Wed, 1 Apr 2015 17:57:30 +0100 Subject: [PATCH 7/8] target-arm: kvm64 sync FP register state MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit For migration to work we need to sync all of the register state. This is especially noticeable when GCC starts using FP registers as spill registers even with integer programs. Signed-off-by: Alex Bennée Signed-off-by: Peter Maydell --- target-arm/kvm64.c | 85 ++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 82 insertions(+), 3 deletions(-) diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c index fed03f2ae6..d6c83b0fb2 100644 --- a/target-arm/kvm64.c +++ b/target-arm/kvm64.c @@ -15,6 +15,7 @@ #include +#include "config-host.h" #include "qemu-common.h" #include "qemu/timer.h" #include "sysemu/sysemu.h" @@ -126,9 +127,16 @@ bool kvm_arm_reg_syncs_via_cpreg_list(uint64_t regidx) #define AARCH64_CORE_REG(x) (KVM_REG_ARM64 | KVM_REG_SIZE_U64 | \ KVM_REG_ARM_CORE | KVM_REG_ARM_CORE_REG(x)) +#define AARCH64_SIMD_CORE_REG(x) (KVM_REG_ARM64 | KVM_REG_SIZE_U128 | \ + KVM_REG_ARM_CORE | KVM_REG_ARM_CORE_REG(x)) + +#define AARCH64_SIMD_CTRL_REG(x) (KVM_REG_ARM64 | KVM_REG_SIZE_U32 | \ + KVM_REG_ARM_CORE | KVM_REG_ARM_CORE_REG(x)) + int kvm_arch_put_registers(CPUState *cs, int level) { struct kvm_one_reg reg; + uint32_t fpr; uint64_t val; int i; int ret; @@ -207,15 +215,48 @@ int kvm_arch_put_registers(CPUState *cs, int level) } } + /* Advanced SIMD and FP registers + * We map Qn = regs[2n+1]:regs[2n] + */ + for (i = 0; i < 32; i++) { + int rd = i << 1; + uint64_t fp_val[2]; +#ifdef HOST_WORDS_BIGENDIAN + fp_val[0] = env->vfp.regs[rd + 1]; + fp_val[1] = env->vfp.regs[rd]; +#else + fp_val[1] = env->vfp.regs[rd + 1]; + fp_val[0] = env->vfp.regs[rd]; +#endif + reg.id = AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]); + reg.addr = (uintptr_t)(&fp_val); + ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, ®); + if (ret) { + return ret; + } + } + + reg.addr = (uintptr_t)(&fpr); + fpr = vfp_get_fpsr(env); + reg.id = AARCH64_SIMD_CTRL_REG(fp_regs.fpsr); + ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, ®); + if (ret) { + return ret; + } + + fpr = vfp_get_fpcr(env); + reg.id = AARCH64_SIMD_CTRL_REG(fp_regs.fpcr); + ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, ®); + if (ret) { + return ret; + } + if (!write_list_to_kvmstate(cpu)) { return EINVAL; } kvm_arm_sync_mpstate_to_kvm(cpu); - /* TODO: - * FP state - */ return ret; } @@ -223,6 +264,7 @@ int kvm_arch_get_registers(CPUState *cs) { struct kvm_one_reg reg; uint64_t val; + uint32_t fpr; int i; int ret; @@ -304,6 +346,43 @@ int kvm_arch_get_registers(CPUState *cs) } } + /* Advanced SIMD and FP registers + * We map Qn = regs[2n+1]:regs[2n] + */ + for (i = 0; i < 32; i++) { + uint64_t fp_val[2]; + reg.id = AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]); + reg.addr = (uintptr_t)(&fp_val); + ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, ®); + if (ret) { + return ret; + } else { + int rd = i << 1; +#ifdef HOST_WORDS_BIGENDIAN + env->vfp.regs[rd + 1] = fp_val[0]; + env->vfp.regs[rd] = fp_val[1]; +#else + env->vfp.regs[rd + 1] = fp_val[1]; + env->vfp.regs[rd] = fp_val[0]; +#endif + } + } + + reg.addr = (uintptr_t)(&fpr); + reg.id = AARCH64_SIMD_CTRL_REG(fp_regs.fpsr); + ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, ®); + if (ret) { + return ret; + } + vfp_set_fpsr(env, fpr); + + reg.id = AARCH64_SIMD_CTRL_REG(fp_regs.fpcr); + ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, ®); + if (ret) { + return ret; + } + vfp_set_fpcr(env, fpr); + if (!write_kvmstate_to_list(cpu)) { return EINVAL; } From 25b9fb107bc1f6735fdb3fce537792f5db95f78d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20Benn=C3=A9e?= Date: Wed, 1 Apr 2015 17:57:30 +0100 Subject: [PATCH 8/8] target-arm: kvm64 fix save/restore of SPSR regs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The current code was negatively indexing the cpu state array and not synchronizing banked spsr register state with the current mode's spsr state, causing occasional failures with migration. Some munging is done to take care of the aarch64 mapping and also to ensure the most current value of the spsr is updated to the banked registers (relevant for KVM<->TCG migration). Signed-off-by: Alex Bennée Signed-off-by: Peter Maydell --- target-arm/kvm64.c | 29 +++++++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c index d6c83b0fb2..93c1ca8b21 100644 --- a/target-arm/kvm64.c +++ b/target-arm/kvm64.c @@ -140,6 +140,7 @@ int kvm_arch_put_registers(CPUState *cs, int level) uint64_t val; int i; int ret; + unsigned int el; ARMCPU *cpu = ARM_CPU(cs); CPUARMState *env = &cpu->env; @@ -206,9 +207,22 @@ int kvm_arch_put_registers(CPUState *cs, int level) return ret; } + /* Saved Program State Registers + * + * Before we restore from the banked_spsr[] array we need to + * ensure that any modifications to env->spsr are correctly + * reflected in the banks. + */ + el = arm_current_el(env); + if (el > 0 && !is_a64(env)) { + i = bank_number(env->uncached_cpsr & CPSR_M); + env->banked_spsr[i] = env->spsr; + } + + /* KVM 0-4 map to QEMU banks 1-5 */ for (i = 0; i < KVM_NR_SPSR; i++) { reg.id = AARCH64_CORE_REG(spsr[i]); - reg.addr = (uintptr_t) &env->banked_spsr[i - 1]; + reg.addr = (uintptr_t) &env->banked_spsr[i + 1]; ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, ®); if (ret) { return ret; @@ -265,6 +279,7 @@ int kvm_arch_get_registers(CPUState *cs) struct kvm_one_reg reg; uint64_t val; uint32_t fpr; + unsigned int el; int i; int ret; @@ -337,15 +352,25 @@ int kvm_arch_get_registers(CPUState *cs) return ret; } + /* Fetch the SPSR registers + * + * KVM SPSRs 0-4 map to QEMU banks 1-5 + */ for (i = 0; i < KVM_NR_SPSR; i++) { reg.id = AARCH64_CORE_REG(spsr[i]); - reg.addr = (uintptr_t) &env->banked_spsr[i - 1]; + reg.addr = (uintptr_t) &env->banked_spsr[i + 1]; ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, ®); if (ret) { return ret; } } + el = arm_current_el(env); + if (el > 0 && !is_a64(env)) { + i = bank_number(env->uncached_cpsr & CPSR_M); + env->spsr = env->banked_spsr[i]; + } + /* Advanced SIMD and FP registers * We map Qn = regs[2n+1]:regs[2n] */