[PATCH 0/2] KVM: arm64: Add support for setting MPIDR

[PATCH 0/2] KVM: arm64: Add support for setting MPIDR

[Ying Fang]

MPIDR is used to show multiprocessor affinity on arm platform. It is
also used to provide an additional processor identification mechanism
for scheduling purposes. To add support for setting MPIDR from usersapce
an vcpu ioctl KVM_CAP_ARM_MP_AFFINITY is introduced. This patch series is
needed to help qemu to build the accurate cpu topology for arm.

Ying Fang (2):
  KVM: arm64: add KVM_CAP_ARM_MP_AFFINITY extension
  kvm/arm: Add mp_affinity for arm vcpu

 Documentation/virt/kvm/api.rst    |  8 ++++++++
 arch/arm64/include/asm/kvm_host.h |  5 +++++
 arch/arm64/kvm/arm.c              |  9 +++++++++
 arch/arm64/kvm/reset.c            | 11 +++++++++++
 arch/arm64/kvm/sys_regs.c         | 30 +++++++++++++++++++-----------
 include/uapi/linux/kvm.h          |  3 +++
 6 files changed, 55 insertions(+), 11 deletions(-)

-- 
2.23.0

[PATCH 0/2] KVM: arm64: Add support for setting MPIDR

[Ying Fang]

MPIDR is used to show multiprocessor affinity on arm platform. It is
also used to provide an additional processor identification mechanism
for scheduling purposes. To add support for setting MPIDR from usersapce
an vcpu ioctl KVM_CAP_ARM_MP_AFFINITY is introduced. This patch series is
needed to help qemu to build the accurate cpu topology for arm.

Ying Fang (2):
  KVM: arm64: add KVM_CAP_ARM_MP_AFFINITY extension
  kvm/arm: Add mp_affinity for arm vcpu

 Documentation/virt/kvm/api.rst    |  8 ++++++++
 arch/arm64/include/asm/kvm_host.h |  5 +++++
 arch/arm64/kvm/arm.c              |  9 +++++++++
 arch/arm64/kvm/reset.c            | 11 +++++++++++
 arch/arm64/kvm/sys_regs.c         | 30 +++++++++++++++++++-----------
 include/uapi/linux/kvm.h          |  3 +++
 6 files changed, 55 insertions(+), 11 deletions(-)

-- 
2.23.0

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

[PATCH 1/2] KVM: arm64: add KVM_CAP_ARM_MP_AFFINITY extension

[Ying Fang]

Add KVM_CAP_ARM_MP_AFFINITY extension for userspace to check
whether KVM supports setting MPIDR on AArch64 platform. Thus
we can give userspace control over the MPIDR to present
cpu topology information.

Signed-off-by: Ying Fang <fangying1@huawei.com>
---
 Documentation/virt/kvm/api.rst | 8 ++++++++
 arch/arm64/kvm/arm.c           | 1 +
 include/uapi/linux/kvm.h       | 1 +
 3 files changed, 10 insertions(+)

diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
index eb3a1316f03e..d2fb18613a34 100644
--- a/Documentation/virt/kvm/api.rst
+++ b/Documentation/virt/kvm/api.rst
@@ -6159,3 +6159,11 @@ KVM can therefore start protected VMs.
 This capability governs the KVM_S390_PV_COMMAND ioctl and the
 KVM_MP_STATE_LOAD MP_STATE. KVM_SET_MP_STATE can fail for protected
 guests when the state change is invalid.
+
+8.24 KVM_CAP_ARM_MP_AFFINITY
+----------------------------
+
+:Architecture: arm64
+
+This capability indicates that KVM_ARM_SET_MP_AFFINITY ioctl is available.
+It is used by to set MPIDR from userspace.
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 46dc3d75cf13..913c8da539b3 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -178,6 +178,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 	case KVM_CAP_ARM_IRQ_LINE_LAYOUT_2:
 	case KVM_CAP_ARM_NISV_TO_USER:
 	case KVM_CAP_ARM_INJECT_EXT_DABT:
+	case KVM_CAP_ARM_MP_AFFINITY:
 		r = 1;
 		break;
 	case KVM_CAP_ARM_SET_DEVICE_ADDR:
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index f6d86033c4fa..c4874905cd9c 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -1035,6 +1035,7 @@ struct kvm_ppc_resize_hpt {
 #define KVM_CAP_LAST_CPU 184
 #define KVM_CAP_SMALLER_MAXPHYADDR 185
 #define KVM_CAP_S390_DIAG318 186
+#define KVM_CAP_ARM_MP_AFFINITY 187
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
-- 
2.23.0

[PATCH 1/2] KVM: arm64: add KVM_CAP_ARM_MP_AFFINITY extension

[Ying Fang]

Add KVM_CAP_ARM_MP_AFFINITY extension for userspace to check
whether KVM supports setting MPIDR on AArch64 platform. Thus
we can give userspace control over the MPIDR to present
cpu topology information.

Signed-off-by: Ying Fang <fangying1@huawei.com>
---
 Documentation/virt/kvm/api.rst | 8 ++++++++
 arch/arm64/kvm/arm.c           | 1 +
 include/uapi/linux/kvm.h       | 1 +
 3 files changed, 10 insertions(+)

diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
index eb3a1316f03e..d2fb18613a34 100644
--- a/Documentation/virt/kvm/api.rst
+++ b/Documentation/virt/kvm/api.rst
@@ -6159,3 +6159,11 @@ KVM can therefore start protected VMs.
 This capability governs the KVM_S390_PV_COMMAND ioctl and the
 KVM_MP_STATE_LOAD MP_STATE. KVM_SET_MP_STATE can fail for protected
 guests when the state change is invalid.
+
+8.24 KVM_CAP_ARM_MP_AFFINITY
+----------------------------
+
+:Architecture: arm64
+
+This capability indicates that KVM_ARM_SET_MP_AFFINITY ioctl is available.
+It is used by to set MPIDR from userspace.
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 46dc3d75cf13..913c8da539b3 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -178,6 +178,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 	case KVM_CAP_ARM_IRQ_LINE_LAYOUT_2:
 	case KVM_CAP_ARM_NISV_TO_USER:
 	case KVM_CAP_ARM_INJECT_EXT_DABT:
+	case KVM_CAP_ARM_MP_AFFINITY:
 		r = 1;
 		break;
 	case KVM_CAP_ARM_SET_DEVICE_ADDR:
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index f6d86033c4fa..c4874905cd9c 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -1035,6 +1035,7 @@ struct kvm_ppc_resize_hpt {
 #define KVM_CAP_LAST_CPU 184
 #define KVM_CAP_SMALLER_MAXPHYADDR 185
 #define KVM_CAP_S390_DIAG318 186
+#define KVM_CAP_ARM_MP_AFFINITY 187
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
-- 
2.23.0

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

[PATCH 2/2] kvm/arm: Add mp_affinity for arm vcpu

[Ying Fang]

Allow userspace to set MPIDR using vcpu ioctl KVM_ARM_SET_MP_AFFINITY,
so that we can support cpu topology for arm.

Signed-off-by: Ying Fang <fangying1@huawei.com>
---
 arch/arm64/include/asm/kvm_host.h |  5 +++++
 arch/arm64/kvm/arm.c              |  8 ++++++++
 arch/arm64/kvm/reset.c            | 11 +++++++++++
 arch/arm64/kvm/sys_regs.c         | 30 +++++++++++++++++++-----------
 include/uapi/linux/kvm.h          |  2 ++
 5 files changed, 45 insertions(+), 11 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index e52c927aade5..7adc351ee70a 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -372,6 +372,9 @@ struct kvm_vcpu_arch {
 		u64 last_steal;
 		gpa_t base;
 	} steal;
+
+	/* vCPU MP Affinity */
+	u64 mp_affinity;
 };
 
 /* Pointer to the vcpu's SVE FFR for sve_{save,load}_state() */
@@ -685,6 +688,8 @@ int kvm_arm_setup_stage2(struct kvm *kvm, unsigned long type);
 int kvm_arm_vcpu_finalize(struct kvm_vcpu *vcpu, int feature);
 bool kvm_arm_vcpu_is_finalized(struct kvm_vcpu *vcpu);
 
+int kvm_arm_vcpu_set_mp_affinity(struct kvm_vcpu *vcpu, uint64_t mpidr);
+
 #define kvm_arm_vcpu_sve_finalized(vcpu) \
 	((vcpu)->arch.flags & KVM_ARM64_VCPU_SVE_FINALIZED)
 
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 913c8da539b3..5f1fa625dc11 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -1178,6 +1178,14 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 
 		return kvm_arm_vcpu_finalize(vcpu, what);
 	}
+	case KVM_ARM_SET_MP_AFFINITY: {
+		u64 mpidr;
+
+		if (get_user(mpidr, (const unsigned int __user *)argp))
+			return -EFAULT;
+
+		return kvm_arm_vcpu_set_mp_affinity(vcpu, mpidr);
+	}
 	default:
 		r = -EINVAL;
 	}
diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
index ee33875c5c2a..4918c967b0c9 100644
--- a/arch/arm64/kvm/reset.c
+++ b/arch/arm64/kvm/reset.c
@@ -188,6 +188,17 @@ int kvm_arm_vcpu_finalize(struct kvm_vcpu *vcpu, int feature)
 	return -EINVAL;
 }
 
+int kvm_arm_vcpu_set_mp_affinity(struct kvm_vcpu *vcpu, uint64_t mpidr)
+{
+	if (!(mpidr & (1ULL << 31))) {
+		pr_warn("invalid mp_affinity format\n");
+		return -EINVAL;
+	}
+
+	vcpu->arch.mp_affinity = mpidr;
+	return 0;
+}
+
 bool kvm_arm_vcpu_is_finalized(struct kvm_vcpu *vcpu)
 {
 	if (vcpu_has_sve(vcpu) && !kvm_arm_vcpu_sve_finalized(vcpu))
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 077293b5115f..e76f483475ad 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -646,17 +646,25 @@ static void reset_mpidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
 {
 	u64 mpidr;
 
-	/*
-	 * Map the vcpu_id into the first three affinity level fields of
-	 * the MPIDR. We limit the number of VCPUs in level 0 due to a
-	 * limitation to 16 CPUs in that level in the ICC_SGIxR registers
-	 * of the GICv3 to be able to address each CPU directly when
-	 * sending IPIs.
-	 */
-	mpidr = (vcpu->vcpu_id & 0x0f) << MPIDR_LEVEL_SHIFT(0);
-	mpidr |= ((vcpu->vcpu_id >> 4) & 0xff) << MPIDR_LEVEL_SHIFT(1);
-	mpidr |= ((vcpu->vcpu_id >> 12) & 0xff) << MPIDR_LEVEL_SHIFT(2);
-	vcpu_write_sys_reg(vcpu, (1ULL << 31) | mpidr, MPIDR_EL1);
+	if (vcpu->arch.mp_affinity) {
+		/* If mp_affinity is set by userspace, it means an customized cpu
+		 * topology is defined. Let it be MPIDR of the vcpu
+		 */
+		mpidr = vcpu->arch.mp_affinity;
+	} else {
+		/*
+		 * Map the vcpu_id into the first three affinity level fields of
+		 * the MPIDR. We limit the number of VCPUs in level 0 due to a
+		 * limitation to 16 CPUs in that level in the ICC_SGIxR registers
+		 * of the GICv3 to be able to address each CPU directly when
+		 * sending IPIs.
+		 */
+		mpidr = (vcpu->vcpu_id & 0x0f) << MPIDR_LEVEL_SHIFT(0);
+		mpidr |= ((vcpu->vcpu_id >> 4) & 0xff) << MPIDR_LEVEL_SHIFT(1);
+		mpidr |= ((vcpu->vcpu_id >> 12) & 0xff) << MPIDR_LEVEL_SHIFT(2);
+		mpidr |= (1ULL << 31);
+	}
+	vcpu_write_sys_reg(vcpu, mpidr, MPIDR_EL1);
 }
 
 static void reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index c4874905cd9c..ae45876a689d 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -1475,6 +1475,8 @@ struct kvm_s390_ucas_mapping {
 #define KVM_S390_SET_CMMA_BITS      _IOW(KVMIO, 0xb9, struct kvm_s390_cmma_log)
 /* Memory Encryption Commands */
 #define KVM_MEMORY_ENCRYPT_OP      _IOWR(KVMIO, 0xba, unsigned long)
+/* Available with KVM_CAP_ARM_MP_AFFINITY */
+#define KVM_ARM_SET_MP_AFFINITY    _IOWR(KVMIO, 0xbb, unsigned long)
 
 struct kvm_enc_region {
 	__u64 addr;
-- 
2.23.0

[PATCH 2/2] kvm/arm: Add mp_affinity for arm vcpu

[Ying Fang]

Allow userspace to set MPIDR using vcpu ioctl KVM_ARM_SET_MP_AFFINITY,
so that we can support cpu topology for arm.

Signed-off-by: Ying Fang <fangying1@huawei.com>
---
 arch/arm64/include/asm/kvm_host.h |  5 +++++
 arch/arm64/kvm/arm.c              |  8 ++++++++
 arch/arm64/kvm/reset.c            | 11 +++++++++++
 arch/arm64/kvm/sys_regs.c         | 30 +++++++++++++++++++-----------
 include/uapi/linux/kvm.h          |  2 ++
 5 files changed, 45 insertions(+), 11 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index e52c927aade5..7adc351ee70a 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -372,6 +372,9 @@ struct kvm_vcpu_arch {
 		u64 last_steal;
 		gpa_t base;
 	} steal;
+
+	/* vCPU MP Affinity */
+	u64 mp_affinity;
 };
 
 /* Pointer to the vcpu's SVE FFR for sve_{save,load}_state() */
@@ -685,6 +688,8 @@ int kvm_arm_setup_stage2(struct kvm *kvm, unsigned long type);
 int kvm_arm_vcpu_finalize(struct kvm_vcpu *vcpu, int feature);
 bool kvm_arm_vcpu_is_finalized(struct kvm_vcpu *vcpu);
 
+int kvm_arm_vcpu_set_mp_affinity(struct kvm_vcpu *vcpu, uint64_t mpidr);
+
 #define kvm_arm_vcpu_sve_finalized(vcpu) \
 	((vcpu)->arch.flags & KVM_ARM64_VCPU_SVE_FINALIZED)
 
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 913c8da539b3..5f1fa625dc11 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -1178,6 +1178,14 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 
 		return kvm_arm_vcpu_finalize(vcpu, what);
 	}
+	case KVM_ARM_SET_MP_AFFINITY: {
+		u64 mpidr;
+
+		if (get_user(mpidr, (const unsigned int __user *)argp))
+			return -EFAULT;
+
+		return kvm_arm_vcpu_set_mp_affinity(vcpu, mpidr);
+	}
 	default:
 		r = -EINVAL;
 	}
diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
index ee33875c5c2a..4918c967b0c9 100644
--- a/arch/arm64/kvm/reset.c
+++ b/arch/arm64/kvm/reset.c
@@ -188,6 +188,17 @@ int kvm_arm_vcpu_finalize(struct kvm_vcpu *vcpu, int feature)
 	return -EINVAL;
 }
 
+int kvm_arm_vcpu_set_mp_affinity(struct kvm_vcpu *vcpu, uint64_t mpidr)
+{
+	if (!(mpidr & (1ULL << 31))) {
+		pr_warn("invalid mp_affinity format\n");
+		return -EINVAL;
+	}
+
+	vcpu->arch.mp_affinity = mpidr;
+	return 0;
+}
+
 bool kvm_arm_vcpu_is_finalized(struct kvm_vcpu *vcpu)
 {
 	if (vcpu_has_sve(vcpu) && !kvm_arm_vcpu_sve_finalized(vcpu))
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 077293b5115f..e76f483475ad 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -646,17 +646,25 @@ static void reset_mpidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
 {
 	u64 mpidr;
 
-	/*
-	 * Map the vcpu_id into the first three affinity level fields of
-	 * the MPIDR. We limit the number of VCPUs in level 0 due to a
-	 * limitation to 16 CPUs in that level in the ICC_SGIxR registers
-	 * of the GICv3 to be able to address each CPU directly when
-	 * sending IPIs.
-	 */
-	mpidr = (vcpu->vcpu_id & 0x0f) << MPIDR_LEVEL_SHIFT(0);
-	mpidr |= ((vcpu->vcpu_id >> 4) & 0xff) << MPIDR_LEVEL_SHIFT(1);
-	mpidr |= ((vcpu->vcpu_id >> 12) & 0xff) << MPIDR_LEVEL_SHIFT(2);
-	vcpu_write_sys_reg(vcpu, (1ULL << 31) | mpidr, MPIDR_EL1);
+	if (vcpu->arch.mp_affinity) {
+		/* If mp_affinity is set by userspace, it means an customized cpu
+		 * topology is defined. Let it be MPIDR of the vcpu
+		 */
+		mpidr = vcpu->arch.mp_affinity;
+	} else {
+		/*
+		 * Map the vcpu_id into the first three affinity level fields of
+		 * the MPIDR. We limit the number of VCPUs in level 0 due to a
+		 * limitation to 16 CPUs in that level in the ICC_SGIxR registers
+		 * of the GICv3 to be able to address each CPU directly when
+		 * sending IPIs.
+		 */
+		mpidr = (vcpu->vcpu_id & 0x0f) << MPIDR_LEVEL_SHIFT(0);
+		mpidr |= ((vcpu->vcpu_id >> 4) & 0xff) << MPIDR_LEVEL_SHIFT(1);
+		mpidr |= ((vcpu->vcpu_id >> 12) & 0xff) << MPIDR_LEVEL_SHIFT(2);
+		mpidr |= (1ULL << 31);
+	}
+	vcpu_write_sys_reg(vcpu, mpidr, MPIDR_EL1);
 }
 
 static void reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index c4874905cd9c..ae45876a689d 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -1475,6 +1475,8 @@ struct kvm_s390_ucas_mapping {
 #define KVM_S390_SET_CMMA_BITS      _IOW(KVMIO, 0xb9, struct kvm_s390_cmma_log)
 /* Memory Encryption Commands */
 #define KVM_MEMORY_ENCRYPT_OP      _IOWR(KVMIO, 0xba, unsigned long)
+/* Available with KVM_CAP_ARM_MP_AFFINITY */
+#define KVM_ARM_SET_MP_AFFINITY    _IOWR(KVMIO, 0xbb, unsigned long)
 
 struct kvm_enc_region {
 	__u64 addr;
-- 
2.23.0

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

Re: [PATCH 1/2] KVM: arm64: add KVM_CAP_ARM_MP_AFFINITY extension

[Andrew Jones]

On Thu, Sep 17, 2020 at 10:30:32AM +0800, Ying Fang wrote:
> Add KVM_CAP_ARM_MP_AFFINITY extension for userspace to check
> whether KVM supports setting MPIDR on AArch64 platform. Thus
> we can give userspace control over the MPIDR to present
> cpu topology information.
> 
> Signed-off-by: Ying Fang <fangying1@huawei.com>
> ---
>  Documentation/virt/kvm/api.rst | 8 ++++++++
>  arch/arm64/kvm/arm.c           | 1 +
>  include/uapi/linux/kvm.h       | 1 +
>  3 files changed, 10 insertions(+)
> 
> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> index eb3a1316f03e..d2fb18613a34 100644
> --- a/Documentation/virt/kvm/api.rst
> +++ b/Documentation/virt/kvm/api.rst
> @@ -6159,3 +6159,11 @@ KVM can therefore start protected VMs.
>  This capability governs the KVM_S390_PV_COMMAND ioctl and the
>  KVM_MP_STATE_LOAD MP_STATE. KVM_SET_MP_STATE can fail for protected
>  guests when the state change is invalid.
> +
> +8.24 KVM_CAP_ARM_MP_AFFINITY
> +----------------------------
> +
> +:Architecture: arm64
> +
> +This capability indicates that KVM_ARM_SET_MP_AFFINITY ioctl is available.
> +It is used by to set MPIDR from userspace.
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 46dc3d75cf13..913c8da539b3 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -178,6 +178,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>  	case KVM_CAP_ARM_IRQ_LINE_LAYOUT_2:
>  	case KVM_CAP_ARM_NISV_TO_USER:
>  	case KVM_CAP_ARM_INJECT_EXT_DABT:
> +	case KVM_CAP_ARM_MP_AFFINITY:
>  		r = 1;

Caps should only return 1 after, or at the same time as, the feature is
introduced, otherwise bisection is broken.

>  		break;
>  	case KVM_CAP_ARM_SET_DEVICE_ADDR:
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index f6d86033c4fa..c4874905cd9c 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1035,6 +1035,7 @@ struct kvm_ppc_resize_hpt {
>  #define KVM_CAP_LAST_CPU 184
>  #define KVM_CAP_SMALLER_MAXPHYADDR 185
>  #define KVM_CAP_S390_DIAG318 186
> +#define KVM_CAP_ARM_MP_AFFINITY 187
>  
>  #ifdef KVM_CAP_IRQ_ROUTING
>  
> -- 
> 2.23.0
>

Thanks,
drew 

Re: [PATCH 1/2] KVM: arm64: add KVM_CAP_ARM_MP_AFFINITY extension

[Andrew Jones]

On Thu, Sep 17, 2020 at 10:30:32AM +0800, Ying Fang wrote:
> Add KVM_CAP_ARM_MP_AFFINITY extension for userspace to check
> whether KVM supports setting MPIDR on AArch64 platform. Thus
> we can give userspace control over the MPIDR to present
> cpu topology information.
> 
> Signed-off-by: Ying Fang <fangying1@huawei.com>
> ---
>  Documentation/virt/kvm/api.rst | 8 ++++++++
>  arch/arm64/kvm/arm.c           | 1 +
>  include/uapi/linux/kvm.h       | 1 +
>  3 files changed, 10 insertions(+)
> 
> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> index eb3a1316f03e..d2fb18613a34 100644
> --- a/Documentation/virt/kvm/api.rst
> +++ b/Documentation/virt/kvm/api.rst
> @@ -6159,3 +6159,11 @@ KVM can therefore start protected VMs.
>  This capability governs the KVM_S390_PV_COMMAND ioctl and the
>  KVM_MP_STATE_LOAD MP_STATE. KVM_SET_MP_STATE can fail for protected
>  guests when the state change is invalid.
> +
> +8.24 KVM_CAP_ARM_MP_AFFINITY
> +----------------------------
> +
> +:Architecture: arm64
> +
> +This capability indicates that KVM_ARM_SET_MP_AFFINITY ioctl is available.
> +It is used by to set MPIDR from userspace.
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 46dc3d75cf13..913c8da539b3 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -178,6 +178,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>  	case KVM_CAP_ARM_IRQ_LINE_LAYOUT_2:
>  	case KVM_CAP_ARM_NISV_TO_USER:
>  	case KVM_CAP_ARM_INJECT_EXT_DABT:
> +	case KVM_CAP_ARM_MP_AFFINITY:
>  		r = 1;

Caps should only return 1 after, or at the same time as, the feature is
introduced, otherwise bisection is broken.

>  		break;
>  	case KVM_CAP_ARM_SET_DEVICE_ADDR:
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index f6d86033c4fa..c4874905cd9c 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1035,6 +1035,7 @@ struct kvm_ppc_resize_hpt {
>  #define KVM_CAP_LAST_CPU 184
>  #define KVM_CAP_SMALLER_MAXPHYADDR 185
>  #define KVM_CAP_S390_DIAG318 186
> +#define KVM_CAP_ARM_MP_AFFINITY 187
>  
>  #ifdef KVM_CAP_IRQ_ROUTING
>  
> -- 
> 2.23.0
>

Thanks,
drew 

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

Re: [PATCH 2/2] kvm/arm: Add mp_affinity for arm vcpu

[Andrew Jones]

On Thu, Sep 17, 2020 at 10:30:33AM +0800, Ying Fang wrote:
> Allow userspace to set MPIDR using vcpu ioctl KVM_ARM_SET_MP_AFFINITY,
> so that we can support cpu topology for arm.
> 
> Signed-off-by: Ying Fang <fangying1@huawei.com>
> ---
>  arch/arm64/include/asm/kvm_host.h |  5 +++++
>  arch/arm64/kvm/arm.c              |  8 ++++++++
>  arch/arm64/kvm/reset.c            | 11 +++++++++++
>  arch/arm64/kvm/sys_regs.c         | 30 +++++++++++++++++++-----------
>  include/uapi/linux/kvm.h          |  2 ++
>  5 files changed, 45 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index e52c927aade5..7adc351ee70a 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -372,6 +372,9 @@ struct kvm_vcpu_arch {
>  		u64 last_steal;
>  		gpa_t base;
>  	} steal;
> +
> +	/* vCPU MP Affinity */
> +	u64 mp_affinity;

We probably don't need to introduce new state since we already have a
place for it in the sys_regs array.

>  };
>  
>  /* Pointer to the vcpu's SVE FFR for sve_{save,load}_state() */
> @@ -685,6 +688,8 @@ int kvm_arm_setup_stage2(struct kvm *kvm, unsigned long type);
>  int kvm_arm_vcpu_finalize(struct kvm_vcpu *vcpu, int feature);
>  bool kvm_arm_vcpu_is_finalized(struct kvm_vcpu *vcpu);
>  
> +int kvm_arm_vcpu_set_mp_affinity(struct kvm_vcpu *vcpu, uint64_t mpidr);
> +
>  #define kvm_arm_vcpu_sve_finalized(vcpu) \
>  	((vcpu)->arch.flags & KVM_ARM64_VCPU_SVE_FINALIZED)
>  
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 913c8da539b3..5f1fa625dc11 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -1178,6 +1178,14 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>  
>  		return kvm_arm_vcpu_finalize(vcpu, what);
>  	}
> +	case KVM_ARM_SET_MP_AFFINITY: {
> +		u64 mpidr;
> +
> +		if (get_user(mpidr, (const unsigned int __user *)argp))
> +			return -EFAULT;
> +
> +		return kvm_arm_vcpu_set_mp_affinity(vcpu, mpidr);
> +	}

Why introduce a new ioctl? We already have SET_ONE_REG.

>  	default:
>  		r = -EINVAL;
>  	}
> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> index ee33875c5c2a..4918c967b0c9 100644
> --- a/arch/arm64/kvm/reset.c
> +++ b/arch/arm64/kvm/reset.c
> @@ -188,6 +188,17 @@ int kvm_arm_vcpu_finalize(struct kvm_vcpu *vcpu, int feature)
>  	return -EINVAL;
>  }
>  
> +int kvm_arm_vcpu_set_mp_affinity(struct kvm_vcpu *vcpu, uint64_t mpidr)
> +{
> +	if (!(mpidr & (1ULL << 31))) {
> +		pr_warn("invalid mp_affinity format\n");
> +		return -EINVAL;
> +	}
> +
> +	vcpu->arch.mp_affinity = mpidr;

No sanity checks? If you look at what I once posted [*] you'll see that I
checked reserved bits, flags consistency, and ID uniqueness.

[*] https://www.spinics.net/lists/kvm-arm/msg23879.html

> +	return 0;
> +}
> +
>  bool kvm_arm_vcpu_is_finalized(struct kvm_vcpu *vcpu)
>  {
>  	if (vcpu_has_sve(vcpu) && !kvm_arm_vcpu_sve_finalized(vcpu))
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 077293b5115f..e76f483475ad 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -646,17 +646,25 @@ static void reset_mpidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
>  {
>  	u64 mpidr;
>  
> -	/*
> -	 * Map the vcpu_id into the first three affinity level fields of
> -	 * the MPIDR. We limit the number of VCPUs in level 0 due to a
> -	 * limitation to 16 CPUs in that level in the ICC_SGIxR registers
> -	 * of the GICv3 to be able to address each CPU directly when
> -	 * sending IPIs.
> -	 */
> -	mpidr = (vcpu->vcpu_id & 0x0f) << MPIDR_LEVEL_SHIFT(0);
> -	mpidr |= ((vcpu->vcpu_id >> 4) & 0xff) << MPIDR_LEVEL_SHIFT(1);
> -	mpidr |= ((vcpu->vcpu_id >> 12) & 0xff) << MPIDR_LEVEL_SHIFT(2);
> -	vcpu_write_sys_reg(vcpu, (1ULL << 31) | mpidr, MPIDR_EL1);
> +	if (vcpu->arch.mp_affinity) {
> +		/* If mp_affinity is set by userspace, it means an customized cpu
> +		 * topology is defined. Let it be MPIDR of the vcpu
> +		 */
> +		mpidr = vcpu->arch.mp_affinity;
> +	} else {
> +		/*
> +		 * Map the vcpu_id into the first three affinity level fields of
> +		 * the MPIDR. We limit the number of VCPUs in level 0 due to a
> +		 * limitation to 16 CPUs in that level in the ICC_SGIxR registers
> +		 * of the GICv3 to be able to address each CPU directly when
> +		 * sending IPIs.
> +		 */
> +		mpidr = (vcpu->vcpu_id & 0x0f) << MPIDR_LEVEL_SHIFT(0);
> +		mpidr |= ((vcpu->vcpu_id >> 4) & 0xff) << MPIDR_LEVEL_SHIFT(1);
> +		mpidr |= ((vcpu->vcpu_id >> 12) & 0xff) << MPIDR_LEVEL_SHIFT(2);
> +		mpidr |= (1ULL << 31);
> +	}
> +	vcpu_write_sys_reg(vcpu, mpidr, MPIDR_EL1);
>  }
>  
>  static void reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index c4874905cd9c..ae45876a689d 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1475,6 +1475,8 @@ struct kvm_s390_ucas_mapping {
>  #define KVM_S390_SET_CMMA_BITS      _IOW(KVMIO, 0xb9, struct kvm_s390_cmma_log)
>  /* Memory Encryption Commands */
>  #define KVM_MEMORY_ENCRYPT_OP      _IOWR(KVMIO, 0xba, unsigned long)
> +/* Available with KVM_CAP_ARM_MP_AFFINITY */
> +#define KVM_ARM_SET_MP_AFFINITY    _IOWR(KVMIO, 0xbb, unsigned long)
>  
>  struct kvm_enc_region {
>  	__u64 addr;
> -- 
> 2.23.0
> 

I suggest approaching this the same way Alex Graf approached PMCR in
https://www.spinics.net/lists/arm-kernel/msg837016.html but also add
sanity checks. The previous patch can also be squashed into this patch.

Thanks,
drew

Re: [PATCH 2/2] kvm/arm: Add mp_affinity for arm vcpu

[Andrew Jones]

On Thu, Sep 17, 2020 at 10:30:33AM +0800, Ying Fang wrote:
> Allow userspace to set MPIDR using vcpu ioctl KVM_ARM_SET_MP_AFFINITY,
> so that we can support cpu topology for arm.
> 
> Signed-off-by: Ying Fang <fangying1@huawei.com>
> ---
>  arch/arm64/include/asm/kvm_host.h |  5 +++++
>  arch/arm64/kvm/arm.c              |  8 ++++++++
>  arch/arm64/kvm/reset.c            | 11 +++++++++++
>  arch/arm64/kvm/sys_regs.c         | 30 +++++++++++++++++++-----------
>  include/uapi/linux/kvm.h          |  2 ++
>  5 files changed, 45 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index e52c927aade5..7adc351ee70a 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -372,6 +372,9 @@ struct kvm_vcpu_arch {
>  		u64 last_steal;
>  		gpa_t base;
>  	} steal;
> +
> +	/* vCPU MP Affinity */
> +	u64 mp_affinity;

We probably don't need to introduce new state since we already have a
place for it in the sys_regs array.

>  };
>  
>  /* Pointer to the vcpu's SVE FFR for sve_{save,load}_state() */
> @@ -685,6 +688,8 @@ int kvm_arm_setup_stage2(struct kvm *kvm, unsigned long type);
>  int kvm_arm_vcpu_finalize(struct kvm_vcpu *vcpu, int feature);
>  bool kvm_arm_vcpu_is_finalized(struct kvm_vcpu *vcpu);
>  
> +int kvm_arm_vcpu_set_mp_affinity(struct kvm_vcpu *vcpu, uint64_t mpidr);
> +
>  #define kvm_arm_vcpu_sve_finalized(vcpu) \
>  	((vcpu)->arch.flags & KVM_ARM64_VCPU_SVE_FINALIZED)
>  
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 913c8da539b3..5f1fa625dc11 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -1178,6 +1178,14 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>  
>  		return kvm_arm_vcpu_finalize(vcpu, what);
>  	}
> +	case KVM_ARM_SET_MP_AFFINITY: {
> +		u64 mpidr;
> +
> +		if (get_user(mpidr, (const unsigned int __user *)argp))
> +			return -EFAULT;
> +
> +		return kvm_arm_vcpu_set_mp_affinity(vcpu, mpidr);
> +	}

Why introduce a new ioctl? We already have SET_ONE_REG.

>  	default:
>  		r = -EINVAL;
>  	}
> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> index ee33875c5c2a..4918c967b0c9 100644
> --- a/arch/arm64/kvm/reset.c
> +++ b/arch/arm64/kvm/reset.c
> @@ -188,6 +188,17 @@ int kvm_arm_vcpu_finalize(struct kvm_vcpu *vcpu, int feature)
>  	return -EINVAL;
>  }
>  
> +int kvm_arm_vcpu_set_mp_affinity(struct kvm_vcpu *vcpu, uint64_t mpidr)
> +{
> +	if (!(mpidr & (1ULL << 31))) {
> +		pr_warn("invalid mp_affinity format\n");
> +		return -EINVAL;
> +	}
> +
> +	vcpu->arch.mp_affinity = mpidr;

No sanity checks? If you look at what I once posted [*] you'll see that I
checked reserved bits, flags consistency, and ID uniqueness.

[*] https://www.spinics.net/lists/kvm-arm/msg23879.html

> +	return 0;
> +}
> +
>  bool kvm_arm_vcpu_is_finalized(struct kvm_vcpu *vcpu)
>  {
>  	if (vcpu_has_sve(vcpu) && !kvm_arm_vcpu_sve_finalized(vcpu))
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 077293b5115f..e76f483475ad 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -646,17 +646,25 @@ static void reset_mpidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
>  {
>  	u64 mpidr;
>  
> -	/*
> -	 * Map the vcpu_id into the first three affinity level fields of
> -	 * the MPIDR. We limit the number of VCPUs in level 0 due to a
> -	 * limitation to 16 CPUs in that level in the ICC_SGIxR registers
> -	 * of the GICv3 to be able to address each CPU directly when
> -	 * sending IPIs.
> -	 */
> -	mpidr = (vcpu->vcpu_id & 0x0f) << MPIDR_LEVEL_SHIFT(0);
> -	mpidr |= ((vcpu->vcpu_id >> 4) & 0xff) << MPIDR_LEVEL_SHIFT(1);
> -	mpidr |= ((vcpu->vcpu_id >> 12) & 0xff) << MPIDR_LEVEL_SHIFT(2);
> -	vcpu_write_sys_reg(vcpu, (1ULL << 31) | mpidr, MPIDR_EL1);
> +	if (vcpu->arch.mp_affinity) {
> +		/* If mp_affinity is set by userspace, it means an customized cpu
> +		 * topology is defined. Let it be MPIDR of the vcpu
> +		 */
> +		mpidr = vcpu->arch.mp_affinity;
> +	} else {
> +		/*
> +		 * Map the vcpu_id into the first three affinity level fields of
> +		 * the MPIDR. We limit the number of VCPUs in level 0 due to a
> +		 * limitation to 16 CPUs in that level in the ICC_SGIxR registers
> +		 * of the GICv3 to be able to address each CPU directly when
> +		 * sending IPIs.
> +		 */
> +		mpidr = (vcpu->vcpu_id & 0x0f) << MPIDR_LEVEL_SHIFT(0);
> +		mpidr |= ((vcpu->vcpu_id >> 4) & 0xff) << MPIDR_LEVEL_SHIFT(1);
> +		mpidr |= ((vcpu->vcpu_id >> 12) & 0xff) << MPIDR_LEVEL_SHIFT(2);
> +		mpidr |= (1ULL << 31);
> +	}
> +	vcpu_write_sys_reg(vcpu, mpidr, MPIDR_EL1);
>  }
>  
>  static void reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index c4874905cd9c..ae45876a689d 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1475,6 +1475,8 @@ struct kvm_s390_ucas_mapping {
>  #define KVM_S390_SET_CMMA_BITS      _IOW(KVMIO, 0xb9, struct kvm_s390_cmma_log)
>  /* Memory Encryption Commands */
>  #define KVM_MEMORY_ENCRYPT_OP      _IOWR(KVMIO, 0xba, unsigned long)
> +/* Available with KVM_CAP_ARM_MP_AFFINITY */
> +#define KVM_ARM_SET_MP_AFFINITY    _IOWR(KVMIO, 0xbb, unsigned long)
>  
>  struct kvm_enc_region {
>  	__u64 addr;
> -- 
> 2.23.0
> 

I suggest approaching this the same way Alex Graf approached PMCR in
https://www.spinics.net/lists/arm-kernel/msg837016.html but also add
sanity checks. The previous patch can also be squashed into this patch.

Thanks,
drew

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

Re: [PATCH 2/2] kvm/arm: Add mp_affinity for arm vcpu

[Marc Zyngier]

On 2020-09-17 03:30, Ying Fang wrote:
> Allow userspace to set MPIDR using vcpu ioctl KVM_ARM_SET_MP_AFFINITY,
> so that we can support cpu topology for arm.

MPIDR has *nothing* to do with CPU topology in the ARM architecture.
I encourage you to have a look at the ARM ARM and find out how often
the word "topology" is used in conjunction with the MPIDR_EL1 register.

> 
> Signed-off-by: Ying Fang <fangying1@huawei.com>
> ---
>  arch/arm64/include/asm/kvm_host.h |  5 +++++
>  arch/arm64/kvm/arm.c              |  8 ++++++++
>  arch/arm64/kvm/reset.c            | 11 +++++++++++
>  arch/arm64/kvm/sys_regs.c         | 30 +++++++++++++++++++-----------
>  include/uapi/linux/kvm.h          |  2 ++
>  5 files changed, 45 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h
> b/arch/arm64/include/asm/kvm_host.h
> index e52c927aade5..7adc351ee70a 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -372,6 +372,9 @@ struct kvm_vcpu_arch {
>  		u64 last_steal;
>  		gpa_t base;
>  	} steal;
> +
> +	/* vCPU MP Affinity */
> +	u64 mp_affinity;

No. We already have a per-CPU MPIDR_EL1 register, we don't need another
piece of state.

>  };
> 
>  /* Pointer to the vcpu's SVE FFR for sve_{save,load}_state() */
> @@ -685,6 +688,8 @@ int kvm_arm_setup_stage2(struct kvm *kvm, unsigned
> long type);
>  int kvm_arm_vcpu_finalize(struct kvm_vcpu *vcpu, int feature);
>  bool kvm_arm_vcpu_is_finalized(struct kvm_vcpu *vcpu);
> 
> +int kvm_arm_vcpu_set_mp_affinity(struct kvm_vcpu *vcpu, uint64_t 
> mpidr);
> +
>  #define kvm_arm_vcpu_sve_finalized(vcpu) \
>  	((vcpu)->arch.flags & KVM_ARM64_VCPU_SVE_FINALIZED)
> 
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 913c8da539b3..5f1fa625dc11 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -1178,6 +1178,14 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
> 
>  		return kvm_arm_vcpu_finalize(vcpu, what);
>  	}
> +	case KVM_ARM_SET_MP_AFFINITY: {
> +		u64 mpidr;
> +
> +		if (get_user(mpidr, (const unsigned int __user *)argp))
> +			return -EFAULT;
> +
> +		return kvm_arm_vcpu_set_mp_affinity(vcpu, mpidr);
> +	}

That's not the way we access system registers from userspace.

>  	default:
>  		r = -EINVAL;
>  	}
> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> index ee33875c5c2a..4918c967b0c9 100644
> --- a/arch/arm64/kvm/reset.c
> +++ b/arch/arm64/kvm/reset.c
> @@ -188,6 +188,17 @@ int kvm_arm_vcpu_finalize(struct kvm_vcpu *vcpu,
> int feature)
>  	return -EINVAL;
>  }
> 
> +int kvm_arm_vcpu_set_mp_affinity(struct kvm_vcpu *vcpu, uint64_t 
> mpidr)
> +{
> +	if (!(mpidr & (1ULL << 31))) {
> +		pr_warn("invalid mp_affinity format\n");
> +		return -EINVAL;
> +	}
> +
> +	vcpu->arch.mp_affinity = mpidr;

This doesn't match the definition of the MPIDR_EL1 register. It also
doesn't take into account any of the existing restrictions for the
supported affinity levels and number of PEs at the lowest affinity
level.

> +	return 0;
> +}
> +
>  bool kvm_arm_vcpu_is_finalized(struct kvm_vcpu *vcpu)
>  {
>  	if (vcpu_has_sve(vcpu) && !kvm_arm_vcpu_sve_finalized(vcpu))
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 077293b5115f..e76f483475ad 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -646,17 +646,25 @@ static void reset_mpidr(struct kvm_vcpu *vcpu,
> const struct sys_reg_desc *r)
>  {
>  	u64 mpidr;
> 
> -	/*
> -	 * Map the vcpu_id into the first three affinity level fields of
> -	 * the MPIDR. We limit the number of VCPUs in level 0 due to a
> -	 * limitation to 16 CPUs in that level in the ICC_SGIxR registers
> -	 * of the GICv3 to be able to address each CPU directly when
> -	 * sending IPIs.
> -	 */
> -	mpidr = (vcpu->vcpu_id & 0x0f) << MPIDR_LEVEL_SHIFT(0);
> -	mpidr |= ((vcpu->vcpu_id >> 4) & 0xff) << MPIDR_LEVEL_SHIFT(1);
> -	mpidr |= ((vcpu->vcpu_id >> 12) & 0xff) << MPIDR_LEVEL_SHIFT(2);
> -	vcpu_write_sys_reg(vcpu, (1ULL << 31) | mpidr, MPIDR_EL1);
> +	if (vcpu->arch.mp_affinity) {
> +		/* If mp_affinity is set by userspace, it means an customized cpu
> +		 * topology is defined. Let it be MPIDR of the vcpu
> +		 */
> +		mpidr = vcpu->arch.mp_affinity;
> +	} else {
> +		/*
> +		 * Map the vcpu_id into the first three affinity level fields of
> +		 * the MPIDR. We limit the number of VCPUs in level 0 due to a
> +		 * limitation to 16 CPUs in that level in the ICC_SGIxR registers
> +		 * of the GICv3 to be able to address each CPU directly when
> +		 * sending IPIs.
> +		 */
> +		mpidr = (vcpu->vcpu_id & 0x0f) << MPIDR_LEVEL_SHIFT(0);
> +		mpidr |= ((vcpu->vcpu_id >> 4) & 0xff) << MPIDR_LEVEL_SHIFT(1);
> +		mpidr |= ((vcpu->vcpu_id >> 12) & 0xff) << MPIDR_LEVEL_SHIFT(2);
> +		mpidr |= (1ULL << 31);
> +	}
> +	vcpu_write_sys_reg(vcpu, mpidr, MPIDR_EL1);
>  }
> 
>  static void reset_pmcr(struct kvm_vcpu *vcpu, const struct 
> sys_reg_desc *r)
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index c4874905cd9c..ae45876a689d 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1475,6 +1475,8 @@ struct kvm_s390_ucas_mapping {
>  #define KVM_S390_SET_CMMA_BITS      _IOW(KVMIO, 0xb9, struct 
> kvm_s390_cmma_log)
>  /* Memory Encryption Commands */
>  #define KVM_MEMORY_ENCRYPT_OP      _IOWR(KVMIO, 0xba, unsigned long)
> +/* Available with KVM_CAP_ARM_MP_AFFINITY */
> +#define KVM_ARM_SET_MP_AFFINITY    _IOWR(KVMIO, 0xbb, unsigned long)
> 
>  struct kvm_enc_region {
>  	__u64 addr;

As it is, this patch is unacceptable. It ignores the requirements of the
architecture, as well as those of imposed by KVM as a platform.
It also pointlessly creates additional state and invents unnecessary
userspace interfaces. In short, it requires some *major* rework.

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

Re: [PATCH 2/2] kvm/arm: Add mp_affinity for arm vcpu

[Marc Zyngier]

On 2020-09-17 03:30, Ying Fang wrote:
> Allow userspace to set MPIDR using vcpu ioctl KVM_ARM_SET_MP_AFFINITY,
> so that we can support cpu topology for arm.

MPIDR has *nothing* to do with CPU topology in the ARM architecture.
I encourage you to have a look at the ARM ARM and find out how often
the word "topology" is used in conjunction with the MPIDR_EL1 register.

> 
> Signed-off-by: Ying Fang <fangying1@huawei.com>
> ---
>  arch/arm64/include/asm/kvm_host.h |  5 +++++
>  arch/arm64/kvm/arm.c              |  8 ++++++++
>  arch/arm64/kvm/reset.c            | 11 +++++++++++
>  arch/arm64/kvm/sys_regs.c         | 30 +++++++++++++++++++-----------
>  include/uapi/linux/kvm.h          |  2 ++
>  5 files changed, 45 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h
> b/arch/arm64/include/asm/kvm_host.h
> index e52c927aade5..7adc351ee70a 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -372,6 +372,9 @@ struct kvm_vcpu_arch {
>  		u64 last_steal;
>  		gpa_t base;
>  	} steal;
> +
> +	/* vCPU MP Affinity */
> +	u64 mp_affinity;

No. We already have a per-CPU MPIDR_EL1 register, we don't need another
piece of state.

>  };
> 
>  /* Pointer to the vcpu's SVE FFR for sve_{save,load}_state() */
> @@ -685,6 +688,8 @@ int kvm_arm_setup_stage2(struct kvm *kvm, unsigned
> long type);
>  int kvm_arm_vcpu_finalize(struct kvm_vcpu *vcpu, int feature);
>  bool kvm_arm_vcpu_is_finalized(struct kvm_vcpu *vcpu);
> 
> +int kvm_arm_vcpu_set_mp_affinity(struct kvm_vcpu *vcpu, uint64_t 
> mpidr);
> +
>  #define kvm_arm_vcpu_sve_finalized(vcpu) \
>  	((vcpu)->arch.flags & KVM_ARM64_VCPU_SVE_FINALIZED)
> 
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 913c8da539b3..5f1fa625dc11 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -1178,6 +1178,14 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
> 
>  		return kvm_arm_vcpu_finalize(vcpu, what);
>  	}
> +	case KVM_ARM_SET_MP_AFFINITY: {
> +		u64 mpidr;
> +
> +		if (get_user(mpidr, (const unsigned int __user *)argp))
> +			return -EFAULT;
> +
> +		return kvm_arm_vcpu_set_mp_affinity(vcpu, mpidr);
> +	}

That's not the way we access system registers from userspace.

>  	default:
>  		r = -EINVAL;
>  	}
> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> index ee33875c5c2a..4918c967b0c9 100644
> --- a/arch/arm64/kvm/reset.c
> +++ b/arch/arm64/kvm/reset.c
> @@ -188,6 +188,17 @@ int kvm_arm_vcpu_finalize(struct kvm_vcpu *vcpu,
> int feature)
>  	return -EINVAL;
>  }
> 
> +int kvm_arm_vcpu_set_mp_affinity(struct kvm_vcpu *vcpu, uint64_t 
> mpidr)
> +{
> +	if (!(mpidr & (1ULL << 31))) {
> +		pr_warn("invalid mp_affinity format\n");
> +		return -EINVAL;
> +	}
> +
> +	vcpu->arch.mp_affinity = mpidr;

This doesn't match the definition of the MPIDR_EL1 register. It also
doesn't take into account any of the existing restrictions for the
supported affinity levels and number of PEs at the lowest affinity
level.

> +	return 0;
> +}
> +
>  bool kvm_arm_vcpu_is_finalized(struct kvm_vcpu *vcpu)
>  {
>  	if (vcpu_has_sve(vcpu) && !kvm_arm_vcpu_sve_finalized(vcpu))
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 077293b5115f..e76f483475ad 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -646,17 +646,25 @@ static void reset_mpidr(struct kvm_vcpu *vcpu,
> const struct sys_reg_desc *r)
>  {
>  	u64 mpidr;
> 
> -	/*
> -	 * Map the vcpu_id into the first three affinity level fields of
> -	 * the MPIDR. We limit the number of VCPUs in level 0 due to a
> -	 * limitation to 16 CPUs in that level in the ICC_SGIxR registers
> -	 * of the GICv3 to be able to address each CPU directly when
> -	 * sending IPIs.
> -	 */
> -	mpidr = (vcpu->vcpu_id & 0x0f) << MPIDR_LEVEL_SHIFT(0);
> -	mpidr |= ((vcpu->vcpu_id >> 4) & 0xff) << MPIDR_LEVEL_SHIFT(1);
> -	mpidr |= ((vcpu->vcpu_id >> 12) & 0xff) << MPIDR_LEVEL_SHIFT(2);
> -	vcpu_write_sys_reg(vcpu, (1ULL << 31) | mpidr, MPIDR_EL1);
> +	if (vcpu->arch.mp_affinity) {
> +		/* If mp_affinity is set by userspace, it means an customized cpu
> +		 * topology is defined. Let it be MPIDR of the vcpu
> +		 */
> +		mpidr = vcpu->arch.mp_affinity;
> +	} else {
> +		/*
> +		 * Map the vcpu_id into the first three affinity level fields of
> +		 * the MPIDR. We limit the number of VCPUs in level 0 due to a
> +		 * limitation to 16 CPUs in that level in the ICC_SGIxR registers
> +		 * of the GICv3 to be able to address each CPU directly when
> +		 * sending IPIs.
> +		 */
> +		mpidr = (vcpu->vcpu_id & 0x0f) << MPIDR_LEVEL_SHIFT(0);
> +		mpidr |= ((vcpu->vcpu_id >> 4) & 0xff) << MPIDR_LEVEL_SHIFT(1);
> +		mpidr |= ((vcpu->vcpu_id >> 12) & 0xff) << MPIDR_LEVEL_SHIFT(2);
> +		mpidr |= (1ULL << 31);
> +	}
> +	vcpu_write_sys_reg(vcpu, mpidr, MPIDR_EL1);
>  }
> 
>  static void reset_pmcr(struct kvm_vcpu *vcpu, const struct 
> sys_reg_desc *r)
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index c4874905cd9c..ae45876a689d 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1475,6 +1475,8 @@ struct kvm_s390_ucas_mapping {
>  #define KVM_S390_SET_CMMA_BITS      _IOW(KVMIO, 0xb9, struct 
> kvm_s390_cmma_log)
>  /* Memory Encryption Commands */
>  #define KVM_MEMORY_ENCRYPT_OP      _IOWR(KVMIO, 0xba, unsigned long)
> +/* Available with KVM_CAP_ARM_MP_AFFINITY */
> +#define KVM_ARM_SET_MP_AFFINITY    _IOWR(KVMIO, 0xbb, unsigned long)
> 
>  struct kvm_enc_region {
>  	__u64 addr;

As it is, this patch is unacceptable. It ignores the requirements of the
architecture, as well as those of imposed by KVM as a platform.
It also pointlessly creates additional state and invents unnecessary
userspace interfaces. In short, it requires some *major* rework.

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

Re: [PATCH 2/2] kvm/arm: Add mp_affinity for arm vcpu

[Andrew Jones]

On Thu, Sep 17, 2020 at 08:47:42AM +0100, Marc Zyngier wrote:
> On 2020-09-17 03:30, Ying Fang wrote:
> > Allow userspace to set MPIDR using vcpu ioctl KVM_ARM_SET_MP_AFFINITY,
> > so that we can support cpu topology for arm.
> 
> MPIDR has *nothing* to do with CPU topology in the ARM architecture.
> I encourage you to have a look at the ARM ARM and find out how often
> the word "topology" is used in conjunction with the MPIDR_EL1 register.
>

Hi Marc,

I mostly agree. However, the CPU topology descriptions use MPIDR to
identify PEs. If userspace wants to build topology descriptions then
it either needs to

1) build them after instantiating all KVM VCPUs in order to query KVM
   for each MPIDR, or
2) have a way to ask KVM for an MPIDR of given VCPU ID in advance
   (maybe just a scratch VCPU), or
3) have control over the MPIDRs so it can choose them when it likes,
   use them for topology descriptions, and then instantiate KVM VCPUs
   with them.

I think (3) is the most robust approach, and it has the least overhead.

Thanks,
drew

Re: [PATCH 2/2] kvm/arm: Add mp_affinity for arm vcpu

[Andrew Jones]

On Thu, Sep 17, 2020 at 08:47:42AM +0100, Marc Zyngier wrote:
> On 2020-09-17 03:30, Ying Fang wrote:
> > Allow userspace to set MPIDR using vcpu ioctl KVM_ARM_SET_MP_AFFINITY,
> > so that we can support cpu topology for arm.
> 
> MPIDR has *nothing* to do with CPU topology in the ARM architecture.
> I encourage you to have a look at the ARM ARM and find out how often
> the word "topology" is used in conjunction with the MPIDR_EL1 register.
>

Hi Marc,

I mostly agree. However, the CPU topology descriptions use MPIDR to
identify PEs. If userspace wants to build topology descriptions then
it either needs to

1) build them after instantiating all KVM VCPUs in order to query KVM
   for each MPIDR, or
2) have a way to ask KVM for an MPIDR of given VCPU ID in advance
   (maybe just a scratch VCPU), or
3) have control over the MPIDRs so it can choose them when it likes,
   use them for topology descriptions, and then instantiate KVM VCPUs
   with them.

I think (3) is the most robust approach, and it has the least overhead.

Thanks,
drew

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

Re: [PATCH 2/2] kvm/arm: Add mp_affinity for arm vcpu

[Marc Zyngier]

On 2020-09-17 09:04, Andrew Jones wrote:
> On Thu, Sep 17, 2020 at 08:47:42AM +0100, Marc Zyngier wrote:
>> On 2020-09-17 03:30, Ying Fang wrote:
>> > Allow userspace to set MPIDR using vcpu ioctl KVM_ARM_SET_MP_AFFINITY,
>> > so that we can support cpu topology for arm.
>> 
>> MPIDR has *nothing* to do with CPU topology in the ARM architecture.
>> I encourage you to have a look at the ARM ARM and find out how often
>> the word "topology" is used in conjunction with the MPIDR_EL1 
>> register.
>> 
> 
> Hi Marc,
> 
> I mostly agree. However, the CPU topology descriptions use MPIDR to
> identify PEs. If userspace wants to build topology descriptions then
> it either needs to
> 
> 1) build them after instantiating all KVM VCPUs in order to query KVM
>    for each MPIDR, or
> 2) have a way to ask KVM for an MPIDR of given VCPU ID in advance
>    (maybe just a scratch VCPU), or
> 3) have control over the MPIDRs so it can choose them when it likes,
>    use them for topology descriptions, and then instantiate KVM VCPUs
>    with them.
> 
> I think (3) is the most robust approach, and it has the least overhead.

I don't disagree with the goal, and not even with the choice of
implementation (though I have huge reservations about its quality).

But the key word here is *userspace*. Only userspace has a notion of
how MPIDR values map to the assumed topology. That's not something
that KVM does nor should interpret (aside from the GIC-induced Aff0
brain-damage). So talking of "topology" in a KVM kernel patch sends
the wrong message, and that's all this remark was about.

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

Re: [PATCH 2/2] kvm/arm: Add mp_affinity for arm vcpu

[Marc Zyngier]

On 2020-09-17 09:04, Andrew Jones wrote:
> On Thu, Sep 17, 2020 at 08:47:42AM +0100, Marc Zyngier wrote:
>> On 2020-09-17 03:30, Ying Fang wrote:
>> > Allow userspace to set MPIDR using vcpu ioctl KVM_ARM_SET_MP_AFFINITY,
>> > so that we can support cpu topology for arm.
>> 
>> MPIDR has *nothing* to do with CPU topology in the ARM architecture.
>> I encourage you to have a look at the ARM ARM and find out how often
>> the word "topology" is used in conjunction with the MPIDR_EL1 
>> register.
>> 
> 
> Hi Marc,
> 
> I mostly agree. However, the CPU topology descriptions use MPIDR to
> identify PEs. If userspace wants to build topology descriptions then
> it either needs to
> 
> 1) build them after instantiating all KVM VCPUs in order to query KVM
>    for each MPIDR, or
> 2) have a way to ask KVM for an MPIDR of given VCPU ID in advance
>    (maybe just a scratch VCPU), or
> 3) have control over the MPIDRs so it can choose them when it likes,
>    use them for topology descriptions, and then instantiate KVM VCPUs
>    with them.
> 
> I think (3) is the most robust approach, and it has the least overhead.

I don't disagree with the goal, and not even with the choice of
implementation (though I have huge reservations about its quality).

But the key word here is *userspace*. Only userspace has a notion of
how MPIDR values map to the assumed topology. That's not something
that KVM does nor should interpret (aside from the GIC-induced Aff0
brain-damage). So talking of "topology" in a KVM kernel patch sends
the wrong message, and that's all this remark was about.

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

Re: [PATCH 2/2] kvm/arm: Add mp_affinity for arm vcpu

[Alexandru Elisei]

Hi,

On 9/17/20 9:42 AM, Marc Zyngier wrote:
> On 2020-09-17 09:04, Andrew Jones wrote:
>> On Thu, Sep 17, 2020 at 08:47:42AM +0100, Marc Zyngier wrote:
>>> On 2020-09-17 03:30, Ying Fang wrote:
>>> > Allow userspace to set MPIDR using vcpu ioctl KVM_ARM_SET_MP_AFFINITY,
>>> > so that we can support cpu topology for arm.
>>>
>>> MPIDR has *nothing* to do with CPU topology in the ARM architecture.
>>> I encourage you to have a look at the ARM ARM and find out how often
>>> the word "topology" is used in conjunction with the MPIDR_EL1 register.
>>>
>>
>> Hi Marc,
>>
>> I mostly agree. However, the CPU topology descriptions use MPIDR to
>> identify PEs. If userspace wants to build topology descriptions then
>> it either needs to
>>
>> 1) build them after instantiating all KVM VCPUs in order to query KVM
>>    for each MPIDR, or
>> 2) have a way to ask KVM for an MPIDR of given VCPU ID in advance
>>    (maybe just a scratch VCPU), or
>> 3) have control over the MPIDRs so it can choose them when it likes,
>>    use them for topology descriptions, and then instantiate KVM VCPUs
>>    with them.
>>
>> I think (3) is the most robust approach, and it has the least overhead.
>
> I don't disagree with the goal, and not even with the choice of
> implementation (though I have huge reservations about its quality).
>
> But the key word here is *userspace*. Only userspace has a notion of
> how MPIDR values map to the assumed topology. That's not something
> that KVM does nor should interpret (aside from the GIC-induced Aff0
> brain-damage). So talking of "topology" in a KVM kernel patch sends
> the wrong message, and that's all this remark was about.

There's also a patch queued for next which removes using MPIDR as a source of
information about CPU topology [1]: "arm64: topology: Stop using MPIDR for
topology information".

I'm not really sure how useful KVM fiddling with the guest MPIDR will be going
forward, at least for a Linux guest.

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git/commit/?id=3102bc0e6ac7

Thanks,
Alex

Re: [PATCH 2/2] kvm/arm: Add mp_affinity for arm vcpu

[Alexandru Elisei]

Hi,

On 9/17/20 9:42 AM, Marc Zyngier wrote:
> On 2020-09-17 09:04, Andrew Jones wrote:
>> On Thu, Sep 17, 2020 at 08:47:42AM +0100, Marc Zyngier wrote:
>>> On 2020-09-17 03:30, Ying Fang wrote:
>>> > Allow userspace to set MPIDR using vcpu ioctl KVM_ARM_SET_MP_AFFINITY,
>>> > so that we can support cpu topology for arm.
>>>
>>> MPIDR has *nothing* to do with CPU topology in the ARM architecture.
>>> I encourage you to have a look at the ARM ARM and find out how often
>>> the word "topology" is used in conjunction with the MPIDR_EL1 register.
>>>
>>
>> Hi Marc,
>>
>> I mostly agree. However, the CPU topology descriptions use MPIDR to
>> identify PEs. If userspace wants to build topology descriptions then
>> it either needs to
>>
>> 1) build them after instantiating all KVM VCPUs in order to query KVM
>>    for each MPIDR, or
>> 2) have a way to ask KVM for an MPIDR of given VCPU ID in advance
>>    (maybe just a scratch VCPU), or
>> 3) have control over the MPIDRs so it can choose them when it likes,
>>    use them for topology descriptions, and then instantiate KVM VCPUs
>>    with them.
>>
>> I think (3) is the most robust approach, and it has the least overhead.
>
> I don't disagree with the goal, and not even with the choice of
> implementation (though I have huge reservations about its quality).
>
> But the key word here is *userspace*. Only userspace has a notion of
> how MPIDR values map to the assumed topology. That's not something
> that KVM does nor should interpret (aside from the GIC-induced Aff0
> brain-damage). So talking of "topology" in a KVM kernel patch sends
> the wrong message, and that's all this remark was about.

There's also a patch queued for next which removes using MPIDR as a source of
information about CPU topology [1]: "arm64: topology: Stop using MPIDR for
topology information".

I'm not really sure how useful KVM fiddling with the guest MPIDR will be going
forward, at least for a Linux guest.

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git/commit/?id=3102bc0e6ac7

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

Re: [PATCH 2/2] kvm/arm: Add mp_affinity for arm vcpu

[Marc Zyngier]

On 2020-09-17 10:47, Alexandru Elisei wrote:
> Hi,
> 
> On 9/17/20 9:42 AM, Marc Zyngier wrote:
>> On 2020-09-17 09:04, Andrew Jones wrote:
>>> On Thu, Sep 17, 2020 at 08:47:42AM +0100, Marc Zyngier wrote:
>>>> On 2020-09-17 03:30, Ying Fang wrote:
>>>> > Allow userspace to set MPIDR using vcpu ioctl KVM_ARM_SET_MP_AFFINITY,
>>>> > so that we can support cpu topology for arm.
>>>> 
>>>> MPIDR has *nothing* to do with CPU topology in the ARM architecture.
>>>> I encourage you to have a look at the ARM ARM and find out how often
>>>> the word "topology" is used in conjunction with the MPIDR_EL1 
>>>> register.
>>>> 
>>> 
>>> Hi Marc,
>>> 
>>> I mostly agree. However, the CPU topology descriptions use MPIDR to
>>> identify PEs. If userspace wants to build topology descriptions then
>>> it either needs to
>>> 
>>> 1) build them after instantiating all KVM VCPUs in order to query KVM
>>>    for each MPIDR, or
>>> 2) have a way to ask KVM for an MPIDR of given VCPU ID in advance
>>>    (maybe just a scratch VCPU), or
>>> 3) have control over the MPIDRs so it can choose them when it likes,
>>>    use them for topology descriptions, and then instantiate KVM VCPUs
>>>    with them.
>>> 
>>> I think (3) is the most robust approach, and it has the least 
>>> overhead.
>> 
>> I don't disagree with the goal, and not even with the choice of
>> implementation (though I have huge reservations about its quality).
>> 
>> But the key word here is *userspace*. Only userspace has a notion of
>> how MPIDR values map to the assumed topology. That's not something
>> that KVM does nor should interpret (aside from the GIC-induced Aff0
>> brain-damage). So talking of "topology" in a KVM kernel patch sends
>> the wrong message, and that's all this remark was about.
> 
> There's also a patch queued for next which removes using MPIDR as a 
> source of
> information about CPU topology [1]: "arm64: topology: Stop using MPIDR 
> for
> topology information".
> 
> I'm not really sure how useful KVM fiddling with the guest MPIDR will 
> be going
> forward, at least for a Linux guest.

I think these are two orthogonal things. There is value in setting MPIDR
to something different as a way to replicate an existing system, for
example. But deriving *any* sort of topology information from MPIDR 
isn't
reliable at all, and is better expressed by firmware tables (and even
that isn't great).

As far as I am concerned, this patch fits in the "cosmetic" department.
It's a "nice to have", but doesn't really solve much. Firmware tables
and userspace placement of the vcpus are key.

Thanks,

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

Re: [PATCH 2/2] kvm/arm: Add mp_affinity for arm vcpu

[Marc Zyngier]

On 2020-09-17 10:47, Alexandru Elisei wrote:
> Hi,
> 
> On 9/17/20 9:42 AM, Marc Zyngier wrote:
>> On 2020-09-17 09:04, Andrew Jones wrote:
>>> On Thu, Sep 17, 2020 at 08:47:42AM +0100, Marc Zyngier wrote:
>>>> On 2020-09-17 03:30, Ying Fang wrote:
>>>> > Allow userspace to set MPIDR using vcpu ioctl KVM_ARM_SET_MP_AFFINITY,
>>>> > so that we can support cpu topology for arm.
>>>> 
>>>> MPIDR has *nothing* to do with CPU topology in the ARM architecture.
>>>> I encourage you to have a look at the ARM ARM and find out how often
>>>> the word "topology" is used in conjunction with the MPIDR_EL1 
>>>> register.
>>>> 
>>> 
>>> Hi Marc,
>>> 
>>> I mostly agree. However, the CPU topology descriptions use MPIDR to
>>> identify PEs. If userspace wants to build topology descriptions then
>>> it either needs to
>>> 
>>> 1) build them after instantiating all KVM VCPUs in order to query KVM
>>>    for each MPIDR, or
>>> 2) have a way to ask KVM for an MPIDR of given VCPU ID in advance
>>>    (maybe just a scratch VCPU), or
>>> 3) have control over the MPIDRs so it can choose them when it likes,
>>>    use them for topology descriptions, and then instantiate KVM VCPUs
>>>    with them.
>>> 
>>> I think (3) is the most robust approach, and it has the least 
>>> overhead.
>> 
>> I don't disagree with the goal, and not even with the choice of
>> implementation (though I have huge reservations about its quality).
>> 
>> But the key word here is *userspace*. Only userspace has a notion of
>> how MPIDR values map to the assumed topology. That's not something
>> that KVM does nor should interpret (aside from the GIC-induced Aff0
>> brain-damage). So talking of "topology" in a KVM kernel patch sends
>> the wrong message, and that's all this remark was about.
> 
> There's also a patch queued for next which removes using MPIDR as a 
> source of
> information about CPU topology [1]: "arm64: topology: Stop using MPIDR 
> for
> topology information".
> 
> I'm not really sure how useful KVM fiddling with the guest MPIDR will 
> be going
> forward, at least for a Linux guest.

I think these are two orthogonal things. There is value in setting MPIDR
to something different as a way to replicate an existing system, for
example. But deriving *any* sort of topology information from MPIDR 
isn't
reliable at all, and is better expressed by firmware tables (and even
that isn't great).

As far as I am concerned, this patch fits in the "cosmetic" department.
It's a "nice to have", but doesn't really solve much. Firmware tables
and userspace placement of the vcpus are key.

Thanks,

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

Re: [PATCH 2/2] kvm/arm: Add mp_affinity for arm vcpu

[Andrew Jones]

On Thu, Sep 17, 2020 at 11:01:39AM +0100, Marc Zyngier wrote:
> On 2020-09-17 10:47, Alexandru Elisei wrote:
> > Hi,
> > 
> > On 9/17/20 9:42 AM, Marc Zyngier wrote:
> > > On 2020-09-17 09:04, Andrew Jones wrote:
> > > > On Thu, Sep 17, 2020 at 08:47:42AM +0100, Marc Zyngier wrote:
> > > > > On 2020-09-17 03:30, Ying Fang wrote:
> > > > > > Allow userspace to set MPIDR using vcpu ioctl KVM_ARM_SET_MP_AFFINITY,
> > > > > > so that we can support cpu topology for arm.
> > > > > 
> > > > > MPIDR has *nothing* to do with CPU topology in the ARM architecture.
> > > > > I encourage you to have a look at the ARM ARM and find out how often
> > > > > the word "topology" is used in conjunction with the
> > > > > MPIDR_EL1 register.
> > > > > 
> > > > 
> > > > Hi Marc,
> > > > 
> > > > I mostly agree. However, the CPU topology descriptions use MPIDR to
> > > > identify PEs. If userspace wants to build topology descriptions then
> > > > it either needs to
> > > > 
> > > > 1) build them after instantiating all KVM VCPUs in order to query KVM
> > > >    for each MPIDR, or
> > > > 2) have a way to ask KVM for an MPIDR of given VCPU ID in advance
> > > >    (maybe just a scratch VCPU), or
> > > > 3) have control over the MPIDRs so it can choose them when it likes,
> > > >    use them for topology descriptions, and then instantiate KVM VCPUs
> > > >    with them.
> > > > 
> > > > I think (3) is the most robust approach, and it has the least
> > > > overhead.
> > > 
> > > I don't disagree with the goal, and not even with the choice of
> > > implementation (though I have huge reservations about its quality).
> > > 
> > > But the key word here is *userspace*. Only userspace has a notion of
> > > how MPIDR values map to the assumed topology. That's not something
> > > that KVM does nor should interpret (aside from the GIC-induced Aff0
> > > brain-damage). So talking of "topology" in a KVM kernel patch sends
> > > the wrong message, and that's all this remark was about.
> > 
> > There's also a patch queued for next which removes using MPIDR as a
> > source of
> > information about CPU topology [1]: "arm64: topology: Stop using MPIDR
> > for
> > topology information".
> > 
> > I'm not really sure how useful KVM fiddling with the guest MPIDR will be
> > going
> > forward, at least for a Linux guest.
> 
> I think these are two orthogonal things. There is value in setting MPIDR
> to something different as a way to replicate an existing system, for
> example. But deriving *any* sort of topology information from MPIDR isn't
> reliable at all, and is better expressed by firmware tables (and even
> that isn't great).
> 

Yes, this is my opinion as well and I'm glad to see the patch that
Alexandru pointed out, since it should stop the MPIDR abuse. Ying Fang
has also posted a QEMU series that populates DT and ACPI[*] to describe
CPU topology to the guest. The user controlled MPIDR is being proposed
in order to support that series.

[*] https://lists.gnu.org/archive/html/qemu-devel/2020-09/msg06027.html

Thanks,
drew

> As far as I am concerned, this patch fits in the "cosmetic" department.
> It's a "nice to have", but doesn't really solve much. Firmware tables
> and userspace placement of the vcpus are key.
> 
> Thanks,
> 
>         M.
> -- 
> Jazz is not dead. It just smells funny...
> 

Re: [PATCH 2/2] kvm/arm: Add mp_affinity for arm vcpu

[Andrew Jones]

On Thu, Sep 17, 2020 at 11:01:39AM +0100, Marc Zyngier wrote:
> On 2020-09-17 10:47, Alexandru Elisei wrote:
> > Hi,
> > 
> > On 9/17/20 9:42 AM, Marc Zyngier wrote:
> > > On 2020-09-17 09:04, Andrew Jones wrote:
> > > > On Thu, Sep 17, 2020 at 08:47:42AM +0100, Marc Zyngier wrote:
> > > > > On 2020-09-17 03:30, Ying Fang wrote:
> > > > > > Allow userspace to set MPIDR using vcpu ioctl KVM_ARM_SET_MP_AFFINITY,
> > > > > > so that we can support cpu topology for arm.
> > > > > 
> > > > > MPIDR has *nothing* to do with CPU topology in the ARM architecture.
> > > > > I encourage you to have a look at the ARM ARM and find out how often
> > > > > the word "topology" is used in conjunction with the
> > > > > MPIDR_EL1 register.
> > > > > 
> > > > 
> > > > Hi Marc,
> > > > 
> > > > I mostly agree. However, the CPU topology descriptions use MPIDR to
> > > > identify PEs. If userspace wants to build topology descriptions then
> > > > it either needs to
> > > > 
> > > > 1) build them after instantiating all KVM VCPUs in order to query KVM
> > > >    for each MPIDR, or
> > > > 2) have a way to ask KVM for an MPIDR of given VCPU ID in advance
> > > >    (maybe just a scratch VCPU), or
> > > > 3) have control over the MPIDRs so it can choose them when it likes,
> > > >    use them for topology descriptions, and then instantiate KVM VCPUs
> > > >    with them.
> > > > 
> > > > I think (3) is the most robust approach, and it has the least
> > > > overhead.
> > > 
> > > I don't disagree with the goal, and not even with the choice of
> > > implementation (though I have huge reservations about its quality).
> > > 
> > > But the key word here is *userspace*. Only userspace has a notion of
> > > how MPIDR values map to the assumed topology. That's not something
> > > that KVM does nor should interpret (aside from the GIC-induced Aff0
> > > brain-damage). So talking of "topology" in a KVM kernel patch sends
> > > the wrong message, and that's all this remark was about.
> > 
> > There's also a patch queued for next which removes using MPIDR as a
> > source of
> > information about CPU topology [1]: "arm64: topology: Stop using MPIDR
> > for
> > topology information".
> > 
> > I'm not really sure how useful KVM fiddling with the guest MPIDR will be
> > going
> > forward, at least for a Linux guest.
> 
> I think these are two orthogonal things. There is value in setting MPIDR
> to something different as a way to replicate an existing system, for
> example. But deriving *any* sort of topology information from MPIDR isn't
> reliable at all, and is better expressed by firmware tables (and even
> that isn't great).
> 

Yes, this is my opinion as well and I'm glad to see the patch that
Alexandru pointed out, since it should stop the MPIDR abuse. Ying Fang
has also posted a QEMU series that populates DT and ACPI[*] to describe
CPU topology to the guest. The user controlled MPIDR is being proposed
in order to support that series.

[*] https://lists.gnu.org/archive/html/qemu-devel/2020-09/msg06027.html

Thanks,
drew

> As far as I am concerned, this patch fits in the "cosmetic" department.
> It's a "nice to have", but doesn't really solve much. Firmware tables
> and userspace placement of the vcpus are key.
> 
> Thanks,
> 
>         M.
> -- 
> Jazz is not dead. It just smells funny...
> 

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

Re: [PATCH 2/2] kvm/arm: Add mp_affinity for arm vcpu

[Ying Fang]

On 9/17/2020 3:47 PM, Marc Zyngier wrote:
> On 2020-09-17 03:30, Ying Fang wrote:
>> Allow userspace to set MPIDR using vcpu ioctl KVM_ARM_SET_MP_AFFINITY,
>> so that we can support cpu topology for arm.
> 
> MPIDR has *nothing* to do with CPU topology in the ARM architecture.
> I encourage you to have a look at the ARM ARM and find out how often
> the word "topology" is used in conjunction with the MPIDR_EL1 register.
> 
>>
>> Signed-off-by: Ying Fang <fangying1@huawei.com>
>> ---
>>  arch/arm64/include/asm/kvm_host.h |  5 +++++
>>  arch/arm64/kvm/arm.c              |  8 ++++++++
>>  arch/arm64/kvm/reset.c            | 11 +++++++++++
>>  arch/arm64/kvm/sys_regs.c         | 30 +++++++++++++++++++-----------
>>  include/uapi/linux/kvm.h          |  2 ++
>>  5 files changed, 45 insertions(+), 11 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/kvm_host.h
>> b/arch/arm64/include/asm/kvm_host.h
>> index e52c927aade5..7adc351ee70a 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -372,6 +372,9 @@ struct kvm_vcpu_arch {
>>          u64 last_steal;
>>          gpa_t base;
>>      } steal;
>> +
>> +    /* vCPU MP Affinity */
>> +    u64 mp_affinity;
> 
> No. We already have a per-CPU MPIDR_EL1 register, we don't need another
> piece of state.
I am also not sure if it is needed here.
> 
>>  };
>>
>>  /* Pointer to the vcpu's SVE FFR for sve_{save,load}_state() */
>> @@ -685,6 +688,8 @@ int kvm_arm_setup_stage2(struct kvm *kvm, unsigned
>> long type);
>>  int kvm_arm_vcpu_finalize(struct kvm_vcpu *vcpu, int feature);
>>  bool kvm_arm_vcpu_is_finalized(struct kvm_vcpu *vcpu);
>>
>> +int kvm_arm_vcpu_set_mp_affinity(struct kvm_vcpu *vcpu, uint64_t mpidr);
>> +
>>  #define kvm_arm_vcpu_sve_finalized(vcpu) \
>>      ((vcpu)->arch.flags & KVM_ARM64_VCPU_SVE_FINALIZED)
>>
>> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
>> index 913c8da539b3..5f1fa625dc11 100644
>> --- a/arch/arm64/kvm/arm.c
>> +++ b/arch/arm64/kvm/arm.c
>> @@ -1178,6 +1178,14 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>>
>>          return kvm_arm_vcpu_finalize(vcpu, what);
>>      }
>> +    case KVM_ARM_SET_MP_AFFINITY: {
>> +        u64 mpidr;
>> +
>> +        if (get_user(mpidr, (const unsigned int __user *)argp))
>> +            return -EFAULT;
>> +
>> +        return kvm_arm_vcpu_set_mp_affinity(vcpu, mpidr);
>> +    }
> 
> That's not the way we access system registers from userspace.

I know that we already had the KVM_SET_ONE_REG api and well designed
sys_regs subsystem.

However when I tried to set MPIDR using the KVM_SET_ONE_REG api in 
kvm_arch_init_vcpu, the value may get erased by cpu_reset called within 
arm_cpu_realizefn. Maybe we should make change on qemu instead of adding
this redundant ioctl.

> 
>>      default:
>>          r = -EINVAL;
>>      }
>> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
>> index ee33875c5c2a..4918c967b0c9 100644
>> --- a/arch/arm64/kvm/reset.c
>> +++ b/arch/arm64/kvm/reset.c
>> @@ -188,6 +188,17 @@ int kvm_arm_vcpu_finalize(struct kvm_vcpu *vcpu,
>> int feature)
>>      return -EINVAL;
>>  }
>>
>> +int kvm_arm_vcpu_set_mp_affinity(struct kvm_vcpu *vcpu, uint64_t mpidr)
>> +{
>> +    if (!(mpidr & (1ULL << 31))) {
>> +        pr_warn("invalid mp_affinity format\n");
>> +        return -EINVAL;
>> +    }
>> +
>> +    vcpu->arch.mp_affinity = mpidr;
> 
> This doesn't match the definition of the MPIDR_EL1 register. It also
> doesn't take into account any of the existing restrictions for the
> supported affinity levels and number of PEs at the lowest affinity
> level.
Yes, we should do sanity check here and we must keep the format of MPIDR
following the Spec definition.

> 
>> +    return 0;
>> +}
>> +
>>  bool kvm_arm_vcpu_is_finalized(struct kvm_vcpu *vcpu)
>>  {
>>      if (vcpu_has_sve(vcpu) && !kvm_arm_vcpu_sve_finalized(vcpu))
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index 077293b5115f..e76f483475ad 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -646,17 +646,25 @@ static void reset_mpidr(struct kvm_vcpu *vcpu,
>> const struct sys_reg_desc *r)
>>  {
>>      u64 mpidr;
>>
>> -    /*
>> -     * Map the vcpu_id into the first three affinity level fields of
>> -     * the MPIDR. We limit the number of VCPUs in level 0 due to a
>> -     * limitation to 16 CPUs in that level in the ICC_SGIxR registers
>> -     * of the GICv3 to be able to address each CPU directly when
>> -     * sending IPIs.
>> -     */
>> -    mpidr = (vcpu->vcpu_id & 0x0f) << MPIDR_LEVEL_SHIFT(0);
>> -    mpidr |= ((vcpu->vcpu_id >> 4) & 0xff) << MPIDR_LEVEL_SHIFT(1);
>> -    mpidr |= ((vcpu->vcpu_id >> 12) & 0xff) << MPIDR_LEVEL_SHIFT(2);
>> -    vcpu_write_sys_reg(vcpu, (1ULL << 31) | mpidr, MPIDR_EL1);
>> +    if (vcpu->arch.mp_affinity) {
>> +        /* If mp_affinity is set by userspace, it means an customized 
>> cpu
>> +         * topology is defined. Let it be MPIDR of the vcpu
>> +         */
>> +        mpidr = vcpu->arch.mp_affinity;
>> +    } else {
>> +        /*
>> +         * Map the vcpu_id into the first three affinity level fields of
>> +         * the MPIDR. We limit the number of VCPUs in level 0 due to a
>> +         * limitation to 16 CPUs in that level in the ICC_SGIxR 
>> registers
>> +         * of the GICv3 to be able to address each CPU directly when
>> +         * sending IPIs.
>> +         */
>> +        mpidr = (vcpu->vcpu_id & 0x0f) << MPIDR_LEVEL_SHIFT(0);
>> +        mpidr |= ((vcpu->vcpu_id >> 4) & 0xff) << MPIDR_LEVEL_SHIFT(1);
>> +        mpidr |= ((vcpu->vcpu_id >> 12) & 0xff) << MPIDR_LEVEL_SHIFT(2);
>> +        mpidr |= (1ULL << 31);
>> +    }
>> +    vcpu_write_sys_reg(vcpu, mpidr, MPIDR_EL1);
>>  }
>>
>>  static void reset_pmcr(struct kvm_vcpu *vcpu, const struct 
>> sys_reg_desc *r)
>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>> index c4874905cd9c..ae45876a689d 100644
>> --- a/include/uapi/linux/kvm.h
>> +++ b/include/uapi/linux/kvm.h
>> @@ -1475,6 +1475,8 @@ struct kvm_s390_ucas_mapping {
>>  #define KVM_S390_SET_CMMA_BITS      _IOW(KVMIO, 0xb9, struct 
>> kvm_s390_cmma_log)
>>  /* Memory Encryption Commands */
>>  #define KVM_MEMORY_ENCRYPT_OP      _IOWR(KVMIO, 0xba, unsigned long)
>> +/* Available with KVM_CAP_ARM_MP_AFFINITY */
>> +#define KVM_ARM_SET_MP_AFFINITY    _IOWR(KVMIO, 0xbb, unsigned long)
>>
>>  struct kvm_enc_region {
>>      __u64 addr;
> 
> As it is, this patch is unacceptable. It ignores the requirements of the
> architecture, as well as those of imposed by KVM as a platform.
> It also pointlessly creates additional state and invents unnecessary
> userspace interfaces. In short, it requires some *major* rework.

Yes, thanks for your comments.

We should set MPIDR from userspace in another elegant way.
I'll drop this and figure it out.
> 
>          M.

Re: [PATCH 2/2] kvm/arm: Add mp_affinity for arm vcpu

[Ying Fang]

On 9/17/2020 3:47 PM, Marc Zyngier wrote:
> On 2020-09-17 03:30, Ying Fang wrote:
>> Allow userspace to set MPIDR using vcpu ioctl KVM_ARM_SET_MP_AFFINITY,
>> so that we can support cpu topology for arm.
> 
> MPIDR has *nothing* to do with CPU topology in the ARM architecture.
> I encourage you to have a look at the ARM ARM and find out how often
> the word "topology" is used in conjunction with the MPIDR_EL1 register.
> 
>>
>> Signed-off-by: Ying Fang <fangying1@huawei.com>
>> ---
>>  arch/arm64/include/asm/kvm_host.h |  5 +++++
>>  arch/arm64/kvm/arm.c              |  8 ++++++++
>>  arch/arm64/kvm/reset.c            | 11 +++++++++++
>>  arch/arm64/kvm/sys_regs.c         | 30 +++++++++++++++++++-----------
>>  include/uapi/linux/kvm.h          |  2 ++
>>  5 files changed, 45 insertions(+), 11 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/kvm_host.h
>> b/arch/arm64/include/asm/kvm_host.h
>> index e52c927aade5..7adc351ee70a 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -372,6 +372,9 @@ struct kvm_vcpu_arch {
>>          u64 last_steal;
>>          gpa_t base;
>>      } steal;
>> +
>> +    /* vCPU MP Affinity */
>> +    u64 mp_affinity;
> 
> No. We already have a per-CPU MPIDR_EL1 register, we don't need another
> piece of state.
I am also not sure if it is needed here.
> 
>>  };
>>
>>  /* Pointer to the vcpu's SVE FFR for sve_{save,load}_state() */
>> @@ -685,6 +688,8 @@ int kvm_arm_setup_stage2(struct kvm *kvm, unsigned
>> long type);
>>  int kvm_arm_vcpu_finalize(struct kvm_vcpu *vcpu, int feature);
>>  bool kvm_arm_vcpu_is_finalized(struct kvm_vcpu *vcpu);
>>
>> +int kvm_arm_vcpu_set_mp_affinity(struct kvm_vcpu *vcpu, uint64_t mpidr);
>> +
>>  #define kvm_arm_vcpu_sve_finalized(vcpu) \
>>      ((vcpu)->arch.flags & KVM_ARM64_VCPU_SVE_FINALIZED)
>>
>> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
>> index 913c8da539b3..5f1fa625dc11 100644
>> --- a/arch/arm64/kvm/arm.c
>> +++ b/arch/arm64/kvm/arm.c
>> @@ -1178,6 +1178,14 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>>
>>          return kvm_arm_vcpu_finalize(vcpu, what);
>>      }
>> +    case KVM_ARM_SET_MP_AFFINITY: {
>> +        u64 mpidr;
>> +
>> +        if (get_user(mpidr, (const unsigned int __user *)argp))
>> +            return -EFAULT;
>> +
>> +        return kvm_arm_vcpu_set_mp_affinity(vcpu, mpidr);
>> +    }
> 
> That's not the way we access system registers from userspace.

I know that we already had the KVM_SET_ONE_REG api and well designed
sys_regs subsystem.

However when I tried to set MPIDR using the KVM_SET_ONE_REG api in 
kvm_arch_init_vcpu, the value may get erased by cpu_reset called within 
arm_cpu_realizefn. Maybe we should make change on qemu instead of adding
this redundant ioctl.

> 
>>      default:
>>          r = -EINVAL;
>>      }
>> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
>> index ee33875c5c2a..4918c967b0c9 100644
>> --- a/arch/arm64/kvm/reset.c
>> +++ b/arch/arm64/kvm/reset.c
>> @@ -188,6 +188,17 @@ int kvm_arm_vcpu_finalize(struct kvm_vcpu *vcpu,
>> int feature)
>>      return -EINVAL;
>>  }
>>
>> +int kvm_arm_vcpu_set_mp_affinity(struct kvm_vcpu *vcpu, uint64_t mpidr)
>> +{
>> +    if (!(mpidr & (1ULL << 31))) {
>> +        pr_warn("invalid mp_affinity format\n");
>> +        return -EINVAL;
>> +    }
>> +
>> +    vcpu->arch.mp_affinity = mpidr;
> 
> This doesn't match the definition of the MPIDR_EL1 register. It also
> doesn't take into account any of the existing restrictions for the
> supported affinity levels and number of PEs at the lowest affinity
> level.
Yes, we should do sanity check here and we must keep the format of MPIDR
following the Spec definition.

> 
>> +    return 0;
>> +}
>> +
>>  bool kvm_arm_vcpu_is_finalized(struct kvm_vcpu *vcpu)
>>  {
>>      if (vcpu_has_sve(vcpu) && !kvm_arm_vcpu_sve_finalized(vcpu))
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index 077293b5115f..e76f483475ad 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -646,17 +646,25 @@ static void reset_mpidr(struct kvm_vcpu *vcpu,
>> const struct sys_reg_desc *r)
>>  {
>>      u64 mpidr;
>>
>> -    /*
>> -     * Map the vcpu_id into the first three affinity level fields of
>> -     * the MPIDR. We limit the number of VCPUs in level 0 due to a
>> -     * limitation to 16 CPUs in that level in the ICC_SGIxR registers
>> -     * of the GICv3 to be able to address each CPU directly when
>> -     * sending IPIs.
>> -     */
>> -    mpidr = (vcpu->vcpu_id & 0x0f) << MPIDR_LEVEL_SHIFT(0);
>> -    mpidr |= ((vcpu->vcpu_id >> 4) & 0xff) << MPIDR_LEVEL_SHIFT(1);
>> -    mpidr |= ((vcpu->vcpu_id >> 12) & 0xff) << MPIDR_LEVEL_SHIFT(2);
>> -    vcpu_write_sys_reg(vcpu, (1ULL << 31) | mpidr, MPIDR_EL1);
>> +    if (vcpu->arch.mp_affinity) {
>> +        /* If mp_affinity is set by userspace, it means an customized 
>> cpu
>> +         * topology is defined. Let it be MPIDR of the vcpu
>> +         */
>> +        mpidr = vcpu->arch.mp_affinity;
>> +    } else {
>> +        /*
>> +         * Map the vcpu_id into the first three affinity level fields of
>> +         * the MPIDR. We limit the number of VCPUs in level 0 due to a
>> +         * limitation to 16 CPUs in that level in the ICC_SGIxR 
>> registers
>> +         * of the GICv3 to be able to address each CPU directly when
>> +         * sending IPIs.
>> +         */
>> +        mpidr = (vcpu->vcpu_id & 0x0f) << MPIDR_LEVEL_SHIFT(0);
>> +        mpidr |= ((vcpu->vcpu_id >> 4) & 0xff) << MPIDR_LEVEL_SHIFT(1);
>> +        mpidr |= ((vcpu->vcpu_id >> 12) & 0xff) << MPIDR_LEVEL_SHIFT(2);
>> +        mpidr |= (1ULL << 31);
>> +    }
>> +    vcpu_write_sys_reg(vcpu, mpidr, MPIDR_EL1);
>>  }
>>
>>  static void reset_pmcr(struct kvm_vcpu *vcpu, const struct 
>> sys_reg_desc *r)
>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>> index c4874905cd9c..ae45876a689d 100644
>> --- a/include/uapi/linux/kvm.h
>> +++ b/include/uapi/linux/kvm.h
>> @@ -1475,6 +1475,8 @@ struct kvm_s390_ucas_mapping {
>>  #define KVM_S390_SET_CMMA_BITS      _IOW(KVMIO, 0xb9, struct 
>> kvm_s390_cmma_log)
>>  /* Memory Encryption Commands */
>>  #define KVM_MEMORY_ENCRYPT_OP      _IOWR(KVMIO, 0xba, unsigned long)
>> +/* Available with KVM_CAP_ARM_MP_AFFINITY */
>> +#define KVM_ARM_SET_MP_AFFINITY    _IOWR(KVMIO, 0xbb, unsigned long)
>>
>>  struct kvm_enc_region {
>>      __u64 addr;
> 
> As it is, this patch is unacceptable. It ignores the requirements of the
> architecture, as well as those of imposed by KVM as a platform.
> It also pointlessly creates additional state and invents unnecessary
> userspace interfaces. In short, it requires some *major* rework.

Yes, thanks for your comments.

We should set MPIDR from userspace in another elegant way.
I'll drop this and figure it out.
> 
>          M.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm