Hi Peng,
[+Will, as we hacked the new ECE (Ectoplasmic Control Enclosure) together]
On Sat, 31 Oct 2020 07:03:02 +0000,
Peng Liang <liangpeng10@xxxxxxxxxx> wrote:
>
> Hi Marc,
> Sorry for disturbing you.
>
> When I try to migrate a VM from Linux v5.9 to Linux v5.10-rc1, QEMU
> reports errors like this:
> qemu-system-aarch64: write 0x603000000013c020(0x0100010011111111) to
> kvm failed
> qemu-system-aarch64: error while loading state for instance 0x0 of
> device 'cpu'
>
> (The first error is added by myself:
> diff --git a/target/arm/kvm.c b/target/arm/kvm.c
> index 8bb7318378..b361f62f7f 100644
> --- a/target/arm/kvm.c
> +++ b/target/arm/kvm.c
> @@ -560,6 +560,7 @@ bool write_list_to_kvmstate(ARMCPU *cpu, int level)
> * "you tried to set a register which is constant with
> * a different value from what it actually contains".
> */
> + error_report("write 0x%016lx(0x%016lx) to kvm failed",
> cpu->cpreg_indexes[i], cpu->cpreg_values[i]);
> ok = false;
> }
> }
> )
>
> If I try to migrate from Linux v5.10-rc1 to v5.9, then the errors are
> changed to:
> qemu-system-aarch64: write 0x603000000013c020(0x0000010011111111) to
> kvm failed
> error while loading state for instance 0x0 of device 'cpu'
>
> However, the migration from v5.9 to v5.9 or from v5.10-rc1 to v5.10-rc1
> are successful.
>
> The source end and destination end of migration have the same hardware
> and the same softwares except the Linux version. And of course, the
> vCPUs of VMs are host-passthrough.
>
> I found that the different register and the different field between
> source and destination is ID_AA64PFR0_EL1.CSV2. I searched in git log
> and found that the commit e1026237f9067 ("KVM: arm64: Set CSV2 for
> guests on hardware unaffected by Spectre-v2") may be the cause of the
> failure?
Thanks for the thorough analysis. Yes, this could well be the issue if
CSV2 isn't explicitly set at the source, and is now set on the target.
For confirmation, what is the value of ID_AA64PFR0_EL1.CSV2 on the
host? What does /sys/devices/system/cpu/vulnerabilities/spectre_v2
contain?
> So do we need to make it possible to migrate VMs between Linux v5.9 and
> Linux v5.10-rc1 with QEMU?
We should fix the migration from 5.9 to 5.10. I don't plan to support
migrating in the other direction, which is consistent with new sysregs
and features appearing in the sysreg space over time (although I would
expect 5.9 -> 5.10 -> 5.9 to work once we fix this issue).
Could you please give the patch below a go? I have boot-tested it, but
I'm not really equipped to test live migration.
Thanks,
M.
>From 2b9202538365bacc0abd01142800234ea1bc5bde Mon Sep 17 00:00:00 2001
From: Marc Zyngier <maz@xxxxxxxxxx>
Date: Sat, 31 Oct 2020 12:28:50 +0000
Subject: [PATCH] KVM: arm64: Allow setting of ID_AA64PFR0_EL1.CSV2 from
userspace
We now expose ID_AA64PFR0_EL1.CSV2=1 to guests running on hosts
that are immune to Spectre-v2, but that don't have this field set,
most likely because they predate the specification.
However, this prevents the migration of guests that have started on
a host the doesn't fake this CSV2 setting to one that does, as KVM
rejects the write to ID_AA64PFR0_EL2 on the grounds that it isn't
what is already there.
In order to fix this, allow userspace to set this field as long as
this doesn't result in a promising more than what is already there
(setting CSV2 to 0 is acceptable, but setting it to 1 when it is
already set to 0 isn't).
Fixes: e1026237f9067 ("KVM: arm64: Set CSV2 for guests on hardware unaffected by Spectre-v2")
Reported-by: Peng Liang <liangpeng10@xxxxxxxxxx>
Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx>
---
arch/arm64/include/asm/kvm_host.h | 2 ++
arch/arm64/kvm/arm.c | 21 +++++++++++++++++
arch/arm64/kvm/sys_regs.c | 38 +++++++++++++++++++++++++++----
3 files changed, 57 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 0aecbab6a7fb..160d783eaf89 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -118,6 +118,8 @@ struct kvm_arch {
*/
unsigned long *pmu_filter;
unsigned int pmuver;
+
+ u8 pfr0_csv2;
};
struct kvm_vcpu_fault_info {
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index f56122eedffc..1a944c9b48b4 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -102,6 +102,25 @@ static int kvm_arm_default_max_vcpus(void)
return vgic_present ? kvm_vgic_get_max_vcpus() : KVM_MAX_VCPUS;
}
+static void set_default_csv2(struct kvm *kvm)
+{
+ u64 val = read_sanitised_ftr_reg(SYS_ID_AA64PFR0_EL1);
+
+ /*
+ * The default is to expose CSV2 == 1 if the HW isn't affected
+ * but doesn't have CSV2 baked in the PFR0 register. Although
+ * this is a per-CPU feature, we make it global because
+ * asymmetric systems are just a nuisance.
+ *
+ * Userspace can override this as long as it doesn't promise
+ * the impossible.
+ */
+ kvm->arch.pfr0_csv2 = FIELD_GET(0xfUL << ID_AA64PFR0_CSV2_SHIFT, val);
+ if (!kvm->arch.pfr0_csv2 &&
+ arm64_get_spectre_v2_state() == SPECTRE_UNAFFECTED)
+ kvm->arch.pfr0_csv2 = 1;
+}
+
/**
* kvm_arch_init_vm - initializes a VM data structure
* @kvm: pointer to the KVM struct
@@ -127,6 +146,8 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
/* The maximum number of VCPUs is limited by the host's GIC model */
kvm->arch.max_vcpus = kvm_arm_default_max_vcpus();
+ set_default_csv2(kvm);
+
return ret;
out_free_stage2_pgd:
kvm_free_stage2_pgd(&kvm->arch.mmu);
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index d9117bc56237..4f5716abbb19 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1128,9 +1128,8 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu,
if (!vcpu_has_sve(vcpu))
val &= ~(0xfUL << ID_AA64PFR0_SVE_SHIFT);
val &= ~(0xfUL << ID_AA64PFR0_AMU_SHIFT);
- if (!(val & (0xfUL << ID_AA64PFR0_CSV2_SHIFT)) &&
- arm64_get_spectre_v2_state() == SPECTRE_UNAFFECTED)
- val |= (1UL << ID_AA64PFR0_CSV2_SHIFT);
+ val &= ~(0xfUL << ID_AA64PFR0_CSV2_SHIFT);
+ val |= ((u64)vcpu->kvm->arch.pfr0_csv2 << ID_AA64PFR0_CSV2_SHIFT);
} else if (id == SYS_ID_AA64PFR1_EL1) {
val &= ~(0xfUL << ID_AA64PFR1_MTE_SHIFT);
} else if (id == SYS_ID_AA64ISAR1_EL1 && !vcpu_has_ptrauth(vcpu)) {
@@ -1260,6 +1259,36 @@ static int set_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
return 0;
}
+static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
+ const struct sys_reg_desc *rd,
+ const struct kvm_one_reg *reg, void __user *uaddr)
+{
+ const u64 id = sys_reg_to_index(rd);
+ int err;
+ u64 val;
+ u8 csv2;
+
+ err = reg_from_user(&val, uaddr, id);
+ if (err)
+ return err;
+
+ /*
+ * Allow AA64PFR0_EL1.CSV2 to be set from userspace as long as
+ * it doesn't promise more than what is actually provided (the
+ * guest could otherwise be covered in ectoplasmic residue).
+ */
+ csv2 = FIELD_GET(0xfUL << ID_AA64PFR0_CSV2_SHIFT, val);
+ if (csv2 > vcpu->kvm->arch.pfr0_csv2)
+ return -EINVAL;
+ vcpu->kvm->arch.pfr0_csv2 = csv2;
+
+ /* This is what we mean by invariant: you can't change it. */
+ if (val != read_id_reg(vcpu, rd, false))
+ return -EINVAL;
+
+ return 0;
+}
+
/*
* cpufeature ID register user accessors
*
@@ -1514,7 +1543,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
/* AArch64 ID registers */
/* CRm=4 */
- ID_SANITISED(ID_AA64PFR0_EL1),
+ { SYS_DESC(SYS_ID_AA64PFR0_EL1), .access = access_id_reg,
+ .get_user = get_id_reg, .set_user = set_id_aa64pfr0_el1, },
ID_SANITISED(ID_AA64PFR1_EL1),
ID_UNALLOCATED(4,2),
ID_UNALLOCATED(4,3),
--
2.28.0
--
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
