On 11/2/2020 11:12 AM, Peng Liang wrote:
> 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.
I applied the patch on v5.10-rc2, then the migration v5.9 -> v5.10 ->
v5.9 is successful.
Thanks,
Peng
>
> 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
