[RFC] [PATCH 0/4] KVM: arm64: emulation for CTR_EL0

[RFC] [PATCH 0/4] KVM: arm64: emulation for CTR_EL0

[Sebastian Ott]

Hej folks,

I'm looking into supporting migration between 2 Ampere Altra (Max)
machines (using Neoverse-N1). They are almost identical regarding
their feature id register state except for CTR_EL0.DIC which is set
on one machine but not the other.

For a complete picture it's worth noting that the machine with
CTR_EL0.DIC = 1 also suffers from erratum 1542419 meaning for
userspace we trap the access and fake DIC=0. A KVM guest still sees
the original host value for that register (I assume that this is
intentional so that a guest OS can apply its own workaround to the
erratum).

Anyway, CTR_EL0 is currently marked as invariant and migrating a VM
between those 2 machines using qemu fails.

With the patches below guest access to CTR_EL0 is emulated and
CTR_EL0.DIC can be disabled using KVM_SET_ONE_REG. I'm sending this
as an RFC since I likely missed something obvious and there's still
stuff that needs improving - e.g. I didn't look into FGT. The last
patch adds a tool to dump the KVM register state together with the
writable masks - not sure if that's helpful to others but I've used
this to compare the register state between different machines
(abusing kvm selftests for this is probably not the best idea - I
just wanted to use the neat helpers they provide).

Thanks,
Sebastian

Sebastian Ott (4):
  KVM: arm64: add emulation for CTR_EL0 register
  KVM: arm64: ensure guest access to CTR_EL0 is trapped
  KVM: arm64: show writable masks for feature registers
  KVM: selftests: aarch64: add tool to dump registers

 arch/arm64/include/asm/kvm_emulate.h          |  7 +-
 arch/arm64/kvm/sys_regs.c                     | 63 ++++++++++------
 tools/testing/selftests/kvm/Makefile          |  1 +
 .../testing/selftests/kvm/aarch64/dump_regs.c | 72 +++++++++++++++++++
 4 files changed, 114 insertions(+), 29 deletions(-)
 create mode 100644 tools/testing/selftests/kvm/aarch64/dump_regs.c

-- 
2.42.0


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

[PATCH 1/4] KVM: arm64: add emulation for CTR_EL0 register

[Sebastian Ott]

CTR_EL0 is currently handled as an invariant register, thus
guests will be presented with the host value of that register.
Add emulation for CTR_EL0 and maintain a per vcpu value. The
only thing that is allowed to be changed compared to the host
value is to switch off the DIC bit which describes Icache
invalidation requirements.

Signed-off-by: Sebastian Ott <sebott@redhat.com>
---
 arch/arm64/kvm/sys_regs.c | 44 +++++++++++++++++++++++++++++++--------
 1 file changed, 35 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 30253bd19917..b2019faa9d73 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1871,10 +1871,42 @@ static bool access_ctr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 	if (p->is_write)
 		return write_to_read_only(vcpu, p, r);
 
-	p->regval = read_sanitised_ftr_reg(SYS_CTR_EL0);
+	p->regval = __vcpu_sys_reg(vcpu, r->reg);
 	return true;
 }
 
+static u64 reset_ctr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd)
+{
+	u64 val = read_sanitised_ftr_reg(SYS_CTR_EL0);
+
+	__vcpu_sys_reg(vcpu, rd->reg) = val;
+	return __vcpu_sys_reg(vcpu, rd->reg);
+}
+
+static int get_ctr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
+		   u64 *val)
+{
+	*val = __vcpu_sys_reg(vcpu, rd->reg);
+	return 0;
+}
+
+static int set_ctr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
+		   u64 val)
+{
+	u64 ctr_el0 = read_sanitised_ftr_reg(SYS_CTR_EL0);
+
+	if (kvm_vm_has_ran_once(vcpu->kvm) &&
+	    val != __vcpu_sys_reg(vcpu, rd->reg))
+		return -EBUSY;
+
+	if (((ctr_el0 & ~CTR_EL0_DIC_MASK) != (val & ~CTR_EL0_DIC_MASK)) ||
+	    ((ctr_el0 & CTR_EL0_DIC_MASK) < (val & CTR_EL0_DIC_MASK)))
+		return -EINVAL;
+
+	__vcpu_sys_reg(vcpu, rd->reg) = val;
+	return 0;
+}
+
 static bool access_clidr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 			 const struct sys_reg_desc *r)
 {
@@ -2461,7 +2493,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	{ SYS_DESC(SYS_CCSIDR2_EL1), undef_access },
 	{ SYS_DESC(SYS_SMIDR_EL1), undef_access },
 	{ SYS_DESC(SYS_CSSELR_EL1), access_csselr, reset_unknown, CSSELR_EL1 },
-	{ SYS_DESC(SYS_CTR_EL0), access_ctr },
+	{ SYS_DESC(SYS_CTR_EL0), access_ctr, .reset = reset_ctr,
+	  .get_user = get_ctr, .set_user = set_ctr},
 	{ SYS_DESC(SYS_SVCR), undef_access },
 
 	{ PMU_SYS_REG(PMCR_EL0), .access = access_pmcr, .reset = reset_pmcr,
@@ -3578,18 +3611,11 @@ FUNCTION_INVARIANT(midr_el1)
 FUNCTION_INVARIANT(revidr_el1)
 FUNCTION_INVARIANT(aidr_el1)
 
-static u64 get_ctr_el0(struct kvm_vcpu *v, const struct sys_reg_desc *r)
-{
-	((struct sys_reg_desc *)r)->val = read_sanitised_ftr_reg(SYS_CTR_EL0);
-	return ((struct sys_reg_desc *)r)->val;
-}
-
 /* ->val is filled in by kvm_sys_reg_table_init() */
 static struct sys_reg_desc invariant_sys_regs[] __ro_after_init = {
 	{ SYS_DESC(SYS_MIDR_EL1), NULL, get_midr_el1 },
 	{ SYS_DESC(SYS_REVIDR_EL1), NULL, get_revidr_el1 },
 	{ SYS_DESC(SYS_AIDR_EL1), NULL, get_aidr_el1 },
-	{ SYS_DESC(SYS_CTR_EL0), NULL, get_ctr_el0 },
 };
 
 static int get_invariant_sys_reg(u64 id, u64 __user *uaddr)
-- 
2.42.0


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

[PATCH 2/4] KVM: arm64: ensure guest access to CTR_EL0 is trapped

[Sebastian Ott]

Ensure that guest access to the CTR_EL0 register is trapped
independent of cpu errata.

Signed-off-by: Sebastian Ott <sebott@redhat.com>
---
 arch/arm64/include/asm/kvm_emulate.h | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
index b804fe832184..daaaf7664219 100644
--- a/arch/arm64/include/asm/kvm_emulate.h
+++ b/arch/arm64/include/asm/kvm_emulate.h
@@ -91,12 +91,7 @@ static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
 		vcpu->arch.hcr_el2 |= HCR_TVM;
 	}
 
-	if (cpus_have_final_cap(ARM64_HAS_EVT) &&
-	    !cpus_have_final_cap(ARM64_MISMATCHED_CACHE_TYPE))
-		vcpu->arch.hcr_el2 |= HCR_TID4;
-	else
-		vcpu->arch.hcr_el2 |= HCR_TID2;
-
+	vcpu->arch.hcr_el2 |= HCR_TID2;
 	if (vcpu_el1_is_32bit(vcpu))
 		vcpu->arch.hcr_el2 &= ~HCR_RW;
 
-- 
2.42.0


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

[PATCH 3/4] KVM: arm64: show writable masks for feature registers

[Sebastian Ott]

Instead of using ~0UL provide the actual writable mask for
non-id feature registers in the output of the
KVM_ARM_GET_REG_WRITABLE_MASKS ioctl.

This changes the mask for the CTR_EL0 register.

Signed-off-by: Sebastian Ott <sebott@redhat.com>
---
 arch/arm64/kvm/sys_regs.c | 21 ++++++---------------
 1 file changed, 6 insertions(+), 15 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index b2019faa9d73..0f8fe7790c35 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -2489,12 +2489,12 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 
 	{ SYS_DESC(SYS_CCSIDR_EL1), access_ccsidr },
 	{ SYS_DESC(SYS_CLIDR_EL1), access_clidr, reset_clidr, CLIDR_EL1,
-	  .set_user = set_clidr },
+	  .set_user = set_clidr, .val = ~0UL },
 	{ SYS_DESC(SYS_CCSIDR2_EL1), undef_access },
 	{ SYS_DESC(SYS_SMIDR_EL1), undef_access },
 	{ SYS_DESC(SYS_CSSELR_EL1), access_csselr, reset_unknown, CSSELR_EL1 },
 	{ SYS_DESC(SYS_CTR_EL0), access_ctr, .reset = reset_ctr,
-	  .get_user = get_ctr, .set_user = set_ctr},
+	  .get_user = get_ctr, .set_user = set_ctr, .val = CTR_EL0_DIC_MASK},
 	{ SYS_DESC(SYS_SVCR), undef_access },
 
 	{ PMU_SYS_REG(PMCR_EL0), .access = access_pmcr, .reset = reset_pmcr,
@@ -3934,20 +3934,11 @@ int kvm_vm_ioctl_get_reg_writable_masks(struct kvm *kvm, struct reg_mask_range *
 		if (!is_feature_id_reg(encoding) || !reg->set_user)
 			continue;
 
-		/*
-		 * For ID registers, we return the writable mask. Other feature
-		 * registers return a full 64bit mask. That's not necessary
-		 * compliant with a given revision of the architecture, but the
-		 * RES0/RES1 definitions allow us to do that.
-		 */
-		if (is_id_reg(encoding)) {
-			if (!reg->val ||
-			    (is_aa32_id_reg(encoding) && !kvm_supports_32bit_el0()))
-				continue;
-			val = reg->val;
-		} else {
-			val = ~0UL;
+		if (!reg->val ||
+		    (is_aa32_id_reg(encoding) && !kvm_supports_32bit_el0())) {
+			continue;
 		}
+		val = reg->val;
 
 		if (put_user(val, (masks + KVM_ARM_FEATURE_ID_RANGE_INDEX(encoding))))
 			return -EFAULT;
-- 
2.42.0


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

[PATCH 4/4] KVM: selftests: aarch64: add tool to dump registers

[Sebastian Ott]

Iterate over registers obtained via KVM_GET_REG_LIST, dump
their values and writable masks.

Signed-off-by: Sebastian Ott <sebott@redhat.com>
---
 tools/testing/selftests/kvm/Makefile          |  1 +
 .../testing/selftests/kvm/aarch64/dump_regs.c | 72 +++++++++++++++++++
 2 files changed, 73 insertions(+)
 create mode 100644 tools/testing/selftests/kvm/aarch64/dump_regs.c

diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index 492e937fab00..3bbb163abb1e 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -145,6 +145,7 @@ TEST_GEN_PROGS_EXTENDED_x86_64 += x86_64/nx_huge_pages_test
 TEST_GEN_PROGS_aarch64 += aarch64/aarch32_id_regs
 TEST_GEN_PROGS_aarch64 += aarch64/arch_timer
 TEST_GEN_PROGS_aarch64 += aarch64/debug-exceptions
+TEST_GEN_PROGS_aarch64 += aarch64/dump_regs
 TEST_GEN_PROGS_aarch64 += aarch64/hypercalls
 TEST_GEN_PROGS_aarch64 += aarch64/page_fault_test
 TEST_GEN_PROGS_aarch64 += aarch64/psci_test
diff --git a/tools/testing/selftests/kvm/aarch64/dump_regs.c b/tools/testing/selftests/kvm/aarch64/dump_regs.c
new file mode 100644
index 000000000000..390b4629ca12
--- /dev/null
+++ b/tools/testing/selftests/kvm/aarch64/dump_regs.c
@@ -0,0 +1,72 @@
+#include <stdint.h>
+#include "kvm_util.h"
+#include "processor.h"
+#include "test_util.h"
+#include <linux/bitfield.h>
+
+static uint64_t masks[KVM_ARM_FEATURE_ID_RANGE_SIZE];
+
+static void guest_code(void)
+{
+}
+
+static void get_writable_masks(struct kvm_vm *vm)
+{
+	struct reg_mask_range range = {
+		.addr = (__u64)masks,
+	};
+
+	memset(range.reserved, 0, sizeof(range.reserved));
+	vm_ioctl(vm, KVM_ARM_GET_REG_WRITABLE_MASKS, &range);
+}
+
+#define kvm_sys_reg_Op0(id) (((id) & KVM_REG_ARM64_SYSREG_OP0_MASK) >> KVM_REG_ARM64_SYSREG_OP0_SHIFT)
+#define kvm_sys_reg_Op1(id) (((id) & KVM_REG_ARM64_SYSREG_OP1_MASK) >> KVM_REG_ARM64_SYSREG_OP1_SHIFT)
+#define kvm_sys_reg_CRn(id) (((id) & KVM_REG_ARM64_SYSREG_CRN_MASK) >> KVM_REG_ARM64_SYSREG_CRN_SHIFT)
+#define kvm_sys_reg_CRm(id) (((id) & KVM_REG_ARM64_SYSREG_CRM_MASK) >> KVM_REG_ARM64_SYSREG_CRM_SHIFT)
+#define kvm_sys_reg_Op2(id) (((id) & KVM_REG_ARM64_SYSREG_OP2_MASK) >> KVM_REG_ARM64_SYSREG_OP2_SHIFT)
+
+static bool reg_in_feature_id_range(uint64_t reg)
+{
+	return kvm_sys_reg_Op0(reg) == 3 &&
+	       kvm_sys_reg_Op1(reg) >= 0 && kvm_sys_reg_Op1(reg) <= 3 &&
+	       kvm_sys_reg_CRn(reg) == 0 &&
+	       kvm_sys_reg_CRm(reg) >= 0 && kvm_sys_reg_CRm(reg) <= 7 &&
+	       kvm_sys_reg_Op2(reg) >= 0 && kvm_sys_reg_Op2(reg) <= 7;
+}
+
+static uint64_t reg_get_mask(uint64_t reg)
+{
+	int idx = KVM_ARM_FEATURE_ID_RANGE_IDX(kvm_sys_reg_Op0(reg), kvm_sys_reg_Op1(reg),
+					       kvm_sys_reg_CRn(reg), kvm_sys_reg_CRm(reg),
+					       kvm_sys_reg_Op2(reg));
+
+	return reg_in_feature_id_range(reg) ? masks[idx] : 0;
+}
+
+static void dump_regs(struct kvm_vcpu *vcpu)
+{
+	struct kvm_reg_list *reg_list = vcpu_get_reg_list(vcpu);
+	uint64_t reg, val[8];
+	int i, ret;
+
+	for (i = 0; i < reg_list->n; i++) {
+		reg = reg_list->reg[i];
+		ret = __vcpu_get_reg(vcpu, reg, val);
+		if (!ret)
+			pr_info("%lx : %16lx %lx\n", reg, val[0], reg_get_mask(reg));
+	}
+
+	free(reg_list);
+}
+
+int main(void)
+{
+	struct kvm_vcpu *vcpu;
+	struct kvm_vm *vm;
+
+	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
+	get_writable_masks(vm);
+	dump_regs(vcpu);
+	kvm_vm_free(vm);
+}
-- 
2.42.0


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

Re: [PATCH 1/4] KVM: arm64: add emulation for CTR_EL0 register

[Marc Zyngier]

Please add all the reviewers and the relevant mailing lists.

On Mon, 18 Mar 2024 11:16:33 +0000,
Sebastian Ott <sebott@redhat.com> wrote:
> 
> CTR_EL0 is currently handled as an invariant register, thus
> guests will be presented with the host value of that register.
> Add emulation for CTR_EL0 and maintain a per vcpu value. The
> only thing that is allowed to be changed compared to the host
> value is to switch off the DIC bit which describes Icache
> invalidation requirements.
> 
> Signed-off-by: Sebastian Ott <sebott@redhat.com>
> ---
>  arch/arm64/kvm/sys_regs.c | 44 +++++++++++++++++++++++++++++++--------
>  1 file changed, 35 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 30253bd19917..b2019faa9d73 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1871,10 +1871,42 @@ static bool access_ctr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>  	if (p->is_write)
>  		return write_to_read_only(vcpu, p, r);
>  
> -	p->regval = read_sanitised_ftr_reg(SYS_CTR_EL0);
> +	p->regval = __vcpu_sys_reg(vcpu, r->reg);
>  	return true;
>  }
>  
> +static u64 reset_ctr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd)
> +{
> +	u64 val = read_sanitised_ftr_reg(SYS_CTR_EL0);
> +
> +	__vcpu_sys_reg(vcpu, rd->reg) = val;
> +	return __vcpu_sys_reg(vcpu, rd->reg);

I really don't think we should make this a per-CPU value, and instead
keep it a VM-wide value, just like any other ID register.

Also, rd->reg isn't set to anything useful, leading to memory
corruption (hint, there is no CTR_EL0 in the vcpu sysreg file).

> +}
> +
> +static int get_ctr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> +		   u64 *val)
> +{
> +	*val = __vcpu_sys_reg(vcpu, rd->reg);
> +	return 0;
> +}
> +
> +static int set_ctr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> +		   u64 val)
> +{
> +	u64 ctr_el0 = read_sanitised_ftr_reg(SYS_CTR_EL0);
> +
> +	if (kvm_vm_has_ran_once(vcpu->kvm) &&
> +	    val != __vcpu_sys_reg(vcpu, rd->reg))
> +		return -EBUSY;
> +
> +	if (((ctr_el0 & ~CTR_EL0_DIC_MASK) != (val & ~CTR_EL0_DIC_MASK)) ||
> +	    ((ctr_el0 & CTR_EL0_DIC_MASK) < (val & CTR_EL0_DIC_MASK)))
> +		return -EINVAL;

Why limit this to DIC only? Anything that advertises something
"weaker" than what the HW has, such as a smaller cache line size, is
equally valid.

> +
> +	__vcpu_sys_reg(vcpu, rd->reg) = val;
> +	return 0;
> +}
> +
>  static bool access_clidr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>  			 const struct sys_reg_desc *r)
>  {
> @@ -2461,7 +2493,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>  	{ SYS_DESC(SYS_CCSIDR2_EL1), undef_access },
>  	{ SYS_DESC(SYS_SMIDR_EL1), undef_access },
>  	{ SYS_DESC(SYS_CSSELR_EL1), access_csselr, reset_unknown, CSSELR_EL1 },
> -	{ SYS_DESC(SYS_CTR_EL0), access_ctr },
> +	{ SYS_DESC(SYS_CTR_EL0), access_ctr, .reset = reset_ctr,
> +	  .get_user = get_ctr, .set_user = set_ctr},

Now, who traps this? Since c876c3f182a5 ("KVM: arm64: Relax trapping
of CTR_EL0 when FEAT_EVT is available"), we don't trap it anymore when
FEAT_EVT is present. Surely you should account for this.

Also, we really shouldn't trap it unless the guest view is different,
as this has a very visible impact on any userspace.

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 1/4] KVM: arm64: add emulation for CTR_EL0 register

[Marc Zyngier]

Please add all the reviewers and the relevant mailing lists.

On Mon, 18 Mar 2024 11:16:33 +0000,
Sebastian Ott <sebott@redhat.com> wrote:
> 
> CTR_EL0 is currently handled as an invariant register, thus
> guests will be presented with the host value of that register.
> Add emulation for CTR_EL0 and maintain a per vcpu value. The
> only thing that is allowed to be changed compared to the host
> value is to switch off the DIC bit which describes Icache
> invalidation requirements.
> 
> Signed-off-by: Sebastian Ott <sebott@redhat.com>
> ---
>  arch/arm64/kvm/sys_regs.c | 44 +++++++++++++++++++++++++++++++--------
>  1 file changed, 35 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 30253bd19917..b2019faa9d73 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1871,10 +1871,42 @@ static bool access_ctr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>  	if (p->is_write)
>  		return write_to_read_only(vcpu, p, r);
>  
> -	p->regval = read_sanitised_ftr_reg(SYS_CTR_EL0);
> +	p->regval = __vcpu_sys_reg(vcpu, r->reg);
>  	return true;
>  }
>  
> +static u64 reset_ctr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd)
> +{
> +	u64 val = read_sanitised_ftr_reg(SYS_CTR_EL0);
> +
> +	__vcpu_sys_reg(vcpu, rd->reg) = val;
> +	return __vcpu_sys_reg(vcpu, rd->reg);

I really don't think we should make this a per-CPU value, and instead
keep it a VM-wide value, just like any other ID register.

Also, rd->reg isn't set to anything useful, leading to memory
corruption (hint, there is no CTR_EL0 in the vcpu sysreg file).

> +}
> +
> +static int get_ctr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> +		   u64 *val)
> +{
> +	*val = __vcpu_sys_reg(vcpu, rd->reg);
> +	return 0;
> +}
> +
> +static int set_ctr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> +		   u64 val)
> +{
> +	u64 ctr_el0 = read_sanitised_ftr_reg(SYS_CTR_EL0);
> +
> +	if (kvm_vm_has_ran_once(vcpu->kvm) &&
> +	    val != __vcpu_sys_reg(vcpu, rd->reg))
> +		return -EBUSY;
> +
> +	if (((ctr_el0 & ~CTR_EL0_DIC_MASK) != (val & ~CTR_EL0_DIC_MASK)) ||
> +	    ((ctr_el0 & CTR_EL0_DIC_MASK) < (val & CTR_EL0_DIC_MASK)))
> +		return -EINVAL;

Why limit this to DIC only? Anything that advertises something
"weaker" than what the HW has, such as a smaller cache line size, is
equally valid.

> +
> +	__vcpu_sys_reg(vcpu, rd->reg) = val;
> +	return 0;
> +}
> +
>  static bool access_clidr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>  			 const struct sys_reg_desc *r)
>  {
> @@ -2461,7 +2493,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>  	{ SYS_DESC(SYS_CCSIDR2_EL1), undef_access },
>  	{ SYS_DESC(SYS_SMIDR_EL1), undef_access },
>  	{ SYS_DESC(SYS_CSSELR_EL1), access_csselr, reset_unknown, CSSELR_EL1 },
> -	{ SYS_DESC(SYS_CTR_EL0), access_ctr },
> +	{ SYS_DESC(SYS_CTR_EL0), access_ctr, .reset = reset_ctr,
> +	  .get_user = get_ctr, .set_user = set_ctr},

Now, who traps this? Since c876c3f182a5 ("KVM: arm64: Relax trapping
of CTR_EL0 when FEAT_EVT is available"), we don't trap it anymore when
FEAT_EVT is present. Surely you should account for this.

Also, we really shouldn't trap it unless the guest view is different,
as this has a very visible impact on any userspace.

	M.

-- 
Without deviation from the norm, progress is not possible.

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

Re: [PATCH 2/4] KVM: arm64: ensure guest access to CTR_EL0 is trapped

[Marc Zyngier]

On Mon, 18 Mar 2024 11:16:34 +0000,
Sebastian Ott <sebott@redhat.com> wrote:
> 
> Ensure that guest access to the CTR_EL0 register is trapped
> independent of cpu errata.
> 
> Signed-off-by: Sebastian Ott <sebott@redhat.com>
> ---
>  arch/arm64/include/asm/kvm_emulate.h | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> index b804fe832184..daaaf7664219 100644
> --- a/arch/arm64/include/asm/kvm_emulate.h
> +++ b/arch/arm64/include/asm/kvm_emulate.h
> @@ -91,12 +91,7 @@ static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
>  		vcpu->arch.hcr_el2 |= HCR_TVM;
>  	}
>  
> -	if (cpus_have_final_cap(ARM64_HAS_EVT) &&
> -	    !cpus_have_final_cap(ARM64_MISMATCHED_CACHE_TYPE))
> -		vcpu->arch.hcr_el2 |= HCR_TID4;
> -	else
> -		vcpu->arch.hcr_el2 |= HCR_TID2;
> -
> +	vcpu->arch.hcr_el2 |= HCR_TID2;

NAK. This is a huge performance sync. You should only revert to this
when absolutely necessary.

	M.

-- 
Without deviation from the norm, progress is not possible.

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

Re: [PATCH 3/4] KVM: arm64: show writable masks for feature registers

[Marc Zyngier]

On Mon, 18 Mar 2024 11:16:35 +0000,
Sebastian Ott <sebott@redhat.com> wrote:
> 
> Instead of using ~0UL provide the actual writable mask for
> non-id feature registers in the output of the
> KVM_ARM_GET_REG_WRITABLE_MASKS ioctl.

CTR_EL0 definitely seems to be in the feature ID range. And so does
CLIDR_EL1.

> 
> This changes the mask for the CTR_EL0 register.

Only that?

> 
> Signed-off-by: Sebastian Ott <sebott@redhat.com>
> ---
>  arch/arm64/kvm/sys_regs.c | 21 ++++++---------------
>  1 file changed, 6 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index b2019faa9d73..0f8fe7790c35 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -2489,12 +2489,12 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>  
>  	{ SYS_DESC(SYS_CCSIDR_EL1), access_ccsidr },
>  	{ SYS_DESC(SYS_CLIDR_EL1), access_clidr, reset_clidr, CLIDR_EL1,
> -	  .set_user = set_clidr },
> +	  .set_user = set_clidr, .val = ~0UL },

How is CLIDR_EL1 (and all the crap that depend on it) recomputed when
CTR_EL0 is changed?

>  	{ SYS_DESC(SYS_CCSIDR2_EL1), undef_access },
>  	{ SYS_DESC(SYS_SMIDR_EL1), undef_access },
>  	{ SYS_DESC(SYS_CSSELR_EL1), access_csselr, reset_unknown, CSSELR_EL1 },
>  	{ SYS_DESC(SYS_CTR_EL0), access_ctr, .reset = reset_ctr,
> -	  .get_user = get_ctr, .set_user = set_ctr},
> +	  .get_user = get_ctr, .set_user = set_ctr, .val = CTR_EL0_DIC_MASK},

This is all extremely fragile. We need a better solution for this. And
we need far more than just DIC here.

>  	{ SYS_DESC(SYS_SVCR), undef_access },
>  
>  	{ PMU_SYS_REG(PMCR_EL0), .access = access_pmcr, .reset = reset_pmcr,
> @@ -3934,20 +3934,11 @@ int kvm_vm_ioctl_get_reg_writable_masks(struct kvm *kvm, struct reg_mask_range *
>  		if (!is_feature_id_reg(encoding) || !reg->set_user)
>  			continue;
>  
> -		/*
> -		 * For ID registers, we return the writable mask. Other feature
> -		 * registers return a full 64bit mask. That's not necessary
> -		 * compliant with a given revision of the architecture, but the
> -		 * RES0/RES1 definitions allow us to do that.
> -		 */
> -		if (is_id_reg(encoding)) {
> -			if (!reg->val ||
> -			    (is_aa32_id_reg(encoding) && !kvm_supports_32bit_el0()))
> -				continue;
> -			val = reg->val;
> -		} else {
> -			val = ~0UL;
> +		if (!reg->val ||
> +		    (is_aa32_id_reg(encoding) && !kvm_supports_32bit_el0())) {
> +			continue;
>  		}
> +		val = reg->val;

Are CLIDR_EL1 and CTR_EL0 the only two ID registers that had a
set_user callback without an encoded mask?

	M.

-- 
Without deviation from the norm, progress is not possible.

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

Re: [RFC] [PATCH 0/4] KVM: arm64: emulation for CTR_EL0

[Marc Zyngier]

On Mon, 18 Mar 2024 11:16:32 +0000,
Sebastian Ott <sebott@redhat.com> wrote:
> 
> Hej folks,
> 
> I'm looking into supporting migration between 2 Ampere Altra (Max)
> machines (using Neoverse-N1). They are almost identical regarding
> their feature id register state except for CTR_EL0.DIC which is set
> on one machine but not the other.
> 
> For a complete picture it's worth noting that the machine with
> CTR_EL0.DIC = 1 also suffers from erratum 1542419 meaning for
> userspace we trap the access and fake DIC=0. A KVM guest still sees
> the original host value for that register (I assume that this is
> intentional so that a guest OS can apply its own workaround to the
> erratum).

Indeed.

The intention is that the EL1 guest will hide DIC for EL0, while EL3
will trap IC instructions from EL0 and replace them with a TLBI.
That's of course assuming that the machine has received an updated
firmware, something that cannot be probed AFAICT.

	M.

-- 
Without deviation from the norm, progress is not possible.

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

Re: [PATCH 3/4] KVM: arm64: show writable masks for feature registers

[Sebastian Ott]

On Mon, 18 Mar 2024, Marc Zyngier wrote:
> On Mon, 18 Mar 2024 11:16:35 +0000,
> Sebastian Ott <sebott@redhat.com> wrote:
>>
>> Instead of using ~0UL provide the actual writable mask for
>> non-id feature registers in the output of the
>> KVM_ARM_GET_REG_WRITABLE_MASKS ioctl.
>
> CTR_EL0 definitely seems to be in the feature ID range. And so does
> CLIDR_EL1.

Sry, bad wording. I mean feature id regs for which is_id_reg() is false.

>>
>> This changes the mask for the CTR_EL0 register.
>
> Only that?

Yes.

>> Signed-off-by: Sebastian Ott <sebott@redhat.com>
>> ---
>>  arch/arm64/kvm/sys_regs.c | 21 ++++++---------------
>>  1 file changed, 6 insertions(+), 15 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index b2019faa9d73..0f8fe7790c35 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -2489,12 +2489,12 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>>
>>  	{ SYS_DESC(SYS_CCSIDR_EL1), access_ccsidr },
>>  	{ SYS_DESC(SYS_CLIDR_EL1), access_clidr, reset_clidr, CLIDR_EL1,
>> -	  .set_user = set_clidr },
>> +	  .set_user = set_clidr, .val = ~0UL },
>
> How is CLIDR_EL1 (and all the crap that depend on it) recomputed when
> CTR_EL0 is changed?

Do we have means to handle dependencies here? There is no natural order
in which userspace is writing to these regs. Nor a good time or a trigger
when the kernel should do a sanity/dependency check..

>
>>  	{ SYS_DESC(SYS_CCSIDR2_EL1), undef_access },
>>  	{ SYS_DESC(SYS_SMIDR_EL1), undef_access },
>>  	{ SYS_DESC(SYS_CSSELR_EL1), access_csselr, reset_unknown, CSSELR_EL1 },
>>  	{ SYS_DESC(SYS_CTR_EL0), access_ctr, .reset = reset_ctr,
>> -	  .get_user = get_ctr, .set_user = set_ctr},
>> +	  .get_user = get_ctr, .set_user = set_ctr, .val = CTR_EL0_DIC_MASK},
>
> This is all extremely fragile. We need a better solution for this. And
> we need far more than just DIC here.
>
>>  	{ SYS_DESC(SYS_SVCR), undef_access },
>>
>>  	{ PMU_SYS_REG(PMCR_EL0), .access = access_pmcr, .reset = reset_pmcr,
>> @@ -3934,20 +3934,11 @@ int kvm_vm_ioctl_get_reg_writable_masks(struct kvm *kvm, struct reg_mask_range *
>>  		if (!is_feature_id_reg(encoding) || !reg->set_user)
>>  			continue;
>>
>> -		/*
>> -		 * For ID registers, we return the writable mask. Other feature
>> -		 * registers return a full 64bit mask. That's not necessary
>> -		 * compliant with a given revision of the architecture, but the
>> -		 * RES0/RES1 definitions allow us to do that.
>> -		 */
>> -		if (is_id_reg(encoding)) {
>> -			if (!reg->val ||
>> -			    (is_aa32_id_reg(encoding) && !kvm_supports_32bit_el0()))
>> -				continue;
>> -			val = reg->val;
>> -		} else {
>> -			val = ~0UL;
>> +		if (!reg->val ||
>> +		    (is_aa32_id_reg(encoding) && !kvm_supports_32bit_el0())) {
>> +			continue;
>>  		}
>> +		val = reg->val;
>
> Are CLIDR_EL1 and CTR_EL0 the only two ID registers that had a
> set_user callback without an encoded mask?

These 2 are the only regs affected by this patch - meaning
!is_id_reg() && reg->set_user && !reg->val


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

Re: [PATCH 3/4] KVM: arm64: show writable masks for feature registers

[Sebastian Ott]

On Mon, 18 Mar 2024, Marc Zyngier wrote:
> On Mon, 18 Mar 2024 11:16:35 +0000,
> Sebastian Ott <sebott@redhat.com> wrote:
>>
>> Instead of using ~0UL provide the actual writable mask for
>> non-id feature registers in the output of the
>> KVM_ARM_GET_REG_WRITABLE_MASKS ioctl.
>
> CTR_EL0 definitely seems to be in the feature ID range. And so does
> CLIDR_EL1.

Sry, bad wording. I mean feature id regs for which is_id_reg() is false.

>>
>> This changes the mask for the CTR_EL0 register.
>
> Only that?

Yes.

>> Signed-off-by: Sebastian Ott <sebott@redhat.com>
>> ---
>>  arch/arm64/kvm/sys_regs.c | 21 ++++++---------------
>>  1 file changed, 6 insertions(+), 15 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index b2019faa9d73..0f8fe7790c35 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -2489,12 +2489,12 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>>
>>  	{ SYS_DESC(SYS_CCSIDR_EL1), access_ccsidr },
>>  	{ SYS_DESC(SYS_CLIDR_EL1), access_clidr, reset_clidr, CLIDR_EL1,
>> -	  .set_user = set_clidr },
>> +	  .set_user = set_clidr, .val = ~0UL },
>
> How is CLIDR_EL1 (and all the crap that depend on it) recomputed when
> CTR_EL0 is changed?

Do we have means to handle dependencies here? There is no natural order
in which userspace is writing to these regs. Nor a good time or a trigger
when the kernel should do a sanity/dependency check..

>
>>  	{ SYS_DESC(SYS_CCSIDR2_EL1), undef_access },
>>  	{ SYS_DESC(SYS_SMIDR_EL1), undef_access },
>>  	{ SYS_DESC(SYS_CSSELR_EL1), access_csselr, reset_unknown, CSSELR_EL1 },
>>  	{ SYS_DESC(SYS_CTR_EL0), access_ctr, .reset = reset_ctr,
>> -	  .get_user = get_ctr, .set_user = set_ctr},
>> +	  .get_user = get_ctr, .set_user = set_ctr, .val = CTR_EL0_DIC_MASK},
>
> This is all extremely fragile. We need a better solution for this. And
> we need far more than just DIC here.
>
>>  	{ SYS_DESC(SYS_SVCR), undef_access },
>>
>>  	{ PMU_SYS_REG(PMCR_EL0), .access = access_pmcr, .reset = reset_pmcr,
>> @@ -3934,20 +3934,11 @@ int kvm_vm_ioctl_get_reg_writable_masks(struct kvm *kvm, struct reg_mask_range *
>>  		if (!is_feature_id_reg(encoding) || !reg->set_user)
>>  			continue;
>>
>> -		/*
>> -		 * For ID registers, we return the writable mask. Other feature
>> -		 * registers return a full 64bit mask. That's not necessary
>> -		 * compliant with a given revision of the architecture, but the
>> -		 * RES0/RES1 definitions allow us to do that.
>> -		 */
>> -		if (is_id_reg(encoding)) {
>> -			if (!reg->val ||
>> -			    (is_aa32_id_reg(encoding) && !kvm_supports_32bit_el0()))
>> -				continue;
>> -			val = reg->val;
>> -		} else {
>> -			val = ~0UL;
>> +		if (!reg->val ||
>> +		    (is_aa32_id_reg(encoding) && !kvm_supports_32bit_el0())) {
>> +			continue;
>>  		}
>> +		val = reg->val;
>
> Are CLIDR_EL1 and CTR_EL0 the only two ID registers that had a
> set_user callback without an encoded mask?

These 2 are the only regs affected by this patch - meaning
!is_id_reg() && reg->set_user && !reg->val


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

Re: [PATCH 3/4] KVM: arm64: show writable masks for feature registers

[Marc Zyngier]

On Mon, 18 Mar 2024 18:20:06 +0000,
Sebastian Ott <sebott@redhat.com> wrote:
> 
> On Mon, 18 Mar 2024, Marc Zyngier wrote:
> > On Mon, 18 Mar 2024 11:16:35 +0000,
> > Sebastian Ott <sebott@redhat.com> wrote:
> >> 
> >> Instead of using ~0UL provide the actual writable mask for
> >> non-id feature registers in the output of the
> >> KVM_ARM_GET_REG_WRITABLE_MASKS ioctl.
> > 
> > CTR_EL0 definitely seems to be in the feature ID range. And so does
> > CLIDR_EL1.
> 
> Sry, bad wording. I mean feature id regs for which is_id_reg() is false.
>

Maybe this means that we should revisit the range delineated by
is_id_reg().

> >> 
> >> This changes the mask for the CTR_EL0 register.
> > 
> > Only that?
> 
> Yes.

The patch shows otherwise, I'm afraid.

> 
> >> Signed-off-by: Sebastian Ott <sebott@redhat.com>
> >> ---
> >>  arch/arm64/kvm/sys_regs.c | 21 ++++++---------------
> >>  1 file changed, 6 insertions(+), 15 deletions(-)
> >> 
> >> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> >> index b2019faa9d73..0f8fe7790c35 100644
> >> --- a/arch/arm64/kvm/sys_regs.c
> >> +++ b/arch/arm64/kvm/sys_regs.c
> >> @@ -2489,12 +2489,12 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> >> 
> >>  	{ SYS_DESC(SYS_CCSIDR_EL1), access_ccsidr },
> >>  	{ SYS_DESC(SYS_CLIDR_EL1), access_clidr, reset_clidr, CLIDR_EL1,
> >> -	  .set_user = set_clidr },
> >> +	  .set_user = set_clidr, .val = ~0UL },
> > 
> > How is CLIDR_EL1 (and all the crap that depend on it) recomputed when
> > CTR_EL0 is changed?
> 
> Do we have means to handle dependencies here? There is no natural order
> in which userspace is writing to these regs. Nor a good time or a trigger
> when the kernel should do a sanity/dependency check..

Well, we're not going to run the guest with something that describes
an inconsistent topology either, so this is the very first thing that
needs to be solved.

Either by restoring acceptable defaults when the topology changes due
to a write to CTR_EL0, or by returning an error to userspace if the
cache topology has been restored but CTR_EL0 is incompatible with it.

	M.

-- 
Without deviation from the norm, progress is not possible.

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