[REPOST PATCH] arm/arm64: KVM: Add PSCI version selection API

[REPOST PATCH] arm/arm64: KVM: Add PSCI version selection API

[Marc Zyngier]

Although we've implemented PSCI 0.1, 0.2 and 1.0, we expose either 0.1
or 1.0 to a guest, defaulting to the latest version of the PSCI
implementation that is compatible with the requested version. This is
no different from doing a firmware upgrade on KVM.

But in order to give a chance to hypothetical badly implemented guests
that would have a fit by discovering something other than PSCI 0.2,
let's provide a new API that allows userspace to pick one particular
version of the API.

This is implemented as a new class of "firmware" registers, where
we expose the PSCI version. This allows the PSCI version to be
save/restored as part of a guest migration, and also set to
any supported version if the guest requires it.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
This is a repost of this proposal for a firmware selection API, as
there was some discussion during the merging window. Now that the core
variant-2 stuff is merged, we can have a go at more
bikesheding. Please open fire!

 Documentation/virtual/kvm/api.txt      |  9 ++++-
 Documentation/virtual/kvm/arm/psci.txt | 30 +++++++++++++++++
 arch/arm/include/asm/kvm_host.h        |  3 ++
 arch/arm/include/uapi/asm/kvm.h        |  6 ++++
 arch/arm/kvm/guest.c                   | 13 ++++++++
 arch/arm64/include/asm/kvm_host.h      |  3 ++
 arch/arm64/include/uapi/asm/kvm.h      |  6 ++++
 arch/arm64/kvm/guest.c                 | 14 +++++++-
 include/kvm/arm_psci.h                 | 16 +++++++--
 virt/kvm/arm/psci.c                    | 60 ++++++++++++++++++++++++++++++++++
 10 files changed, 156 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/virtual/kvm/arm/psci.txt

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index 57d3ee9e4bde..d2b41e1608b4 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -1943,6 +1943,9 @@ ARM 32-bit VFP control registers have the following id bit patterns:
 ARM 64-bit FP registers have the following id bit patterns:
   0x4030 0000 0012 0 <regno:12>
 
+ARM firmware pseudo-registers have the following bit pattern:
+  0x4030 0000 0014 <regno:16>
+
 
 arm64 registers are mapped using the lower 32 bits. The upper 16 of
 that is the register group type, or coprocessor number:
@@ -1959,6 +1962,9 @@ arm64 CCSIDR registers are demultiplexed by CSSELR value:
 arm64 system registers have the following id bit patterns:
   0x6030 0000 0013 <op0:2> <op1:3> <crn:4> <crm:4> <op2:3>
 
+arm64 firmware pseudo-registers have the following bit pattern:
+  0x6030 0000 0014 <regno:16>
+
 
 MIPS registers are mapped using the lower 32 bits.  The upper 16 of that is
 the register group type:
@@ -2493,7 +2499,8 @@ Possible features:
 	  and execute guest code when KVM_RUN is called.
 	- KVM_ARM_VCPU_EL1_32BIT: Starts the CPU in a 32bit mode.
 	  Depends on KVM_CAP_ARM_EL1_32BIT (arm64 only).
-	- KVM_ARM_VCPU_PSCI_0_2: Emulate PSCI v0.2 for the CPU.
+	- KVM_ARM_VCPU_PSCI_0_2: Emulate PSCI v0.2 (or a future revision
+          backward compatible with v0.2) for the CPU.
 	  Depends on KVM_CAP_ARM_PSCI_0_2.
 	- KVM_ARM_VCPU_PMU_V3: Emulate PMUv3 for the CPU.
 	  Depends on KVM_CAP_ARM_PMU_V3.
diff --git a/Documentation/virtual/kvm/arm/psci.txt b/Documentation/virtual/kvm/arm/psci.txt
new file mode 100644
index 000000000000..aafdab887b04
--- /dev/null
+++ b/Documentation/virtual/kvm/arm/psci.txt
@@ -0,0 +1,30 @@
+KVM implements the PSCI (Power State Coordination Interface)
+specification in order to provide services such as CPU on/off, reset
+and power-off to the guest.
+
+The PSCI specification is regularly updated to provide new features,
+and KVM implements these updates if they make sense from a virtualization
+point of view.
+
+This means that a guest booted on two different versions of KVM can
+observe two different "firmware" revisions. This could cause issues if
+a given guest is tied to a particular PSCI revision (unlikely), or if
+a migration causes a different PSCI version to be exposed out of the
+blue to an unsuspecting guest.
+
+In order to remedy this situation, KVM exposes a set of "firmware
+pseudo-registers" that can be manipulated using the GET/SET_ONE_REG
+interface. These registers can be saved/restored by userspace, and set
+to a convenient value if required.
+
+The following register is defined:
+
+* KVM_REG_ARM_PSCI_VERSION:
+
+  - Only valid if the vcpu has the KVM_ARM_VCPU_PSCI_0_2 feature set
+    (and thus has already been initialized)
+  - Returns the current PSCI version on GET_ONE_REG (defaulting to the
+    highest PSCI version implemented by KVM and compatible with v0.2)
+  - Allows any PSCI version implemented by KVM and compatible with
+    v0.2 to be set with SET_ONE_REG
+  - Affects the whole VM (even if the register view is per-vcpu)
diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index ef54013b5b9f..6c05e3b13081 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -75,6 +75,9 @@ struct kvm_arch {
 	/* Interrupt controller */
 	struct vgic_dist	vgic;
 	int max_vcpus;
+
+	/* Mandated version of PSCI */
+	u32 psci_version;
 };
 
 #define KVM_NR_MEM_OBJS     40
diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h
index 6edd177bb1c7..47dfc99f5cd0 100644
--- a/arch/arm/include/uapi/asm/kvm.h
+++ b/arch/arm/include/uapi/asm/kvm.h
@@ -186,6 +186,12 @@ struct kvm_arch_memory_slot {
 #define KVM_REG_ARM_VFP_FPINST		0x1009
 #define KVM_REG_ARM_VFP_FPINST2		0x100A
 
+/* KVM-as-firmware specific pseudo-registers */
+#define KVM_REG_ARM_FW			(0x0014 << KVM_REG_ARM_COPROC_SHIFT)
+#define KVM_REG_ARM_FW_REG(r)		(KVM_REG_ARM | KVM_REG_SIZE_U64 | \
+					 KVM_REG_ARM_FW | ((r) & 0xffff))
+#define KVM_REG_ARM_PSCI_VERSION	KVM_REG_ARM_FW_REG(0)
+
 /* Device Control API: ARM VGIC */
 #define KVM_DEV_ARM_VGIC_GRP_ADDR	0
 #define KVM_DEV_ARM_VGIC_GRP_DIST_REGS	1
diff --git a/arch/arm/kvm/guest.c b/arch/arm/kvm/guest.c
index 1e0784ebbfd6..a18f33edc471 100644
--- a/arch/arm/kvm/guest.c
+++ b/arch/arm/kvm/guest.c
@@ -22,6 +22,7 @@
 #include <linux/module.h>
 #include <linux/vmalloc.h>
 #include <linux/fs.h>
+#include <kvm/arm_psci.h>
 #include <asm/cputype.h>
 #include <linux/uaccess.h>
 #include <asm/kvm.h>
@@ -176,6 +177,7 @@ static unsigned long num_core_regs(void)
 unsigned long kvm_arm_num_regs(struct kvm_vcpu *vcpu)
 {
 	return num_core_regs() + kvm_arm_num_coproc_regs(vcpu)
+		+ kvm_arm_get_fw_num_regs(vcpu)
 		+ NUM_TIMER_REGS;
 }
 
@@ -196,6 +198,11 @@ int kvm_arm_copy_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
 		uindices++;
 	}
 
+	ret = kvm_arm_copy_fw_reg_indices(vcpu, uindices);
+	if (ret)
+		return ret;
+	uindices += kvm_arm_get_fw_num_regs(vcpu);
+
 	ret = copy_timer_indices(vcpu, uindices);
 	if (ret)
 		return ret;
@@ -214,6 +221,9 @@ int kvm_arm_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 	if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_CORE)
 		return get_core_reg(vcpu, reg);
 
+	if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_FW)
+		return kvm_arm_get_fw_reg(vcpu, reg);
+
 	if (is_timer_reg(reg->id))
 		return get_timer_reg(vcpu, reg);
 
@@ -230,6 +240,9 @@ int kvm_arm_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 	if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_CORE)
 		return set_core_reg(vcpu, reg);
 
+	if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_FW)
+		return kvm_arm_set_fw_reg(vcpu, reg);
+
 	if (is_timer_reg(reg->id))
 		return set_timer_reg(vcpu, reg);
 
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index a73f63aca68e..448d3b9a58cb 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -73,6 +73,9 @@ struct kvm_arch {
 
 	/* Interrupt controller */
 	struct vgic_dist	vgic;
+
+	/* Mandated version of PSCI */
+	u32 psci_version;
 };
 
 #define KVM_NR_MEM_OBJS     40
diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
index 9abbf3044654..04b3256f8e6d 100644
--- a/arch/arm64/include/uapi/asm/kvm.h
+++ b/arch/arm64/include/uapi/asm/kvm.h
@@ -206,6 +206,12 @@ struct kvm_arch_memory_slot {
 #define KVM_REG_ARM_TIMER_CNT		ARM64_SYS_REG(3, 3, 14, 3, 2)
 #define KVM_REG_ARM_TIMER_CVAL		ARM64_SYS_REG(3, 3, 14, 0, 2)
 
+/* KVM-as-firmware specific pseudo-registers */
+#define KVM_REG_ARM_FW			(0x0014 << KVM_REG_ARM_COPROC_SHIFT)
+#define KVM_REG_ARM_FW_REG(r)		(KVM_REG_ARM64 | KVM_REG_SIZE_U64 | \
+					 KVM_REG_ARM_FW | ((r) & 0xffff))
+#define KVM_REG_ARM_PSCI_VERSION	KVM_REG_ARM_FW_REG(0)
+
 /* Device Control API: ARM VGIC */
 #define KVM_DEV_ARM_VGIC_GRP_ADDR	0
 #define KVM_DEV_ARM_VGIC_GRP_DIST_REGS	1
diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
index 5c7f657dd207..811f04c5760e 100644
--- a/arch/arm64/kvm/guest.c
+++ b/arch/arm64/kvm/guest.c
@@ -25,6 +25,7 @@
 #include <linux/module.h>
 #include <linux/vmalloc.h>
 #include <linux/fs.h>
+#include <kvm/arm_psci.h>
 #include <asm/cputype.h>
 #include <linux/uaccess.h>
 #include <asm/kvm.h>
@@ -205,7 +206,7 @@ static int get_timer_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 unsigned long kvm_arm_num_regs(struct kvm_vcpu *vcpu)
 {
 	return num_core_regs() + kvm_arm_num_sys_reg_descs(vcpu)
-                + NUM_TIMER_REGS;
+		+ kvm_arm_get_fw_num_regs(vcpu)	+ NUM_TIMER_REGS;
 }
 
 /**
@@ -225,6 +226,11 @@ int kvm_arm_copy_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
 		uindices++;
 	}
 
+	ret = kvm_arm_copy_fw_reg_indices(vcpu, uindices);
+	if (ret)
+		return ret;
+	uindices += kvm_arm_get_fw_num_regs(vcpu);
+
 	ret = copy_timer_indices(vcpu, uindices);
 	if (ret)
 		return ret;
@@ -243,6 +249,9 @@ int kvm_arm_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 	if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_CORE)
 		return get_core_reg(vcpu, reg);
 
+	if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_FW)
+		return kvm_arm_get_fw_reg(vcpu, reg);
+
 	if (is_timer_reg(reg->id))
 		return get_timer_reg(vcpu, reg);
 
@@ -259,6 +268,9 @@ int kvm_arm_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 	if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_CORE)
 		return set_core_reg(vcpu, reg);
 
+	if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_FW)
+		return kvm_arm_set_fw_reg(vcpu, reg);
+
 	if (is_timer_reg(reg->id))
 		return set_timer_reg(vcpu, reg);
 
diff --git a/include/kvm/arm_psci.h b/include/kvm/arm_psci.h
index e518e4e3dfb5..4b1548129fa2 100644
--- a/include/kvm/arm_psci.h
+++ b/include/kvm/arm_psci.h
@@ -37,10 +37,15 @@ static inline int kvm_psci_version(struct kvm_vcpu *vcpu, struct kvm *kvm)
 	 * Our PSCI implementation stays the same across versions from
 	 * v0.2 onward, only adding the few mandatory functions (such
 	 * as FEATURES with 1.0) that are required by newer
-	 * revisions. It is thus safe to return the latest.
+	 * revisions. It is thus safe to return the latest, unless
+	 * userspace has instructed us otherwise.
 	 */
-	if (test_bit(KVM_ARM_VCPU_PSCI_0_2, vcpu->arch.features))
+	if (test_bit(KVM_ARM_VCPU_PSCI_0_2, vcpu->arch.features)) {
+		if (vcpu->kvm->arch.psci_version)
+			return vcpu->kvm->arch.psci_version;
+
 		return KVM_ARM_PSCI_LATEST;
+	}
 
 	return KVM_ARM_PSCI_0_1;
 }
@@ -48,4 +53,11 @@ static inline int kvm_psci_version(struct kvm_vcpu *vcpu, struct kvm *kvm)
 
 int kvm_hvc_call_handler(struct kvm_vcpu *vcpu);
 
+struct kvm_one_reg;
+
+int kvm_arm_get_fw_num_regs(struct kvm_vcpu *vcpu);
+int kvm_arm_copy_fw_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices);
+int kvm_arm_get_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg);
+int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg);
+
 #endif /* __KVM_ARM_PSCI_H__ */
diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c
index 6919352cbf15..c4762bef13c6 100644
--- a/virt/kvm/arm/psci.c
+++ b/virt/kvm/arm/psci.c
@@ -18,6 +18,7 @@
 #include <linux/arm-smccc.h>
 #include <linux/preempt.h>
 #include <linux/kvm_host.h>
+#include <linux/uaccess.h>
 #include <linux/wait.h>
 
 #include <asm/cputype.h>
@@ -427,3 +428,62 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
 	smccc_set_retval(vcpu, val, 0, 0, 0);
 	return 1;
 }
+
+int kvm_arm_get_fw_num_regs(struct kvm_vcpu *vcpu)
+{
+	return 1;		/* PSCI version */
+}
+
+int kvm_arm_copy_fw_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
+{
+	if (put_user(KVM_REG_ARM_PSCI_VERSION, uindices))
+		return -EFAULT;
+
+	return 0;
+}
+
+int kvm_arm_get_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
+{
+	if (reg->id == KVM_REG_ARM_PSCI_VERSION) {
+		void __user *uaddr = (void __user *)(long)reg->addr;
+		u64 val;
+
+		val = kvm_psci_version(vcpu, vcpu->kvm);
+		if (copy_to_user(uaddr, &val, KVM_REG_SIZE(reg->id)))
+			return -EFAULT;
+
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
+{
+	if (reg->id == KVM_REG_ARM_PSCI_VERSION) {
+		void __user *uaddr = (void __user *)(long)reg->addr;
+		bool wants_02;
+		u64 val;
+
+		if (copy_from_user(&val, uaddr, KVM_REG_SIZE(reg->id)))
+			return -EFAULT;
+
+		wants_02 = test_bit(KVM_ARM_VCPU_PSCI_0_2, vcpu->arch.features);
+
+		switch (val) {
+		case KVM_ARM_PSCI_0_1:
+			if (wants_02)
+				return -EINVAL;
+			vcpu->kvm->arch.psci_version = val;
+			return 0;
+		case KVM_ARM_PSCI_0_2:
+		case KVM_ARM_PSCI_1_0:
+			if (!wants_02)
+				return -EINVAL;
+			vcpu->kvm->arch.psci_version = val;
+			return 0;
+		}
+	}
+
+	return -EINVAL;
+}
-- 
2.14.2

Re: [REPOST PATCH] arm/arm64: KVM: Add PSCI version selection API

[Auger Eric]

Hi Marc,

On 15/02/18 18:58, Marc Zyngier wrote:
> Although we've implemented PSCI 0.1, 0.2 and 1.0, we expose either 0.1
> or 1.0 to a guest, defaulting to the latest version of the PSCI
> implementation that is compatible with the requested version. This is
> no different from doing a firmware upgrade on KVM.
> 
> But in order to give a chance to hypothetical badly implemented guests
> that would have a fit by discovering something other than PSCI 0.2,
> let's provide a new API that allows userspace to pick one particular
> version of the API.

I understand the get/set is called as part of the migration process.
So my understanding is the benefit of this series is migration fails in
those cases:

>=0.2 source -> 0.1 destination
0.1 source -> >=0.2 destination

However in the above mentioned use case where the guest would absolutely
need a 0.2 and not a 1.0 I don't get on which event the userspace would
do a set to force 0.2?
> 
> This is implemented as a new class of "firmware" registers, where
> we expose the PSCI version. This allows the PSCI version to be
> save/restored as part of a guest migration, and also set to
> any supported version if the guest requires it.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
> This is a repost of this proposal for a firmware selection API, as
> there was some discussion during the merging window. Now that the core
> variant-2 stuff is merged, we can have a go at more
> bikesheding. Please open fire!
> 
>  Documentation/virtual/kvm/api.txt      |  9 ++++-
>  Documentation/virtual/kvm/arm/psci.txt | 30 +++++++++++++++++
>  arch/arm/include/asm/kvm_host.h        |  3 ++
>  arch/arm/include/uapi/asm/kvm.h        |  6 ++++
>  arch/arm/kvm/guest.c                   | 13 ++++++++
>  arch/arm64/include/asm/kvm_host.h      |  3 ++
>  arch/arm64/include/uapi/asm/kvm.h      |  6 ++++
>  arch/arm64/kvm/guest.c                 | 14 +++++++-
>  include/kvm/arm_psci.h                 | 16 +++++++--
>  virt/kvm/arm/psci.c                    | 60 ++++++++++++++++++++++++++++++++++
>  10 files changed, 156 insertions(+), 4 deletions(-)
>  create mode 100644 Documentation/virtual/kvm/arm/psci.txt

> 
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index 57d3ee9e4bde..d2b41e1608b4 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -1943,6 +1943,9 @@ ARM 32-bit VFP control registers have the following id bit patterns:
>  ARM 64-bit FP registers have the following id bit patterns:
>    0x4030 0000 0012 0 <regno:12>
>  
> +ARM firmware pseudo-registers have the following bit pattern:
s/bit pattern/id bit patterns?
> +  0x4030 0000 0014 <regno:16>
> +
>  
>  arm64 registers are mapped using the lower 32 bits. The upper 16 of
>  that is the register group type, or coprocessor number:
> @@ -1959,6 +1962,9 @@ arm64 CCSIDR registers are demultiplexed by CSSELR value:
>  arm64 system registers have the following id bit patterns:
>    0x6030 0000 0013 <op0:2> <op1:3> <crn:4> <crm:4> <op2:3>
>  
> +arm64 firmware pseudo-registers have the following bit pattern:
s/bit pattern/id bit patterns?
> +  0x6030 0000 0014 <regno:16>
> +
>  
>  MIPS registers are mapped using the lower 32 bits.  The upper 16 of that is
>  the register group type:
> @@ -2493,7 +2499,8 @@ Possible features:
>  	  and execute guest code when KVM_RUN is called.
>  	- KVM_ARM_VCPU_EL1_32BIT: Starts the CPU in a 32bit mode.
>  	  Depends on KVM_CAP_ARM_EL1_32BIT (arm64 only).
> -	- KVM_ARM_VCPU_PSCI_0_2: Emulate PSCI v0.2 for the CPU.
> +	- KVM_ARM_VCPU_PSCI_0_2: Emulate PSCI v0.2 (or a future revision
> +          backward compatible with v0.2) for the CPU.
>  	  Depends on KVM_CAP_ARM_PSCI_0_2.
>  	- KVM_ARM_VCPU_PMU_V3: Emulate PMUv3 for the CPU.
>  	  Depends on KVM_CAP_ARM_PMU_V3.
> diff --git a/Documentation/virtual/kvm/arm/psci.txt b/Documentation/virtual/kvm/arm/psci.txt
> new file mode 100644
> index 000000000000..aafdab887b04
> --- /dev/null
> +++ b/Documentation/virtual/kvm/arm/psci.txt
> @@ -0,0 +1,30 @@
> +KVM implements the PSCI (Power State Coordination Interface)
> +specification in order to provide services such as CPU on/off, reset
> +and power-off to the guest.
> +
> +The PSCI specification is regularly updated to provide new features,
> +and KVM implements these updates if they make sense from a virtualization
> +point of view.
> +
> +This means that a guest booted on two different versions of KVM can
> +observe two different "firmware" revisions. This could cause issues if
> +a given guest is tied to a particular PSCI revision (unlikely), or if
> +a migration causes a different PSCI version to be exposed out of the
> +blue to an unsuspecting guest.
> +
> +In order to remedy this situation, KVM exposes a set of "firmware
> +pseudo-registers" that can be manipulated using the GET/SET_ONE_REG
> +interface. These registers can be saved/restored by userspace, and set
> +to a convenient value if required.
> +
> +The following register is defined:
> +
> +* KVM_REG_ARM_PSCI_VERSION:
> +
> +  - Only valid if the vcpu has the KVM_ARM_VCPU_PSCI_0_2 feature set
> +    (and thus has already been initialized)
> +  - Returns the current PSCI version on GET_ONE_REG (defaulting to the
> +    highest PSCI version implemented by KVM and compatible with v0.2)
backwards compatible?
> +  - Allows any PSCI version implemented by KVM and compatible with
backwards compatible?
> +    v0.2 to be set with SET_ONE_REG
> +  - Affects the whole VM (even if the register view is per-vcpu)
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index ef54013b5b9f..6c05e3b13081 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -75,6 +75,9 @@ struct kvm_arch {
>  	/* Interrupt controller */
>  	struct vgic_dist	vgic;
>  	int max_vcpus;
> +
> +	/* Mandated version of PSCI */
> +	u32 psci_version;
>  };
>  
>  #define KVM_NR_MEM_OBJS     40
> diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h
> index 6edd177bb1c7..47dfc99f5cd0 100644
> --- a/arch/arm/include/uapi/asm/kvm.h
> +++ b/arch/arm/include/uapi/asm/kvm.h
> @@ -186,6 +186,12 @@ struct kvm_arch_memory_slot {
>  #define KVM_REG_ARM_VFP_FPINST		0x1009
>  #define KVM_REG_ARM_VFP_FPINST2		0x100A
>  
> +/* KVM-as-firmware specific pseudo-registers */
> +#define KVM_REG_ARM_FW			(0x0014 << KVM_REG_ARM_COPROC_SHIFT)
> +#define KVM_REG_ARM_FW_REG(r)		(KVM_REG_ARM | KVM_REG_SIZE_U64 | \
> +					 KVM_REG_ARM_FW | ((r) & 0xffff))
> +#define KVM_REG_ARM_PSCI_VERSION	KVM_REG_ARM_FW_REG(0)

#define KVM_ARM_VCPU_PSCI_0_2           2 /* CPU uses PSCI v0.2 */
Comment may deserve an upgrade too
> +
>  /* Device Control API: ARM VGIC */
>  #define KVM_DEV_ARM_VGIC_GRP_ADDR	0
>  #define KVM_DEV_ARM_VGIC_GRP_DIST_REGS	1
> diff --git a/arch/arm/kvm/guest.c b/arch/arm/kvm/guest.c
> index 1e0784ebbfd6..a18f33edc471 100644
> --- a/arch/arm/kvm/guest.c
> +++ b/arch/arm/kvm/guest.c
> @@ -22,6 +22,7 @@
>  #include <linux/module.h>
>  #include <linux/vmalloc.h>
>  #include <linux/fs.h>
> +#include <kvm/arm_psci.h>
>  #include <asm/cputype.h>
>  #include <linux/uaccess.h>
>  #include <asm/kvm.h>
> @@ -176,6 +177,7 @@ static unsigned long num_core_regs(void)
>  unsigned long kvm_arm_num_regs(struct kvm_vcpu *vcpu)
>  {
>  	return num_core_regs() + kvm_arm_num_coproc_regs(vcpu)
> +		+ kvm_arm_get_fw_num_regs(vcpu)
>  		+ NUM_TIMER_REGS;
>  }
>  
> @@ -196,6 +198,11 @@ int kvm_arm_copy_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
>  		uindices++;
>  	}
>  
> +	ret = kvm_arm_copy_fw_reg_indices(vcpu, uindices);
> +	if (ret)
> +		return ret;
> +	uindices += kvm_arm_get_fw_num_regs(vcpu);
> +
>  	ret = copy_timer_indices(vcpu, uindices);
>  	if (ret)
>  		return ret;
> @@ -214,6 +221,9 @@ int kvm_arm_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
>  	if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_CORE)
>  		return get_core_reg(vcpu, reg);
>  
> +	if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_FW)
> +		return kvm_arm_get_fw_reg(vcpu, reg);
> +
>  	if (is_timer_reg(reg->id))
>  		return get_timer_reg(vcpu, reg);
>  
> @@ -230,6 +240,9 @@ int kvm_arm_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
>  	if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_CORE)
>  		return set_core_reg(vcpu, reg);
>  
> +	if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_FW)
> +		return kvm_arm_set_fw_reg(vcpu, reg);
> +
>  	if (is_timer_reg(reg->id))
>  		return set_timer_reg(vcpu, reg);
>  
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index a73f63aca68e..448d3b9a58cb 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -73,6 +73,9 @@ struct kvm_arch {
>  
>  	/* Interrupt controller */
>  	struct vgic_dist	vgic;
> +
> +	/* Mandated version of PSCI */
> +	u32 psci_version;
>  };
>  
>  #define KVM_NR_MEM_OBJS     40
> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> index 9abbf3044654..04b3256f8e6d 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -206,6 +206,12 @@ struct kvm_arch_memory_slot {
>  #define KVM_REG_ARM_TIMER_CNT		ARM64_SYS_REG(3, 3, 14, 3, 2)
>  #define KVM_REG_ARM_TIMER_CVAL		ARM64_SYS_REG(3, 3, 14, 0, 2)
>  
> +/* KVM-as-firmware specific pseudo-registers */
> +#define KVM_REG_ARM_FW			(0x0014 << KVM_REG_ARM_COPROC_SHIFT)
> +#define KVM_REG_ARM_FW_REG(r)		(KVM_REG_ARM64 | KVM_REG_SIZE_U64 | \
> +					 KVM_REG_ARM_FW | ((r) & 0xffff))
> +#define KVM_REG_ARM_PSCI_VERSION	KVM_REG_ARM_FW_REG(0)
> +
>  /* Device Control API: ARM VGIC */
>  #define KVM_DEV_ARM_VGIC_GRP_ADDR	0
>  #define KVM_DEV_ARM_VGIC_GRP_DIST_REGS	1
> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> index 5c7f657dd207..811f04c5760e 100644
> --- a/arch/arm64/kvm/guest.c
> +++ b/arch/arm64/kvm/guest.c
> @@ -25,6 +25,7 @@
>  #include <linux/module.h>
>  #include <linux/vmalloc.h>
>  #include <linux/fs.h>
> +#include <kvm/arm_psci.h>
>  #include <asm/cputype.h>
>  #include <linux/uaccess.h>
>  #include <asm/kvm.h>
> @@ -205,7 +206,7 @@ static int get_timer_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
>  unsigned long kvm_arm_num_regs(struct kvm_vcpu *vcpu)
>  {
>  	return num_core_regs() + kvm_arm_num_sys_reg_descs(vcpu)
> -                + NUM_TIMER_REGS;
> +		+ kvm_arm_get_fw_num_regs(vcpu)	+ NUM_TIMER_REGS;
>  }
>  
>  /**
function kerneldoc comment may be updated
> @@ -225,6 +226,11 @@ int kvm_arm_copy_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
>  		uindices++;
>  	}
>  
> +	ret = kvm_arm_copy_fw_reg_indices(vcpu, uindices);
> +	if (ret)
> +		return ret;
> +	uindices += kvm_arm_get_fw_num_regs(vcpu);
> +
>  	ret = copy_timer_indices(vcpu, uindices);
>  	if (ret)
>  		return ret;
> @@ -243,6 +249,9 @@ int kvm_arm_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
>  	if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_CORE)
>  		return get_core_reg(vcpu, reg);
>  
> +	if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_FW)
> +		return kvm_arm_get_fw_reg(vcpu, reg);
> +
>  	if (is_timer_reg(reg->id))
>  		return get_timer_reg(vcpu, reg);
>  
> @@ -259,6 +268,9 @@ int kvm_arm_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
>  	if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_CORE)
>  		return set_core_reg(vcpu, reg);
>  
> +	if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_FW)
> +		return kvm_arm_set_fw_reg(vcpu, reg);
> +
>  	if (is_timer_reg(reg->id))
>  		return set_timer_reg(vcpu, reg);
>  
> diff --git a/include/kvm/arm_psci.h b/include/kvm/arm_psci.h
> index e518e4e3dfb5..4b1548129fa2 100644
> --- a/include/kvm/arm_psci.h
> +++ b/include/kvm/arm_psci.h
> @@ -37,10 +37,15 @@ static inline int kvm_psci_version(struct kvm_vcpu *vcpu, struct kvm *kvm)
>  	 * Our PSCI implementation stays the same across versions from
>  	 * v0.2 onward, only adding the few mandatory functions (such
>  	 * as FEATURES with 1.0) that are required by newer
> -	 * revisions. It is thus safe to return the latest.
> +	 * revisions. It is thus safe to return the latest, unless
> +	 * userspace has instructed us otherwise.
>  	 */
> -	if (test_bit(KVM_ARM_VCPU_PSCI_0_2, vcpu->arch.features))
> +	if (test_bit(KVM_ARM_VCPU_PSCI_0_2, vcpu->arch.features)) {
> +		if (vcpu->kvm->arch.psci_version)
> +			return vcpu->kvm->arch.psci_version;
> +
>  		return KVM_ARM_PSCI_LATEST;
> +	}
>  
>  	return KVM_ARM_PSCI_0_1;
>  }
> @@ -48,4 +53,11 @@ static inline int kvm_psci_version(struct kvm_vcpu *vcpu, struct kvm *kvm)
>  
>  int kvm_hvc_call_handler(struct kvm_vcpu *vcpu);
>  
> +struct kvm_one_reg;
> +
> +int kvm_arm_get_fw_num_regs(struct kvm_vcpu *vcpu);
> +int kvm_arm_copy_fw_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices);
> +int kvm_arm_get_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg);
> +int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg);
> +
>  #endif /* __KVM_ARM_PSCI_H__ */
> diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c
> index 6919352cbf15..c4762bef13c6 100644
> --- a/virt/kvm/arm/psci.c
> +++ b/virt/kvm/arm/psci.c
> @@ -18,6 +18,7 @@
>  #include <linux/arm-smccc.h>
>  #include <linux/preempt.h>
>  #include <linux/kvm_host.h>
> +#include <linux/uaccess.h>
>  #include <linux/wait.h>
>  
>  #include <asm/cputype.h>
> @@ -427,3 +428,62 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
>  	smccc_set_retval(vcpu, val, 0, 0, 0);
>  	return 1;
>  }
> +
> +int kvm_arm_get_fw_num_regs(struct kvm_vcpu *vcpu)
> +{
> +	return 1;		/* PSCI version */
> +}
> +
> +int kvm_arm_copy_fw_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
> +{
> +	if (put_user(KVM_REG_ARM_PSCI_VERSION, uindices))
> +		return -EFAULT;
> +
> +	return 0;
> +}
> +
> +int kvm_arm_get_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> +{
> +	if (reg->id == KVM_REG_ARM_PSCI_VERSION) {
> +		void __user *uaddr = (void __user *)(long)reg->addr;
> +		u64 val;
> +
> +		val = kvm_psci_version(vcpu, vcpu->kvm);
> +		if (copy_to_user(uaddr, &val, KVM_REG_SIZE(reg->id)))
> +			return -EFAULT;
> +
> +		return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> +{
> +	if (reg->id == KVM_REG_ARM_PSCI_VERSION) {
> +		void __user *uaddr = (void __user *)(long)reg->addr;
> +		bool wants_02;
> +		u64 val;
> +
> +		if (copy_from_user(&val, uaddr, KVM_REG_SIZE(reg->id)))
> +			return -EFAULT;
> +
> +		wants_02 = test_bit(KVM_ARM_VCPU_PSCI_0_2, vcpu->arch.features);
> +
> +		switch (val) {
> +		case KVM_ARM_PSCI_0_1:
> +			if (wants_02)
> +				return -EINVAL;
> +			vcpu->kvm->arch.psci_version = val;
> +			return 0;
> +		case KVM_ARM_PSCI_0_2:
> +		case KVM_ARM_PSCI_1_0:
> +			if (!wants_02)
> +				return -EINVAL;
> +			vcpu->kvm->arch.psci_version = val;
> +			return 0;
> +		}
> +	}
> +
> +	return -EINVAL;
> +}
> 
Thanks

Eric

Re: [REPOST PATCH] arm/arm64: KVM: Add PSCI version selection API

[Marc Zyngier]

On Fri, 02 Mar 2018 10:44:48 +0000,
Auger Eric wrote:
> 
> Hi Marc,
> 
> On 15/02/18 18:58, Marc Zyngier wrote:
> > Although we've implemented PSCI 0.1, 0.2 and 1.0, we expose either 0.1
> > or 1.0 to a guest, defaulting to the latest version of the PSCI
> > implementation that is compatible with the requested version. This is
> > no different from doing a firmware upgrade on KVM.
> > 
> > But in order to give a chance to hypothetical badly implemented guests
> > that would have a fit by discovering something other than PSCI 0.2,
> > let's provide a new API that allows userspace to pick one particular
> > version of the API.
> 
> I understand the get/set is called as part of the migration process.
> So my understanding is the benefit of this series is migration fails in
> those cases:
> 
> >=0.2 source -> 0.1 destination
> 0.1 source -> >=0.2 destination

It also fails in the case where you migrate a 1.0 guest to something
that cannot support it.

> However in the above mentioned use case where the guest would absolutely
> need a 0.2 and not a 1.0 I don't get on which event the userspace would
> do a set to force 0.2?

You'd have to know that your guest cannot support 0.2, and force 0.2
by doing a SET_ONE_REG(KVM_REG_ARM_PSCI_VERSION, PSCI_VERSION(0, 2)).

Or am I missing what you're asking for?

> > 
> > This is implemented as a new class of "firmware" registers, where
> > we expose the PSCI version. This allows the PSCI version to be
> > save/restored as part of a guest migration, and also set to
> > any supported version if the guest requires it.
> > 
> > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> > ---
> > This is a repost of this proposal for a firmware selection API, as
> > there was some discussion during the merging window. Now that the core
> > variant-2 stuff is merged, we can have a go at more
> > bikesheding. Please open fire!
> > 
> >  Documentation/virtual/kvm/api.txt      |  9 ++++-
> >  Documentation/virtual/kvm/arm/psci.txt | 30 +++++++++++++++++
> >  arch/arm/include/asm/kvm_host.h        |  3 ++
> >  arch/arm/include/uapi/asm/kvm.h        |  6 ++++
> >  arch/arm/kvm/guest.c                   | 13 ++++++++
> >  arch/arm64/include/asm/kvm_host.h      |  3 ++
> >  arch/arm64/include/uapi/asm/kvm.h      |  6 ++++
> >  arch/arm64/kvm/guest.c                 | 14 +++++++-
> >  include/kvm/arm_psci.h                 | 16 +++++++--
> >  virt/kvm/arm/psci.c                    | 60 ++++++++++++++++++++++++++++++++++
> >  10 files changed, 156 insertions(+), 4 deletions(-)
> >  create mode 100644 Documentation/virtual/kvm/arm/psci.txt
> 
> > 
> > diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> > index 57d3ee9e4bde..d2b41e1608b4 100644
> > --- a/Documentation/virtual/kvm/api.txt
> > +++ b/Documentation/virtual/kvm/api.txt
> > @@ -1943,6 +1943,9 @@ ARM 32-bit VFP control registers have the following id bit patterns:
> >  ARM 64-bit FP registers have the following id bit patterns:
> >    0x4030 0000 0012 0 <regno:12>
> >  
> > +ARM firmware pseudo-registers have the following bit pattern:
> s/bit pattern/id bit patterns?
> > +  0x4030 0000 0014 <regno:16>
> > +
> >  
> >  arm64 registers are mapped using the lower 32 bits. The upper 16 of
> >  that is the register group type, or coprocessor number:
> > @@ -1959,6 +1962,9 @@ arm64 CCSIDR registers are demultiplexed by CSSELR value:
> >  arm64 system registers have the following id bit patterns:
> >    0x6030 0000 0013 <op0:2> <op1:3> <crn:4> <crm:4> <op2:3>
> >  
> > +arm64 firmware pseudo-registers have the following bit pattern:
> s/bit pattern/id bit patterns?
> > +  0x6030 0000 0014 <regno:16>
> > +
> >  
> >  MIPS registers are mapped using the lower 32 bits.  The upper 16 of that is
> >  the register group type:
> > @@ -2493,7 +2499,8 @@ Possible features:
> >  	  and execute guest code when KVM_RUN is called.
> >  	- KVM_ARM_VCPU_EL1_32BIT: Starts the CPU in a 32bit mode.
> >  	  Depends on KVM_CAP_ARM_EL1_32BIT (arm64 only).
> > -	- KVM_ARM_VCPU_PSCI_0_2: Emulate PSCI v0.2 for the CPU.
> > +	- KVM_ARM_VCPU_PSCI_0_2: Emulate PSCI v0.2 (or a future revision
> > +          backward compatible with v0.2) for the CPU.
> >  	  Depends on KVM_CAP_ARM_PSCI_0_2.
> >  	- KVM_ARM_VCPU_PMU_V3: Emulate PMUv3 for the CPU.
> >  	  Depends on KVM_CAP_ARM_PMU_V3.
> > diff --git a/Documentation/virtual/kvm/arm/psci.txt b/Documentation/virtual/kvm/arm/psci.txt
> > new file mode 100644
> > index 000000000000..aafdab887b04
> > --- /dev/null
> > +++ b/Documentation/virtual/kvm/arm/psci.txt
> > @@ -0,0 +1,30 @@
> > +KVM implements the PSCI (Power State Coordination Interface)
> > +specification in order to provide services such as CPU on/off, reset
> > +and power-off to the guest.
> > +
> > +The PSCI specification is regularly updated to provide new features,
> > +and KVM implements these updates if they make sense from a virtualization
> > +point of view.
> > +
> > +This means that a guest booted on two different versions of KVM can
> > +observe two different "firmware" revisions. This could cause issues if
> > +a given guest is tied to a particular PSCI revision (unlikely), or if
> > +a migration causes a different PSCI version to be exposed out of the
> > +blue to an unsuspecting guest.
> > +
> > +In order to remedy this situation, KVM exposes a set of "firmware
> > +pseudo-registers" that can be manipulated using the GET/SET_ONE_REG
> > +interface. These registers can be saved/restored by userspace, and set
> > +to a convenient value if required.
> > +
> > +The following register is defined:
> > +
> > +* KVM_REG_ARM_PSCI_VERSION:
> > +
> > +  - Only valid if the vcpu has the KVM_ARM_VCPU_PSCI_0_2 feature set
> > +    (and thus has already been initialized)
> > +  - Returns the current PSCI version on GET_ONE_REG (defaulting to the
> > +    highest PSCI version implemented by KVM and compatible with v0.2)
> backwards compatible?
> > +  - Allows any PSCI version implemented by KVM and compatible with
> backwards compatible?
> > +    v0.2 to be set with SET_ONE_REG
> > +  - Affects the whole VM (even if the register view is per-vcpu)
> > diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> > index ef54013b5b9f..6c05e3b13081 100644
> > --- a/arch/arm/include/asm/kvm_host.h
> > +++ b/arch/arm/include/asm/kvm_host.h
> > @@ -75,6 +75,9 @@ struct kvm_arch {
> >  	/* Interrupt controller */
> >  	struct vgic_dist	vgic;
> >  	int max_vcpus;
> > +
> > +	/* Mandated version of PSCI */
> > +	u32 psci_version;
> >  };
> >  
> >  #define KVM_NR_MEM_OBJS     40
> > diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h
> > index 6edd177bb1c7..47dfc99f5cd0 100644
> > --- a/arch/arm/include/uapi/asm/kvm.h
> > +++ b/arch/arm/include/uapi/asm/kvm.h
> > @@ -186,6 +186,12 @@ struct kvm_arch_memory_slot {
> >  #define KVM_REG_ARM_VFP_FPINST		0x1009
> >  #define KVM_REG_ARM_VFP_FPINST2		0x100A
> >  
> > +/* KVM-as-firmware specific pseudo-registers */
> > +#define KVM_REG_ARM_FW			(0x0014 << KVM_REG_ARM_COPROC_SHIFT)
> > +#define KVM_REG_ARM_FW_REG(r)		(KVM_REG_ARM | KVM_REG_SIZE_U64 | \
> > +					 KVM_REG_ARM_FW | ((r) & 0xffff))
> > +#define KVM_REG_ARM_PSCI_VERSION	KVM_REG_ARM_FW_REG(0)
> 
> #define KVM_ARM_VCPU_PSCI_0_2           2 /* CPU uses PSCI v0.2 */
> Comment may deserve an upgrade too

I'm not sure we want this. KVM_REG_PSCI_VERSION takes a PSCI version
number, as indicated in the PSCI spec. You should use
PSCI_VERSION(0,2), as defined in include/uapi/linux/psci.h.

Thanks,

	M.

-- 
Jazz is not dead, it just smell funny.

Re: [REPOST PATCH] arm/arm64: KVM: Add PSCI version selection API

[Auger Eric]

Hi Marc,
On 02/03/18 12:11, Marc Zyngier wrote:
> On Fri, 02 Mar 2018 10:44:48 +0000,
> Auger Eric wrote:
>>
>> Hi Marc,
>>
>> On 15/02/18 18:58, Marc Zyngier wrote:
>>> Although we've implemented PSCI 0.1, 0.2 and 1.0, we expose either 0.1
>>> or 1.0 to a guest, defaulting to the latest version of the PSCI
>>> implementation that is compatible with the requested version. This is
>>> no different from doing a firmware upgrade on KVM.
>>>
>>> But in order to give a chance to hypothetical badly implemented guests
>>> that would have a fit by discovering something other than PSCI 0.2,
>>> let's provide a new API that allows userspace to pick one particular
>>> version of the API.
>>
>> I understand the get/set is called as part of the migration process.
>> So my understanding is the benefit of this series is migration fails in
>> those cases:
>>
>>> =0.2 source -> 0.1 destination
>> 0.1 source -> >=0.2 destination
> 
> It also fails in the case where you migrate a 1.0 guest to something
> that cannot support it.

That's because on the destination, the number of regs is less than on
source, right?

My previous reasoning was:

At the moment, in QEMU, IIUC, we check the KVM_CAP_ARM_PSCI_0_2
capability and if it is advertised, we initialize the vpcu with
KVM_ARM_VCPU_PSCI_0_2. This holds for a 0.2 or 1.0 kernel.

Assuming the source is 1.0 capable and the destination has a < 4.16
kernel so 0.2 only. On migration the fw reg get() returns
KVM_ARM_PSCI_1_0. On Set, wants_02 is set and
vcpu->kvm->arch.psci_version gets set to KVM_ARM_PSCI_1_0.

> 
>> However in the above mentioned use case where the guest would absolutely
>> need a 0.2 and not a 1.0 I don't get on which event the userspace would
>> do a set to force 0.2?
> 
> You'd have to know that your guest cannot support 0.2, and force 0.2
> by doing a SET_ONE_REG(KVM_REG_ARM_PSCI_VERSION, PSCI_VERSION(0, 2)).
> 
> Or am I missing what you're asking for?
My question rather was how do you get to know?
> 
>>>
>>> This is implemented as a new class of "firmware" registers, where
>>> we expose the PSCI version. This allows the PSCI version to be
>>> save/restored as part of a guest migration, and also set to
>>> any supported version if the guest requires it.
>>>
>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>> ---
>>> This is a repost of this proposal for a firmware selection API, as
>>> there was some discussion during the merging window. Now that the core
>>> variant-2 stuff is merged, we can have a go at more
>>> bikesheding. Please open fire!
>>>
>>>  Documentation/virtual/kvm/api.txt      |  9 ++++-
>>>  Documentation/virtual/kvm/arm/psci.txt | 30 +++++++++++++++++
>>>  arch/arm/include/asm/kvm_host.h        |  3 ++
>>>  arch/arm/include/uapi/asm/kvm.h        |  6 ++++
>>>  arch/arm/kvm/guest.c                   | 13 ++++++++
>>>  arch/arm64/include/asm/kvm_host.h      |  3 ++
>>>  arch/arm64/include/uapi/asm/kvm.h      |  6 ++++
>>>  arch/arm64/kvm/guest.c                 | 14 +++++++-
>>>  include/kvm/arm_psci.h                 | 16 +++++++--
>>>  virt/kvm/arm/psci.c                    | 60 ++++++++++++++++++++++++++++++++++
>>>  10 files changed, 156 insertions(+), 4 deletions(-)
>>>  create mode 100644 Documentation/virtual/kvm/arm/psci.txt
>>
>>>
>>> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
>>> index 57d3ee9e4bde..d2b41e1608b4 100644
>>> --- a/Documentation/virtual/kvm/api.txt
>>> +++ b/Documentation/virtual/kvm/api.txt
>>> @@ -1943,6 +1943,9 @@ ARM 32-bit VFP control registers have the following id bit patterns:
>>>  ARM 64-bit FP registers have the following id bit patterns:
>>>    0x4030 0000 0012 0 <regno:12>
>>>  
>>> +ARM firmware pseudo-registers have the following bit pattern:
>> s/bit pattern/id bit patterns?
>>> +  0x4030 0000 0014 <regno:16>
>>> +
>>>  
>>>  arm64 registers are mapped using the lower 32 bits. The upper 16 of
>>>  that is the register group type, or coprocessor number:
>>> @@ -1959,6 +1962,9 @@ arm64 CCSIDR registers are demultiplexed by CSSELR value:
>>>  arm64 system registers have the following id bit patterns:
>>>    0x6030 0000 0013 <op0:2> <op1:3> <crn:4> <crm:4> <op2:3>
>>>  
>>> +arm64 firmware pseudo-registers have the following bit pattern:
>> s/bit pattern/id bit patterns?
>>> +  0x6030 0000 0014 <regno:16>
>>> +
>>>  
>>>  MIPS registers are mapped using the lower 32 bits.  The upper 16 of that is
>>>  the register group type:
>>> @@ -2493,7 +2499,8 @@ Possible features:
>>>  	  and execute guest code when KVM_RUN is called.
>>>  	- KVM_ARM_VCPU_EL1_32BIT: Starts the CPU in a 32bit mode.
>>>  	  Depends on KVM_CAP_ARM_EL1_32BIT (arm64 only).
>>> -	- KVM_ARM_VCPU_PSCI_0_2: Emulate PSCI v0.2 for the CPU.
>>> +	- KVM_ARM_VCPU_PSCI_0_2: Emulate PSCI v0.2 (or a future revision
>>> +          backward compatible with v0.2) for the CPU.
>>>  	  Depends on KVM_CAP_ARM_PSCI_0_2.
>>>  	- KVM_ARM_VCPU_PMU_V3: Emulate PMUv3 for the CPU.
>>>  	  Depends on KVM_CAP_ARM_PMU_V3.
>>> diff --git a/Documentation/virtual/kvm/arm/psci.txt b/Documentation/virtual/kvm/arm/psci.txt
>>> new file mode 100644
>>> index 000000000000..aafdab887b04
>>> --- /dev/null
>>> +++ b/Documentation/virtual/kvm/arm/psci.txt
>>> @@ -0,0 +1,30 @@
>>> +KVM implements the PSCI (Power State Coordination Interface)
>>> +specification in order to provide services such as CPU on/off, reset
>>> +and power-off to the guest.
>>> +
>>> +The PSCI specification is regularly updated to provide new features,
>>> +and KVM implements these updates if they make sense from a virtualization
>>> +point of view.
>>> +
>>> +This means that a guest booted on two different versions of KVM can
>>> +observe two different "firmware" revisions. This could cause issues if
>>> +a given guest is tied to a particular PSCI revision (unlikely), or if
>>> +a migration causes a different PSCI version to be exposed out of the
>>> +blue to an unsuspecting guest.
>>> +
>>> +In order to remedy this situation, KVM exposes a set of "firmware
>>> +pseudo-registers" that can be manipulated using the GET/SET_ONE_REG
>>> +interface. These registers can be saved/restored by userspace, and set
>>> +to a convenient value if required.
>>> +
>>> +The following register is defined:
>>> +
>>> +* KVM_REG_ARM_PSCI_VERSION:
>>> +
>>> +  - Only valid if the vcpu has the KVM_ARM_VCPU_PSCI_0_2 feature set
>>> +    (and thus has already been initialized)
>>> +  - Returns the current PSCI version on GET_ONE_REG (defaulting to the
>>> +    highest PSCI version implemented by KVM and compatible with v0.2)
>> backwards compatible?
>>> +  - Allows any PSCI version implemented by KVM and compatible with
>> backwards compatible?
>>> +    v0.2 to be set with SET_ONE_REG
>>> +  - Affects the whole VM (even if the register view is per-vcpu)
>>> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
>>> index ef54013b5b9f..6c05e3b13081 100644
>>> --- a/arch/arm/include/asm/kvm_host.h
>>> +++ b/arch/arm/include/asm/kvm_host.h
>>> @@ -75,6 +75,9 @@ struct kvm_arch {
>>>  	/* Interrupt controller */
>>>  	struct vgic_dist	vgic;
>>>  	int max_vcpus;
>>> +
>>> +	/* Mandated version of PSCI */
>>> +	u32 psci_version;
>>>  };
>>>  
>>>  #define KVM_NR_MEM_OBJS     40
>>> diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h
>>> index 6edd177bb1c7..47dfc99f5cd0 100644
>>> --- a/arch/arm/include/uapi/asm/kvm.h
>>> +++ b/arch/arm/include/uapi/asm/kvm.h
>>> @@ -186,6 +186,12 @@ struct kvm_arch_memory_slot {
>>>  #define KVM_REG_ARM_VFP_FPINST		0x1009
>>>  #define KVM_REG_ARM_VFP_FPINST2		0x100A
>>>  
>>> +/* KVM-as-firmware specific pseudo-registers */
>>> +#define KVM_REG_ARM_FW			(0x0014 << KVM_REG_ARM_COPROC_SHIFT)
>>> +#define KVM_REG_ARM_FW_REG(r)		(KVM_REG_ARM | KVM_REG_SIZE_U64 | \
>>> +					 KVM_REG_ARM_FW | ((r) & 0xffff))
>>> +#define KVM_REG_ARM_PSCI_VERSION	KVM_REG_ARM_FW_REG(0)
>>
>> #define KVM_ARM_VCPU_PSCI_0_2           2 /* CPU uses PSCI v0.2 */
>> Comment may deserve an upgrade too
> 
> I'm not sure we want this. KVM_REG_PSCI_VERSION takes a PSCI version
> number, as indicated in the PSCI spec. You should use
> PSCI_VERSION(0,2), as defined in include/uapi/linux/psci.h.
The above define is used when initializing the vcpu, right? This now
means we are likely to have a >=0.2 PSCI version and not strictly
speaking a 0.2 one?

Thanks

Eric
> 
> Thanks,
> 
> 	M.
> 

Re: [REPOST PATCH] arm/arm64: KVM: Add PSCI version selection API

[Peter Maydell]

On 2 March 2018 at 12:26, Auger Eric <eric.auger@redhat.com> wrote:
> Hi Marc,
> On 02/03/18 12:11, Marc Zyngier wrote:
>> On Fri, 02 Mar 2018 10:44:48 +0000,
>> Auger Eric wrote:
>>> I understand the get/set is called as part of the migration process.
>>> So my understanding is the benefit of this series is migration fails in
>>> those cases:
>>>
>>>> =0.2 source -> 0.1 destination
>>> 0.1 source -> >=0.2 destination
>>
>> It also fails in the case where you migrate a 1.0 guest to something
>> that cannot support it.
>
> That's because on the destination, the number of regs is less than on
> source, right?

I think it fails because the KVM_REG_ARM_PSCI_VERSION register will be
in the migration state but not in the destination's list of registers:
the code in QEMU's target/arm/machine.c:cpu_post_load() function that
checks "register in their list but not ours: fail migration" will
catch this.

That also means that we will fail migration from a new kernel where
we've specifically asked for PSCI 0.2 to an old PSCI-0.2-only kernel
(because the KVM_REG_ARM_PSCI_VERSION reg will appear in the migration
stream even if its value is the one value that matches the old kernel
behaviour). I don't know if we care about that.

thanks
-- PMM

Re: [REPOST PATCH] arm/arm64: KVM: Add PSCI version selection API

[Peter Maydell]

On 2 March 2018 at 11:11, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On Fri, 02 Mar 2018 10:44:48 +0000,
> Auger Eric wrote:
>> I understand the get/set is called as part of the migration process.
>> So my understanding is the benefit of this series is migration fails in
>> those cases:
>>
>> >=0.2 source -> 0.1 destination
>> 0.1 source -> >=0.2 destination
>
> It also fails in the case where you migrate a 1.0 guest to something
> that cannot support it.

I think it would be useful if we could write out the various
combinations of source, destination and what we expect/want to
have happen. My gut feeling here is that we're sacrificing
exact migration compatibility in favour of having the guest
automatically get the variant-2 mitigations, but it's not clear
to me exactly which migration combinations that's intended to
happen for. Marc?

If this wasn't a mitigation issue the desired behaviour would be
straightforward:
 * kernel should default to 0.2 on the basis that
   that's what it did before
 * new QEMU version should enable 1.0 by default for virt-2.12
   and 0.2 for virt-2.11 and earlier
 * PSCI version info shouldn't appear in migration stream unless
   it's something other than 0.2
But that would leave some setups (which?) unnecessarily without the
mitigation, so we're not doing that. The question is, exactly
what *are* we aiming for?

thanks
-- PMM

Re: [REPOST PATCH] arm/arm64: KVM: Add PSCI version selection API

[Auger Eric]

Hi Peter,

On 05/03/18 17:31, Peter Maydell wrote:
> On 2 March 2018 at 12:26, Auger Eric <eric.auger@redhat.com> wrote:
>> Hi Marc,
>> On 02/03/18 12:11, Marc Zyngier wrote:
>>> On Fri, 02 Mar 2018 10:44:48 +0000,
>>> Auger Eric wrote:
>>>> I understand the get/set is called as part of the migration process.
>>>> So my understanding is the benefit of this series is migration fails in
>>>> those cases:
>>>>
>>>>> =0.2 source -> 0.1 destination
>>>> 0.1 source -> >=0.2 destination
>>>
>>> It also fails in the case where you migrate a 1.0 guest to something
>>> that cannot support it.
>>
>> That's because on the destination, the number of regs is less than on
>> source, right?
> 
> I think it fails because the KVM_REG_ARM_PSCI_VERSION register will be
> in the migration state but not in the destination's list of registers:
> the code in QEMU's target/arm/machine.c:cpu_post_load() function that
> checks "register in their list but not ours: fail migration" will
> catch this.

Thank you for the pointer. Yes at the time I reviewed the patch and just
focusing on the kernel code, this was not immediate to me.

> 
> That also means that we will fail migration from a new kernel where
> we've specifically asked for PSCI 0.2 to an old PSCI-0.2-only kernel
> (because the KVM_REG_ARM_PSCI_VERSION reg will appear in the migration
> stream even if its value is the one value that matches the old kernel
> behaviour). I don't know if we care about that.

Do you know when are we likely to force PSCI 0.2 on a new kernel? At
which layer is the decision supposed to be made and on which criteria?

Thanks

Eric
> 
> thanks
> -- PMM
> 

Re: [REPOST PATCH] arm/arm64: KVM: Add PSCI version selection API

[Andrew Jones]

On Mon, Mar 05, 2018 at 04:47:55PM +0000, Peter Maydell wrote:
> On 2 March 2018 at 11:11, Marc Zyngier <marc.zyngier@arm.com> wrote:
> > On Fri, 02 Mar 2018 10:44:48 +0000,
> > Auger Eric wrote:
> >> I understand the get/set is called as part of the migration process.
> >> So my understanding is the benefit of this series is migration fails in
> >> those cases:
> >>
> >> >=0.2 source -> 0.1 destination
> >> 0.1 source -> >=0.2 destination
> >
> > It also fails in the case where you migrate a 1.0 guest to something
> > that cannot support it.
> 
> I think it would be useful if we could write out the various
> combinations of source, destination and what we expect/want to
> have happen. My gut feeling here is that we're sacrificing
> exact migration compatibility in favour of having the guest
> automatically get the variant-2 mitigations, but it's not clear
> to me exactly which migration combinations that's intended to
> happen for. Marc?
> 
> If this wasn't a mitigation issue the desired behaviour would be
> straightforward:
>  * kernel should default to 0.2 on the basis that
>    that's what it did before
>  * new QEMU version should enable 1.0 by default for virt-2.12
>    and 0.2 for virt-2.11 and earlier
>  * PSCI version info shouldn't appear in migration stream unless
>    it's something other than 0.2
> But that would leave some setups (which?) unnecessarily without the
> mitigation, so we're not doing that. The question is, exactly
> what *are* we aiming for?

The reason Marc dropped this patch from the series it was first introduced
in was because we didn't have the aim 100% understood. We want the
mitigation by default, but also to have the least chance of migration
failure, and when we must fail (because we're not doing the
straightforward approach listed above, which would prevent failures), then
we want to fail with the least amount of damage to the user.

I experimented with a couple different approaches and provided tables[1]
with my results. I even recommended an approach, but I may have changed
my mind after reading Marc's follow-up[2]. The thread continues from
there as well with follow-ups from Christoffer, Marc, and myself. Anyway,
Marc did this repost for us to debate it and work out the best approach
here.

Thanks,
drew

[1] https://www.spinics.net/lists/arm-kernel/msg632355.html
[2] https://www.spinics.net/lists/arm-kernel/msg632385.html

Re: [REPOST PATCH] arm/arm64: KVM: Add PSCI version selection API

[Marc Zyngier]

On 05/03/18 20:37, Auger Eric wrote:
> Hi Peter,
> 
> On 05/03/18 17:31, Peter Maydell wrote:
>> On 2 March 2018 at 12:26, Auger Eric <eric.auger@redhat.com> wrote:
>>> Hi Marc,
>>> On 02/03/18 12:11, Marc Zyngier wrote:
>>>> On Fri, 02 Mar 2018 10:44:48 +0000,
>>>> Auger Eric wrote:
>>>>> I understand the get/set is called as part of the migration process.
>>>>> So my understanding is the benefit of this series is migration fails in
>>>>> those cases:
>>>>>
>>>>>> =0.2 source -> 0.1 destination
>>>>> 0.1 source -> >=0.2 destination
>>>>
>>>> It also fails in the case where you migrate a 1.0 guest to something
>>>> that cannot support it.
>>>
>>> That's because on the destination, the number of regs is less than on
>>> source, right?
>>
>> I think it fails because the KVM_REG_ARM_PSCI_VERSION register will be
>> in the migration state but not in the destination's list of registers:
>> the code in QEMU's target/arm/machine.c:cpu_post_load() function that
>> checks "register in their list but not ours: fail migration" will
>> catch this.
> 
> Thank you for the pointer. Yes at the time I reviewed the patch and just
> focusing on the kernel code, this was not immediate to me.
> 
>>
>> That also means that we will fail migration from a new kernel where
>> we've specifically asked for PSCI 0.2 to an old PSCI-0.2-only kernel
>> (because the KVM_REG_ARM_PSCI_VERSION reg will appear in the migration
>> stream even if its value is the one value that matches the old kernel
>> behaviour). I don't know if we care about that.
> 
> Do you know when are we likely to force PSCI 0.2 on a new kernel? At
> which layer is the decision supposed to be made and on which criteria?

No decent SW should need this. But if you've written a guest that cannot
work if it doesn't get "2" as response to PSCI_VERSION, you can override it.

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [REPOST PATCH] arm/arm64: KVM: Add PSCI version selection API

[Peter Maydell]

On 6 March 2018 at 09:50, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 05/03/18 20:37, Auger Eric wrote:
>> On 05/03/18 17:31, Peter Maydell wrote:
>>> That also means that we will fail migration from a new kernel where
>>> we've specifically asked for PSCI 0.2 to an old PSCI-0.2-only kernel
>>> (because the KVM_REG_ARM_PSCI_VERSION reg will appear in the migration
>>> stream even if its value is the one value that matches the old kernel
>>> behaviour). I don't know if we care about that.
>>
>> Do you know when are we likely to force PSCI 0.2 on a new kernel? At
>> which layer is the decision supposed to be made and on which criteria?
>
> No decent SW should need this. But if you've written a guest that cannot
> work if it doesn't get "2" as response to PSCI_VERSION, you can override it.

...but if you want to be able to migrate back from a new kernel to
an old one, then you need to ask the new kernel for 0.2 so it
behaves the same way as the old one. As it stands this code wouldn't
let you do that migration even if you did specifically ask for 0.2.
(As I said, I don't know if we care about that.)

thanks
-- PMM

Re: [REPOST PATCH] arm/arm64: KVM: Add PSCI version selection API

[Marc Zyngier]

On Tue, 06 Mar 2018 10:12:42 +0000,
peter maydell wrote:
> 
> On 6 March 2018 at 09:50, Marc Zyngier <marc.zyngier@arm.com> wrote:
> > On 05/03/18 20:37, Auger Eric wrote:
> >> On 05/03/18 17:31, Peter Maydell wrote:
> >>> That also means that we will fail migration from a new kernel where
> >>> we've specifically asked for PSCI 0.2 to an old PSCI-0.2-only kernel
> >>> (because the KVM_REG_ARM_PSCI_VERSION reg will appear in the migration
> >>> stream even if its value is the one value that matches the old kernel
> >>> behaviour). I don't know if we care about that.
> >>
> >> Do you know when are we likely to force PSCI 0.2 on a new kernel? At
> >> which layer is the decision supposed to be made and on which criteria?
> >
> > No decent SW should need this. But if you've written a guest that cannot
> > work if it doesn't get "2" as response to PSCI_VERSION, you can override it.
> 
> ...but if you want to be able to migrate back from a new kernel to
> an old one, then you need to ask the new kernel for 0.2 so it
> behaves the same way as the old one. As it stands this code wouldn't
> let you do that migration even if you did specifically ask for 0.2.
> (As I said, I don't know if we care about that.)

Absolutely. The moment we introduce a new sysreg, we create a
migration barrier. I'm not sure how the kernel can help in this
respect.

	M.

-- 
Jazz is not dead, it just smell funny.

Re: [REPOST PATCH] arm/arm64: KVM: Add PSCI version selection API

[Marc Zyngier]

On 06/03/18 09:21, Andrew Jones wrote:
> On Mon, Mar 05, 2018 at 04:47:55PM +0000, Peter Maydell wrote:
>> On 2 March 2018 at 11:11, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>> On Fri, 02 Mar 2018 10:44:48 +0000,
>>> Auger Eric wrote:
>>>> I understand the get/set is called as part of the migration process.
>>>> So my understanding is the benefit of this series is migration fails in
>>>> those cases:
>>>>
>>>>> =0.2 source -> 0.1 destination
>>>> 0.1 source -> >=0.2 destination
>>>
>>> It also fails in the case where you migrate a 1.0 guest to something
>>> that cannot support it.
>>
>> I think it would be useful if we could write out the various
>> combinations of source, destination and what we expect/want to
>> have happen. My gut feeling here is that we're sacrificing
>> exact migration compatibility in favour of having the guest
>> automatically get the variant-2 mitigations, but it's not clear
>> to me exactly which migration combinations that's intended to
>> happen for. Marc?
>>
>> If this wasn't a mitigation issue the desired behaviour would be
>> straightforward:
>>  * kernel should default to 0.2 on the basis that
>>    that's what it did before
>>  * new QEMU version should enable 1.0 by default for virt-2.12
>>    and 0.2 for virt-2.11 and earlier
>>  * PSCI version info shouldn't appear in migration stream unless
>>    it's something other than 0.2
>> But that would leave some setups (which?) unnecessarily without the
>> mitigation, so we're not doing that. The question is, exactly
>> what *are* we aiming for?
> 
> The reason Marc dropped this patch from the series it was first introduced
> in was because we didn't have the aim 100% understood. We want the
> mitigation by default, but also to have the least chance of migration
> failure, and when we must fail (because we're not doing the
> straightforward approach listed above, which would prevent failures), then
> we want to fail with the least amount of damage to the user.
> 
> I experimented with a couple different approaches and provided tables[1]
> with my results. I even recommended an approach, but I may have changed
> my mind after reading Marc's follow-up[2]. The thread continues from
> there as well with follow-ups from Christoffer, Marc, and myself. Anyway,
> Marc did this repost for us to debate it and work out the best approach
> here.
It doesn't look like we've made much progress on this, which makes me
think that we probably don't need anything of the like.

Going, going... Gone?

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [REPOST PATCH] arm/arm64: KVM: Add PSCI version selection API

[Peter Maydell]

On 15 March 2018 at 19:00, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 06/03/18 09:21, Andrew Jones wrote:
>> On Mon, Mar 05, 2018 at 04:47:55PM +0000, Peter Maydell wrote:
>>> On 2 March 2018 at 11:11, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>>> On Fri, 02 Mar 2018 10:44:48 +0000,
>>>> Auger Eric wrote:
>>>>> I understand the get/set is called as part of the migration process.
>>>>> So my understanding is the benefit of this series is migration fails in
>>>>> those cases:
>>>>>
>>>>>> =0.2 source -> 0.1 destination
>>>>> 0.1 source -> >=0.2 destination
>>>>
>>>> It also fails in the case where you migrate a 1.0 guest to something
>>>> that cannot support it.
>>>
>>> I think it would be useful if we could write out the various
>>> combinations of source, destination and what we expect/want to
>>> have happen. My gut feeling here is that we're sacrificing
>>> exact migration compatibility in favour of having the guest
>>> automatically get the variant-2 mitigations, but it's not clear
>>> to me exactly which migration combinations that's intended to
>>> happen for. Marc?
>>>
>>> If this wasn't a mitigation issue the desired behaviour would be
>>> straightforward:
>>>  * kernel should default to 0.2 on the basis that
>>>    that's what it did before
>>>  * new QEMU version should enable 1.0 by default for virt-2.12
>>>    and 0.2 for virt-2.11 and earlier
>>>  * PSCI version info shouldn't appear in migration stream unless
>>>    it's something other than 0.2
>>> But that would leave some setups (which?) unnecessarily without the
>>> mitigation, so we're not doing that. The question is, exactly
>>> what *are* we aiming for?
>>
>> The reason Marc dropped this patch from the series it was first introduced
>> in was because we didn't have the aim 100% understood. We want the
>> mitigation by default, but also to have the least chance of migration
>> failure, and when we must fail (because we're not doing the
>> straightforward approach listed above, which would prevent failures), then
>> we want to fail with the least amount of damage to the user.
>>
>> I experimented with a couple different approaches and provided tables[1]
>> with my results. I even recommended an approach, but I may have changed
>> my mind after reading Marc's follow-up[2]. The thread continues from
>> there as well with follow-ups from Christoffer, Marc, and myself. Anyway,
>> Marc did this repost for us to debate it and work out the best approach
>> here.
> It doesn't look like we've made much progress on this, which makes me
> think that we probably don't need anything of the like.

I was waiting for a better explanation from you of what we're trying to
achieve. If you want to take the "do nothing" approach then a list
also of what migrations succeed/fail/break in that case would also
be useful.

(I am somewhat lazily trying to avoid having to spend time reverse
engineering the "what are we trying to do and what effects are
we accepting" parts from the patch and the code that's already gone
into the kernel.)

thanks
-- PMM

Re: [REPOST PATCH] arm/arm64: KVM: Add PSCI version selection API

[Marc Zyngier]

On 15/03/18 19:13, Peter Maydell wrote:
> On 15 March 2018 at 19:00, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> On 06/03/18 09:21, Andrew Jones wrote:
>>> On Mon, Mar 05, 2018 at 04:47:55PM +0000, Peter Maydell wrote:
>>>> On 2 March 2018 at 11:11, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>>>> On Fri, 02 Mar 2018 10:44:48 +0000,
>>>>> Auger Eric wrote:
>>>>>> I understand the get/set is called as part of the migration process.
>>>>>> So my understanding is the benefit of this series is migration fails in
>>>>>> those cases:
>>>>>>
>>>>>>> =0.2 source -> 0.1 destination
>>>>>> 0.1 source -> >=0.2 destination
>>>>>
>>>>> It also fails in the case where you migrate a 1.0 guest to something
>>>>> that cannot support it.
>>>>
>>>> I think it would be useful if we could write out the various
>>>> combinations of source, destination and what we expect/want to
>>>> have happen. My gut feeling here is that we're sacrificing
>>>> exact migration compatibility in favour of having the guest
>>>> automatically get the variant-2 mitigations, but it's not clear
>>>> to me exactly which migration combinations that's intended to
>>>> happen for. Marc?
>>>>
>>>> If this wasn't a mitigation issue the desired behaviour would be
>>>> straightforward:
>>>>  * kernel should default to 0.2 on the basis that
>>>>    that's what it did before
>>>>  * new QEMU version should enable 1.0 by default for virt-2.12
>>>>    and 0.2 for virt-2.11 and earlier
>>>>  * PSCI version info shouldn't appear in migration stream unless
>>>>    it's something other than 0.2
>>>> But that would leave some setups (which?) unnecessarily without the
>>>> mitigation, so we're not doing that. The question is, exactly
>>>> what *are* we aiming for?
>>>
>>> The reason Marc dropped this patch from the series it was first introduced
>>> in was because we didn't have the aim 100% understood. We want the
>>> mitigation by default, but also to have the least chance of migration
>>> failure, and when we must fail (because we're not doing the
>>> straightforward approach listed above, which would prevent failures), then
>>> we want to fail with the least amount of damage to the user.
>>>
>>> I experimented with a couple different approaches and provided tables[1]
>>> with my results. I even recommended an approach, but I may have changed
>>> my mind after reading Marc's follow-up[2]. The thread continues from
>>> there as well with follow-ups from Christoffer, Marc, and myself. Anyway,
>>> Marc did this repost for us to debate it and work out the best approach
>>> here.
>> It doesn't look like we've made much progress on this, which makes me
>> think that we probably don't need anything of the like.
> 
> I was waiting for a better explanation from you of what we're trying to
> achieve. If you want to take the "do nothing" approach then a list
> also of what migrations succeed/fail/break in that case would also
> be useful.
> 
> (I am somewhat lazily trying to avoid having to spend time reverse
> engineering the "what are we trying to do and what effects are
> we accepting" parts from the patch and the code that's already gone
> into the kernel.)

OK, let me (re)state the problem:

For a guest that requests PSCI 0.2 (i.e. all guests from the past 4 or 5
years), we now silently upgrade the PSCI version to 1.0 allowing the new
SMCCC to be discovered, and the ARCH_WORKAROUND_1 service to be called.

Things get funny, specially with migration (and the way QEMU works).

If we "do nothing":

(1) A guest migrating from an "old" host to a "new" host will silently
see its PSCI version upgraded. Not a big deal in my opinion, as 1.0 is a
strict superset of 0.2 (apart from the version number...).

(2) A guest migrating from a "new" host to an "old" host will silently
loose its Spectre v2 mitigation. That's quite a big deal.

(3, not related to migration) A guest having a hardcoded knowledge of
PSCI 0.2 will se that we've changed something, and may decide to catch
fire. Oh well.

If we take this patch:

(1) still exists

(2) will now fail to migrate. I see this as a feature.

(3) can be worked around by setting the "PSCI version pseudo register"
to 0.2.

These are the main things I can think of at the moment.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [REPOST PATCH] arm/arm64: KVM: Add PSCI version selection API

[Christoffer Dall]

On Thu, Mar 15, 2018 at 07:26:48PM +0000, Marc Zyngier wrote:
> On 15/03/18 19:13, Peter Maydell wrote:
> > On 15 March 2018 at 19:00, Marc Zyngier <marc.zyngier@arm.com> wrote:
> >> On 06/03/18 09:21, Andrew Jones wrote:
> >>> On Mon, Mar 05, 2018 at 04:47:55PM +0000, Peter Maydell wrote:
> >>>> On 2 March 2018 at 11:11, Marc Zyngier <marc.zyngier@arm.com> wrote:
> >>>>> On Fri, 02 Mar 2018 10:44:48 +0000,
> >>>>> Auger Eric wrote:
> >>>>>> I understand the get/set is called as part of the migration process.
> >>>>>> So my understanding is the benefit of this series is migration fails in
> >>>>>> those cases:
> >>>>>>
> >>>>>>> =0.2 source -> 0.1 destination
> >>>>>> 0.1 source -> >=0.2 destination
> >>>>>
> >>>>> It also fails in the case where you migrate a 1.0 guest to something
> >>>>> that cannot support it.
> >>>>
> >>>> I think it would be useful if we could write out the various
> >>>> combinations of source, destination and what we expect/want to
> >>>> have happen. My gut feeling here is that we're sacrificing
> >>>> exact migration compatibility in favour of having the guest
> >>>> automatically get the variant-2 mitigations, but it's not clear
> >>>> to me exactly which migration combinations that's intended to
> >>>> happen for. Marc?
> >>>>
> >>>> If this wasn't a mitigation issue the desired behaviour would be
> >>>> straightforward:
> >>>>  * kernel should default to 0.2 on the basis that
> >>>>    that's what it did before
> >>>>  * new QEMU version should enable 1.0 by default for virt-2.12
> >>>>    and 0.2 for virt-2.11 and earlier
> >>>>  * PSCI version info shouldn't appear in migration stream unless
> >>>>    it's something other than 0.2
> >>>> But that would leave some setups (which?) unnecessarily without the
> >>>> mitigation, so we're not doing that. The question is, exactly
> >>>> what *are* we aiming for?
> >>>
> >>> The reason Marc dropped this patch from the series it was first introduced
> >>> in was because we didn't have the aim 100% understood. We want the
> >>> mitigation by default, but also to have the least chance of migration
> >>> failure, and when we must fail (because we're not doing the
> >>> straightforward approach listed above, which would prevent failures), then
> >>> we want to fail with the least amount of damage to the user.
> >>>
> >>> I experimented with a couple different approaches and provided tables[1]
> >>> with my results. I even recommended an approach, but I may have changed
> >>> my mind after reading Marc's follow-up[2]. The thread continues from
> >>> there as well with follow-ups from Christoffer, Marc, and myself. Anyway,
> >>> Marc did this repost for us to debate it and work out the best approach
> >>> here.
> >> It doesn't look like we've made much progress on this, which makes me
> >> think that we probably don't need anything of the like.
> > 
> > I was waiting for a better explanation from you of what we're trying to
> > achieve. If you want to take the "do nothing" approach then a list
> > also of what migrations succeed/fail/break in that case would also
> > be useful.
> > 
> > (I am somewhat lazily trying to avoid having to spend time reverse
> > engineering the "what are we trying to do and what effects are
> > we accepting" parts from the patch and the code that's already gone
> > into the kernel.)
> 
> OK, let me (re)state the problem:
> 
> For a guest that requests PSCI 0.2 (i.e. all guests from the past 4 or 5
> years), we now silently upgrade the PSCI version to 1.0 allowing the new
> SMCCC to be discovered, and the ARCH_WORKAROUND_1 service to be called.
> 
> Things get funny, specially with migration (and the way QEMU works).
> 
> If we "do nothing":
> 
> (1) A guest migrating from an "old" host to a "new" host will silently
> see its PSCI version upgraded. Not a big deal in my opinion, as 1.0 is a
> strict superset of 0.2 (apart from the version number...).
> 
> (2) A guest migrating from a "new" host to an "old" host will silently
> loose its Spectre v2 mitigation. That's quite a big deal.
> 
> (3, not related to migration) A guest having a hardcoded knowledge of
> PSCI 0.2 will se that we've changed something, and may decide to catch
> fire. Oh well.
> 
> If we take this patch:
> 
> (1) still exists

No problem, IMHO.

> 
> (2) will now fail to migrate. I see this as a feature.

Yes, I agree.  This is actually the most important reason for doing
anything beyond what's already merged.

> 
> (3) can be worked around by setting the "PSCI version pseudo register"
> to 0.2.

Nice to have, but we're probably not expecting this to be of major
concern.  I initially thought it was a nice debugging feature as well,
but that may be a ridiculous point.

> 
> These are the main things I can think of at the moment.

So I think we we should merge this patch.

If userspace then wants to support "migrate from explicitly set v0.2 new
kernel to old kernel", then it must add specific support to filter out
the register from the register list; not that I think anyone will need
that or bother to implement it.

In other words, I think you should merge this:

Reviewed-by: Christoffer Dall <cdall@kernel.org>

Re: [REPOST PATCH] arm/arm64: KVM: Add PSCI version selection API

[Marc Zyngier]

+Drew, who's look at the whole save/restore thing extensively

On 09/04/18 13:30, Christoffer Dall wrote:
> On Thu, Mar 15, 2018 at 07:26:48PM +0000, Marc Zyngier wrote:
>> On 15/03/18 19:13, Peter Maydell wrote:
>>> On 15 March 2018 at 19:00, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>>> On 06/03/18 09:21, Andrew Jones wrote:
>>>>> On Mon, Mar 05, 2018 at 04:47:55PM +0000, Peter Maydell wrote:
>>>>>> On 2 March 2018 at 11:11, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>>>>>> On Fri, 02 Mar 2018 10:44:48 +0000,
>>>>>>> Auger Eric wrote:
>>>>>>>> I understand the get/set is called as part of the migration process.
>>>>>>>> So my understanding is the benefit of this series is migration fails in
>>>>>>>> those cases:
>>>>>>>>
>>>>>>>>> =0.2 source -> 0.1 destination
>>>>>>>> 0.1 source -> >=0.2 destination
>>>>>>>
>>>>>>> It also fails in the case where you migrate a 1.0 guest to something
>>>>>>> that cannot support it.
>>>>>>
>>>>>> I think it would be useful if we could write out the various
>>>>>> combinations of source, destination and what we expect/want to
>>>>>> have happen. My gut feeling here is that we're sacrificing
>>>>>> exact migration compatibility in favour of having the guest
>>>>>> automatically get the variant-2 mitigations, but it's not clear
>>>>>> to me exactly which migration combinations that's intended to
>>>>>> happen for. Marc?
>>>>>>
>>>>>> If this wasn't a mitigation issue the desired behaviour would be
>>>>>> straightforward:
>>>>>>  * kernel should default to 0.2 on the basis that
>>>>>>    that's what it did before
>>>>>>  * new QEMU version should enable 1.0 by default for virt-2.12
>>>>>>    and 0.2 for virt-2.11 and earlier
>>>>>>  * PSCI version info shouldn't appear in migration stream unless
>>>>>>    it's something other than 0.2
>>>>>> But that would leave some setups (which?) unnecessarily without the
>>>>>> mitigation, so we're not doing that. The question is, exactly
>>>>>> what *are* we aiming for?
>>>>>
>>>>> The reason Marc dropped this patch from the series it was first introduced
>>>>> in was because we didn't have the aim 100% understood. We want the
>>>>> mitigation by default, but also to have the least chance of migration
>>>>> failure, and when we must fail (because we're not doing the
>>>>> straightforward approach listed above, which would prevent failures), then
>>>>> we want to fail with the least amount of damage to the user.
>>>>>
>>>>> I experimented with a couple different approaches and provided tables[1]
>>>>> with my results. I even recommended an approach, but I may have changed
>>>>> my mind after reading Marc's follow-up[2]. The thread continues from
>>>>> there as well with follow-ups from Christoffer, Marc, and myself. Anyway,
>>>>> Marc did this repost for us to debate it and work out the best approach
>>>>> here.
>>>> It doesn't look like we've made much progress on this, which makes me
>>>> think that we probably don't need anything of the like.
>>>
>>> I was waiting for a better explanation from you of what we're trying to
>>> achieve. If you want to take the "do nothing" approach then a list
>>> also of what migrations succeed/fail/break in that case would also
>>> be useful.
>>>
>>> (I am somewhat lazily trying to avoid having to spend time reverse
>>> engineering the "what are we trying to do and what effects are
>>> we accepting" parts from the patch and the code that's already gone
>>> into the kernel.)
>>
>> OK, let me (re)state the problem:
>>
>> For a guest that requests PSCI 0.2 (i.e. all guests from the past 4 or 5
>> years), we now silently upgrade the PSCI version to 1.0 allowing the new
>> SMCCC to be discovered, and the ARCH_WORKAROUND_1 service to be called.
>>
>> Things get funny, specially with migration (and the way QEMU works).
>>
>> If we "do nothing":
>>
>> (1) A guest migrating from an "old" host to a "new" host will silently
>> see its PSCI version upgraded. Not a big deal in my opinion, as 1.0 is a
>> strict superset of 0.2 (apart from the version number...).
>>
>> (2) A guest migrating from a "new" host to an "old" host will silently
>> loose its Spectre v2 mitigation. That's quite a big deal.
>>
>> (3, not related to migration) A guest having a hardcoded knowledge of
>> PSCI 0.2 will se that we've changed something, and may decide to catch
>> fire. Oh well.
>>
>> If we take this patch:
>>
>> (1) still exists
> 
> No problem, IMHO.
> 
>>
>> (2) will now fail to migrate. I see this as a feature.
> 
> Yes, I agree.  This is actually the most important reason for doing
> anything beyond what's already merged.

Indeed, and that's the reason I wrote this patch the first place.

> 
>>
>> (3) can be worked around by setting the "PSCI version pseudo register"
>> to 0.2.
> 
> Nice to have, but we're probably not expecting this to be of major
> concern.  I initially thought it was a nice debugging feature as well,
> but that may be a ridiculous point.
> 
>>
>> These are the main things I can think of at the moment.
> 
> So I think we we should merge this patch.
> 
> If userspace then wants to support "migrate from explicitly set v0.2 new
> kernel to old kernel", then it must add specific support to filter out
> the register from the register list; not that I think anyone will need
> that or bother to implement it.
> 
> In other words, I think you should merge this:
> 
> Reviewed-by: Christoffer Dall <cdall@kernel.org>
> 

Thanks. One issue is that we've now missed the 4.16 train, and that this
effectively is an ABI change (a fairly minor one, but still). Would we
consider slapping this as a retrospective fix to 4.16-stable, or keep it
as a 4.17 feature?

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [REPOST PATCH] arm/arm64: KVM: Add PSCI version selection API

[Christoffer Dall]

On Mon, Apr 09, 2018 at 01:47:50PM +0100, Marc Zyngier wrote:
> +Drew, who's look at the whole save/restore thing extensively
> 
> On 09/04/18 13:30, Christoffer Dall wrote:
> > On Thu, Mar 15, 2018 at 07:26:48PM +0000, Marc Zyngier wrote:
> >> On 15/03/18 19:13, Peter Maydell wrote:
> >>> On 15 March 2018 at 19:00, Marc Zyngier <marc.zyngier@arm.com> wrote:
> >>>> On 06/03/18 09:21, Andrew Jones wrote:
> >>>>> On Mon, Mar 05, 2018 at 04:47:55PM +0000, Peter Maydell wrote:
> >>>>>> On 2 March 2018 at 11:11, Marc Zyngier <marc.zyngier@arm.com> wrote:
> >>>>>>> On Fri, 02 Mar 2018 10:44:48 +0000,
> >>>>>>> Auger Eric wrote:
> >>>>>>>> I understand the get/set is called as part of the migration process.
> >>>>>>>> So my understanding is the benefit of this series is migration fails in
> >>>>>>>> those cases:
> >>>>>>>>
> >>>>>>>>> =0.2 source -> 0.1 destination
> >>>>>>>> 0.1 source -> >=0.2 destination
> >>>>>>>
> >>>>>>> It also fails in the case where you migrate a 1.0 guest to something
> >>>>>>> that cannot support it.
> >>>>>>
> >>>>>> I think it would be useful if we could write out the various
> >>>>>> combinations of source, destination and what we expect/want to
> >>>>>> have happen. My gut feeling here is that we're sacrificing
> >>>>>> exact migration compatibility in favour of having the guest
> >>>>>> automatically get the variant-2 mitigations, but it's not clear
> >>>>>> to me exactly which migration combinations that's intended to
> >>>>>> happen for. Marc?
> >>>>>>
> >>>>>> If this wasn't a mitigation issue the desired behaviour would be
> >>>>>> straightforward:
> >>>>>>  * kernel should default to 0.2 on the basis that
> >>>>>>    that's what it did before
> >>>>>>  * new QEMU version should enable 1.0 by default for virt-2.12
> >>>>>>    and 0.2 for virt-2.11 and earlier
> >>>>>>  * PSCI version info shouldn't appear in migration stream unless
> >>>>>>    it's something other than 0.2
> >>>>>> But that would leave some setups (which?) unnecessarily without the
> >>>>>> mitigation, so we're not doing that. The question is, exactly
> >>>>>> what *are* we aiming for?
> >>>>>
> >>>>> The reason Marc dropped this patch from the series it was first introduced
> >>>>> in was because we didn't have the aim 100% understood. We want the
> >>>>> mitigation by default, but also to have the least chance of migration
> >>>>> failure, and when we must fail (because we're not doing the
> >>>>> straightforward approach listed above, which would prevent failures), then
> >>>>> we want to fail with the least amount of damage to the user.
> >>>>>
> >>>>> I experimented with a couple different approaches and provided tables[1]
> >>>>> with my results. I even recommended an approach, but I may have changed
> >>>>> my mind after reading Marc's follow-up[2]. The thread continues from
> >>>>> there as well with follow-ups from Christoffer, Marc, and myself. Anyway,
> >>>>> Marc did this repost for us to debate it and work out the best approach
> >>>>> here.
> >>>> It doesn't look like we've made much progress on this, which makes me
> >>>> think that we probably don't need anything of the like.
> >>>
> >>> I was waiting for a better explanation from you of what we're trying to
> >>> achieve. If you want to take the "do nothing" approach then a list
> >>> also of what migrations succeed/fail/break in that case would also
> >>> be useful.
> >>>
> >>> (I am somewhat lazily trying to avoid having to spend time reverse
> >>> engineering the "what are we trying to do and what effects are
> >>> we accepting" parts from the patch and the code that's already gone
> >>> into the kernel.)
> >>
> >> OK, let me (re)state the problem:
> >>
> >> For a guest that requests PSCI 0.2 (i.e. all guests from the past 4 or 5
> >> years), we now silently upgrade the PSCI version to 1.0 allowing the new
> >> SMCCC to be discovered, and the ARCH_WORKAROUND_1 service to be called.
> >>
> >> Things get funny, specially with migration (and the way QEMU works).
> >>
> >> If we "do nothing":
> >>
> >> (1) A guest migrating from an "old" host to a "new" host will silently
> >> see its PSCI version upgraded. Not a big deal in my opinion, as 1.0 is a
> >> strict superset of 0.2 (apart from the version number...).
> >>
> >> (2) A guest migrating from a "new" host to an "old" host will silently
> >> loose its Spectre v2 mitigation. That's quite a big deal.
> >>
> >> (3, not related to migration) A guest having a hardcoded knowledge of
> >> PSCI 0.2 will se that we've changed something, and may decide to catch
> >> fire. Oh well.
> >>
> >> If we take this patch:
> >>
> >> (1) still exists
> > 
> > No problem, IMHO.
> > 
> >>
> >> (2) will now fail to migrate. I see this as a feature.
> > 
> > Yes, I agree.  This is actually the most important reason for doing
> > anything beyond what's already merged.
> 
> Indeed, and that's the reason I wrote this patch the first place.
> 
> > 
> >>
> >> (3) can be worked around by setting the "PSCI version pseudo register"
> >> to 0.2.
> > 
> > Nice to have, but we're probably not expecting this to be of major
> > concern.  I initially thought it was a nice debugging feature as well,
> > but that may be a ridiculous point.
> > 
> >>
> >> These are the main things I can think of at the moment.
> > 
> > So I think we we should merge this patch.
> > 
> > If userspace then wants to support "migrate from explicitly set v0.2 new
> > kernel to old kernel", then it must add specific support to filter out
> > the register from the register list; not that I think anyone will need
> > that or bother to implement it.
> > 
> > In other words, I think you should merge this:
> > 
> > Reviewed-by: Christoffer Dall <cdall@kernel.org>
> > 
> 
> Thanks. One issue is that we've now missed the 4.16 train, and that this
> effectively is an ABI change (a fairly minor one, but still). Would we
> consider slapping this as a retrospective fix to 4.16-stable, or keep it
> as a 4.17 feature?

Given that it fixes a potentially dangerous migration, and it's a fairly
simple patch, I think it's reasonable to apply as a fix to the next 4.16
release.  Would we be violating any hard-set rules in doing so?

Thanks,
-Christoffer

Re: [REPOST PATCH] arm/arm64: KVM: Add PSCI version selection API

[Marc Zyngier]

On 09/04/18 14:05, Christoffer Dall wrote:
> On Mon, Apr 09, 2018 at 01:47:50PM +0100, Marc Zyngier wrote:
>> +Drew, who's look at the whole save/restore thing extensively
>>
>> On 09/04/18 13:30, Christoffer Dall wrote:
>>> On Thu, Mar 15, 2018 at 07:26:48PM +0000, Marc Zyngier wrote:
>>>> On 15/03/18 19:13, Peter Maydell wrote:
>>>>> On 15 March 2018 at 19:00, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>>>>> On 06/03/18 09:21, Andrew Jones wrote:
>>>>>>> On Mon, Mar 05, 2018 at 04:47:55PM +0000, Peter Maydell wrote:
>>>>>>>> On 2 March 2018 at 11:11, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>>>>>>>> On Fri, 02 Mar 2018 10:44:48 +0000,
>>>>>>>>> Auger Eric wrote:
>>>>>>>>>> I understand the get/set is called as part of the migration process.
>>>>>>>>>> So my understanding is the benefit of this series is migration fails in
>>>>>>>>>> those cases:
>>>>>>>>>>
>>>>>>>>>>> =0.2 source -> 0.1 destination
>>>>>>>>>> 0.1 source -> >=0.2 destination
>>>>>>>>>
>>>>>>>>> It also fails in the case where you migrate a 1.0 guest to something
>>>>>>>>> that cannot support it.
>>>>>>>>
>>>>>>>> I think it would be useful if we could write out the various
>>>>>>>> combinations of source, destination and what we expect/want to
>>>>>>>> have happen. My gut feeling here is that we're sacrificing
>>>>>>>> exact migration compatibility in favour of having the guest
>>>>>>>> automatically get the variant-2 mitigations, but it's not clear
>>>>>>>> to me exactly which migration combinations that's intended to
>>>>>>>> happen for. Marc?
>>>>>>>>
>>>>>>>> If this wasn't a mitigation issue the desired behaviour would be
>>>>>>>> straightforward:
>>>>>>>>  * kernel should default to 0.2 on the basis that
>>>>>>>>    that's what it did before
>>>>>>>>  * new QEMU version should enable 1.0 by default for virt-2.12
>>>>>>>>    and 0.2 for virt-2.11 and earlier
>>>>>>>>  * PSCI version info shouldn't appear in migration stream unless
>>>>>>>>    it's something other than 0.2
>>>>>>>> But that would leave some setups (which?) unnecessarily without the
>>>>>>>> mitigation, so we're not doing that. The question is, exactly
>>>>>>>> what *are* we aiming for?
>>>>>>>
>>>>>>> The reason Marc dropped this patch from the series it was first introduced
>>>>>>> in was because we didn't have the aim 100% understood. We want the
>>>>>>> mitigation by default, but also to have the least chance of migration
>>>>>>> failure, and when we must fail (because we're not doing the
>>>>>>> straightforward approach listed above, which would prevent failures), then
>>>>>>> we want to fail with the least amount of damage to the user.
>>>>>>>
>>>>>>> I experimented with a couple different approaches and provided tables[1]
>>>>>>> with my results. I even recommended an approach, but I may have changed
>>>>>>> my mind after reading Marc's follow-up[2]. The thread continues from
>>>>>>> there as well with follow-ups from Christoffer, Marc, and myself. Anyway,
>>>>>>> Marc did this repost for us to debate it and work out the best approach
>>>>>>> here.
>>>>>> It doesn't look like we've made much progress on this, which makes me
>>>>>> think that we probably don't need anything of the like.
>>>>>
>>>>> I was waiting for a better explanation from you of what we're trying to
>>>>> achieve. If you want to take the "do nothing" approach then a list
>>>>> also of what migrations succeed/fail/break in that case would also
>>>>> be useful.
>>>>>
>>>>> (I am somewhat lazily trying to avoid having to spend time reverse
>>>>> engineering the "what are we trying to do and what effects are
>>>>> we accepting" parts from the patch and the code that's already gone
>>>>> into the kernel.)
>>>>
>>>> OK, let me (re)state the problem:
>>>>
>>>> For a guest that requests PSCI 0.2 (i.e. all guests from the past 4 or 5
>>>> years), we now silently upgrade the PSCI version to 1.0 allowing the new
>>>> SMCCC to be discovered, and the ARCH_WORKAROUND_1 service to be called.
>>>>
>>>> Things get funny, specially with migration (and the way QEMU works).
>>>>
>>>> If we "do nothing":
>>>>
>>>> (1) A guest migrating from an "old" host to a "new" host will silently
>>>> see its PSCI version upgraded. Not a big deal in my opinion, as 1.0 is a
>>>> strict superset of 0.2 (apart from the version number...).
>>>>
>>>> (2) A guest migrating from a "new" host to an "old" host will silently
>>>> loose its Spectre v2 mitigation. That's quite a big deal.
>>>>
>>>> (3, not related to migration) A guest having a hardcoded knowledge of
>>>> PSCI 0.2 will se that we've changed something, and may decide to catch
>>>> fire. Oh well.
>>>>
>>>> If we take this patch:
>>>>
>>>> (1) still exists
>>>
>>> No problem, IMHO.
>>>
>>>>
>>>> (2) will now fail to migrate. I see this as a feature.
>>>
>>> Yes, I agree.  This is actually the most important reason for doing
>>> anything beyond what's already merged.
>>
>> Indeed, and that's the reason I wrote this patch the first place.
>>
>>>
>>>>
>>>> (3) can be worked around by setting the "PSCI version pseudo register"
>>>> to 0.2.
>>>
>>> Nice to have, but we're probably not expecting this to be of major
>>> concern.  I initially thought it was a nice debugging feature as well,
>>> but that may be a ridiculous point.
>>>
>>>>
>>>> These are the main things I can think of at the moment.
>>>
>>> So I think we we should merge this patch.
>>>
>>> If userspace then wants to support "migrate from explicitly set v0.2 new
>>> kernel to old kernel", then it must add specific support to filter out
>>> the register from the register list; not that I think anyone will need
>>> that or bother to implement it.
>>>
>>> In other words, I think you should merge this:
>>>
>>> Reviewed-by: Christoffer Dall <cdall@kernel.org>
>>>
>>
>> Thanks. One issue is that we've now missed the 4.16 train, and that this
>> effectively is an ABI change (a fairly minor one, but still). Would we
>> consider slapping this as a retrospective fix to 4.16-stable, or keep it
>> as a 4.17 feature?
> 
> Given that it fixes a potentially dangerous migration, and it's a fairly
> simple patch, I think it's reasonable to apply as a fix to the next 4.16
> release.  Would we be violating any hard-set rules in doing so?

I don't think so, but I'd welcome comments on it.

If nobody shouts by the end of the week, I'll send it in as a fix for
4.17, earmarked for 4.16 backport.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...