[PATCH v4 0/3] KVM: arm64: mixed-width check should be skipped for uninitialized vCPUs

[PATCH v4 0/3] KVM: arm64: mixed-width check should be skipped for uninitialized vCPUs

[Reiji Watanabe]

KVM allows userspace to configure either all EL1 32bit or 64bit vCPUs
for a guest.  At vCPU reset, vcpu_allowed_register_width() checks
if the vcpu's register width is consistent with all other vCPUs'.
Since the checking is done even against vCPUs that are not initialized
(KVM_ARM_VCPU_INIT has not been done) yet, the uninitialized vCPUs
are erroneously treated as 64bit vCPU, which causes the function to
incorrectly detect a mixed-width VM.

This series will fix this problem by introducing a new VM flag that
indicates the guest needs to be configured with all 32bit or 64bit
vCPUs and checking vcpu's register width against the new flag at
the vcpu's KVM_ARM_VCPU_INIT (instead of against other vCPUs'
register width).

Patch-1 introduces a new field 'flags' for kvm_arch (authored by Marc [1]).
The flags will replace a set of booleans for VM features.

Patch-2 introduces KVM_ARCH_FLAG_EL1_32BIT and
KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED bits for kvm->arch.flags and
uses them to check vcpu's register width to fix the problem.

Patch-3 introduces a selftest that can test non-mixed-width vCPUs (all
64bit vCPUs or all 32bit vcPUs) can be configured, and mixed-width
vCPUs cannot be configured.

The series is based on v5.17-rc7.

v4:
  - Use different implementation of vcpu_el1_is_32bit() depending on
    the context. [Marc]
  - Rename kvm_register_width_check_or_init() to kvm_set_vm_width(), and
    call it from kvm_rest_vcpu() instead of from kvm_vcpu_set_target()
  - Remove vcpu_allowed_register_width(), and does the same checking
    in kvm_set_vm_width() instead.

v3: https://lore.kernel.org/all/20220303035408.3708241-1-reijiw@google.com/
  - Introduced 'flags' to kvm_arch, and use bits of the flags for
    a set of booleans for VM feature.
  - Changed 'el1_reg_width' to two bits of 'flags' of kvm_arch.

v2: https://lore.kernel.org/all/20220118041923.3384602-1-reijiw@google.com/
  - Introduced 'el1_reg_width' for kvm_arch and use it to check vcpu's
    register width against the flag at the vcpu's KVM_ARM_VCPU_INIT.

v1: https://lore.kernel.org/all/20220110054042.1079932-1-reijiw@google.com/

[1] https://lore.kernel.org/all/20210715163159.1480168-2-maz@kernel.org/

Marc Zyngier (1):
  KVM: arm64: Generalise VM features into a set of flags

Reiji Watanabe (2):
  KVM: arm64: mixed-width check should be skipped for uninitialized
    vCPUs
  KVM: arm64: selftests: Introduce vcpu_width_config

 arch/arm64/include/asm/kvm_emulate.h          |  27 ++--
 arch/arm64/include/asm/kvm_host.h             |  21 ++-
 arch/arm64/kvm/arm.c                          |   5 +-
 arch/arm64/kvm/mmio.c                         |   3 +-
 arch/arm64/kvm/reset.c                        |  64 ++++++---
 tools/testing/selftests/kvm/.gitignore        |   1 +
 tools/testing/selftests/kvm/Makefile          |   1 +
 .../selftests/kvm/aarch64/vcpu_width_config.c | 125 ++++++++++++++++++
 8 files changed, 209 insertions(+), 38 deletions(-)
 create mode 100644 tools/testing/selftests/kvm/aarch64/vcpu_width_config.c

-- 
2.35.1.723.g4982287a31-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 1/3] KVM: arm64: Generalise VM features into a set of flags

[Reiji Watanabe]

From: Marc Zyngier <maz@kernel.org>

We currently deal with a set of booleans for VM features,
while they could be better represented as set of flags
contained in an unsigned long, similarily to what we are
doing on the CPU side.

Signed-off-by: Marc Zyngier <maz@kernel.org>
Reviewed-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Reiji Watanabe <reijiw@google.com>
---
 arch/arm64/include/asm/kvm_host.h | 12 +++++++-----
 arch/arm64/kvm/arm.c              |  5 +++--
 arch/arm64/kvm/mmio.c             |  3 ++-
 3 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 5bc01e62c08a..11a7ae747ded 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -122,7 +122,10 @@ struct kvm_arch {
 	 * should) opt in to this feature if KVM_CAP_ARM_NISV_TO_USER is
 	 * supported.
 	 */
-	bool return_nisv_io_abort_to_user;
+#define KVM_ARCH_FLAG_RETURN_NISV_IO_ABORT_TO_USER	0
+	/* Memory Tagging Extension enabled for the guest */
+#define KVM_ARCH_FLAG_MTE_ENABLED			1
+	unsigned long flags;
 
 	/*
 	 * VM-wide PMU filter, implemented as a bitmap and big enough for
@@ -133,9 +136,6 @@ struct kvm_arch {
 
 	u8 pfr0_csv2;
 	u8 pfr0_csv3;
-
-	/* Memory Tagging Extension enabled for the guest */
-	bool mte_enabled;
 };
 
 struct kvm_vcpu_fault_info {
@@ -786,7 +786,9 @@ bool kvm_arm_vcpu_is_finalized(struct kvm_vcpu *vcpu);
 #define kvm_arm_vcpu_sve_finalized(vcpu) \
 	((vcpu)->arch.flags & KVM_ARM64_VCPU_SVE_FINALIZED)
 
-#define kvm_has_mte(kvm) (system_supports_mte() && (kvm)->arch.mte_enabled)
+#define kvm_has_mte(kvm)					\
+	(system_supports_mte() &&				\
+	 test_bit(KVM_ARCH_FLAG_MTE_ENABLED, &(kvm)->arch.flags))
 #define kvm_vcpu_has_pmu(vcpu)					\
 	(test_bit(KVM_ARM_VCPU_PMU_V3, (vcpu)->arch.features))
 
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index ecc5958e27fe..9a2d240ef6a3 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -89,7 +89,8 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
 	switch (cap->cap) {
 	case KVM_CAP_ARM_NISV_TO_USER:
 		r = 0;
-		kvm->arch.return_nisv_io_abort_to_user = true;
+		set_bit(KVM_ARCH_FLAG_RETURN_NISV_IO_ABORT_TO_USER,
+			&kvm->arch.flags);
 		break;
 	case KVM_CAP_ARM_MTE:
 		mutex_lock(&kvm->lock);
@@ -97,7 +98,7 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
 			r = -EINVAL;
 		} else {
 			r = 0;
-			kvm->arch.mte_enabled = true;
+			set_bit(KVM_ARCH_FLAG_MTE_ENABLED, &kvm->arch.flags);
 		}
 		mutex_unlock(&kvm->lock);
 		break;
diff --git a/arch/arm64/kvm/mmio.c b/arch/arm64/kvm/mmio.c
index 3e2d8ba11a02..3dd38a151d2a 100644
--- a/arch/arm64/kvm/mmio.c
+++ b/arch/arm64/kvm/mmio.c
@@ -135,7 +135,8 @@ int io_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa)
 	 * volunteered to do so, and bail out otherwise.
 	 */
 	if (!kvm_vcpu_dabt_isvalid(vcpu)) {
-		if (vcpu->kvm->arch.return_nisv_io_abort_to_user) {
+		if (test_bit(KVM_ARCH_FLAG_RETURN_NISV_IO_ABORT_TO_USER,
+			     &vcpu->kvm->arch.flags)) {
 			run->exit_reason = KVM_EXIT_ARM_NISV;
 			run->arm_nisv.esr_iss = kvm_vcpu_dabt_iss_nisv_sanitized(vcpu);
 			run->arm_nisv.fault_ipa = fault_ipa;
-- 
2.35.1.723.g4982287a31-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 1/3] KVM: arm64: Generalise VM features into a set of flags

[Oliver Upton]

On Sun, Mar 13, 2022 at 11:19:57PM -0700, Reiji Watanabe wrote:
> From: Marc Zyngier <maz@kernel.org>
> 
> We currently deal with a set of booleans for VM features,
> while they could be better represented as set of flags
> contained in an unsigned long, similarily to what we are
> doing on the CPU side.
> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> Reviewed-by: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Reiji Watanabe <reijiw@google.com>

Gee, that's one popular patch! Just as a heads up, I rebased it on top
of kvmarm/next in:

  http://lore.kernel.org/r/20220311174001.605719-2-oupton@google.com

since kvm_arch picked up another bool.

--
Oliver

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 2/3] KVM: arm64: mixed-width check should be skipped for uninitialized vCPUs

[Reiji Watanabe]

KVM allows userspace to configure either all EL1 32bit or 64bit vCPUs
for a guest.  At vCPU reset, vcpu_allowed_register_width() checks
if the vcpu's register width is consistent with all other vCPUs'.
Since the checking is done even against vCPUs that are not initialized
(KVM_ARM_VCPU_INIT has not been done) yet, the uninitialized vCPUs
are erroneously treated as 64bit vCPU, which causes the function to
incorrectly detect a mixed-width VM.

Introduce KVM_ARCH_FLAG_EL1_32BIT and KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED
bits for kvm->arch.flags.  A value of the EL1_32BIT bit indicates that
the guest needs to be configured with all 32bit or 64bit vCPUs, and
a value of the REG_WIDTH_CONFIGURED bit indicates if a value of the
EL1_32BIT bit is valid (already set up). Values in those bits are set at
the first KVM_ARM_VCPU_INIT for the guest based on KVM_ARM_VCPU_EL1_32BIT
configuration for the vCPU.

Check vcpu's register width against those new bits at the vcpu's
KVM_ARM_VCPU_INIT (instead of against other vCPUs' register width).

Fixes: 66e94d5cafd4 ("KVM: arm64: Prevent mixed-width VM creation")
Signed-off-by: Reiji Watanabe <reijiw@google.com>
---
 arch/arm64/include/asm/kvm_emulate.h | 27 ++++++++----
 arch/arm64/include/asm/kvm_host.h    |  9 ++++
 arch/arm64/kvm/reset.c               | 64 ++++++++++++++++++----------
 3 files changed, 70 insertions(+), 30 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
index d62405ce3e6d..7496deab025a 100644
--- a/arch/arm64/include/asm/kvm_emulate.h
+++ b/arch/arm64/include/asm/kvm_emulate.h
@@ -43,10 +43,22 @@ void kvm_inject_pabt(struct kvm_vcpu *vcpu, unsigned long addr);
 
 void kvm_vcpu_wfi(struct kvm_vcpu *vcpu);
 
+#if defined(__KVM_VHE_HYPERVISOR__) || defined(__KVM_NVHE_HYPERVISOR__)
 static __always_inline bool vcpu_el1_is_32bit(struct kvm_vcpu *vcpu)
 {
 	return !(vcpu->arch.hcr_el2 & HCR_RW);
 }
+#else
+static __always_inline bool vcpu_el1_is_32bit(struct kvm_vcpu *vcpu)
+{
+	struct kvm *kvm = vcpu->kvm;
+
+	WARN_ON_ONCE(!test_bit(KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED,
+			       &kvm->arch.flags));
+
+	return test_bit(KVM_ARCH_FLAG_EL1_32BIT, &kvm->arch.flags);
+}
+#endif
 
 static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
 {
@@ -72,15 +84,14 @@ static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
 		vcpu->arch.hcr_el2 |= HCR_TVM;
 	}
 
-	if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features))
+	if (vcpu_el1_is_32bit(vcpu))
 		vcpu->arch.hcr_el2 &= ~HCR_RW;
-
-	/*
-	 * TID3: trap feature register accesses that we virtualise.
-	 * For now this is conditional, since no AArch32 feature regs
-	 * are currently virtualised.
-	 */
-	if (!vcpu_el1_is_32bit(vcpu))
+	else
+		/*
+		 * TID3: trap feature register accesses that we virtualise.
+		 * For now this is conditional, since no AArch32 feature regs
+		 * are currently virtualised.
+		 */
 		vcpu->arch.hcr_el2 |= HCR_TID3;
 
 	if (cpus_have_const_cap(ARM64_MISMATCHED_CACHE_TYPE) ||
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 11a7ae747ded..22ad977069f5 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -125,6 +125,15 @@ struct kvm_arch {
 #define KVM_ARCH_FLAG_RETURN_NISV_IO_ABORT_TO_USER	0
 	/* Memory Tagging Extension enabled for the guest */
 #define KVM_ARCH_FLAG_MTE_ENABLED			1
+	/*
+	 * The following two bits are used to indicate the guest's EL1
+	 * register width configuration. A value of KVM_ARCH_FLAG_EL1_32BIT
+	 * bit is valid only when KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED is set.
+	 * Otherwise, the guest's EL1 register width has not yet been
+	 * determined yet.
+	 */
+#define KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED		2
+#define KVM_ARCH_FLAG_EL1_32BIT				3
 	unsigned long flags;
 
 	/*
diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
index ecc40c8cd6f6..cbeb6216ee25 100644
--- a/arch/arm64/kvm/reset.c
+++ b/arch/arm64/kvm/reset.c
@@ -181,27 +181,45 @@ static int kvm_vcpu_enable_ptrauth(struct kvm_vcpu *vcpu)
 	return 0;
 }
 
-static bool vcpu_allowed_register_width(struct kvm_vcpu *vcpu)
+/*
+ * A guest can have either all EL1 32bit or 64bit vcpus only. It is
+ * indicated by a value of KVM_ARCH_FLAG_EL1_32BIT bit in kvm->arch.flags,
+ * which is valid only when KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED in
+ * kvm->arch.flags is set.
+ * This function sets the EL1_32BIT bit based on the given @is32bit (and
+ * sets REG_WIDTH_CONFIGURED bit). When those flags are already set,
+ * @is32bit must be consistent with the flags.
+ * Returns 0 on success, or non-zero otherwise.
+ */
+static int kvm_set_vm_width(struct kvm *kvm, bool is32bit)
 {
-	struct kvm_vcpu *tmp;
-	bool is32bit;
-	unsigned long i;
+	bool allowed;
+
+	lockdep_assert_held(&kvm->lock);
+
+	if (test_bit(KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED, &kvm->arch.flags)) {
+		/*
+		 * The guest's register width is already configured.
+		 * Make sure that @is32bit is consistent with it.
+		 */
+		allowed = (is32bit ==
+			   test_bit(KVM_ARCH_FLAG_EL1_32BIT, &kvm->arch.flags));
+		return allowed ? 0 : -EINVAL;
+	}
 
-	is32bit = vcpu_has_feature(vcpu, KVM_ARM_VCPU_EL1_32BIT);
 	if (!cpus_have_const_cap(ARM64_HAS_32BIT_EL1) && is32bit)
-		return false;
+		return -EINVAL;
 
 	/* MTE is incompatible with AArch32 */
-	if (kvm_has_mte(vcpu->kvm) && is32bit)
-		return false;
+	if (kvm_has_mte(kvm) && is32bit)
+		return -EINVAL;
 
-	/* Check that the vcpus are either all 32bit or all 64bit */
-	kvm_for_each_vcpu(i, tmp, vcpu->kvm) {
-		if (vcpu_has_feature(tmp, KVM_ARM_VCPU_EL1_32BIT) != is32bit)
-			return false;
-	}
+	if (is32bit)
+		set_bit(KVM_ARCH_FLAG_EL1_32BIT, &kvm->arch.flags);
 
-	return true;
+	set_bit(KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED, &kvm->arch.flags);
+
+	return 0;
 }
 
 /**
@@ -230,10 +248,17 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
 	u32 pstate;
 
 	mutex_lock(&vcpu->kvm->lock);
-	reset_state = vcpu->arch.reset_state;
-	WRITE_ONCE(vcpu->arch.reset_state.reset, false);
+	ret = kvm_set_vm_width(vcpu->kvm,
+			       vcpu_has_feature(vcpu, KVM_ARM_VCPU_EL1_32BIT));
+	if (!ret) {
+		reset_state = vcpu->arch.reset_state;
+		WRITE_ONCE(vcpu->arch.reset_state.reset, false);
+	}
 	mutex_unlock(&vcpu->kvm->lock);
 
+	if (ret)
+		return ret;
+
 	/* Reset PMU outside of the non-preemptible section */
 	kvm_pmu_vcpu_reset(vcpu);
 
@@ -260,14 +285,9 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
 		}
 	}
 
-	if (!vcpu_allowed_register_width(vcpu)) {
-		ret = -EINVAL;
-		goto out;
-	}
-
 	switch (vcpu->arch.target) {
 	default:
-		if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features)) {
+		if (vcpu_el1_is_32bit(vcpu)) {
 			pstate = VCPU_RESET_PSTATE_SVC;
 		} else {
 			pstate = VCPU_RESET_PSTATE_EL1;
-- 
2.35.1.723.g4982287a31-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 2/3] KVM: arm64: mixed-width check should be skipped for uninitialized vCPUs

[Oliver Upton]

On Sun, Mar 13, 2022 at 11:19:58PM -0700, Reiji Watanabe wrote:
> KVM allows userspace to configure either all EL1 32bit or 64bit vCPUs
> for a guest.  At vCPU reset, vcpu_allowed_register_width() checks
> if the vcpu's register width is consistent with all other vCPUs'.
> Since the checking is done even against vCPUs that are not initialized
> (KVM_ARM_VCPU_INIT has not been done) yet, the uninitialized vCPUs
> are erroneously treated as 64bit vCPU, which causes the function to
> incorrectly detect a mixed-width VM.
> 
> Introduce KVM_ARCH_FLAG_EL1_32BIT and KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED
> bits for kvm->arch.flags.  A value of the EL1_32BIT bit indicates that
> the guest needs to be configured with all 32bit or 64bit vCPUs, and
> a value of the REG_WIDTH_CONFIGURED bit indicates if a value of the
> EL1_32BIT bit is valid (already set up). Values in those bits are set at
> the first KVM_ARM_VCPU_INIT for the guest based on KVM_ARM_VCPU_EL1_32BIT
> configuration for the vCPU.
> 
> Check vcpu's register width against those new bits at the vcpu's
> KVM_ARM_VCPU_INIT (instead of against other vCPUs' register width).
> 
> Fixes: 66e94d5cafd4 ("KVM: arm64: Prevent mixed-width VM creation")
> Signed-off-by: Reiji Watanabe <reijiw@google.com>

Hrmph... I hate to be asking this question so late in the game, but...

Are there any bits that we really allow variation per-vCPU besides
KVM_ARM_VCPU_POWER_OFF? We unintentionally allow for variance with the
KVM_ARM_VCPU_PSCI_0_2 bit even though that's complete nonsense.

Stated plainly, should we just deny any attempts at asymmetry besides
POWER_OFF?

Besides the nits, I see nothing objectionable with the patch. I'd really
like to see more generalized constraints on vCPU configuration, but if
this is the route we take:

Reviewed-by: Oliver Upton <oupton@google.com>

> ---
>  arch/arm64/include/asm/kvm_emulate.h | 27 ++++++++----
>  arch/arm64/include/asm/kvm_host.h    |  9 ++++
>  arch/arm64/kvm/reset.c               | 64 ++++++++++++++++++----------
>  3 files changed, 70 insertions(+), 30 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> index d62405ce3e6d..7496deab025a 100644
> --- a/arch/arm64/include/asm/kvm_emulate.h
> +++ b/arch/arm64/include/asm/kvm_emulate.h
> @@ -43,10 +43,22 @@ void kvm_inject_pabt(struct kvm_vcpu *vcpu, unsigned long addr);
>  
>  void kvm_vcpu_wfi(struct kvm_vcpu *vcpu);
>  
> +#if defined(__KVM_VHE_HYPERVISOR__) || defined(__KVM_NVHE_HYPERVISOR__)
>  static __always_inline bool vcpu_el1_is_32bit(struct kvm_vcpu *vcpu)
>  {
>  	return !(vcpu->arch.hcr_el2 & HCR_RW);
>  }
> +#else
> +static __always_inline bool vcpu_el1_is_32bit(struct kvm_vcpu *vcpu)
> +{
> +	struct kvm *kvm = vcpu->kvm;
> +
> +	WARN_ON_ONCE(!test_bit(KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED,
> +			       &kvm->arch.flags));
> +
> +	return test_bit(KVM_ARCH_FLAG_EL1_32BIT, &kvm->arch.flags);
> +}
> +#endif
>  
>  static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
>  {
> @@ -72,15 +84,14 @@ static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
>  		vcpu->arch.hcr_el2 |= HCR_TVM;
>  	}
>  
> -	if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features))
> +	if (vcpu_el1_is_32bit(vcpu))
>  		vcpu->arch.hcr_el2 &= ~HCR_RW;
> -
> -	/*
> -	 * TID3: trap feature register accesses that we virtualise.
> -	 * For now this is conditional, since no AArch32 feature regs
> -	 * are currently virtualised.
> -	 */
> -	if (!vcpu_el1_is_32bit(vcpu))
> +	else
> +		/*
> +		 * TID3: trap feature register accesses that we virtualise.
> +		 * For now this is conditional, since no AArch32 feature regs
> +		 * are currently virtualised.
> +		 */
>  		vcpu->arch.hcr_el2 |= HCR_TID3;
>  
>  	if (cpus_have_const_cap(ARM64_MISMATCHED_CACHE_TYPE) ||
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 11a7ae747ded..22ad977069f5 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -125,6 +125,15 @@ struct kvm_arch {
>  #define KVM_ARCH_FLAG_RETURN_NISV_IO_ABORT_TO_USER	0
>  	/* Memory Tagging Extension enabled for the guest */
>  #define KVM_ARCH_FLAG_MTE_ENABLED			1
> +	/*
> +	 * The following two bits are used to indicate the guest's EL1
> +	 * register width configuration. A value of KVM_ARCH_FLAG_EL1_32BIT
> +	 * bit is valid only when KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED is set.
> +	 * Otherwise, the guest's EL1 register width has not yet been
> +	 * determined yet.
> +	 */
> +#define KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED		2
> +#define KVM_ARCH_FLAG_EL1_32BIT				3
>  	unsigned long flags;
>  
>  	/*
> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> index ecc40c8cd6f6..cbeb6216ee25 100644
> --- a/arch/arm64/kvm/reset.c
> +++ b/arch/arm64/kvm/reset.c
> @@ -181,27 +181,45 @@ static int kvm_vcpu_enable_ptrauth(struct kvm_vcpu *vcpu)
>  	return 0;
>  }
>  
> -static bool vcpu_allowed_register_width(struct kvm_vcpu *vcpu)
> +/*
> + * A guest can have either all EL1 32bit or 64bit vcpus only. It is
> + * indicated by a value of KVM_ARCH_FLAG_EL1_32BIT bit in kvm->arch.flags,
> + * which is valid only when KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED in
> + * kvm->arch.flags is set.
> + * This function sets the EL1_32BIT bit based on the given @is32bit (and
> + * sets REG_WIDTH_CONFIGURED bit). When those flags are already set,
> + * @is32bit must be consistent with the flags.
> + * Returns 0 on success, or non-zero otherwise.
> + */

nit: use kerneldoc style:

  https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html

> +static int kvm_set_vm_width(struct kvm *kvm, bool is32bit)
>  {
> -	struct kvm_vcpu *tmp;
> -	bool is32bit;
> -	unsigned long i;
> +	bool allowed;
> +
> +	lockdep_assert_held(&kvm->lock);
> +
> +	if (test_bit(KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED, &kvm->arch.flags)) {
> +		/*
> +		 * The guest's register width is already configured.
> +		 * Make sure that @is32bit is consistent with it.
> +		 */
> +		allowed = (is32bit ==
> +			   test_bit(KVM_ARCH_FLAG_EL1_32BIT, &kvm->arch.flags));
> +		return allowed ? 0 : -EINVAL;

nit: I'd avoid the ternary and just use a boring if/else (though I could
be in the minority here).

> +	}
>  
> -	is32bit = vcpu_has_feature(vcpu, KVM_ARM_VCPU_EL1_32BIT);
>  	if (!cpus_have_const_cap(ARM64_HAS_32BIT_EL1) && is32bit)
> -		return false;
> +		return -EINVAL;
>  
>  	/* MTE is incompatible with AArch32 */
> -	if (kvm_has_mte(vcpu->kvm) && is32bit)
> -		return false;
> +	if (kvm_has_mte(kvm) && is32bit)
> +		return -EINVAL;
>  
> -	/* Check that the vcpus are either all 32bit or all 64bit */
> -	kvm_for_each_vcpu(i, tmp, vcpu->kvm) {
> -		if (vcpu_has_feature(tmp, KVM_ARM_VCPU_EL1_32BIT) != is32bit)
> -			return false;
> -	}
> +	if (is32bit)
> +		set_bit(KVM_ARCH_FLAG_EL1_32BIT, &kvm->arch.flags);
>  
> -	return true;
> +	set_bit(KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED, &kvm->arch.flags);
> +
> +	return 0;
>  }
>  
>  /**
> @@ -230,10 +248,17 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
>  	u32 pstate;
>  
>  	mutex_lock(&vcpu->kvm->lock);
> -	reset_state = vcpu->arch.reset_state;
> -	WRITE_ONCE(vcpu->arch.reset_state.reset, false);
> +	ret = kvm_set_vm_width(vcpu->kvm,
> +			       vcpu_has_feature(vcpu, KVM_ARM_VCPU_EL1_32BIT));
> +	if (!ret) {
> +		reset_state = vcpu->arch.reset_state;
> +		WRITE_ONCE(vcpu->arch.reset_state.reset, false);
> +	}
>  	mutex_unlock(&vcpu->kvm->lock);
>  
> +	if (ret)
> +		return ret;
> +
>  	/* Reset PMU outside of the non-preemptible section */
>  	kvm_pmu_vcpu_reset(vcpu);
>  
> @@ -260,14 +285,9 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
>  		}
>  	}
>  
> -	if (!vcpu_allowed_register_width(vcpu)) {
> -		ret = -EINVAL;
> -		goto out;
> -	}
> -
>  	switch (vcpu->arch.target) {
>  	default:
> -		if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features)) {
> +		if (vcpu_el1_is_32bit(vcpu)) {
>  			pstate = VCPU_RESET_PSTATE_SVC;
>  		} else {
>  			pstate = VCPU_RESET_PSTATE_EL1;
> -- 
> 2.35.1.723.g4982287a31-goog
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 2/3] KVM: arm64: mixed-width check should be skipped for uninitialized vCPUs

[Reiji Watanabe]

Hi Oliver,

On 3/14/22 1:22 PM, Oliver Upton wrote:
> On Sun, Mar 13, 2022 at 11:19:58PM -0700, Reiji Watanabe wrote:
>> KVM allows userspace to configure either all EL1 32bit or 64bit vCPUs
>> for a guest.  At vCPU reset, vcpu_allowed_register_width() checks
>> if the vcpu's register width is consistent with all other vCPUs'.
>> Since the checking is done even against vCPUs that are not initialized
>> (KVM_ARM_VCPU_INIT has not been done) yet, the uninitialized vCPUs
>> are erroneously treated as 64bit vCPU, which causes the function to
>> incorrectly detect a mixed-width VM.
>>
>> Introduce KVM_ARCH_FLAG_EL1_32BIT and KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED
>> bits for kvm->arch.flags.  A value of the EL1_32BIT bit indicates that
>> the guest needs to be configured with all 32bit or 64bit vCPUs, and
>> a value of the REG_WIDTH_CONFIGURED bit indicates if a value of the
>> EL1_32BIT bit is valid (already set up). Values in those bits are set at
>> the first KVM_ARM_VCPU_INIT for the guest based on KVM_ARM_VCPU_EL1_32BIT
>> configuration for the vCPU.
>>
>> Check vcpu's register width against those new bits at the vcpu's
>> KVM_ARM_VCPU_INIT (instead of against other vCPUs' register width).
>>
>> Fixes: 66e94d5cafd4 ("KVM: arm64: Prevent mixed-width VM creation")
>> Signed-off-by: Reiji Watanabe <reijiw@google.com>
> 
> Hrmph... I hate to be asking this question so late in the game, but...
> 
> Are there any bits that we really allow variation per-vCPU besides
> KVM_ARM_VCPU_POWER_OFF? We unintentionally allow for variance with the
> KVM_ARM_VCPU_PSCI_0_2 bit even though that's complete nonsense.
> 
> Stated plainly, should we just deny any attempts at asymmetry besides
> POWER_OFF?> 
> Besides the nits, I see nothing objectionable with the patch. I'd really
> like to see more generalized constraints on vCPU configuration, but if
> this is the route we take:

Prohibiting the mixed width configuration is not a new constraint that
this patch creates (this patch fixes a bug that erroneously detects
mixed-width configuration), and enforcing symmetry of other features
among vCPUs is a bit different matter.

Having said that, I like the idea, which will be more consistent with
my ID register series (it can simplify things).  But, I'm not sure
if creating the constraint for those features would be a problem for
existing userspace even if allowing variation per-vCPU for the features
was not our intention.
I would guess having the constraint for KVM_ARM_VCPU_PSCI_0_2 should
be fine.  Do you think that should be fine for PMU, SVE, and PTRAUTH*
as well ?


> 
> Reviewed-by: Oliver Upton <oupton@google.com>
> 
>> ---
>>   arch/arm64/include/asm/kvm_emulate.h | 27 ++++++++----
>>   arch/arm64/include/asm/kvm_host.h    |  9 ++++
>>   arch/arm64/kvm/reset.c               | 64 ++++++++++++++++++----------
>>   3 files changed, 70 insertions(+), 30 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
>> index d62405ce3e6d..7496deab025a 100644
>> --- a/arch/arm64/include/asm/kvm_emulate.h
>> +++ b/arch/arm64/include/asm/kvm_emulate.h
>> @@ -43,10 +43,22 @@ void kvm_inject_pabt(struct kvm_vcpu *vcpu, unsigned long addr);
>>   
>>   void kvm_vcpu_wfi(struct kvm_vcpu *vcpu);
>>   
>> +#if defined(__KVM_VHE_HYPERVISOR__) || defined(__KVM_NVHE_HYPERVISOR__)
>>   static __always_inline bool vcpu_el1_is_32bit(struct kvm_vcpu *vcpu)
>>   {
>>   	return !(vcpu->arch.hcr_el2 & HCR_RW);
>>   }
>> +#else
>> +static __always_inline bool vcpu_el1_is_32bit(struct kvm_vcpu *vcpu)
>> +{
>> +	struct kvm *kvm = vcpu->kvm;
>> +
>> +	WARN_ON_ONCE(!test_bit(KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED,
>> +			       &kvm->arch.flags));
>> +
>> +	return test_bit(KVM_ARCH_FLAG_EL1_32BIT, &kvm->arch.flags);
>> +}
>> +#endif
>>   
>>   static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
>>   {
>> @@ -72,15 +84,14 @@ static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
>>   		vcpu->arch.hcr_el2 |= HCR_TVM;
>>   	}
>>   
>> -	if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features))
>> +	if (vcpu_el1_is_32bit(vcpu))
>>   		vcpu->arch.hcr_el2 &= ~HCR_RW;
>> -
>> -	/*
>> -	 * TID3: trap feature register accesses that we virtualise.
>> -	 * For now this is conditional, since no AArch32 feature regs
>> -	 * are currently virtualised.
>> -	 */
>> -	if (!vcpu_el1_is_32bit(vcpu))
>> +	else
>> +		/*
>> +		 * TID3: trap feature register accesses that we virtualise.
>> +		 * For now this is conditional, since no AArch32 feature regs
>> +		 * are currently virtualised.
>> +		 */
>>   		vcpu->arch.hcr_el2 |= HCR_TID3;
>>   
>>   	if (cpus_have_const_cap(ARM64_MISMATCHED_CACHE_TYPE) ||
>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>> index 11a7ae747ded..22ad977069f5 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -125,6 +125,15 @@ struct kvm_arch {
>>   #define KVM_ARCH_FLAG_RETURN_NISV_IO_ABORT_TO_USER	0
>>   	/* Memory Tagging Extension enabled for the guest */
>>   #define KVM_ARCH_FLAG_MTE_ENABLED			1
>> +	/*
>> +	 * The following two bits are used to indicate the guest's EL1
>> +	 * register width configuration. A value of KVM_ARCH_FLAG_EL1_32BIT
>> +	 * bit is valid only when KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED is set.
>> +	 * Otherwise, the guest's EL1 register width has not yet been
>> +	 * determined yet.
>> +	 */
>> +#define KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED		2
>> +#define KVM_ARCH_FLAG_EL1_32BIT				3
>>   	unsigned long flags;
>>   
>>   	/*
>> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
>> index ecc40c8cd6f6..cbeb6216ee25 100644
>> --- a/arch/arm64/kvm/reset.c
>> +++ b/arch/arm64/kvm/reset.c
>> @@ -181,27 +181,45 @@ static int kvm_vcpu_enable_ptrauth(struct kvm_vcpu *vcpu)
>>   	return 0;
>>   }
>>   
>> -static bool vcpu_allowed_register_width(struct kvm_vcpu *vcpu)
>> +/*
>> + * A guest can have either all EL1 32bit or 64bit vcpus only. It is
>> + * indicated by a value of KVM_ARCH_FLAG_EL1_32BIT bit in kvm->arch.flags,
>> + * which is valid only when KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED in
>> + * kvm->arch.flags is set.
>> + * This function sets the EL1_32BIT bit based on the given @is32bit (and
>> + * sets REG_WIDTH_CONFIGURED bit). When those flags are already set,
>> + * @is32bit must be consistent with the flags.
>> + * Returns 0 on success, or non-zero otherwise.
>> + */
> 
> nit: use kerneldoc style:
> 
>    https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html

Sure, I can fix the comment to use kerneldoc style.


> 
>> +static int kvm_set_vm_width(struct kvm *kvm, bool is32bit)
>>   {
>> -	struct kvm_vcpu *tmp;
>> -	bool is32bit;
>> -	unsigned long i;
>> +	bool allowed;
>> +
>> +	lockdep_assert_held(&kvm->lock);
>> +
>> +	if (test_bit(KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED, &kvm->arch.flags)) {
>> +		/*
>> +		 * The guest's register width is already configured.
>> +		 * Make sure that @is32bit is consistent with it.
>> +		 */
>> +		allowed = (is32bit ==
>> +			   test_bit(KVM_ARCH_FLAG_EL1_32BIT, &kvm->arch.flags));
>> +		return allowed ? 0 : -EINVAL;
> 
> nit: I'd avoid the ternary and just use a boring if/else (though I could
> be in the minority here).

I agree with you and will fix it.
(The ternary with 'allowed' was just copied from the previous patch,
  and I should have changed that in this patch...)

Thanks,
Reiji


> 
>> +	}
>>   
>> -	is32bit = vcpu_has_feature(vcpu, KVM_ARM_VCPU_EL1_32BIT);
>>   	if (!cpus_have_const_cap(ARM64_HAS_32BIT_EL1) && is32bit)
>> -		return false;
>> +		return -EINVAL;
>>   
>>   	/* MTE is incompatible with AArch32 */
>> -	if (kvm_has_mte(vcpu->kvm) && is32bit)
>> -		return false;
>> +	if (kvm_has_mte(kvm) && is32bit)
>> +		return -EINVAL;
>>   
>> -	/* Check that the vcpus are either all 32bit or all 64bit */
>> -	kvm_for_each_vcpu(i, tmp, vcpu->kvm) {
>> -		if (vcpu_has_feature(tmp, KVM_ARM_VCPU_EL1_32BIT) != is32bit)
>> -			return false;
>> -	}
>> +	if (is32bit)
>> +		set_bit(KVM_ARCH_FLAG_EL1_32BIT, &kvm->arch.flags);
>>   
>> -	return true;
>> +	set_bit(KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED, &kvm->arch.flags);
>> +
>> +	return 0;
>>   }
>>   
>>   /**
>> @@ -230,10 +248,17 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
>>   	u32 pstate;
>>   
>>   	mutex_lock(&vcpu->kvm->lock);
>> -	reset_state = vcpu->arch.reset_state;
>> -	WRITE_ONCE(vcpu->arch.reset_state.reset, false);
>> +	ret = kvm_set_vm_width(vcpu->kvm,
>> +			       vcpu_has_feature(vcpu, KVM_ARM_VCPU_EL1_32BIT));
>> +	if (!ret) {
>> +		reset_state = vcpu->arch.reset_state;
>> +		WRITE_ONCE(vcpu->arch.reset_state.reset, false);
>> +	}
>>   	mutex_unlock(&vcpu->kvm->lock);
>>   
>> +	if (ret)
>> +		return ret;
>> +
>>   	/* Reset PMU outside of the non-preemptible section */
>>   	kvm_pmu_vcpu_reset(vcpu);
>>   
>> @@ -260,14 +285,9 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
>>   		}
>>   	}
>>   
>> -	if (!vcpu_allowed_register_width(vcpu)) {
>> -		ret = -EINVAL;
>> -		goto out;
>> -	}
>> -
>>   	switch (vcpu->arch.target) {
>>   	default:
>> -		if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features)) {
>> +		if (vcpu_el1_is_32bit(vcpu)) {
>>   			pstate = VCPU_RESET_PSTATE_SVC;
>>   		} else {
>>   			pstate = VCPU_RESET_PSTATE_EL1;
>> -- 
>> 2.35.1.723.g4982287a31-goog
>>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 2/3] KVM: arm64: mixed-width check should be skipped for uninitialized vCPUs

[Oliver Upton]

On Mon, Mar 14, 2022 at 11:19 PM Reiji Watanabe <reijiw@google.com> wrote:
>
> Hi Oliver,
>
> On 3/14/22 1:22 PM, Oliver Upton wrote:
> > On Sun, Mar 13, 2022 at 11:19:58PM -0700, Reiji Watanabe wrote:
> >> KVM allows userspace to configure either all EL1 32bit or 64bit vCPUs
> >> for a guest.  At vCPU reset, vcpu_allowed_register_width() checks
> >> if the vcpu's register width is consistent with all other vCPUs'.
> >> Since the checking is done even against vCPUs that are not initialized
> >> (KVM_ARM_VCPU_INIT has not been done) yet, the uninitialized vCPUs
> >> are erroneously treated as 64bit vCPU, which causes the function to
> >> incorrectly detect a mixed-width VM.
> >>
> >> Introduce KVM_ARCH_FLAG_EL1_32BIT and KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED
> >> bits for kvm->arch.flags.  A value of the EL1_32BIT bit indicates that
> >> the guest needs to be configured with all 32bit or 64bit vCPUs, and
> >> a value of the REG_WIDTH_CONFIGURED bit indicates if a value of the
> >> EL1_32BIT bit is valid (already set up). Values in those bits are set at
> >> the first KVM_ARM_VCPU_INIT for the guest based on KVM_ARM_VCPU_EL1_32BIT
> >> configuration for the vCPU.
> >>
> >> Check vcpu's register width against those new bits at the vcpu's
> >> KVM_ARM_VCPU_INIT (instead of against other vCPUs' register width).
> >>
> >> Fixes: 66e94d5cafd4 ("KVM: arm64: Prevent mixed-width VM creation")
> >> Signed-off-by: Reiji Watanabe <reijiw@google.com>
> >
> > Hrmph... I hate to be asking this question so late in the game, but...
> >
> > Are there any bits that we really allow variation per-vCPU besides
> > KVM_ARM_VCPU_POWER_OFF? We unintentionally allow for variance with the
> > KVM_ARM_VCPU_PSCI_0_2 bit even though that's complete nonsense.
> >
> > Stated plainly, should we just deny any attempts at asymmetry besides
> > POWER_OFF?>
> > Besides the nits, I see nothing objectionable with the patch. I'd really
> > like to see more generalized constraints on vCPU configuration, but if
> > this is the route we take:
>
> Prohibiting the mixed width configuration is not a new constraint that
> this patch creates (this patch fixes a bug that erroneously detects
> mixed-width configuration), and enforcing symmetry of other features
> among vCPUs is a bit different matter.

Right, I had managed to forget that context for a moment when I
replied to you. Then I fully agree with this patch, and the other
feature flags can be handled later.

>
> Having said that, I like the idea, which will be more consistent with
> my ID register series (it can simplify things).  But, I'm not sure
> if creating the constraint for those features would be a problem for
> existing userspace even if allowing variation per-vCPU for the features
> was not our intention.
> I would guess having the constraint for KVM_ARM_VCPU_PSCI_0_2 should
> be fine.  Do you think that should be fine for PMU, SVE, and PTRAUTH*
> as well ?

Personally, yes, but it prompts the question of if we could break
userspace by applying restrictions after the fact. The original patch
that applied the register width restrictions didn't cause much of a
stir, so it seems possible we could get away with it.

> >
> > Reviewed-by: Oliver Upton <oupton@google.com>
> >
> >> ---
> >>   arch/arm64/include/asm/kvm_emulate.h | 27 ++++++++----
> >>   arch/arm64/include/asm/kvm_host.h    |  9 ++++
> >>   arch/arm64/kvm/reset.c               | 64 ++++++++++++++++++----------
> >>   3 files changed, 70 insertions(+), 30 deletions(-)
> >>
> >> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> >> index d62405ce3e6d..7496deab025a 100644
> >> --- a/arch/arm64/include/asm/kvm_emulate.h
> >> +++ b/arch/arm64/include/asm/kvm_emulate.h
> >> @@ -43,10 +43,22 @@ void kvm_inject_pabt(struct kvm_vcpu *vcpu, unsigned long addr);
> >>
> >>   void kvm_vcpu_wfi(struct kvm_vcpu *vcpu);
> >>
> >> +#if defined(__KVM_VHE_HYPERVISOR__) || defined(__KVM_NVHE_HYPERVISOR__)
> >>   static __always_inline bool vcpu_el1_is_32bit(struct kvm_vcpu *vcpu)
> >>   {
> >>      return !(vcpu->arch.hcr_el2 & HCR_RW);
> >>   }
> >> +#else
> >> +static __always_inline bool vcpu_el1_is_32bit(struct kvm_vcpu *vcpu)
> >> +{
> >> +    struct kvm *kvm = vcpu->kvm;
> >> +
> >> +    WARN_ON_ONCE(!test_bit(KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED,
> >> +                           &kvm->arch.flags));
> >> +
> >> +    return test_bit(KVM_ARCH_FLAG_EL1_32BIT, &kvm->arch.flags);
> >> +}
> >> +#endif
> >>
> >>   static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
> >>   {
> >> @@ -72,15 +84,14 @@ static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
> >>              vcpu->arch.hcr_el2 |= HCR_TVM;
> >>      }
> >>
> >> -    if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features))
> >> +    if (vcpu_el1_is_32bit(vcpu))
> >>              vcpu->arch.hcr_el2 &= ~HCR_RW;
> >> -
> >> -    /*
> >> -     * TID3: trap feature register accesses that we virtualise.
> >> -     * For now this is conditional, since no AArch32 feature regs
> >> -     * are currently virtualised.
> >> -     */
> >> -    if (!vcpu_el1_is_32bit(vcpu))
> >> +    else
> >> +            /*
> >> +             * TID3: trap feature register accesses that we virtualise.
> >> +             * For now this is conditional, since no AArch32 feature regs
> >> +             * are currently virtualised.
> >> +             */
> >>              vcpu->arch.hcr_el2 |= HCR_TID3;
> >>
> >>      if (cpus_have_const_cap(ARM64_MISMATCHED_CACHE_TYPE) ||
> >> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> >> index 11a7ae747ded..22ad977069f5 100644
> >> --- a/arch/arm64/include/asm/kvm_host.h
> >> +++ b/arch/arm64/include/asm/kvm_host.h
> >> @@ -125,6 +125,15 @@ struct kvm_arch {
> >>   #define KVM_ARCH_FLAG_RETURN_NISV_IO_ABORT_TO_USER 0
> >>      /* Memory Tagging Extension enabled for the guest */
> >>   #define KVM_ARCH_FLAG_MTE_ENABLED                  1
> >> +    /*
> >> +     * The following two bits are used to indicate the guest's EL1
> >> +     * register width configuration. A value of KVM_ARCH_FLAG_EL1_32BIT
> >> +     * bit is valid only when KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED is set.
> >> +     * Otherwise, the guest's EL1 register width has not yet been
> >> +     * determined yet.
> >> +     */
> >> +#define KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED          2
> >> +#define KVM_ARCH_FLAG_EL1_32BIT                             3
> >>      unsigned long flags;
> >>
> >>      /*
> >> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> >> index ecc40c8cd6f6..cbeb6216ee25 100644
> >> --- a/arch/arm64/kvm/reset.c
> >> +++ b/arch/arm64/kvm/reset.c
> >> @@ -181,27 +181,45 @@ static int kvm_vcpu_enable_ptrauth(struct kvm_vcpu *vcpu)
> >>      return 0;
> >>   }
> >>
> >> -static bool vcpu_allowed_register_width(struct kvm_vcpu *vcpu)
> >> +/*
> >> + * A guest can have either all EL1 32bit or 64bit vcpus only. It is
> >> + * indicated by a value of KVM_ARCH_FLAG_EL1_32BIT bit in kvm->arch.flags,
> >> + * which is valid only when KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED in
> >> + * kvm->arch.flags is set.
> >> + * This function sets the EL1_32BIT bit based on the given @is32bit (and
> >> + * sets REG_WIDTH_CONFIGURED bit). When those flags are already set,
> >> + * @is32bit must be consistent with the flags.
> >> + * Returns 0 on success, or non-zero otherwise.
> >> + */
> >
> > nit: use kerneldoc style:
> >
> >    https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html
>
> Sure, I can fix the comment to use kerneldoc style.
>
>
> >
> >> +static int kvm_set_vm_width(struct kvm *kvm, bool is32bit)
> >>   {
> >> -    struct kvm_vcpu *tmp;
> >> -    bool is32bit;
> >> -    unsigned long i;
> >> +    bool allowed;
> >> +
> >> +    lockdep_assert_held(&kvm->lock);
> >> +
> >> +    if (test_bit(KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED, &kvm->arch.flags)) {
> >> +            /*
> >> +             * The guest's register width is already configured.
> >> +             * Make sure that @is32bit is consistent with it.
> >> +             */
> >> +            allowed = (is32bit ==
> >> +                       test_bit(KVM_ARCH_FLAG_EL1_32BIT, &kvm->arch.flags));
> >> +            return allowed ? 0 : -EINVAL;
> >
> > nit: I'd avoid the ternary and just use a boring if/else (though I could
> > be in the minority here).
>
> I agree with you and will fix it.
> (The ternary with 'allowed' was just copied from the previous patch,
>   and I should have changed that in this patch...)
>
> Thanks,
> Reiji
>
>
> >
> >> +    }
> >>
> >> -    is32bit = vcpu_has_feature(vcpu, KVM_ARM_VCPU_EL1_32BIT);
> >>      if (!cpus_have_const_cap(ARM64_HAS_32BIT_EL1) && is32bit)
> >> -            return false;
> >> +            return -EINVAL;
> >>
> >>      /* MTE is incompatible with AArch32 */
> >> -    if (kvm_has_mte(vcpu->kvm) && is32bit)
> >> -            return false;
> >> +    if (kvm_has_mte(kvm) && is32bit)
> >> +            return -EINVAL;
> >>
> >> -    /* Check that the vcpus are either all 32bit or all 64bit */
> >> -    kvm_for_each_vcpu(i, tmp, vcpu->kvm) {
> >> -            if (vcpu_has_feature(tmp, KVM_ARM_VCPU_EL1_32BIT) != is32bit)
> >> -                    return false;
> >> -    }
> >> +    if (is32bit)
> >> +            set_bit(KVM_ARCH_FLAG_EL1_32BIT, &kvm->arch.flags);
> >>
> >> -    return true;
> >> +    set_bit(KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED, &kvm->arch.flags);
> >> +
> >> +    return 0;
> >>   }
> >>
> >>   /**
> >> @@ -230,10 +248,17 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
> >>      u32 pstate;
> >>
> >>      mutex_lock(&vcpu->kvm->lock);
> >> -    reset_state = vcpu->arch.reset_state;
> >> -    WRITE_ONCE(vcpu->arch.reset_state.reset, false);
> >> +    ret = kvm_set_vm_width(vcpu->kvm,
> >> +                           vcpu_has_feature(vcpu, KVM_ARM_VCPU_EL1_32BIT));
> >> +    if (!ret) {
> >> +            reset_state = vcpu->arch.reset_state;
> >> +            WRITE_ONCE(vcpu->arch.reset_state.reset, false);
> >> +    }
> >>      mutex_unlock(&vcpu->kvm->lock);
> >>
> >> +    if (ret)
> >> +            return ret;
> >> +
> >>      /* Reset PMU outside of the non-preemptible section */
> >>      kvm_pmu_vcpu_reset(vcpu);
> >>
> >> @@ -260,14 +285,9 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
> >>              }
> >>      }
> >>
> >> -    if (!vcpu_allowed_register_width(vcpu)) {
> >> -            ret = -EINVAL;
> >> -            goto out;
> >> -    }
> >> -
> >>      switch (vcpu->arch.target) {
> >>      default:
> >> -            if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features)) {
> >> +            if (vcpu_el1_is_32bit(vcpu)) {
> >>                      pstate = VCPU_RESET_PSTATE_SVC;
> >>              } else {
> >>                      pstate = VCPU_RESET_PSTATE_EL1;
> >> --
> >> 2.35.1.723.g4982287a31-goog
> >>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 2/3] KVM: arm64: mixed-width check should be skipped for uninitialized vCPUs

[Reiji Watanabe]

Hi Oliver,

On Tue, Mar 15, 2022 at 12:48 AM Oliver Upton <oupton@google.com> wrote:
>
> On Mon, Mar 14, 2022 at 11:19 PM Reiji Watanabe <reijiw@google.com> wrote:
> >
> > Hi Oliver,
> >
> > On 3/14/22 1:22 PM, Oliver Upton wrote:
> > > On Sun, Mar 13, 2022 at 11:19:58PM -0700, Reiji Watanabe wrote:
> > >> KVM allows userspace to configure either all EL1 32bit or 64bit vCPUs
> > >> for a guest.  At vCPU reset, vcpu_allowed_register_width() checks
> > >> if the vcpu's register width is consistent with all other vCPUs'.
> > >> Since the checking is done even against vCPUs that are not initialized
> > >> (KVM_ARM_VCPU_INIT has not been done) yet, the uninitialized vCPUs
> > >> are erroneously treated as 64bit vCPU, which causes the function to
> > >> incorrectly detect a mixed-width VM.
> > >>
> > >> Introduce KVM_ARCH_FLAG_EL1_32BIT and KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED
> > >> bits for kvm->arch.flags.  A value of the EL1_32BIT bit indicates that
> > >> the guest needs to be configured with all 32bit or 64bit vCPUs, and
> > >> a value of the REG_WIDTH_CONFIGURED bit indicates if a value of the
> > >> EL1_32BIT bit is valid (already set up). Values in those bits are set at
> > >> the first KVM_ARM_VCPU_INIT for the guest based on KVM_ARM_VCPU_EL1_32BIT
> > >> configuration for the vCPU.
> > >>
> > >> Check vcpu's register width against those new bits at the vcpu's
> > >> KVM_ARM_VCPU_INIT (instead of against other vCPUs' register width).
> > >>
> > >> Fixes: 66e94d5cafd4 ("KVM: arm64: Prevent mixed-width VM creation")
> > >> Signed-off-by: Reiji Watanabe <reijiw@google.com>
> > >
> > > Hrmph... I hate to be asking this question so late in the game, but...
> > >
> > > Are there any bits that we really allow variation per-vCPU besides
> > > KVM_ARM_VCPU_POWER_OFF? We unintentionally allow for variance with the
> > > KVM_ARM_VCPU_PSCI_0_2 bit even though that's complete nonsense.
> > >
> > > Stated plainly, should we just deny any attempts at asymmetry besides
> > > POWER_OFF?>
> > > Besides the nits, I see nothing objectionable with the patch. I'd really
> > > like to see more generalized constraints on vCPU configuration, but if
> > > this is the route we take:
> >
> > Prohibiting the mixed width configuration is not a new constraint that
> > this patch creates (this patch fixes a bug that erroneously detects
> > mixed-width configuration), and enforcing symmetry of other features
> > among vCPUs is a bit different matter.
>
> Right, I had managed to forget that context for a moment when I
> replied to you. Then I fully agree with this patch, and the other
> feature flags can be handled later.
>
> >
> > Having said that, I like the idea, which will be more consistent with
> > my ID register series (it can simplify things).  But, I'm not sure
> > if creating the constraint for those features would be a problem for
> > existing userspace even if allowing variation per-vCPU for the features
> > was not our intention.
> > I would guess having the constraint for KVM_ARM_VCPU_PSCI_0_2 should
> > be fine.  Do you think that should be fine for PMU, SVE, and PTRAUTH*
> > as well ?
>
> Personally, yes, but it prompts the question of if we could break
> userspace by applying restrictions after the fact. The original patch
> that applied the register width restrictions didn't cause much of a
> stir, so it seems possible we could get away with it.


I agree that it's possible we might get away with it, and I can try
that for the other features besides KVM_ARM_VCPU_POWER_OFF :)
(I will work it on separately from this series)

BTW, if there had been a general interface to configure per-VM feature,
I would guess that interface might have been chosen for PSCI_0_2.
Perhaps we should consider creating it the next time per-VM feature
is introduced.

Thanks,
Reiji


>
> > >
> > > Reviewed-by: Oliver Upton <oupton@google.com>
> > >
> > >> ---
> > >>   arch/arm64/include/asm/kvm_emulate.h | 27 ++++++++----
> > >>   arch/arm64/include/asm/kvm_host.h    |  9 ++++
> > >>   arch/arm64/kvm/reset.c               | 64 ++++++++++++++++++----------
> > >>   3 files changed, 70 insertions(+), 30 deletions(-)
> > >>
> > >> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> > >> index d62405ce3e6d..7496deab025a 100644
> > >> --- a/arch/arm64/include/asm/kvm_emulate.h
> > >> +++ b/arch/arm64/include/asm/kvm_emulate.h
> > >> @@ -43,10 +43,22 @@ void kvm_inject_pabt(struct kvm_vcpu *vcpu, unsigned long addr);
> > >>
> > >>   void kvm_vcpu_wfi(struct kvm_vcpu *vcpu);
> > >>
> > >> +#if defined(__KVM_VHE_HYPERVISOR__) || defined(__KVM_NVHE_HYPERVISOR__)
> > >>   static __always_inline bool vcpu_el1_is_32bit(struct kvm_vcpu *vcpu)
> > >>   {
> > >>      return !(vcpu->arch.hcr_el2 & HCR_RW);
> > >>   }
> > >> +#else
> > >> +static __always_inline bool vcpu_el1_is_32bit(struct kvm_vcpu *vcpu)
> > >> +{
> > >> +    struct kvm *kvm = vcpu->kvm;
> > >> +
> > >> +    WARN_ON_ONCE(!test_bit(KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED,
> > >> +                           &kvm->arch.flags));
> > >> +
> > >> +    return test_bit(KVM_ARCH_FLAG_EL1_32BIT, &kvm->arch.flags);
> > >> +}
> > >> +#endif
> > >>
> > >>   static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
> > >>   {
> > >> @@ -72,15 +84,14 @@ static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
> > >>              vcpu->arch.hcr_el2 |= HCR_TVM;
> > >>      }
> > >>
> > >> -    if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features))
> > >> +    if (vcpu_el1_is_32bit(vcpu))
> > >>              vcpu->arch.hcr_el2 &= ~HCR_RW;
> > >> -
> > >> -    /*
> > >> -     * TID3: trap feature register accesses that we virtualise.
> > >> -     * For now this is conditional, since no AArch32 feature regs
> > >> -     * are currently virtualised.
> > >> -     */
> > >> -    if (!vcpu_el1_is_32bit(vcpu))
> > >> +    else
> > >> +            /*
> > >> +             * TID3: trap feature register accesses that we virtualise.
> > >> +             * For now this is conditional, since no AArch32 feature regs
> > >> +             * are currently virtualised.
> > >> +             */
> > >>              vcpu->arch.hcr_el2 |= HCR_TID3;
> > >>
> > >>      if (cpus_have_const_cap(ARM64_MISMATCHED_CACHE_TYPE) ||
> > >> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > >> index 11a7ae747ded..22ad977069f5 100644
> > >> --- a/arch/arm64/include/asm/kvm_host.h
> > >> +++ b/arch/arm64/include/asm/kvm_host.h
> > >> @@ -125,6 +125,15 @@ struct kvm_arch {
> > >>   #define KVM_ARCH_FLAG_RETURN_NISV_IO_ABORT_TO_USER 0
> > >>      /* Memory Tagging Extension enabled for the guest */
> > >>   #define KVM_ARCH_FLAG_MTE_ENABLED                  1
> > >> +    /*
> > >> +     * The following two bits are used to indicate the guest's EL1
> > >> +     * register width configuration. A value of KVM_ARCH_FLAG_EL1_32BIT
> > >> +     * bit is valid only when KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED is set.
> > >> +     * Otherwise, the guest's EL1 register width has not yet been
> > >> +     * determined yet.
> > >> +     */
> > >> +#define KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED          2
> > >> +#define KVM_ARCH_FLAG_EL1_32BIT                             3
> > >>      unsigned long flags;
> > >>
> > >>      /*
> > >> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> > >> index ecc40c8cd6f6..cbeb6216ee25 100644
> > >> --- a/arch/arm64/kvm/reset.c
> > >> +++ b/arch/arm64/kvm/reset.c
> > >> @@ -181,27 +181,45 @@ static int kvm_vcpu_enable_ptrauth(struct kvm_vcpu *vcpu)
> > >>      return 0;
> > >>   }
> > >>
> > >> -static bool vcpu_allowed_register_width(struct kvm_vcpu *vcpu)
> > >> +/*
> > >> + * A guest can have either all EL1 32bit or 64bit vcpus only. It is
> > >> + * indicated by a value of KVM_ARCH_FLAG_EL1_32BIT bit in kvm->arch.flags,
> > >> + * which is valid only when KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED in
> > >> + * kvm->arch.flags is set.
> > >> + * This function sets the EL1_32BIT bit based on the given @is32bit (and
> > >> + * sets REG_WIDTH_CONFIGURED bit). When those flags are already set,
> > >> + * @is32bit must be consistent with the flags.
> > >> + * Returns 0 on success, or non-zero otherwise.
> > >> + */
> > >
> > > nit: use kerneldoc style:
> > >
> > >    https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html
> >
> > Sure, I can fix the comment to use kerneldoc style.
> >
> >
> > >
> > >> +static int kvm_set_vm_width(struct kvm *kvm, bool is32bit)
> > >>   {
> > >> -    struct kvm_vcpu *tmp;
> > >> -    bool is32bit;
> > >> -    unsigned long i;
> > >> +    bool allowed;
> > >> +
> > >> +    lockdep_assert_held(&kvm->lock);
> > >> +
> > >> +    if (test_bit(KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED, &kvm->arch.flags)) {
> > >> +            /*
> > >> +             * The guest's register width is already configured.
> > >> +             * Make sure that @is32bit is consistent with it.
> > >> +             */
> > >> +            allowed = (is32bit ==
> > >> +                       test_bit(KVM_ARCH_FLAG_EL1_32BIT, &kvm->arch.flags));
> > >> +            return allowed ? 0 : -EINVAL;
> > >
> > > nit: I'd avoid the ternary and just use a boring if/else (though I could
> > > be in the minority here).
> >
> > I agree with you and will fix it.
> > (The ternary with 'allowed' was just copied from the previous patch,
> >   and I should have changed that in this patch...)
> >
> > Thanks,
> > Reiji
> >
> >
> > >
> > >> +    }
> > >>
> > >> -    is32bit = vcpu_has_feature(vcpu, KVM_ARM_VCPU_EL1_32BIT);
> > >>      if (!cpus_have_const_cap(ARM64_HAS_32BIT_EL1) && is32bit)
> > >> -            return false;
> > >> +            return -EINVAL;
> > >>
> > >>      /* MTE is incompatible with AArch32 */
> > >> -    if (kvm_has_mte(vcpu->kvm) && is32bit)
> > >> -            return false;
> > >> +    if (kvm_has_mte(kvm) && is32bit)
> > >> +            return -EINVAL;
> > >>
> > >> -    /* Check that the vcpus are either all 32bit or all 64bit */
> > >> -    kvm_for_each_vcpu(i, tmp, vcpu->kvm) {
> > >> -            if (vcpu_has_feature(tmp, KVM_ARM_VCPU_EL1_32BIT) != is32bit)
> > >> -                    return false;
> > >> -    }
> > >> +    if (is32bit)
> > >> +            set_bit(KVM_ARCH_FLAG_EL1_32BIT, &kvm->arch.flags);
> > >>
> > >> -    return true;
> > >> +    set_bit(KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED, &kvm->arch.flags);
> > >> +
> > >> +    return 0;
> > >>   }
> > >>
> > >>   /**
> > >> @@ -230,10 +248,17 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
> > >>      u32 pstate;
> > >>
> > >>      mutex_lock(&vcpu->kvm->lock);
> > >> -    reset_state = vcpu->arch.reset_state;
> > >> -    WRITE_ONCE(vcpu->arch.reset_state.reset, false);
> > >> +    ret = kvm_set_vm_width(vcpu->kvm,
> > >> +                           vcpu_has_feature(vcpu, KVM_ARM_VCPU_EL1_32BIT));
> > >> +    if (!ret) {
> > >> +            reset_state = vcpu->arch.reset_state;
> > >> +            WRITE_ONCE(vcpu->arch.reset_state.reset, false);
> > >> +    }
> > >>      mutex_unlock(&vcpu->kvm->lock);
> > >>
> > >> +    if (ret)
> > >> +            return ret;
> > >> +
> > >>      /* Reset PMU outside of the non-preemptible section */
> > >>      kvm_pmu_vcpu_reset(vcpu);
> > >>
> > >> @@ -260,14 +285,9 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
> > >>              }
> > >>      }
> > >>
> > >> -    if (!vcpu_allowed_register_width(vcpu)) {
> > >> -            ret = -EINVAL;
> > >> -            goto out;
> > >> -    }
> > >> -
> > >>      switch (vcpu->arch.target) {
> > >>      default:
> > >> -            if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features)) {
> > >> +            if (vcpu_el1_is_32bit(vcpu)) {
> > >>                      pstate = VCPU_RESET_PSTATE_SVC;
> > >>              } else {
> > >>                      pstate = VCPU_RESET_PSTATE_EL1;
> > >> --
> > >> 2.35.1.723.g4982287a31-goog
> > >>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 2/3] KVM: arm64: mixed-width check should be skipped for uninitialized vCPUs

[Oliver Upton]

On Tue, Mar 15, 2022 at 09:22:10PM -0700, Reiji Watanabe wrote:
> Hi Oliver,
> 
> On Tue, Mar 15, 2022 at 12:48 AM Oliver Upton <oupton@google.com> wrote:
> > 
> > On Mon, Mar 14, 2022 at 11:19 PM Reiji Watanabe <reijiw@google.com> wrote:
> > >
> > > Hi Oliver,
> > >
> > > On 3/14/22 1:22 PM, Oliver Upton wrote:
> > > > On Sun, Mar 13, 2022 at 11:19:58PM -0700, Reiji Watanabe wrote:
> > > >> KVM allows userspace to configure either all EL1 32bit or 64bit vCPUs
> > > >> for a guest.  At vCPU reset, vcpu_allowed_register_width() checks
> > > >> if the vcpu's register width is consistent with all other vCPUs'.
> > > >> Since the checking is done even against vCPUs that are not initialized
> > > >> (KVM_ARM_VCPU_INIT has not been done) yet, the uninitialized vCPUs
> > > >> are erroneously treated as 64bit vCPU, which causes the function to
> > > >> incorrectly detect a mixed-width VM.
> > > >>
> > > >> Introduce KVM_ARCH_FLAG_EL1_32BIT and KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED
> > > >> bits for kvm->arch.flags.  A value of the EL1_32BIT bit indicates that
> > > >> the guest needs to be configured with all 32bit or 64bit vCPUs, and
> > > >> a value of the REG_WIDTH_CONFIGURED bit indicates if a value of the
> > > >> EL1_32BIT bit is valid (already set up). Values in those bits are set at
> > > >> the first KVM_ARM_VCPU_INIT for the guest based on KVM_ARM_VCPU_EL1_32BIT
> > > >> configuration for the vCPU.
> > > >>
> > > >> Check vcpu's register width against those new bits at the vcpu's
> > > >> KVM_ARM_VCPU_INIT (instead of against other vCPUs' register width).
> > > >>
> > > >> Fixes: 66e94d5cafd4 ("KVM: arm64: Prevent mixed-width VM creation")
> > > >> Signed-off-by: Reiji Watanabe <reijiw@google.com>
> > > >
> > > > Hrmph... I hate to be asking this question so late in the game, but...
> > > >
> > > > Are there any bits that we really allow variation per-vCPU besides
> > > > KVM_ARM_VCPU_POWER_OFF? We unintentionally allow for variance with the
> > > > KVM_ARM_VCPU_PSCI_0_2 bit even though that's complete nonsense.
> > > >
> > > > Stated plainly, should we just deny any attempts at asymmetry besides
> > > > POWER_OFF?>
> > > > Besides the nits, I see nothing objectionable with the patch. I'd really
> > > > like to see more generalized constraints on vCPU configuration, but if
> > > > this is the route we take:
> > >
> > > Prohibiting the mixed width configuration is not a new constraint that
> > > this patch creates (this patch fixes a bug that erroneously detects
> > > mixed-width configuration), and enforcing symmetry of other features
> > > among vCPUs is a bit different matter.
> > 
> > Right, I had managed to forget that context for a moment when I
> > replied to you. Then I fully agree with this patch, and the other
> > feature flags can be handled later.
> > 
> > >
> > > Having said that, I like the idea, which will be more consistent with
> > > my ID register series (it can simplify things).  But, I'm not sure
> > > if creating the constraint for those features would be a problem for
> > > existing userspace even if allowing variation per-vCPU for the features
> > > was not our intention.
> > > I would guess having the constraint for KVM_ARM_VCPU_PSCI_0_2 should
> > > be fine.  Do you think that should be fine for PMU, SVE, and PTRAUTH*
> > > as well ?
> > 
> > Personally, yes, but it prompts the question of if we could break
> > userspace by applying restrictions after the fact. The original patch
> > that applied the register width restrictions didn't cause much of a
> > stir, so it seems possible we could get away with it.
> 
> 
> I agree that it's possible we might get away with it, and I can try
> that for the other features besides KVM_ARM_VCPU_POWER_OFF :)
> (I will work it on separately from this series)
> 

Oh, that'd be great! I was just whining publicly :-)

> BTW, if there had been a general interface to configure per-VM feature,
> I would guess that interface might have been chosen for PSCI_0_2.
> Perhaps we should consider creating it the next time per-VM feature
> is introduced.
> 

I believe there is a lot in KVM we could go back and do better if we had
the patience for it ;-) On a more serious note, I do agree that we need
better mechanisms for VM-scoped bits going forward. 

> Thanks,
> Reiji
> 
> 
> > 
> > > >
> > > > Reviewed-by: Oliver Upton <oupton@google.com>
> > > >
> > > >> ---
> > > >>   arch/arm64/include/asm/kvm_emulate.h | 27 ++++++++----
> > > >>   arch/arm64/include/asm/kvm_host.h    |  9 ++++
> > > >>   arch/arm64/kvm/reset.c               | 64 ++++++++++++++++++----------
> > > >>   3 files changed, 70 insertions(+), 30 deletions(-)
> > > >>
> > > >> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> > > >> index d62405ce3e6d..7496deab025a 100644
> > > >> --- a/arch/arm64/include/asm/kvm_emulate.h
> > > >> +++ b/arch/arm64/include/asm/kvm_emulate.h
> > > >> @@ -43,10 +43,22 @@ void kvm_inject_pabt(struct kvm_vcpu *vcpu, unsigned long addr);
> > > >>
> > > >>   void kvm_vcpu_wfi(struct kvm_vcpu *vcpu);
> > > >>
> > > >> +#if defined(__KVM_VHE_HYPERVISOR__) || defined(__KVM_NVHE_HYPERVISOR__)
> > > >>   static __always_inline bool vcpu_el1_is_32bit(struct kvm_vcpu *vcpu)
> > > >>   {
> > > >>      return !(vcpu->arch.hcr_el2 & HCR_RW);
> > > >>   }
> > > >> +#else
> > > >> +static __always_inline bool vcpu_el1_is_32bit(struct kvm_vcpu *vcpu)
> > > >> +{
> > > >> +    struct kvm *kvm = vcpu->kvm;
> > > >> +
> > > >> +    WARN_ON_ONCE(!test_bit(KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED,
> > > >> +                           &kvm->arch.flags));
> > > >> +
> > > >> +    return test_bit(KVM_ARCH_FLAG_EL1_32BIT, &kvm->arch.flags);
> > > >> +}
> > > >> +#endif
> > > >>
> > > >>   static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
> > > >>   {
> > > >> @@ -72,15 +84,14 @@ static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
> > > >>              vcpu->arch.hcr_el2 |= HCR_TVM;
> > > >>      }
> > > >>
> > > >> -    if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features))
> > > >> +    if (vcpu_el1_is_32bit(vcpu))
> > > >>              vcpu->arch.hcr_el2 &= ~HCR_RW;
> > > >> -
> > > >> -    /*
> > > >> -     * TID3: trap feature register accesses that we virtualise.
> > > >> -     * For now this is conditional, since no AArch32 feature regs
> > > >> -     * are currently virtualised.
> > > >> -     */
> > > >> -    if (!vcpu_el1_is_32bit(vcpu))
> > > >> +    else
> > > >> +            /*
> > > >> +             * TID3: trap feature register accesses that we virtualise.
> > > >> +             * For now this is conditional, since no AArch32 feature regs
> > > >> +             * are currently virtualised.
> > > >> +             */
> > > >>              vcpu->arch.hcr_el2 |= HCR_TID3;
> > > >>
> > > >>      if (cpus_have_const_cap(ARM64_MISMATCHED_CACHE_TYPE) ||
> > > >> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > > >> index 11a7ae747ded..22ad977069f5 100644
> > > >> --- a/arch/arm64/include/asm/kvm_host.h
> > > >> +++ b/arch/arm64/include/asm/kvm_host.h
> > > >> @@ -125,6 +125,15 @@ struct kvm_arch {
> > > >>   #define KVM_ARCH_FLAG_RETURN_NISV_IO_ABORT_TO_USER 0
> > > >>      /* Memory Tagging Extension enabled for the guest */
> > > >>   #define KVM_ARCH_FLAG_MTE_ENABLED                  1
> > > >> +    /*
> > > >> +     * The following two bits are used to indicate the guest's EL1
> > > >> +     * register width configuration. A value of KVM_ARCH_FLAG_EL1_32BIT
> > > >> +     * bit is valid only when KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED is set.
> > > >> +     * Otherwise, the guest's EL1 register width has not yet been
> > > >> +     * determined yet.
> > > >> +     */
> > > >> +#define KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED          2
> > > >> +#define KVM_ARCH_FLAG_EL1_32BIT                             3
> > > >>      unsigned long flags;
> > > >>
> > > >>      /*
> > > >> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> > > >> index ecc40c8cd6f6..cbeb6216ee25 100644
> > > >> --- a/arch/arm64/kvm/reset.c
> > > >> +++ b/arch/arm64/kvm/reset.c
> > > >> @@ -181,27 +181,45 @@ static int kvm_vcpu_enable_ptrauth(struct kvm_vcpu *vcpu)
> > > >>      return 0;
> > > >>   }
> > > >>
> > > >> -static bool vcpu_allowed_register_width(struct kvm_vcpu *vcpu)
> > > >> +/*
> > > >> + * A guest can have either all EL1 32bit or 64bit vcpus only. It is
> > > >> + * indicated by a value of KVM_ARCH_FLAG_EL1_32BIT bit in kvm->arch.flags,
> > > >> + * which is valid only when KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED in
> > > >> + * kvm->arch.flags is set.
> > > >> + * This function sets the EL1_32BIT bit based on the given @is32bit (and
> > > >> + * sets REG_WIDTH_CONFIGURED bit). When those flags are already set,
> > > >> + * @is32bit must be consistent with the flags.
> > > >> + * Returns 0 on success, or non-zero otherwise.
> > > >> + */
> > > >
> > > > nit: use kerneldoc style:
> > > >
> > > >    https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html
> > >
> > > Sure, I can fix the comment to use kerneldoc style.
> > >
> > >
> > > >
> > > >> +static int kvm_set_vm_width(struct kvm *kvm, bool is32bit)
> > > >>   {
> > > >> -    struct kvm_vcpu *tmp;
> > > >> -    bool is32bit;
> > > >> -    unsigned long i;
> > > >> +    bool allowed;
> > > >> +
> > > >> +    lockdep_assert_held(&kvm->lock);
> > > >> +
> > > >> +    if (test_bit(KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED, &kvm->arch.flags)) {
> > > >> +            /*
> > > >> +             * The guest's register width is already configured.
> > > >> +             * Make sure that @is32bit is consistent with it.
> > > >> +             */
> > > >> +            allowed = (is32bit ==
> > > >> +                       test_bit(KVM_ARCH_FLAG_EL1_32BIT, &kvm->arch.flags));
> > > >> +            return allowed ? 0 : -EINVAL;
> > > >
> > > > nit: I'd avoid the ternary and just use a boring if/else (though I could
> > > > be in the minority here).
> > >
> > > I agree with you and will fix it.
> > > (The ternary with 'allowed' was just copied from the previous patch,
> > >   and I should have changed that in this patch...)
> > >
> > > Thanks,
> > > Reiji
> > >
> > >
> > > >
> > > >> +    }
> > > >>
> > > >> -    is32bit = vcpu_has_feature(vcpu, KVM_ARM_VCPU_EL1_32BIT);
> > > >>      if (!cpus_have_const_cap(ARM64_HAS_32BIT_EL1) && is32bit)
> > > >> -            return false;
> > > >> +            return -EINVAL;
> > > >>
> > > >>      /* MTE is incompatible with AArch32 */
> > > >> -    if (kvm_has_mte(vcpu->kvm) && is32bit)
> > > >> -            return false;
> > > >> +    if (kvm_has_mte(kvm) && is32bit)
> > > >> +            return -EINVAL;
> > > >>
> > > >> -    /* Check that the vcpus are either all 32bit or all 64bit */
> > > >> -    kvm_for_each_vcpu(i, tmp, vcpu->kvm) {
> > > >> -            if (vcpu_has_feature(tmp, KVM_ARM_VCPU_EL1_32BIT) != is32bit)
> > > >> -                    return false;
> > > >> -    }
> > > >> +    if (is32bit)
> > > >> +            set_bit(KVM_ARCH_FLAG_EL1_32BIT, &kvm->arch.flags);
> > > >>
> > > >> -    return true;
> > > >> +    set_bit(KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED, &kvm->arch.flags);
> > > >> +
> > > >> +    return 0;
> > > >>   }
> > > >>
> > > >>   /**
> > > >> @@ -230,10 +248,17 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
> > > >>      u32 pstate;
> > > >>
> > > >>      mutex_lock(&vcpu->kvm->lock);
> > > >> -    reset_state = vcpu->arch.reset_state;
> > > >> -    WRITE_ONCE(vcpu->arch.reset_state.reset, false);
> > > >> +    ret = kvm_set_vm_width(vcpu->kvm,
> > > >> +                           vcpu_has_feature(vcpu, KVM_ARM_VCPU_EL1_32BIT));
> > > >> +    if (!ret) {
> > > >> +            reset_state = vcpu->arch.reset_state;
> > > >> +            WRITE_ONCE(vcpu->arch.reset_state.reset, false);
> > > >> +    }
> > > >>      mutex_unlock(&vcpu->kvm->lock);
> > > >>
> > > >> +    if (ret)
> > > >> +            return ret;
> > > >> +
> > > >>      /* Reset PMU outside of the non-preemptible section */
> > > >>      kvm_pmu_vcpu_reset(vcpu);
> > > >>
> > > >> @@ -260,14 +285,9 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
> > > >>              }
> > > >>      }
> > > >>
> > > >> -    if (!vcpu_allowed_register_width(vcpu)) {
> > > >> -            ret = -EINVAL;
> > > >> -            goto out;
> > > >> -    }
> > > >> -
> > > >>      switch (vcpu->arch.target) {
> > > >>      default:
> > > >> -            if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features)) {
> > > >> +            if (vcpu_el1_is_32bit(vcpu)) {
> > > >>                      pstate = VCPU_RESET_PSTATE_SVC;
> > > >>              } else {
> > > >>                      pstate = VCPU_RESET_PSTATE_EL1;
> > > >> --
> > > >> 2.35.1.723.g4982287a31-goog
> > > >>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 3/3] KVM: arm64: selftests: Introduce vcpu_width_config

[Reiji Watanabe]

Introduce a test for aarch64 that ensures non-mixed-width vCPUs
(all 64bit vCPUs or all 32bit vcPUs) can be configured, and
mixed-width vCPUs cannot be configured.

Reviewed-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Reiji Watanabe <reijiw@google.com>
---
 tools/testing/selftests/kvm/.gitignore        |   1 +
 tools/testing/selftests/kvm/Makefile          |   1 +
 .../selftests/kvm/aarch64/vcpu_width_config.c | 125 ++++++++++++++++++
 3 files changed, 127 insertions(+)
 create mode 100644 tools/testing/selftests/kvm/aarch64/vcpu_width_config.c

diff --git a/tools/testing/selftests/kvm/.gitignore b/tools/testing/selftests/kvm/.gitignore
index dce7de7755e6..4e884e29b2a8 100644
--- a/tools/testing/selftests/kvm/.gitignore
+++ b/tools/testing/selftests/kvm/.gitignore
@@ -3,6 +3,7 @@
 /aarch64/debug-exceptions
 /aarch64/get-reg-list
 /aarch64/psci_cpu_on_test
+/aarch64/vcpu_width_config
 /aarch64/vgic_init
 /aarch64/vgic_irq
 /s390x/memop
diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index 17c3f0749f05..3482586c6e33 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -103,6 +103,7 @@ TEST_GEN_PROGS_aarch64 += aarch64/arch_timer
 TEST_GEN_PROGS_aarch64 += aarch64/debug-exceptions
 TEST_GEN_PROGS_aarch64 += aarch64/get-reg-list
 TEST_GEN_PROGS_aarch64 += aarch64/psci_cpu_on_test
+TEST_GEN_PROGS_aarch64 += aarch64/vcpu_width_config
 TEST_GEN_PROGS_aarch64 += aarch64/vgic_init
 TEST_GEN_PROGS_aarch64 += aarch64/vgic_irq
 TEST_GEN_PROGS_aarch64 += demand_paging_test
diff --git a/tools/testing/selftests/kvm/aarch64/vcpu_width_config.c b/tools/testing/selftests/kvm/aarch64/vcpu_width_config.c
new file mode 100644
index 000000000000..6e6e6a9f69e3
--- /dev/null
+++ b/tools/testing/selftests/kvm/aarch64/vcpu_width_config.c
@@ -0,0 +1,125 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * vcpu_width_config - Test KVM_ARM_VCPU_INIT() with KVM_ARM_VCPU_EL1_32BIT.
+ *
+ * Copyright (c) 2022 Google LLC.
+ *
+ * This is a test that ensures that non-mixed-width vCPUs (all 64bit vCPUs
+ * or all 32bit vcPUs) can be configured and mixed-width vCPUs cannot be
+ * configured.
+ */
+
+#define _GNU_SOURCE
+
+#include "kvm_util.h"
+#include "processor.h"
+#include "test_util.h"
+
+
+/*
+ * Add a vCPU, run KVM_ARM_VCPU_INIT with @init1, and then
+ * add another vCPU, and run KVM_ARM_VCPU_INIT with @init2.
+ */
+static int add_init_2vcpus(struct kvm_vcpu_init *init1,
+			   struct kvm_vcpu_init *init2)
+{
+	struct kvm_vm *vm;
+	int ret;
+
+	vm = vm_create(VM_MODE_DEFAULT, DEFAULT_GUEST_PHY_PAGES, O_RDWR);
+
+	vm_vcpu_add(vm, 0);
+	ret = _vcpu_ioctl(vm, 0, KVM_ARM_VCPU_INIT, init1);
+	if (ret)
+		goto free_exit;
+
+	vm_vcpu_add(vm, 1);
+	ret = _vcpu_ioctl(vm, 1, KVM_ARM_VCPU_INIT, init2);
+
+free_exit:
+	kvm_vm_free(vm);
+	return ret;
+}
+
+/*
+ * Add two vCPUs, then run KVM_ARM_VCPU_INIT for one vCPU with @init1,
+ * and run KVM_ARM_VCPU_INIT for another vCPU with @init2.
+ */
+static int add_2vcpus_init_2vcpus(struct kvm_vcpu_init *init1,
+				  struct kvm_vcpu_init *init2)
+{
+	struct kvm_vm *vm;
+	int ret;
+
+	vm = vm_create(VM_MODE_DEFAULT, DEFAULT_GUEST_PHY_PAGES, O_RDWR);
+
+	vm_vcpu_add(vm, 0);
+	vm_vcpu_add(vm, 1);
+
+	ret = _vcpu_ioctl(vm, 0, KVM_ARM_VCPU_INIT, init1);
+	if (ret)
+		goto free_exit;
+
+	ret = _vcpu_ioctl(vm, 1, KVM_ARM_VCPU_INIT, init2);
+
+free_exit:
+	kvm_vm_free(vm);
+	return ret;
+}
+
+/*
+ * Tests that two 64bit vCPUs can be configured, two 32bit vCPUs can be
+ * configured, and two mixed-witgh vCPUs cannot be configured.
+ * Each of those three cases, configure vCPUs in two different orders.
+ * The one is running KVM_CREATE_VCPU for 2 vCPUs, and then running
+ * KVM_ARM_VCPU_INIT for them.
+ * The other is running KVM_CREATE_VCPU and KVM_ARM_VCPU_INIT for a vCPU,
+ * and then run those commands for another vCPU.
+ */
+int main(void)
+{
+	struct kvm_vcpu_init init1, init2;
+	struct kvm_vm *vm;
+	int ret;
+
+	if (kvm_check_cap(KVM_CAP_ARM_EL1_32BIT) <= 0) {
+		print_skip("KVM_CAP_ARM_EL1_32BIT is not supported");
+		exit(KSFT_SKIP);
+	}
+
+	/* Get the preferred target type and copy that to init2 */
+	vm = vm_create(VM_MODE_DEFAULT, DEFAULT_GUEST_PHY_PAGES, O_RDWR);
+	vm_ioctl(vm, KVM_ARM_PREFERRED_TARGET, &init1);
+	kvm_vm_free(vm);
+	memcpy(&init2, &init1, sizeof(init2));
+
+	/* Test with 64bit vCPUs */
+	ret = add_init_2vcpus(&init1, &init2);
+	TEST_ASSERT(ret == 0,
+		    "Configuring 64bit EL1 vCPUs failed unexpectedly");
+	ret = add_2vcpus_init_2vcpus(&init1, &init2);
+	TEST_ASSERT(ret == 0,
+		    "Configuring 64bit EL1 vCPUs failed unexpectedly");
+
+	/* Test with 32bit vCPUs */
+	init1.features[0] = (1 << KVM_ARM_VCPU_EL1_32BIT);
+	init2.features[0] = (1 << KVM_ARM_VCPU_EL1_32BIT);
+	ret = add_init_2vcpus(&init1, &init2);
+	TEST_ASSERT(ret == 0,
+		    "Configuring 32bit EL1 vCPUs failed unexpectedly");
+	ret = add_2vcpus_init_2vcpus(&init1, &init2);
+	TEST_ASSERT(ret == 0,
+		    "Configuring 32bit EL1 vCPUs failed unexpectedly");
+
+	/* Test with mixed-width vCPUs  */
+	init1.features[0] = 0;
+	init2.features[0] = (1 << KVM_ARM_VCPU_EL1_32BIT);
+	ret = add_init_2vcpus(&init1, &init2);
+	TEST_ASSERT(ret != 0,
+		    "Configuring mixed-width vCPUs worked unexpectedly");
+	ret = add_2vcpus_init_2vcpus(&init1, &init2);
+	TEST_ASSERT(ret != 0,
+		    "Configuring mixed-width vCPUs worked unexpectedly");
+
+	return 0;
+}
-- 
2.35.1.723.g4982287a31-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel