[RFC PATCH v2 0/3] KVM: arm64: Errata management for VM Live migration

[RFC PATCH v2 0/3] KVM: arm64: Errata management for VM Live migration

[Shameer Kolothum]

This is another attempt on this based on the feedback received
for RFC v1. Thanks for all the discussions and suggestions.

Changes from RFC v1[0]:
 -Introduced hypercalls to retrieve target CPUs info from user space VMM.
  see patch #1 for details.
 -Patch #2 uses the hypercall to retrieve the target CPU info if any.
 -Use the target CPUs MIDR/REVIDR in errata enablement. See patch #3.

Sanity tested on a HiSilicon ARM64 platform using a hacked Qemu that can
handle the above-mentioned hypercalls.

Please take a look and let me know if this is in the right direction or
not.

Also TBD:
1. Do we need some way of notifying the userspace VMM in case the Guest
   fails to update target CPUs?
2. Usage of synthetic MIDR mentioned in RFC v1 discussion. How do we
   envision the synthetic MIDR to be used/supported?
   1. VMM writes synthetic MIDR to KVM(SET_ONE_REG)
   2. Uses Guest hypercall to update the target CPUs info.

   If this is the case what happens if an old Guest (without hypercall
   support) is used?  VMM will abort the Guest if no hypercall is 
   received but a synthetic MIDR is set?

Thanks,
Shameer

[0]https://lore.kernel.org/kvmarm/20241011075053.80540-1-shameerali.kolothum.thodi@huawei.com/

Background from v1:

On ARM64 platforms most of the errata workarounds are based on CPU
MIDR/REVIDR values and a number of these workarounds need to be
implemented by the Guest kernel as well. This creates a problem when
Guest needs to be migrated to a platform that differs in these
MIDR/REVIDR values even if the VMM can come up with a common minimum
feature list for the Guest using the recently introduced "Writable
ID registers" support.

(This is roughly based on a discussion I had with Marc and Oliver
at KVM forum. Marc outlined his idea for a solution and this is an
attempt to implement it. Thanks to both and I take all the blame
if this is nowhere near what is intended/required)

Shameer Kolothum (3):
  KVM: arm64: Add hypercall support for retrieving migration targets
  KVM: arm64: Use hypercall to retrieve any migration targets
  KVM: arm64: Enable errata based on migration target CPUs

 Documentation/virt/kvm/arm/hypercalls.rst | 77 +++++++++++++++++++++++
 arch/arm64/include/asm/cpufeature.h       |  7 +++
 arch/arm64/include/asm/paravirt.h         |  3 +
 arch/arm64/kernel/cpu_errata.c            | 49 ++++++++++++---
 arch/arm64/kernel/cpufeature.c            |  3 +
 arch/arm64/kernel/paravirt.c              | 48 ++++++++++++++
 include/linux/arm-smccc.h                 | 13 ++++
 7 files changed, 190 insertions(+), 10 deletions(-)

-- 
2.45.2

[RFC PATCH v2 1/3] KVM: arm64: Add hypercall support for retrieving migration targets

[Shameer Kolothum]

If the VM requires migration to multiple targets, these hypercalls
will provide a way to inform the Guest kernel about those targets.
These hypercalls must be handled by userspace VMM to provide the
required details. In subsequent patches Guest kernel will use
these hypercalls to retrieve the target CPU information.

Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 Documentation/virt/kvm/arm/hypercalls.rst | 77 +++++++++++++++++++++++
 include/linux/arm-smccc.h                 | 13 ++++
 2 files changed, 90 insertions(+)

diff --git a/Documentation/virt/kvm/arm/hypercalls.rst b/Documentation/virt/kvm/arm/hypercalls.rst
index af7bc2c2e0cb..ebd59fb72567 100644
--- a/Documentation/virt/kvm/arm/hypercalls.rst
+++ b/Documentation/virt/kvm/arm/hypercalls.rst
@@ -142,3 +142,80 @@ region is equal to the memory protection granule advertised by
 |                     |          |    +---------------------------------------------+
 |                     |          |    | ``INVALID_PARAMETER (-3)``                  |
 +---------------------+----------+----+---------------------------------------------+
+
+``ARM_SMCCC_VENDOR_HYP_KVM_MIGRN_TARGET_NUM_FUNC_ID``
+------------------------------------------------------
+
+Query total number of migration target CPUs the Guest VM will be running during its
+lifetime and version information applicable to the data format used for
+``ARM_SMCCC_VENDOR_HYP_KVM_MIGRN_TARGET_CPUS_FUNC_ID``. The maximum number of targets
+supported is 64. Also the version number supported currently is 1.0. This hypercall
+must be handled by the userspace VMM.
+
++---------------------+-------------------------------------------------------------+
+| Presence:           | Optional; KVM/ARM64 Guests only                             |
++---------------------+-------------------------------------------------------------+
+| Calling convention: | HVC64                                                       |
++---------------------+----------+--------------------------------------------------+
+| Function ID:        | (uint32) | 0xC600007D                                       |
++---------------------+----------+--------------------------------------------------+
+| Arguments:          | None                                                        |
++---------------------+----------+----+---------------------------------------------+
+| Return Values:      | (int64)  | R0 | ``NOT_SUPPORTED (-1)`` on error, else       |
+|                     |          |    | [0-31] total migration targets              |
+|                     |          |    | [32-63] version number                      |
++---------------------+----------+----+---------------------------------------------+
+|                     | (uint64) | R1 | Reserved / Must be zero                     |
+|                     +----------+----+---------------------------------------------+
+|                     | (uint64) | R2 | Reserved / Must be zero                     |
++---------------------+----------+----+---------------------------------------------+
+|                     | (uint64) | R3 | Reserved / Must be zero                     |
++---------------------+----------+----+---------------------------------------------+
+
+
+``ARM_SMCCC_VENDOR_HYP_KVM_MIGRN_TARGET_CPUS_FUNC_ID``
+-------------------------------------------------------
+
+Request migration target CPU information for the Guest VM. The information must be
+provided as per the format described by the version info in
+``ARM_SMCCC_VENDOR_HYP_KVM_MIGRN_TARGET_NUM_FUNC_ID``. At present, we only support
+the below format which corresponds to version 1.0. This hypercall will always be
+preceded by ``ARM_SMCCC_VENDOR_HYP_KVM_MIGRN_TARGET_NUM_FUNC_ID`` and may be
+invoked multiple times to retrieve the total number of target CPUs information
+advertised. This hypercall must be handled by the userspace VMM.
+
+A typical userspace usage scenario will be like below:
+
+1. Receives ``ARM_SMCCC_VENDOR_HYP_KVM_MIGRN_TARGET_NUM_FUNC_ID``
+
+   * Returns total number of migration targets and version number
+   * Reset current target index to zero
+2. Receives ``ARM_SMCCC_VENDOR_HYP_KVM_MIGRN_TARGET_CPUS_FUNC_ID``
+
+   * Returns MIDR/REVIDR info in return register fields. Can return up to 4
+   * Update current target index based on returned target info
+   * If there are remaining register fields, return zero to indicate the end
+   * Repeat step 2 until current target index == total number of migration targets
+
++---------------------+-------------------------------------------------------------+
+| Presence:           | Optional; KVM/ARM64 Guests only                             |
++---------------------+-------------------------------------------------------------+
+| Calling convention: | HVC64                                                       |
++---------------------+----------+--------------------------------------------------+
+| Function ID:        | (uint32) | 0xC600007E                                       |
++---------------------+----------+----+---------------------------------------------+
+| Arguments:          | None                                                        |
++---------------------+----------+----+---------------------------------------------+
+| Return Values:      | (int64)  | R0 | ``NOT_SUPPORTED (-1)`` on error, else       |
+|                     |          |    | [0-31] MIDR, [31-63] REVIDR, else           |
+|                     |          |    | [0-63] Zero to mark end.                    |
++---------------------+----------+----+---------------------------------------------+
+|                     | (uint64) | R1 | [0-31] MIDR, [32-63] REVIDR, else           |
+|                     |          |    | [0-63] Zero to mark end.                    |
++---------------------+----------+----+---------------------------------------------+
+|                     | (uint64) | R2 | [0-31] MIDR, [32-63] REVIDR, else           |
+|                     |          |    | [0-63] Zero to mark end.                    |
++---------------------+----------+----+---------------------------------------------+
+|                     | (uint64) | R3 | [0-31] MIDR, [32-63] REVIDR, else           |
+|                     |          |    | [0-63] Zero to mark end.                    |
++---------------------+----------+----+---------------------------------------------+
diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
index f59099a213d0..df87ce48ae9e 100644
--- a/include/linux/arm-smccc.h
+++ b/include/linux/arm-smccc.h
@@ -179,6 +179,8 @@
 #define ARM_SMCCC_KVM_FUNC_PKVM_RESV_62		62
 #define ARM_SMCCC_KVM_FUNC_PKVM_RESV_63		63
 /* End of pKVM hypercall range */
+#define ARM_SMCCC_KVM_FUNC_MIGRN_TARGET_NUM	125
+#define ARM_SMCCC_KVM_FUNC_MIGRN_TARGET_CPUS	126
 #define ARM_SMCCC_KVM_FUNC_FEATURES_2		127
 #define ARM_SMCCC_KVM_NUM_FUNCS			128
 
@@ -225,6 +227,17 @@
 			   ARM_SMCCC_OWNER_VENDOR_HYP,			\
 			   ARM_SMCCC_KVM_FUNC_MMIO_GUARD)
 
+#define ARM_SMCCC_VENDOR_HYP_KVM_MIGRN_TARGET_NUM_FUNC_ID	\
+	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,			\
+			   ARM_SMCCC_SMC_64,			\
+			   ARM_SMCCC_OWNER_VENDOR_HYP,		\
+			   ARM_SMCCC_KVM_FUNC_MIGRN_TARGET_NUM)
+
+#define ARM_SMCCC_VENDOR_HYP_KVM_MIGRN_TARGET_CPUS_FUNC_ID	\
+	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,			\
+			   ARM_SMCCC_SMC_64,			\
+			   ARM_SMCCC_OWNER_VENDOR_HYP,		\
+			   ARM_SMCCC_KVM_FUNC_MIGRN_TARGET_CPUS)
 /* ptp_kvm counter type ID */
 #define KVM_PTP_VIRT_COUNTER			0
 #define KVM_PTP_PHYS_COUNTER			1
-- 
2.45.2

[RFC PATCH v2 2/3] KVM: arm64: Use hypercall to retrieve any migration targets

[Shameer Kolothum]

Provides a paravirt helper function to retrieve any migration target
CPUs if available and store it. This will be used in subsequent patch
to enable any errata associated with those targets

Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
ToDo:
Not sure it make sense to use kvm_arm_hyp_service_available() here
as these hypercalls will be handled by userspace VMM.

---
 arch/arm64/include/asm/cpufeature.h |  7 +++++
 arch/arm64/include/asm/paravirt.h   |  3 ++
 arch/arm64/kernel/cpu_errata.c      |  3 ++
 arch/arm64/kernel/paravirt.c        | 48 +++++++++++++++++++++++++++++
 4 files changed, 61 insertions(+)

diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
index 985d966787b1..6623f47814bc 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -386,6 +386,13 @@ struct arm64_cpu_capabilities {
 	const struct cpumask *cpus;
 };
 
+#define MIGRN_TARGET_CPUS_HYP_CALL_VER_1	1
+#define MAX_MIGRN_TARGET_CPUS			64
+struct migrn_target {
+	u32 midr;
+	u32 revidr;
+};
+
 static inline int cpucap_default_scope(const struct arm64_cpu_capabilities *cap)
 {
 	return cap->type & ARM64_CPUCAP_SCOPE_MASK;
diff --git a/arch/arm64/include/asm/paravirt.h b/arch/arm64/include/asm/paravirt.h
index 9aa193e0e8f2..19cd97dafbbc 100644
--- a/arch/arm64/include/asm/paravirt.h
+++ b/arch/arm64/include/asm/paravirt.h
@@ -19,11 +19,14 @@ static inline u64 paravirt_steal_clock(int cpu)
 }
 
 int __init pv_time_init(void);
+void __init pv_errata_migrn_target_init(void);
 
 #else
 
 #define pv_time_init() do {} while (0)
 
+#define pv_errata_migrn_target_init() do {} while (0)
+
 #endif // CONFIG_PARAVIRT
 
 #endif
diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
index dfefbdf4073a..feaaf2b11f46 100644
--- a/arch/arm64/kernel/cpu_errata.c
+++ b/arch/arm64/kernel/cpu_errata.c
@@ -14,6 +14,9 @@
 #include <asm/kvm_asm.h>
 #include <asm/smp_plat.h>
 
+u32 __ro_after_init errata_migrn_target_num;
+struct migrn_target __ro_after_init errata_migrn_target_cpus[MAX_MIGRN_TARGET_CPUS];
+
 static bool __maybe_unused
 is_affected_midr_range(const struct arm64_cpu_capabilities *entry, int scope)
 {
diff --git a/arch/arm64/kernel/paravirt.c b/arch/arm64/kernel/paravirt.c
index aa718d6a9274..675424258419 100644
--- a/arch/arm64/kernel/paravirt.c
+++ b/arch/arm64/kernel/paravirt.c
@@ -153,6 +153,54 @@ static bool __init has_pv_steal_clock(void)
 	return (res.a0 == SMCCC_RET_SUCCESS);
 }
 
+extern struct migrn_target __ro_after_init errata_migrn_target_cpus[];
+extern u32 __ro_after_init errata_migrn_target_num;
+void  __init pv_errata_migrn_target_init(void)
+{
+	u32 target_num;
+	struct arm_smccc_res res;
+	u32 curr = 0;
+
+	/* Check we have already set errata targets */
+	if (errata_migrn_target_num)
+		return;
+
+	arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_KVM_MIGRN_TARGET_NUM_FUNC_ID, &res);
+	if (res.a0 == SMCCC_RET_NOT_SUPPORTED)
+		return;
+
+	target_num = lower_32_bits(res.a0);
+	if (!target_num || target_num > MAX_MIGRN_TARGET_CPUS ||
+	    upper_32_bits(res.a0) != MIGRN_TARGET_CPUS_HYP_CALL_VER_1)
+		return;
+
+	while (curr < target_num) {
+		unsigned long *ptr;
+		int j;
+
+		arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_KVM_MIGRN_TARGET_CPUS_FUNC_ID, &res);
+		if (res.a0 == SMCCC_RET_NOT_SUPPORTED)
+			break;
+
+		ptr = (unsigned long *)&res;
+		for (j = 0; j < 4; j++, ptr++) {
+			if (!(*ptr))
+				goto out;
+			errata_migrn_target_cpus[curr].midr = lower_32_bits(*ptr);
+			errata_migrn_target_cpus[curr].revidr = upper_32_bits(*ptr);
+			curr++;
+		}
+	}
+out:
+	if (curr != target_num) {
+		pr_warn("Failed to retrieve all migration target CPUs\n");
+		return;
+	}
+
+	errata_migrn_target_num = target_num;
+	pr_info("Migration target CPUs set to %d\n", target_num);
+}
+
 int __init pv_time_init(void)
 {
 	int ret;
-- 
2.45.2

[RFC PATCH v2 3/3] KVM: arm64: Enable errata based on migration target CPUs

[Shameer Kolothum]

If the Guest has migration target CPUs set, enable all errata
that are based on target MIDR/REVIDR.

Also make sure we call the paravirt helper to retrieve migration
targets if any.

Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 arch/arm64/kernel/cpu_errata.c | 46 ++++++++++++++++++++++++++--------
 arch/arm64/kernel/cpufeature.c |  3 +++
 2 files changed, 39 insertions(+), 10 deletions(-)

diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
index feaaf2b11f46..df6d32dfd5c0 100644
--- a/arch/arm64/kernel/cpu_errata.c
+++ b/arch/arm64/kernel/cpu_errata.c
@@ -18,17 +18,14 @@ u32 __ro_after_init errata_migrn_target_num;
 struct migrn_target __ro_after_init errata_migrn_target_cpus[MAX_MIGRN_TARGET_CPUS];
 
 static bool __maybe_unused
-is_affected_midr_range(const struct arm64_cpu_capabilities *entry, int scope)
+__is_affected_midr_range(const struct arm64_cpu_capabilities *entry, u32 midr, u32 revidr)
 {
 	const struct arm64_midr_revidr *fix;
-	u32 midr = read_cpuid_id(), revidr;
 
-	WARN_ON(scope != SCOPE_LOCAL_CPU || preemptible());
 	if (!is_midr_in_range(midr, &entry->midr_range))
 		return false;
 
 	midr &= MIDR_REVISION_MASK | MIDR_VARIANT_MASK;
-	revidr = read_cpuid(REVIDR_EL1);
 	for (fix = entry->fixed_revs; fix && fix->revidr_mask; fix++)
 		if (midr == fix->midr_rv && (revidr & fix->revidr_mask))
 			return false;
@@ -37,27 +34,56 @@ is_affected_midr_range(const struct arm64_cpu_capabilities *entry, int scope)
 }
 
 static bool __maybe_unused
-is_affected_midr_range_list(const struct arm64_cpu_capabilities *entry,
-			    int scope)
+is_affected_midr_range(const struct arm64_cpu_capabilities *entry, int scope)
 {
+	int i;
+
+	for (i = 0; i < errata_migrn_target_num; i++) {
+		if (__is_affected_midr_range(entry, errata_migrn_target_cpus[i].midr,
+					     errata_migrn_target_cpus[i].revidr))
+			return true;
+	}
 	WARN_ON(scope != SCOPE_LOCAL_CPU || preemptible());
-	return is_midr_in_range_list(read_cpuid_id(), entry->midr_range_list);
+	return __is_affected_midr_range(entry, read_cpuid_id(), read_cpuid(REVIDR_EL1));
 }
 
 static bool __maybe_unused
-is_kryo_midr(const struct arm64_cpu_capabilities *entry, int scope)
+is_affected_midr_range_list(const struct arm64_cpu_capabilities *entry,
+			    int scope)
 {
-	u32 model;
+	int i;
 
+	for (i = 0; i < errata_migrn_target_num; i++) {
+		if (is_midr_in_range_list(errata_migrn_target_cpus[i].midr,
+					  entry->midr_range_list))
+			return true;
+	}
 	WARN_ON(scope != SCOPE_LOCAL_CPU || preemptible());
+	return is_midr_in_range_list(read_cpuid_id(), entry->midr_range_list);
+}
 
-	model = read_cpuid_id();
+static bool __maybe_unused
+__is_kryo_midr(const struct arm64_cpu_capabilities *entry, u32 model)
+{
 	model &= MIDR_IMPLEMENTOR_MASK | (0xf00 << MIDR_PARTNUM_SHIFT) |
 		 MIDR_ARCHITECTURE_MASK;
 
 	return model == entry->midr_range.model;
 }
 
+static bool __maybe_unused
+is_kryo_midr(const struct arm64_cpu_capabilities *entry, int scope)
+{
+	int i;
+
+	for (i = 0; i < errata_migrn_target_num; i++) {
+		if (__is_kryo_midr(entry, errata_migrn_target_cpus[i].midr))
+			return true;
+	}
+	WARN_ON(scope != SCOPE_LOCAL_CPU || preemptible());
+	return __is_kryo_midr(entry, read_cpuid_id());
+}
+
 static bool
 has_mismatched_cache_type(const struct arm64_cpu_capabilities *entry,
 			  int scope)
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index f7fd7e3259e4..390f4ffa773c 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -86,6 +86,7 @@
 #include <asm/mmu_context.h>
 #include <asm/mpam.h>
 #include <asm/mte.h>
+#include <asm/paravirt.h>
 #include <asm/processor.h>
 #include <asm/smp.h>
 #include <asm/sysreg.h>
@@ -3597,6 +3598,8 @@ unsigned long cpu_get_elf_hwcap2(void)
 
 static void __init setup_boot_cpu_capabilities(void)
 {
+	pv_errata_migrn_target_init();
+
 	/*
 	 * The boot CPU's feature register values have been recorded. Detect
 	 * boot cpucaps and local cpucaps for the boot CPU, then enable and
-- 
2.45.2

Re: [RFC PATCH v2 1/3] KVM: arm64: Add hypercall support for retrieving migration targets

[Oliver Upton]

Hi,

On Thu, Oct 24, 2024 at 10:40:10AM +0100, Shameer Kolothum wrote:
> +``ARM_SMCCC_VENDOR_HYP_KVM_MIGRN_TARGET_NUM_FUNC_ID``
> +------------------------------------------------------
> +
> +Query total number of migration target CPUs the Guest VM will be running during its
> +lifetime and version information applicable to the data format used for
> +``ARM_SMCCC_VENDOR_HYP_KVM_MIGRN_TARGET_CPUS_FUNC_ID``. The maximum number of targets
> +supported is 64. Also the version number supported currently is 1.0. This hypercall
> +must be handled by the userspace VMM.
> +
> ++---------------------+-------------------------------------------------------------+
> +| Presence:           | Optional; KVM/ARM64 Guests only                             |
> ++---------------------+-------------------------------------------------------------+
> +| Calling convention: | HVC64                                                       |
> ++---------------------+----------+--------------------------------------------------+
> +| Function ID:        | (uint32) | 0xC600007D                                       |
> ++---------------------+----------+--------------------------------------------------+
> +| Arguments:          | None                                                        |
> ++---------------------+----------+----+---------------------------------------------+
> +| Return Values:      | (int64)  | R0 | ``NOT_SUPPORTED (-1)`` on error, else       |
> +|                     |          |    | [0-31] total migration targets              |
> +|                     |          |    | [32-63] version number                      |
> ++---------------------+----------+----+---------------------------------------------+

We can't treat a single register as both a signed quantity *and* a full
64 bits of bitfields. Maybe just scrap the version and have this thing
either return a negative error or positive quantity of implementations.

> +``ARM_SMCCC_VENDOR_HYP_KVM_MIGRN_TARGET_CPUS_FUNC_ID``
> +-------------------------------------------------------
> +
> +Request migration target CPU information for the Guest VM. The information must be
> +provided as per the format described by the version info in
> +``ARM_SMCCC_VENDOR_HYP_KVM_MIGRN_TARGET_NUM_FUNC_ID``. At present, we only support
> +the below format which corresponds to version 1.0. This hypercall will always be
> +preceded by ``ARM_SMCCC_VENDOR_HYP_KVM_MIGRN_TARGET_NUM_FUNC_ID`` and may be
> +invoked multiple times to retrieve the total number of target CPUs information
> +advertised. This hypercall must be handled by the userspace VMM.
> +
> +A typical userspace usage scenario will be like below:
> +
> +1. Receives ``ARM_SMCCC_VENDOR_HYP_KVM_MIGRN_TARGET_NUM_FUNC_ID``
> +
> +   * Returns total number of migration targets and version number
> +   * Reset current target index to zero
> +2. Receives ``ARM_SMCCC_VENDOR_HYP_KVM_MIGRN_TARGET_CPUS_FUNC_ID``
> +
> +   * Returns MIDR/REVIDR info in return register fields. Can return up to 4
> +   * Update current target index based on returned target info
> +   * If there are remaining register fields, return zero to indicate the end
> +   * Repeat step 2 until current target index == total number of migration targets

Hmm... I'd rather we make the guest supply the target index of the
implementation it wants to discover. Otherwise, the VMM implementation
of this hypercall interface is *stateful* and needs to remember to
migrate that...

> ++---------------------+-------------------------------------------------------------+
> +| Presence:           | Optional; KVM/ARM64 Guests only                             |
> ++---------------------+-------------------------------------------------------------+
> +| Calling convention: | HVC64                                                       |
> ++---------------------+----------+--------------------------------------------------+
> +| Function ID:        | (uint32) | 0xC600007E                                       |
> ++---------------------+----------+----+---------------------------------------------+
> +| Arguments:          | None                                                        |
> ++---------------------+----------+----+---------------------------------------------+
> +| Return Values:      | (int64)  | R0 | ``NOT_SUPPORTED (-1)`` on error, else       |
> +|                     |          |    | [0-31] MIDR, [31-63] REVIDR, else           |
> +|                     |          |    | [0-63] Zero to mark end.                    |
> ++---------------------+----------+----+---------------------------------------------+

Same deal here, x0 needs to always be treated as a signed quantity.

How about -1 on error, 0 on success?

Then, in the remaining registers:

> +|                     | (uint64) | R1 | [0-31] MIDR, [32-63] REVIDR, else           |
> +|                     |          |    | [0-63] Zero to mark end.                    |
> ++---------------------+----------+----+---------------------------------------------+

Both MIDR_EL1 and REVIDR_EL1 are 64 bit registers in AArch64. So we need
to transfer a full 64 bits, even if the top half is _presently_ RES0 in
MIDR_EL1.

> +|                     | (uint64) | R2 | [0-31] MIDR, [32-63] REVIDR, else           |
> +|                     |          |    | [0-63] Zero to mark end.                    |
> ++---------------------+----------+----+---------------------------------------------+
> +|                     | (uint64) | R3 | [0-31] MIDR, [32-63] REVIDR, else           |
> +|                     |          |    | [0-63] Zero to mark end.                    |
> ++---------------------+----------+----+---------------------------------------------+

I think it's fine to have this hypercall return a single MIDR/REVIDR
pair at a time. This is not a performance sensitive interface and will
only be called a few times at boot.

Actually -- what if we crammed everything into a single hypercall?

DISCOVER_IMPLEMENTATION_FUNC_ID

 - Arguments:
   - arg0: selected implementation index

 - Return value:
   - r0: -1 on error, otherwise the maximum possible implementation index
   - r1: MIDR_EL1 of the selected implementation
   - r2: REVIDR_EL1 of the selected implementation

We're guaranteed at least one CPU implementation of course, so the guest
can just start w/ index 0 and iterate from there.

-- 
Thanks,
Oliver

Re: [RFC PATCH v2 3/3] KVM: arm64: Enable errata based on migration target CPUs

[Oliver Upton]

nitpick: shortlog shouldn't use a KVM prefix if the patch isn't touching
KVM.

On Thu, Oct 24, 2024 at 10:40:12AM +0100, Shameer Kolothum wrote:
> If the Guest has migration target CPUs set, enable all errata
> that are based on target MIDR/REVIDR.
> 
> Also make sure we call the paravirt helper to retrieve migration
> targets if any.
> 
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>

I don't know if you saw my suggestion on v1 [*], but it'd be great if we
can hide the array of implementations from users of is_midr_in_range()
and friends.

There's other junk keyed off MIDR (e.g. Spectre) that also needs to be
aware of all the implementations where the VM might run. The easiest way
to do that is to stop using a caller-provided MIDR and have
is_midr_in_range() either walk the array of implementations or read
MIDR_EL1.

[*]: https://lore.kernel.org/kvmarm/ZwlbTCwoKQyh3vmF@linux.dev/

-- 
Thanks,
Oliver

RE: [RFC PATCH v2 3/3] KVM: arm64: Enable errata based on migration target CPUs

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Oliver Upton <oliver.upton@linux.dev>
> Sent: Friday, October 25, 2024 2:37 AM
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: kvmarm@lists.linux.dev; maz@kernel.org; catalin.marinas@arm.com;
> will@kernel.org; mark.rutland@arm.com; cohuck@redhat.com;
> eric.auger@redhat.com; yuzenghui <yuzenghui@huawei.com>; Wangzhou
> (B) <wangzhou1@hisilicon.com>; jiangkunkun <jiangkunkun@huawei.com>;
> Jonathan Cameron <jonathan.cameron@huawei.com>; Anthony Jebson
> <anthony.jebson@huawei.com>; linux-arm-kernel@lists.infradead.org;
> Linuxarm <linuxarm@huawei.com>
> Subject: Re: [RFC PATCH v2 3/3] KVM: arm64: Enable errata based on
> migration target CPUs
> 
> nitpick: shortlog shouldn't use a KVM prefix if the patch isn't touching
> KVM.
> 
> On Thu, Oct 24, 2024 at 10:40:12AM +0100, Shameer Kolothum wrote:
> > If the Guest has migration target CPUs set, enable all errata
> > that are based on target MIDR/REVIDR.
> >
> > Also make sure we call the paravirt helper to retrieve migration
> > targets if any.
> >
> > Signed-off-by: Shameer Kolothum
> <shameerali.kolothum.thodi@huawei.com>
> 
> I don't know if you saw my suggestion on v1 [*], but it'd be great if we
> can hide the array of implementations from users of is_midr_in_range()
> and friends.

I did see your suggestion but my bad,  misunderstood it and thought that you are
referring to _midr_range() functions in cpu_errata.c only.
 
> There's other junk keyed off MIDR (e.g. Spectre) that also needs to be
> aware of all the implementations where the VM might run. The easiest way
> to do that is to stop using a caller-provided MIDR and have
> is_midr_in_range() either walk the array of implementations or read
> MIDR_EL1.

So the suggestion is to use something like this?

bool is_midr_in_range(struct midr_range const *range)
{
        int i;

        for (i = 0; i < errata_migrn_target_num; i++) {
                if (midr_is_cpu_model_range(errata_migrn_target_cpus[i].midr,
                                            range->model,
                                            range->rv_min, range->rv_max))
                        return true;
        }

        return midr_is_cpu_model_range(read_cpuid_id(), range->model,
                                       range->rv_min, range->rv_max);
}

Or do we need some kind of hint to these functions to specify which 
MIDR to use? I think there are at least couple of places where it looks like
it make sense to do the checking against MIDR_EL1 only.
(eg: has_neoverse_n1_erratum_1542419().

Thanks,
Shameer

RE: [RFC PATCH v2 1/3] KVM: arm64: Add hypercall support for retrieving migration targets

[Shameerali Kolothum Thodi]

Hi Oliver,

> -----Original Message-----
> From: Oliver Upton <oliver.upton@linux.dev>
> Sent: Friday, October 25, 2024 2:25 AM
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: kvmarm@lists.linux.dev; maz@kernel.org; catalin.marinas@arm.com;
> will@kernel.org; mark.rutland@arm.com; cohuck@redhat.com;
> eric.auger@redhat.com; yuzenghui <yuzenghui@huawei.com>; Wangzhou
> (B) <wangzhou1@hisilicon.com>; jiangkunkun <jiangkunkun@huawei.com>;
> Jonathan Cameron <jonathan.cameron@huawei.com>; Anthony Jebson
> <anthony.jebson@huawei.com>; linux-arm-kernel@lists.infradead.org;
> Linuxarm <linuxarm@huawei.com>
> Subject: Re: [RFC PATCH v2 1/3] KVM: arm64: Add hypercall support for
> retrieving migration targets
> 
> Hi,
> 
> On Thu, Oct 24, 2024 at 10:40:10AM +0100, Shameer Kolothum wrote:
> > +``ARM_SMCCC_VENDOR_HYP_KVM_MIGRN_TARGET_NUM_FUNC_ID``
> > +------------------------------------------------------
> > +
> > +Query total number of migration target CPUs the Guest VM will be
> running during its
> > +lifetime and version information applicable to the data format used for
> > +``ARM_SMCCC_VENDOR_HYP_KVM_MIGRN_TARGET_CPUS_FUNC_ID``.
> The maximum number of targets
> > +supported is 64. Also the version number supported currently is 1.0. This
> hypercall
> > +must be handled by the userspace VMM.
> > +
> > ++---------------------+-------------------------------------------------------------+
> > +| Presence:           | Optional; KVM/ARM64 Guests only                             |
> > ++---------------------+-------------------------------------------------------------+
> > +| Calling convention: | HVC64                                                       |
> > ++---------------------+----------+--------------------------------------------------+
> > +| Function ID:        | (uint32) | 0xC600007D                                       |
> > ++---------------------+----------+--------------------------------------------------+
> > +| Arguments:          | None                                                        |
> > ++---------------------+----------+----+---------------------------------------------+
> > +| Return Values:      | (int64)  | R0 | ``NOT_SUPPORTED (-1)`` on error,
> else       |
> > +|                     |          |    | [0-31] total migration targets              |
> > +|                     |          |    | [32-63] version number                      |
> > ++---------------------+----------+----+---------------------------------------------+
> 
> We can't treat a single register as both a signed quantity *and* a full
> 64 bits of bitfields. Maybe just scrap the version and have this thing
> either return a negative error or positive quantity of implementations.

Ok. I had  a look at PV_TIME_ST/ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID
and got that idea. Separate registers make sense though.

Do we really need to skip the version number? The idea was to use that as a
future proof for data format in case we realize that MIDR/REVIDR is not good
enough for errata later.

> 
> > +``ARM_SMCCC_VENDOR_HYP_KVM_MIGRN_TARGET_CPUS_FUNC_ID``
> > +-------------------------------------------------------
> > +
> > +Request migration target CPU information for the Guest VM. The
> information must be
> > +provided as per the format described by the version info in
> > +``ARM_SMCCC_VENDOR_HYP_KVM_MIGRN_TARGET_NUM_FUNC_ID``.
> At present, we only support
> > +the below format which corresponds to version 1.0. This hypercall will
> always be
> > +preceded by
> ``ARM_SMCCC_VENDOR_HYP_KVM_MIGRN_TARGET_NUM_FUNC_ID`` and
> may be
> > +invoked multiple times to retrieve the total number of target CPUs
> information
> > +advertised. This hypercall must be handled by the userspace VMM.
> > +
> > +A typical userspace usage scenario will be like below:
> > +
> > +1. Receives
> ``ARM_SMCCC_VENDOR_HYP_KVM_MIGRN_TARGET_NUM_FUNC_ID``
> > +
> > +   * Returns total number of migration targets and version number
> > +   * Reset current target index to zero
> > +2. Receives
> ``ARM_SMCCC_VENDOR_HYP_KVM_MIGRN_TARGET_CPUS_FUNC_ID``
> > +
> > +   * Returns MIDR/REVIDR info in return register fields. Can return up to
> 4
> > +   * Update current target index based on returned target info
> > +   * If there are remaining register fields, return zero to indicate the end
> > +   * Repeat step 2 until current target index == total number of migration
> targets
> 
> Hmm... I'd rather we make the guest supply the target index of the
> implementation it wants to discover. Otherwise, the VMM implementation
> of this hypercall interface is *stateful* and needs to remember to
> migrate that...

Ok. I had the same feedback from Jonathan in an internal discussion as well
and on re-reading Marc's comments on v1, he also had the same suggestion
that I missed. Will change that.
 
> > ++---------------------+-------------------------------------------------------------+
> > +| Presence:           | Optional; KVM/ARM64 Guests only                             |
> > ++---------------------+-------------------------------------------------------------+
> > +| Calling convention: | HVC64                                                       |
> > ++---------------------+----------+--------------------------------------------------+
> > +| Function ID:        | (uint32) | 0xC600007E                                       |
> > ++---------------------+----------+----+---------------------------------------------+
> > +| Arguments:          | None                                                        |
> > ++---------------------+----------+----+---------------------------------------------+
> > +| Return Values:      | (int64)  | R0 | ``NOT_SUPPORTED (-1)`` on error,
> else       |
> > +|                     |          |    | [0-31] MIDR, [31-63] REVIDR, else           |
> > +|                     |          |    | [0-63] Zero to mark end.                    |
> > ++---------------------+----------+----+---------------------------------------------+
> 
> Same deal here, x0 needs to always be treated as a signed quantity.
> 
> How about -1 on error, 0 on success?
> 
> Then, in the remaining registers:
> 
> > +|                     | (uint64) | R1 | [0-31] MIDR, [32-63] REVIDR, else           |
> > +|                     |          |    | [0-63] Zero to mark end.                    |
> > ++---------------------+----------+----+---------------------------------------------+
> 
> Both MIDR_EL1 and REVIDR_EL1 are 64 bit registers in AArch64. So we need
> to transfer a full 64 bits, even if the top half is _presently_ RES0 in
> MIDR_EL1.

Ok.

> 
> > +|                     | (uint64) | R2 | [0-31] MIDR, [32-63] REVIDR, else           |
> > +|                     |          |    | [0-63] Zero to mark end.                    |
> > ++---------------------+----------+----+---------------------------------------------+
> > +|                     | (uint64) | R3 | [0-31] MIDR, [32-63] REVIDR, else           |
> > +|                     |          |    | [0-63] Zero to mark end.                    |
> > ++---------------------+----------+----+---------------------------------------------+
> 
> I think it's fine to have this hypercall return a single MIDR/REVIDR
> pair at a time. This is not a performance sensitive interface and will
> only be called a few times at boot.
> 
> Actually -- what if we crammed everything into a single hypercall?
> 
> DISCOVER_IMPLEMENTATION_FUNC_ID
> 
>  - Arguments:
>    - arg0: selected implementation index
> 
>  - Return value:
>    - r0: -1 on error, otherwise the maximum possible implementation index
>    - r1: MIDR_EL1 of the selected implementation
>    - r2: REVIDR_EL1 of the selected implementation
> 
> We're guaranteed at least one CPU implementation of course, so the guest
> can just start w/ index 0 and iterate from there.

Ok. I will rework based on these and will sent out something soon.

Thanks,
Shameer

Re: [RFC PATCH v2 3/3] KVM: arm64: Enable errata based on migration target CPUs

[Oliver Upton]

Hey Shameer,

On Mon, Oct 28, 2024 at 05:29:14PM +0000, Shameerali Kolothum Thodi wrote:
> > I don't know if you saw my suggestion on v1 [*], but it'd be great if we
> > can hide the array of implementations from users of is_midr_in_range()
> > and friends.
> 
> I did see your suggestion but my bad,  misunderstood it and thought that you are
> referring to _midr_range() functions in cpu_errata.c only.

Ah, no worries. And yeah, for this to be a general solution we're going
to need to cover all our bases where MIDR checks are used.

> > There's other junk keyed off MIDR (e.g. Spectre) that also needs to be
> > aware of all the implementations where the VM might run. The easiest way
> > to do that is to stop using a caller-provided MIDR and have
> > is_midr_in_range() either walk the array of implementations or read
> > MIDR_EL1.
> 
> So the suggestion is to use something like this?
> 
> bool is_midr_in_range(struct midr_range const *range)
> {
>         int i;
> 
>         for (i = 0; i < errata_migrn_target_num; i++) {
>                 if (midr_is_cpu_model_range(errata_migrn_target_cpus[i].midr,
>                                             range->model,
>                                             range->rv_min, range->rv_max))
>                         return true;
>         }
> 
>         return midr_is_cpu_model_range(read_cpuid_id(), range->model,
>                                        range->rv_min, range->rv_max);
> }

Yep, pretty much like that. Except that the guest should either use the
values from the hypercall *or* the value of MIDR_EL1, not both.

> Or do we need some kind of hint to these functions to specify which 
> MIDR to use? I think there are at least couple of places where it looks like
> it make sense to do the checking against MIDR_EL1 only.
> (eg: has_neoverse_n1_erratum_1542419().

I don't think we need a hint or anything. The fact that the hypervisor
is exposing this hypercall interface indicates the MIDR of the
implementation the vCPU currently runs on is subject to change.

So the concept of a per-CPU erratum is completely out of the window, and
every CPU is subject to the errata of every implementation advertised by
the hypervisor.

-- 
Thanks,
Oliver

Re: [RFC PATCH v2 1/3] KVM: arm64: Add hypercall support for retrieving migration targets

[Oliver Upton]

On Tue, Oct 29, 2024 at 04:00:39PM +0000, Shameerali Kolothum Thodi wrote:
> > We can't treat a single register as both a signed quantity *and* a full
> > 64 bits of bitfields. Maybe just scrap the version and have this thing
> > either return a negative error or positive quantity of implementations.
> 
> Ok. I had  a look at PV_TIME_ST/ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID
> and got that idea. Separate registers make sense though.
> 
> Do we really need to skip the version number? The idea was to use that as a
> future proof for data format in case we realize that MIDR/REVIDR is not good
> enough for errata later.

That is definitely an approach we can take. The alternative I had in
mind was that we'd allocate a new function ID if we needed to break ABI
to correct shortcomings of the original interface.

-- 
Thanks,
Oliver