For Arm, the format we use on the wire for migration of sysregs (cpregs on AArch32) is a set of arrays, where cpreg_indexes[] holds the register IDs and cpreg_values[] holds the corresponding values. The ID format we use is KVM's ID format, which is 64 bits. This is not quite the same as the 32-bit ID format QEMU uses internally for things like the coprocessor register hashtable keys, and so we have conversion functions kvm_to_cpreg_id() and cpreg_to_kvm_id() to swap between them.
This conversion has never handled AArch32 Secure coprocessor registers correctly: the KVM ID format has no way to indicate a Secure register, because KVM VMs on AArch32 were always NonSecure. The current code doesn't touch the CP_REG_AA32_NS_MASK bit when converting from the 32-bit ID format to a KVM ID, and it forces the NS bit to 1 when converting from a KVM ID to a cpreg. The effect of this on migration is that the indexes in cpreg_indexes[] have NS = 0 for the Secure banked registers and NS = 1 for the NS banked registers, but when we read and write the values into the CPUState by finding the register using kvm_to_cpreg_id(cpreg_indexes[i]) we always find the NS register, and so the S register values aren't actually migrated. This went unnoticed most of the time because a typical Linux guest is running only in NS and doesn't really care about the S register state, but it breaks migration of a guest which is running in S, such as Linux on the imx boards. This would be more awkward to fix if KVM still supported AArch32 hosts, but luckily that was dropped some years ago, and AArch32 guests on an AArch64 host expose the sysregs as CP_REG_ARM64, not CP_REG_ARM32. So the only place where we use a CP_REG_ARM32 KVM register ID is for TCG migrations. We can therefore (continue to) steal a bit in the KVM format for the NS bit, with the semantics we've given it on the wire in the past. We just need to let kvm_to_cpreg_id() pass it through rather than forcing it to 1. To retain compatibility with migration from older QEMU versions without this fix, we need to add a subsection to the migration data that tells the destination that it can trust the values for Secure banked registers. For incoming migrations, if we don't see this then we ignore all the values for Secure banked registers, giving the same behaviour as before. This means we continue to work for migrations where the Secure world exists but the guest is basically not using it, such as a direct kernel boot on boards which don't set arm_boot_info::secure_boot to true; this includes the cubieboard, bananapi and orangepi. Attempting migration from a new QEMU to an older QEMU version will now fail cleanly (because the destination doesn't recognize the new subsection) rather than silently corrupting the Secure banked registers. Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/467 Signed-off-by: Peter Maydell <[email protected]> --- target/arm/cpregs.h | 21 ++++++++---- target/arm/cpu.h | 6 ++++ target/arm/machine.c | 80 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 100 insertions(+), 7 deletions(-) diff --git a/target/arm/cpregs.h b/target/arm/cpregs.h index 9f2532fe667..ce651a4faa6 100644 --- a/target/arm/cpregs.h +++ b/target/arm/cpregs.h @@ -215,7 +215,20 @@ enum { /* * Convert a full 64 bit KVM register ID to the truncated 32 bit - * version used as a key for the coprocessor register hashtable + * version used as a key for the coprocessor register hashtable. + * + * Note that we deviate slightly from the KVM register ID format as + * used by the kernel for AArch32 cpregs, by using bit 29 as "1 + * for NonSecure, 0 for Secure". (When KVM still supported AArch32 + * hosts it didn't set this bit; all sysregs for KVM guests are + * NonSecure anyway.) This shouldn't cause any issues as KVM no longer + * supports AArch32 hosts (and other accelerators never did), so the + * only thing that generates KVM regids for AArch32 cpregs is QEMU + * TCG. + * + * The NS bit being in the KVM ID is implicit in the fact that we + * don't mask out CP_REG_AA32_NS_MASK in the conversions to and from + * the QEMU hashtable key ID format. */ static inline uint32_t kvm_to_cpreg_id(uint64_t kvmid) { @@ -226,12 +239,6 @@ static inline uint32_t kvm_to_cpreg_id(uint64_t kvmid) if ((kvmid & CP_REG_SIZE_MASK) == CP_REG_SIZE_U64) { cpregid |= CP_REG_AA32_64BIT_MASK; } - - /* - * KVM is always non-secure so add the NS flag on AArch32 register - * entries. - */ - cpregid |= CP_REG_AA32_NS_MASK; } return cpregid; } diff --git a/target/arm/cpu.h b/target/arm/cpu.h index e3f931dba26..231fdf710b7 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -1128,6 +1128,12 @@ struct ArchCPU { bool prop_pauth_qarma5; bool prop_lpa2; + /* + * Used only during migration, to handle back-compat with older QEMU + * that mishandled migration of AArch32 banked cpregs. + */ + bool secure_banked_regs_ok; + /* GM blocksize, in log_2(words), ie low 4 bits of GMID_EL0 */ uint8_t gm_blocksize; diff --git a/target/arm/machine.c b/target/arm/machine.c index 89127e5d83c..1fc3acc31ca 100644 --- a/target/arm/machine.c +++ b/target/arm/machine.c @@ -979,6 +979,39 @@ static const VMStateDescription vmstate_fpmr = { }, }; +static bool secure_banked_regs_ok_needed(void *opaque) +{ + ARMCPU *cpu = opaque; + + /* + * We must send this subsection if this is an AArch32 CPU with + * banked coprocessor registers. Older QEMU mishandled migration + * of these by listing both Secure and NonSecure banked registers + * in the cpreg_vmstate_indexes but reading and writing the + * NonSecure register for both indexes. Providing this subsection + * tells the destination that we do not have this bug and it + * should not ignore the Secure banked register values. + * + * We don't need the subsection for CPUs without banked registers + * (notably AArch64 ones and M-profile ones), and don't send + * it to avoid breaking migration compat for them. + */ + return !arm_feature(&cpu->env, ARM_FEATURE_AARCH64) && + !arm_feature(&cpu->env, ARM_FEATURE_M) && + arm_feature(&cpu->env, ARM_FEATURE_EL3); +} + +static const VMStateDescription vmstate_secure_banked_regs_ok = { + .name = "cpu/secure-banked-regs-ok", + .version_id = 1, + .minimum_version_id = 1, + .needed = secure_banked_regs_ok_needed, + .fields = (const VMStateField[]) { + VMSTATE_BOOL(secure_banked_regs_ok, ARMCPU), + VMSTATE_END_OF_LIST() + }, +}; + static int cpu_pre_save(void *opaque) { ARMCPU *cpu = opaque; @@ -1014,6 +1047,9 @@ static int cpu_pre_save(void *opaque) cpu->cpreg_vmstate_values = cpu->cpreg_values; cpu->cpreg_vmstate_array_len = cpu->cpreg_array_len; + /* We don't have the bug where we send wrong data for Secure regs */ + cpu->secure_banked_regs_ok = true; + return 0; } @@ -1062,6 +1098,9 @@ static int cpu_pre_load(void *opaque) g_assert(!cpu->cpreg_vmstate_indexes); g_assert(!cpu->cpreg_vmstate_values); + /* So cpu_post_load() can see if we saw secure-banked-regs-ok */ + cpu->secure_banked_regs_ok = false; + return 0; } @@ -1120,6 +1159,7 @@ static int cpu_post_load(void *opaque, int version_id) ARMCPU *cpu = opaque; CPUARMState *env = &cpu->env; bool fail = false; + bool ignore_s_regs; int i, v; trace_cpu_post_load(cpu->cpreg_vmstate_array_len, @@ -1142,6 +1182,21 @@ static int cpu_post_load(void *opaque, int version_id) CPU_INTERRUPT_VIRQ | CPU_INTERRUPT_VFIQ); } + /* + * Handle migration compatibility from an old QEMU which didn't get + * AArch32 Secure banked cpregs right. That QEMU will not have sent + * us the secure-banked-regs-ok subsection, and although its + * vmstate_indexes will include the S banked regs, the values in + * vmstate_values will be duplicates of the values of the NS banked + * regs. Ignore the S banked registers, which is the same effective + * behaviour of an old->old migration. (That is, the S regs will + * be at their reset values, which is usually good enough for the + * case of "guest is actually executing in NS and doesn't care + * about the S state".) + */ + ignore_s_regs = secure_banked_regs_ok_needed(cpu) && + !cpu->secure_banked_regs_ok; + /* Update the values list from the incoming migration data. * Anything in the incoming data which we don't know about is * a migration failure; anything we know about but the incoming @@ -1165,6 +1220,30 @@ static int cpu_post_load(void *opaque, int version_id) continue; } /* matching register, copy the value over */ + + if (ignore_s_regs) { + /* + * If this is an AArch32 Secure cpreg, read the current (reset) + * value instead of using the migration state value. That way + * write_list_to_cpustate() will effectively be a NOP. + */ + uint64_t kvmidx = cpu->cpreg_vmstate_indexes[v]; + + if ((kvmidx & CP_REG_ARCH_MASK) == CP_REG_ARM && + (kvmidx & CP_REG_AA32_NS_MASK) == 0) { + uint32_t regidx = kvm_to_cpreg_id(kvmidx); + const ARMCPRegInfo *ri = get_arm_cp_reginfo(cpu->cp_regs, + regidx); + /* + * Missing ri or NO_RAW ri will be ignored or errored in + * write_list_to_cpustate() later, so safe to skip. + */ + if (ri && !(ri->type & ARM_CP_NO_RAW)) { + cpu->cpreg_vmstate_values[v] = read_raw_cp_reg(env, ri); + } + } + } + cpu->cpreg_values[i] = cpu->cpreg_vmstate_values[v]; i++; v++; @@ -1343,6 +1422,7 @@ const VMStateDescription vmstate_arm_cpu = { &vmstate_pstate64, &vmstate_event, &vmstate_fpmr, + &vmstate_secure_banked_regs_ok, NULL } }; -- 2.43.0
