VM live migration failed from Linux v5.9 to Linux v5.10-rc1

VM live migration failed from Linux v5.9 to Linux v5.10-rc1

[Peng Liang]

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?

So do we need to make it possible to migrate VMs between Linux v5.9 and
Linux v5.10-rc1 with QEMU?

And here is the information of my environment:
CPU: Kunpeng-920
QEMU: v5.1.0, built with `../configure --target-list=aarch64-softmmu
--disable-werror`
source end:
  Linux: v5.9, configured with `make defconfig`
destination end:
  Linux: v5.10-rc1, configured with `make defconfig`

Thanks,
Peng
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: VM live migration failed from Linux v5.9 to Linux v5.10-rc1

[Marc Zyngier]

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@huawei.com> 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@kernel.org>
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@huawei.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 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@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: VM live migration failed from Linux v5.9 to Linux v5.10-rc1

[Peng Liang]

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@huawei.com> 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@kernel.org>
> 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@huawei.com>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  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@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: VM live migration failed from Linux v5.9 to Linux v5.10-rc1

[Peng Liang]

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@huawei.com> 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@kernel.org>
>> 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@huawei.com>
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>> ---
>>  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@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: VM live migration failed from Linux v5.9 to Linux v5.10-rc1

[Marc Zyngier]

On 2020-11-02 03:12, 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@huawei.com> 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

Right. I guess this system supports WA1 and reports "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.

What if kernel 5.11 adds unconditional support for a feature and that
results in a new sysreg gets exposed? How do you plan to restore this
guest on 5.10?

In may work in limited cases, but it doesn't in general. To make that
work, you'd need to implement some explicit buy-in from userspace on
each and every feature that gets added to the hypervisor. This is
completely impractical, and I have no desire to support it.


         M.
-- 
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: VM live migration failed from Linux v5.9 to Linux v5.10-rc1

[Will Deacon]

On Sat, Oct 31, 2020 at 01:25:17PM +0000, 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@huawei.com> 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@kernel.org>
> 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@huawei.com>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  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;

We might need to be careful here, as this means the guest can now see a
value of '2' and expect to use the SCXTNUM registers. I haven't checked
what we do with those, but we never advertise them in the current code
afaict.

Will
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: VM live migration failed from Linux v5.9 to Linux v5.10-rc1

[Marc Zyngier]

On 2020-11-02 10:29, Will Deacon wrote:
> On Sat, Oct 31, 2020 at 01:25:17PM +0000, Marc Zyngier wrote:

[...]

>> +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;
> 
> We might need to be careful here, as this means the guest can now see a
> value of '2' and expect to use the SCXTNUM registers. I haven't checked
> what we do with those, but we never advertise them in the current code
> afaict.

I think a guest can already see CSV2=2 if supported on all the physical
CPUs (the current logic only overrides CSV2 if it is zero). Pretty easy
to fix (just cap it to 1), but we should definitely add the switching
capability to support CSV2=2.

I'll have a look at an additional patch address this.

         M.
-- 
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm