Hi Marc,
Sorry for my late reply.
On 10/31/2020 9:25 PM, Marc Zyngier wrote:
> 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:
>>
>> [...]
>>
>> 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?
On host:
ID_AA64PFR0_EL1.CSV2: 0
/sys/devices/system/cpu/vulnerabilities/spectre_v2: Not affected
>
>> 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).
BTW, there always be new sysregs/features introduced to kernel over
time. If that happened, do we need to make migration successful from a
older version without the new sysregs/features? I think there is no
reason to not allow migration from old version to new version if the
source end and the destination end have the same hardware just because
old version doesn't expose some sysregs/features to guest but new
version does.
>
> Could you please give the patch below a go? I have boot-tested it, but
> I'm not really equipped to test live migration.
Great! I'll test live migration as soon as possible.
Thanks,
Peng
>
> 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),
>
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
