[PATCH v6 0/6] Enable writable for idregs DFR0,PFR0, MMFR{0,1,2, 3}

[PATCH v6 0/6] Enable writable for idregs DFR0,PFR0, MMFR{0,1,2, 3}

[Jing Zhang]

This patch series enable userspace writable for below idregs:
ID_AA64DFR0_EL1, ID_DFR0_EL1, ID_AA64PFR0_EL1, ID_AA64MMFR{0, 1, 2, 3}_EL1.

It is based on v6.5-rc1 which contains infrastructure for writable idregs.

A selftest is added to verify that KVM handles the writings from user space
correctly.

A relevant patch from Oliver is picked from [3].

---

* v5 -> v6
  - Override the type of field AA64DFR0_EL1_DebugVer to be FTR_LOWER_SAFE by the
    discussion of Oliver and Suraj.

* v4 -> v5
  - Rebase on v6.4-rc1 which contains infrastructure for writable idregs.
  - Use guest ID registers values for the sake of emulation.
  - Added a selftest to verify idreg userspace writing.

* v3 -> v4
  - Rebase on v11 of writable idregs series at [2].

* v2 -> v3
  - Rebase on v6 of writable idregs series.
  - Enable writable for ID_AA64PFR0_EL1 and ID_AA64MMFR{0, 1, 2}_EL1.

* v1 -> v2
  - Rebase on latest patch series [1] of enabling writable ID register.

[1] https://lore.kernel.org/all/20230402183735.3011540-1-jingzhangos@google.com
[2] https://lore.kernel.org/all/20230602005118.2899664-1-jingzhangos@google.com
[3] https://lore.kernel.org/kvmarm/20230623205232.2837077-1-oliver.upton@linux.dev

[v1] https://lore.kernel.org/all/20230326011950.405749-1-jingzhangos@google.com
[v2] https://lore.kernel.org/all/20230403003723.3199828-1-jingzhangos@google.com
[v3] https://lore.kernel.org/all/20230405172146.297208-1-jingzhangos@google.com
[v4] https://lore.kernel.org/all/20230607194554.87359-1-jingzhangos@google.com
[v5] https://lore.kernel.org/all/20230710192430.1992246-1-jingzhangos@google.com

---

Jing Zhang (4):
  KVM: arm64: Use guest ID register values for the sake of emulation
  KVM: arm64: Enable writable for ID_AA64PFR0_EL1
  KVM: arm64: Enable writable for ID_AA64MMFR{0, 1, 2, 3}_EL1
  KVM: arm64: selftests: Test for setting ID register from usersapce

Oliver Upton (1):
  KVM: arm64: Reject attempts to set invalid debug arch version

 arch/arm64/kvm/sys_regs.c                     |  80 +++++++--
 tools/testing/selftests/kvm/Makefile          |   1 +
 .../selftests/kvm/aarch64/set_id_regs.c       | 163 ++++++++++++++++++
 3 files changed, 230 insertions(+), 14 deletions(-)
 create mode 100644 tools/testing/selftests/kvm/aarch64/set_id_regs.c


base-commit: 06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5
-- 
2.41.0.255.g8b1d071c50-goog


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

Re: [PATCH v6 0/6] Enable writable for idregs DFR0,PFR0, MMFR{0,1,2, 3}

[Jing Zhang]

Please ignore this post of v6 which missed a commit. I'll send out the
correct v6 soon.

Thanks,
Jing

On Mon, Jul 17, 2023 at 8:27 AM Jing Zhang <jingzhangos@google.com> wrote:
>
> This patch series enable userspace writable for below idregs:
> ID_AA64DFR0_EL1, ID_DFR0_EL1, ID_AA64PFR0_EL1, ID_AA64MMFR{0, 1, 2, 3}_EL1.
>
> It is based on v6.5-rc1 which contains infrastructure for writable idregs.
>
> A selftest is added to verify that KVM handles the writings from user space
> correctly.
>
> A relevant patch from Oliver is picked from [3].
>
> ---
>
> * v5 -> v6
>   - Override the type of field AA64DFR0_EL1_DebugVer to be FTR_LOWER_SAFE by the
>     discussion of Oliver and Suraj.
>
> * v4 -> v5
>   - Rebase on v6.4-rc1 which contains infrastructure for writable idregs.
>   - Use guest ID registers values for the sake of emulation.
>   - Added a selftest to verify idreg userspace writing.
>
> * v3 -> v4
>   - Rebase on v11 of writable idregs series at [2].
>
> * v2 -> v3
>   - Rebase on v6 of writable idregs series.
>   - Enable writable for ID_AA64PFR0_EL1 and ID_AA64MMFR{0, 1, 2}_EL1.
>
> * v1 -> v2
>   - Rebase on latest patch series [1] of enabling writable ID register.
>
> [1] https://lore.kernel.org/all/20230402183735.3011540-1-jingzhangos@google.com
> [2] https://lore.kernel.org/all/20230602005118.2899664-1-jingzhangos@google.com
> [3] https://lore.kernel.org/kvmarm/20230623205232.2837077-1-oliver.upton@linux.dev
>
> [v1] https://lore.kernel.org/all/20230326011950.405749-1-jingzhangos@google.com
> [v2] https://lore.kernel.org/all/20230403003723.3199828-1-jingzhangos@google.com
> [v3] https://lore.kernel.org/all/20230405172146.297208-1-jingzhangos@google.com
> [v4] https://lore.kernel.org/all/20230607194554.87359-1-jingzhangos@google.com
> [v5] https://lore.kernel.org/all/20230710192430.1992246-1-jingzhangos@google.com
>
> ---
>
> Jing Zhang (4):
>   KVM: arm64: Use guest ID register values for the sake of emulation
>   KVM: arm64: Enable writable for ID_AA64PFR0_EL1
>   KVM: arm64: Enable writable for ID_AA64MMFR{0, 1, 2, 3}_EL1
>   KVM: arm64: selftests: Test for setting ID register from usersapce
>
> Oliver Upton (1):
>   KVM: arm64: Reject attempts to set invalid debug arch version
>
>  arch/arm64/kvm/sys_regs.c                     |  80 +++++++--
>  tools/testing/selftests/kvm/Makefile          |   1 +
>  .../selftests/kvm/aarch64/set_id_regs.c       | 163 ++++++++++++++++++
>  3 files changed, 230 insertions(+), 14 deletions(-)
>  create mode 100644 tools/testing/selftests/kvm/aarch64/set_id_regs.c
>
>
> base-commit: 06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5
> --
> 2.41.0.255.g8b1d071c50-goog
>

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

[PATCH v6 0/6] Enable writable for idregs DFR0,PFR0, MMFR{0,1,2, 3}

[Jing Zhang]

This patch series enable userspace writable for below idregs:
ID_AA64DFR0_EL1, ID_DFR0_EL1, ID_AA64PFR0_EL1, ID_AA64MMFR{0, 1, 2, 3}_EL1.

It is based on v6.5-rc1 which contains infrastructure for writable idregs.

A selftest is added to verify that KVM handles the writings from user space
correctly.

A relevant patch from Oliver is picked from [3].

---

* v5 -> v6
  - Override the type of field AA64DFR0_EL1_DebugVer to be FTR_LOWER_SAFE by the
    discussion of Oliver and Suraj.

* v4 -> v5
  - Rebase on v6.4-rc1 which contains infrastructure for writable idregs.
  - Use guest ID registers values for the sake of emulation.
  - Added a selftest to verify idreg userspace writing.

* v3 -> v4
  - Rebase on v11 of writable idregs series at [2].

* v2 -> v3
  - Rebase on v6 of writable idregs series.
  - Enable writable for ID_AA64PFR0_EL1 and ID_AA64MMFR{0, 1, 2}_EL1.

* v1 -> v2
  - Rebase on latest patch series [1] of enabling writable ID register.

[1] https://lore.kernel.org/all/20230402183735.3011540-1-jingzhangos@google.com
[2] https://lore.kernel.org/all/20230602005118.2899664-1-jingzhangos@google.com
[3] https://lore.kernel.org/kvmarm/20230623205232.2837077-1-oliver.upton@linux.dev

[v1] https://lore.kernel.org/all/20230326011950.405749-1-jingzhangos@google.com
[v2] https://lore.kernel.org/all/20230403003723.3199828-1-jingzhangos@google.com
[v3] https://lore.kernel.org/all/20230405172146.297208-1-jingzhangos@google.com
[v4] https://lore.kernel.org/all/20230607194554.87359-1-jingzhangos@google.com
[v5] https://lore.kernel.org/all/20230710192430.1992246-1-jingzhangos@google.com

---

Jing Zhang (5):
  KVM: arm64: Use guest ID register values for the sake of emulation
  KVM: arm64: Enable writable for ID_AA64DFR0_EL1 and ID_DFR0_EL1
  KVM: arm64: Enable writable for ID_AA64PFR0_EL1
  KVM: arm64: Enable writable for ID_AA64MMFR{0, 1, 2, 3}_EL1
  KVM: arm64: selftests: Test for setting ID register from usersapce

Oliver Upton (1):
  KVM: arm64: Reject attempts to set invalid debug arch version

 arch/arm64/kvm/sys_regs.c                     |  84 +++++++--
 tools/testing/selftests/kvm/Makefile          |   1 +
 .../selftests/kvm/aarch64/set_id_regs.c       | 163 ++++++++++++++++++
 3 files changed, 232 insertions(+), 16 deletions(-)
 create mode 100644 tools/testing/selftests/kvm/aarch64/set_id_regs.c


base-commit: 06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5
-- 
2.41.0.255.g8b1d071c50-goog


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

[PATCH v6 1/6] KVM: arm64: Use guest ID register values for the sake of emulation

[Jing Zhang]

Since KVM now supports per-VM ID registers, use per-VM ID register
values for the sake of emulation for DBGDIDR and LORegion.

Signed-off-by: Jing Zhang <jingzhangos@google.com>
---
 arch/arm64/kvm/sys_regs.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index bd3431823ec5..c1a5ec1a016e 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -379,7 +379,7 @@ static bool trap_loregion(struct kvm_vcpu *vcpu,
 			  struct sys_reg_params *p,
 			  const struct sys_reg_desc *r)
 {
-	u64 val = read_sanitised_ftr_reg(SYS_ID_AA64MMFR1_EL1);
+	u64 val = IDREG(vcpu->kvm, SYS_ID_AA64MMFR1_EL1);
 	u32 sr = reg_to_encoding(r);
 
 	if (!(val & (0xfUL << ID_AA64MMFR1_EL1_LO_SHIFT))) {
@@ -2429,8 +2429,8 @@ static bool trap_dbgdidr(struct kvm_vcpu *vcpu,
 	if (p->is_write) {
 		return ignore_write(vcpu, p);
 	} else {
-		u64 dfr = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1);
-		u64 pfr = read_sanitised_ftr_reg(SYS_ID_AA64PFR0_EL1);
+		u64 dfr = IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1);
+		u64 pfr = IDREG(vcpu->kvm, SYS_ID_AA64PFR0_EL1);
 		u32 el3 = !!cpuid_feature_extract_unsigned_field(pfr, ID_AA64PFR0_EL1_EL3_SHIFT);
 
 		p->regval = ((((dfr >> ID_AA64DFR0_EL1_WRPs_SHIFT) & 0xf) << 28) |
-- 
2.41.0.255.g8b1d071c50-goog


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

[PATCH v6 2/6] KVM: arm64: Reject attempts to set invalid debug arch version

[Jing Zhang]

From: Oliver Upton <oliver.upton@linux.dev>

The debug architecture is mandatory in ARMv8, so KVM should not allow
userspace to configure a vCPU with less than that. Of course, this isn't
handled elegantly by the generic ID register plumbing, as the respective
ID register fields have a nonzero starting value.

Add an explicit check for debug versions less than v8 of the
architecture.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
Signed-off-by: Jing Zhang <jingzhangos@google.com>
---
 arch/arm64/kvm/sys_regs.c | 34 +++++++++++++++++++++++++++++++---
 1 file changed, 31 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index c1a5ec1a016e..053d8057ff1e 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1216,8 +1216,14 @@ static s64 kvm_arm64_ftr_safe_value(u32 id, const struct arm64_ftr_bits *ftrp,
 	/* Some features have different safe value type in KVM than host features */
 	switch (id) {
 	case SYS_ID_AA64DFR0_EL1:
-		if (kvm_ftr.shift == ID_AA64DFR0_EL1_PMUVer_SHIFT)
+		switch (kvm_ftr.shift) {
+		case ID_AA64DFR0_EL1_PMUVer_SHIFT:
 			kvm_ftr.type = FTR_LOWER_SAFE;
+			break;
+		case ID_AA64DFR0_EL1_DebugVer_SHIFT:
+			kvm_ftr.type = FTR_LOWER_SAFE;
+			break;
+		}
 		break;
 	case SYS_ID_DFR0_EL1:
 		if (kvm_ftr.shift == ID_DFR0_EL1_PerfMon_SHIFT)
@@ -1469,14 +1475,22 @@ static u64 read_sanitised_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
 	return val;
 }
 
+#define ID_REG_LIMIT_FIELD_ENUM(val, reg, field, limit)			       \
+({									       \
+	u64 __f_val = FIELD_GET(reg##_##field##_MASK, val);		       \
+	(val) &= ~reg##_##field##_MASK;					       \
+	(val) |= FIELD_PREP(reg##_##field##_MASK,			       \
+			min(__f_val, (u64)reg##_##field##_##limit));	       \
+	(val);								       \
+})
+
 static u64 read_sanitised_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
 					  const struct sys_reg_desc *rd)
 {
 	u64 val = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1);
 
 	/* Limit debug to ARMv8.0 */
-	val &= ~ID_AA64DFR0_EL1_DebugVer_MASK;
-	val |= SYS_FIELD_PREP_ENUM(ID_AA64DFR0_EL1, DebugVer, IMP);
+	val = ID_REG_LIMIT_FIELD_ENUM(val, ID_AA64DFR0_EL1, DebugVer, V8P8);
 
 	/*
 	 * Only initialize the PMU version if the vCPU was configured with one.
@@ -1496,6 +1510,7 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
 			       const struct sys_reg_desc *rd,
 			       u64 val)
 {
+	u8 debugver = SYS_FIELD_GET(ID_AA64DFR0_EL1, DebugVer, val);
 	u8 pmuver = SYS_FIELD_GET(ID_AA64DFR0_EL1, PMUVer, val);
 
 	/*
@@ -1515,6 +1530,13 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
 	if (pmuver == ID_AA64DFR0_EL1_PMUVer_IMP_DEF)
 		val &= ~ID_AA64DFR0_EL1_PMUVer_MASK;
 
+	/*
+	 * ID_AA64DFR0_EL1.DebugVer is one of those awkward fields with a
+	 * nonzero minimum safe value.
+	 */
+	if (debugver < ID_AA64DFR0_EL1_DebugVer_IMP)
+		return -EINVAL;
+
 	return set_id_reg(vcpu, rd, val);
 }
 
@@ -1528,6 +1550,8 @@ static u64 read_sanitised_id_dfr0_el1(struct kvm_vcpu *vcpu,
 	if (kvm_vcpu_has_pmu(vcpu))
 		val |= SYS_FIELD_PREP(ID_DFR0_EL1, PerfMon, perfmon);
 
+	val = ID_REG_LIMIT_FIELD_ENUM(val, ID_DFR0_EL1, CopDbg, Debugv8p8);
+
 	return val;
 }
 
@@ -1536,6 +1560,7 @@ static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
 			   u64 val)
 {
 	u8 perfmon = SYS_FIELD_GET(ID_DFR0_EL1, PerfMon, val);
+	u8 copdbg = SYS_FIELD_GET(ID_DFR0_EL1, CopDbg, val);
 
 	if (perfmon == ID_DFR0_EL1_PerfMon_IMPDEF) {
 		val &= ~ID_DFR0_EL1_PerfMon_MASK;
@@ -1551,6 +1576,9 @@ static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
 	if (perfmon != 0 && perfmon < ID_DFR0_EL1_PerfMon_PMUv3)
 		return -EINVAL;
 
+	if (copdbg < ID_DFR0_EL1_CopDbg_Armv8)
+		return -EINVAL;
+
 	return set_id_reg(vcpu, rd, val);
 }
 
-- 
2.41.0.255.g8b1d071c50-goog


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

[PATCH v6 3/6] KVM: arm64: Enable writable for ID_AA64DFR0_EL1 and ID_DFR0_EL1

[Jing Zhang]

All valid fields in ID_AA64DFR0_EL1 and ID_DFR0_EL1 are writable
from usrespace with this change.

Signed-off-by: Jing Zhang <jingzhangos@google.com>
---
 arch/arm64/kvm/sys_regs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 053d8057ff1e..f33aec83f1b4 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -2008,7 +2008,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	  .set_user = set_id_dfr0_el1,
 	  .visibility = aa32_id_visibility,
 	  .reset = read_sanitised_id_dfr0_el1,
-	  .val = ID_DFR0_EL1_PerfMon_MASK, },
+	  .val = GENMASK(63, 0), },
 	ID_HIDDEN(ID_AFR0_EL1),
 	AA32_ID_SANITISED(ID_MMFR0_EL1),
 	AA32_ID_SANITISED(ID_MMFR1_EL1),
@@ -2057,7 +2057,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	  .get_user = get_id_reg,
 	  .set_user = set_id_aa64dfr0_el1,
 	  .reset = read_sanitised_id_aa64dfr0_el1,
-	  .val = ID_AA64DFR0_EL1_PMUVer_MASK, },
+	  .val = GENMASK(63, 0), },
 	ID_SANITISED(ID_AA64DFR1_EL1),
 	ID_UNALLOCATED(5,2),
 	ID_UNALLOCATED(5,3),
-- 
2.41.0.255.g8b1d071c50-goog


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

[PATCH v6 4/6] KVM: arm64: Enable writable for ID_AA64PFR0_EL1

[Jing Zhang]

All valid fields in ID_AA64PFR0_EL1 are writable from usrespace
with this change.

Signed-off-by: Jing Zhang <jingzhangos@google.com>
---
 arch/arm64/kvm/sys_regs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index f33aec83f1b4..12b81ab27dbe 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -2042,7 +2042,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	  .get_user = get_id_reg,
 	  .set_user = set_id_reg,
 	  .reset = read_sanitised_id_aa64pfr0_el1,
-	  .val = ID_AA64PFR0_EL1_CSV2_MASK | ID_AA64PFR0_EL1_CSV3_MASK, },
+	  .val = GENMASK(63, 0), },
 	ID_SANITISED(ID_AA64PFR1_EL1),
 	ID_UNALLOCATED(4,2),
 	ID_UNALLOCATED(4,3),
-- 
2.41.0.255.g8b1d071c50-goog


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

[PATCH v6 5/6] KVM: arm64: Enable writable for ID_AA64MMFR{0, 1, 2, 3}_EL1

[Jing Zhang]

Enable writable from userspace for ID_AA64MMFR{0, 1, 2, 3}_EL1.
Added a macro for defining general writable idregs.

Signed-off-by: Jing Zhang <jingzhangos@google.com>
---
 arch/arm64/kvm/sys_regs.c | 38 +++++++++++++++++++++++++++++++-------
 1 file changed, 31 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 12b81ab27dbe..d560fc178a76 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1346,9 +1346,6 @@ static u64 __kvm_read_sanitised_id_reg(const struct kvm_vcpu *vcpu,
 			val &= ~ARM64_FEATURE_MASK(ID_AA64ISAR2_EL1_WFxT);
 		val &= ~ARM64_FEATURE_MASK(ID_AA64ISAR2_EL1_MOPS);
 		break;
-	case SYS_ID_AA64MMFR2_EL1:
-		val &= ~ID_AA64MMFR2_EL1_CCIDX_MASK;
-		break;
 	case SYS_ID_MMFR4_EL1:
 		val &= ~ARM64_FEATURE_MASK(ID_MMFR4_EL1_CCIDX);
 		break;
@@ -1582,6 +1579,18 @@ static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
 	return set_id_reg(vcpu, rd, val);
 }
 
+static u64 read_sanitised_id_aa64mmfr2_el1(struct kvm_vcpu *vcpu,
+					   const struct sys_reg_desc *rd)
+{
+	u64 val;
+	u32 id = reg_to_encoding(rd);
+
+	val = read_sanitised_ftr_reg(id);
+	val &= ~ID_AA64MMFR2_EL1_CCIDX_MASK;
+
+	return val;
+}
+
 /*
  * cpufeature ID register user accessors
  *
@@ -1856,6 +1865,16 @@ static unsigned int elx2_visibility(const struct kvm_vcpu *vcpu,
 	.val = 0,				\
 }
 
+#define ID_SANITISED_WRITABLE(name) {		\
+	SYS_DESC(SYS_##name),			\
+	.access	= access_id_reg,		\
+	.get_user = get_id_reg,			\
+	.set_user = set_id_reg,			\
+	.visibility = id_visibility,		\
+	.reset = kvm_read_sanitised_id_reg,	\
+	.val = GENMASK(63, 0),			\
+}
+
 /* sys_reg_desc initialiser for known cpufeature ID registers */
 #define AA32_ID_SANITISED(name) {		\
 	SYS_DESC(SYS_##name),			\
@@ -2077,10 +2096,15 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	ID_UNALLOCATED(6,7),
 
 	/* CRm=7 */
-	ID_SANITISED(ID_AA64MMFR0_EL1),
-	ID_SANITISED(ID_AA64MMFR1_EL1),
-	ID_SANITISED(ID_AA64MMFR2_EL1),
-	ID_SANITISED(ID_AA64MMFR3_EL1),
+	ID_SANITISED_WRITABLE(ID_AA64MMFR0_EL1),
+	ID_SANITISED_WRITABLE(ID_AA64MMFR1_EL1),
+	{ SYS_DESC(SYS_ID_AA64MMFR2_EL1),
+	  .access = access_id_reg,
+	  .get_user = get_id_reg,
+	  .set_user = set_id_reg,
+	  .reset = read_sanitised_id_aa64mmfr2_el1,
+	  .val = GENMASK(63, 0), },
+	ID_SANITISED_WRITABLE(ID_AA64MMFR3_EL1),
 	ID_UNALLOCATED(7,4),
 	ID_UNALLOCATED(7,5),
 	ID_UNALLOCATED(7,6),
-- 
2.41.0.255.g8b1d071c50-goog


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

[PATCH v6 6/6] KVM: arm64: selftests: Test for setting ID register from usersapce

[Jing Zhang]

Add a test to verify setting ID registers from userapce is handled
correctly by KVM.

Signed-off-by: Jing Zhang <jingzhangos@google.com>
---
 tools/testing/selftests/kvm/Makefile          |   1 +
 .../selftests/kvm/aarch64/set_id_regs.c       | 163 ++++++++++++++++++
 2 files changed, 164 insertions(+)
 create mode 100644 tools/testing/selftests/kvm/aarch64/set_id_regs.c

diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index c692cc86e7da..87ceadc1292a 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -144,6 +144,7 @@ TEST_GEN_PROGS_aarch64 += aarch64/get-reg-list
 TEST_GEN_PROGS_aarch64 += aarch64/hypercalls
 TEST_GEN_PROGS_aarch64 += aarch64/page_fault_test
 TEST_GEN_PROGS_aarch64 += aarch64/psci_test
+TEST_GEN_PROGS_aarch64 += aarch64/set_id_regs
 TEST_GEN_PROGS_aarch64 += aarch64/smccc_filter
 TEST_GEN_PROGS_aarch64 += aarch64/vcpu_width_config
 TEST_GEN_PROGS_aarch64 += aarch64/vgic_init
diff --git a/tools/testing/selftests/kvm/aarch64/set_id_regs.c b/tools/testing/selftests/kvm/aarch64/set_id_regs.c
new file mode 100644
index 000000000000..e2242ef36bab
--- /dev/null
+++ b/tools/testing/selftests/kvm/aarch64/set_id_regs.c
@@ -0,0 +1,163 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * set_id_regs - Test for setting ID register from usersapce.
+ *
+ * Copyright (c) 2023 Google LLC.
+ *
+ *
+ * Test that KVM supports setting ID registers from userspace and handles the
+ * feature set correctly.
+ */
+
+#include <stdint.h>
+#include "kvm_util.h"
+#include "processor.h"
+#include "test_util.h"
+#include <linux/bitfield.h>
+
+#define field_get(_mask, _reg) (((_reg) & (_mask)) >> (ffs(_mask) - 1))
+#define field_prep(_mask, _val) (((_val) << (ffs(_mask) - 1)) & (_mask))
+
+struct reg_feature {
+	uint64_t reg;
+	uint64_t ftr_mask;
+};
+
+static void guest_code(void)
+{
+	for (;;)
+		GUEST_SYNC(0);
+}
+
+static struct reg_feature lower_safe_reg_ftrs[] = {
+	{ KVM_ARM64_SYS_REG(SYS_ID_AA64DFR0_EL1), ARM64_FEATURE_MASK(ID_AA64DFR0_BRPS) },
+	{ KVM_ARM64_SYS_REG(SYS_ID_DFR0_EL1), ARM64_FEATURE_MASK(ID_DFR0_COPDBG) },
+	{ KVM_ARM64_SYS_REG(SYS_ID_AA64PFR0_EL1), ARM64_FEATURE_MASK(ID_AA64PFR0_EL3) },
+	{ KVM_ARM64_SYS_REG(SYS_ID_AA64MMFR0_EL1), ARM64_FEATURE_MASK(ID_AA64MMFR0_TGRAN4) },
+};
+
+static void test_user_set_lower_safe(struct kvm_vcpu *vcpu)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(lower_safe_reg_ftrs); i++) {
+		struct reg_feature *reg_ftr = lower_safe_reg_ftrs + i;
+		uint64_t val, new_val, ftr;
+
+		vcpu_get_reg(vcpu, reg_ftr->reg, &val);
+		ftr = field_get(reg_ftr->ftr_mask, val);
+
+		/* Set a safe value for the feature */
+		if (ftr > 0)
+			ftr--;
+
+		val &= ~reg_ftr->ftr_mask;
+		val |= field_prep(reg_ftr->ftr_mask, ftr);
+
+		vcpu_set_reg(vcpu, reg_ftr->reg, val);
+		vcpu_get_reg(vcpu, reg_ftr->reg, &new_val);
+		ASSERT_EQ(new_val, val);
+	}
+}
+
+static struct reg_feature exact_reg_ftrs[] = {
+	{ KVM_ARM64_SYS_REG(SYS_ID_AA64DFR0_EL1), ARM64_FEATURE_MASK(ID_AA64DFR0_DEBUGVER) },
+};
+
+static void test_user_set_exact(struct kvm_vcpu *vcpu)
+{
+	int i, r;
+
+	for (i = 0; i < ARRAY_SIZE(exact_reg_ftrs); i++) {
+		struct reg_feature *reg_ftr = exact_reg_ftrs + i;
+		uint64_t val, old_val, ftr;
+
+		vcpu_get_reg(vcpu, reg_ftr->reg, &val);
+		ftr = field_get(reg_ftr->ftr_mask, val);
+		old_val = val;
+
+		/* Exact match */
+		vcpu_set_reg(vcpu, reg_ftr->reg, val);
+		vcpu_get_reg(vcpu, reg_ftr->reg, &val);
+		ASSERT_EQ(val, old_val);
+
+		/* Smaller value */
+		if (ftr > 0)
+			ftr--;
+		val &= ~reg_ftr->ftr_mask;
+		val |= field_prep(reg_ftr->ftr_mask, ftr);
+		r = __vcpu_set_reg(vcpu, reg_ftr->reg, val);
+		TEST_ASSERT(r < 0 && errno == EINVAL,
+			    "Unexpected KVM_SET_ONE_REG error: r=%d, errno=%d", r, errno);
+		vcpu_get_reg(vcpu, reg_ftr->reg, &val);
+		ASSERT_EQ(val, old_val);
+
+		/* Bigger value */
+		ftr += 2;
+		val &= ~reg_ftr->ftr_mask;
+		val |= field_prep(reg_ftr->ftr_mask, ftr);
+		r = __vcpu_set_reg(vcpu, reg_ftr->reg, val);
+		TEST_ASSERT(r < 0 && errno == EINVAL,
+			    "Unexpected KVM_SET_ONE_REG error: r=%d, errno=%d", r, errno);
+		vcpu_get_reg(vcpu, reg_ftr->reg, &val);
+		ASSERT_EQ(val, old_val);
+	}
+}
+
+static struct reg_feature fail_reg_ftrs[] = {
+	{ KVM_ARM64_SYS_REG(SYS_ID_AA64DFR0_EL1), ARM64_FEATURE_MASK(ID_AA64DFR0_WRPS) },
+	{ KVM_ARM64_SYS_REG(SYS_ID_DFR0_EL1), ARM64_FEATURE_MASK(ID_DFR0_MPROFDBG) },
+	{ KVM_ARM64_SYS_REG(SYS_ID_AA64PFR0_EL1), ARM64_FEATURE_MASK(ID_AA64PFR0_EL2) },
+	{ KVM_ARM64_SYS_REG(SYS_ID_AA64MMFR0_EL1), ARM64_FEATURE_MASK(ID_AA64MMFR0_TGRAN64) },
+};
+
+static void test_user_set_fail(struct kvm_vcpu *vcpu)
+{
+	int i, r;
+
+	for (i = 0; i < ARRAY_SIZE(fail_reg_ftrs); i++) {
+		struct reg_feature *reg_ftr = fail_reg_ftrs + i;
+		uint64_t val, old_val, ftr;
+
+		vcpu_get_reg(vcpu, reg_ftr->reg, &val);
+		ftr = field_get(reg_ftr->ftr_mask, val);
+
+		/* Set a invalid value (too big) for the feature */
+		ftr++;
+
+		old_val = val;
+		val &= ~reg_ftr->ftr_mask;
+		val |= field_prep(reg_ftr->ftr_mask, ftr);
+
+		r = __vcpu_set_reg(vcpu, reg_ftr->reg, val);
+		TEST_ASSERT(r < 0 && errno == EINVAL,
+			    "Unexpected KVM_SET_ONE_REG error: r=%d, errno=%d", r, errno);
+
+		vcpu_get_reg(vcpu, reg_ftr->reg, &val);
+		ASSERT_EQ(val, old_val);
+	}
+}
+
+int main(void)
+{
+	struct kvm_vcpu *vcpu;
+	struct kvm_vm *vm;
+
+	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
+
+	ksft_print_header();
+	ksft_set_plan(3);
+
+	test_user_set_lower_safe(vcpu);
+	ksft_test_result_pass("test_user_set_lower_safe\n");
+
+	test_user_set_exact(vcpu);
+	ksft_test_result_pass("test_user_set_exact\n");
+
+	test_user_set_fail(vcpu);
+	ksft_test_result_pass("test_user_set_fail\n");
+
+	kvm_vm_free(vm);
+
+	ksft_finished();
+}
-- 
2.41.0.255.g8b1d071c50-goog


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

Re: [PATCH v6 3/6] KVM: arm64: Enable writable for ID_AA64DFR0_EL1 and ID_DFR0_EL1

[Cornelia Huck]

On Tue, Jul 18 2023, Jing Zhang <jingzhangos@google.com> wrote:

> All valid fields in ID_AA64DFR0_EL1 and ID_DFR0_EL1 are writable
> from usrespace with this change.

Typo: s/usrespace/userspace/

>
> Signed-off-by: Jing Zhang <jingzhangos@google.com>
> ---
>  arch/arm64/kvm/sys_regs.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 053d8057ff1e..f33aec83f1b4 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -2008,7 +2008,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>  	  .set_user = set_id_dfr0_el1,
>  	  .visibility = aa32_id_visibility,
>  	  .reset = read_sanitised_id_dfr0_el1,
> -	  .val = ID_DFR0_EL1_PerfMon_MASK, },
> +	  .val = GENMASK(63, 0), },
>  	ID_HIDDEN(ID_AFR0_EL1),
>  	AA32_ID_SANITISED(ID_MMFR0_EL1),
>  	AA32_ID_SANITISED(ID_MMFR1_EL1),
> @@ -2057,7 +2057,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>  	  .get_user = get_id_reg,
>  	  .set_user = set_id_aa64dfr0_el1,
>  	  .reset = read_sanitised_id_aa64dfr0_el1,
> -	  .val = ID_AA64DFR0_EL1_PMUVer_MASK, },
> +	  .val = GENMASK(63, 0), },
>  	ID_SANITISED(ID_AA64DFR1_EL1),
>  	ID_UNALLOCATED(5,2),
>  	ID_UNALLOCATED(5,3),

How does userspace find out whether a given id reg is actually writable,
other than trying to write to it?


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

Re: [PATCH v6 3/6] KVM: arm64: Enable writable for ID_AA64DFR0_EL1 and ID_DFR0_EL1

[Jing Zhang]

Hi Cornelia,

On Thu, Jul 20, 2023 at 1:52 AM Cornelia Huck <cohuck@redhat.com> wrote:
>
> On Tue, Jul 18 2023, Jing Zhang <jingzhangos@google.com> wrote:
>
> > All valid fields in ID_AA64DFR0_EL1 and ID_DFR0_EL1 are writable
> > from usrespace with this change.
>
> Typo: s/usrespace/userspace/
Thanks.
>
> >
> > Signed-off-by: Jing Zhang <jingzhangos@google.com>
> > ---
> >  arch/arm64/kvm/sys_regs.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index 053d8057ff1e..f33aec83f1b4 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -2008,7 +2008,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> >         .set_user = set_id_dfr0_el1,
> >         .visibility = aa32_id_visibility,
> >         .reset = read_sanitised_id_dfr0_el1,
> > -       .val = ID_DFR0_EL1_PerfMon_MASK, },
> > +       .val = GENMASK(63, 0), },
> >       ID_HIDDEN(ID_AFR0_EL1),
> >       AA32_ID_SANITISED(ID_MMFR0_EL1),
> >       AA32_ID_SANITISED(ID_MMFR1_EL1),
> > @@ -2057,7 +2057,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> >         .get_user = get_id_reg,
> >         .set_user = set_id_aa64dfr0_el1,
> >         .reset = read_sanitised_id_aa64dfr0_el1,
> > -       .val = ID_AA64DFR0_EL1_PMUVer_MASK, },
> > +       .val = GENMASK(63, 0), },
> >       ID_SANITISED(ID_AA64DFR1_EL1),
> >       ID_UNALLOCATED(5,2),
> >       ID_UNALLOCATED(5,3),
>
> How does userspace find out whether a given id reg is actually writable,
> other than trying to write to it?
>
No mechanism was provided to userspace to discover if a given idreg or
any fields of a given idreg is writable. The write to a readonly idreg
can also succeed (write ignored) without any error if what's written
is exactly the same as what the idreg holds or if it is a write to
AArch32 idregs on an AArch64-only system.
Not sure if it is worth adding an API to return the writable mask for
idregs, since we want to enable the writable for all allocated
unhidden idregs eventually.

Thanks,
Jing

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

Re: [PATCH v6 3/6] KVM: arm64: Enable writable for ID_AA64DFR0_EL1 and ID_DFR0_EL1

[Cornelia Huck]

On Thu, Jul 20 2023, Jing Zhang <jingzhangos@google.com> wrote:

> Hi Cornelia,
>
> On Thu, Jul 20, 2023 at 1:52 AM Cornelia Huck <cohuck@redhat.com> wrote:
>>
>> On Tue, Jul 18 2023, Jing Zhang <jingzhangos@google.com> wrote:
>>
>> > All valid fields in ID_AA64DFR0_EL1 and ID_DFR0_EL1 are writable
>> > from usrespace with this change.
>>
>> Typo: s/usrespace/userspace/
> Thanks.
>>
>> >
>> > Signed-off-by: Jing Zhang <jingzhangos@google.com>
>> > ---
>> >  arch/arm64/kvm/sys_regs.c | 4 ++--
>> >  1 file changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> > index 053d8057ff1e..f33aec83f1b4 100644
>> > --- a/arch/arm64/kvm/sys_regs.c
>> > +++ b/arch/arm64/kvm/sys_regs.c
>> > @@ -2008,7 +2008,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>> >         .set_user = set_id_dfr0_el1,
>> >         .visibility = aa32_id_visibility,
>> >         .reset = read_sanitised_id_dfr0_el1,
>> > -       .val = ID_DFR0_EL1_PerfMon_MASK, },
>> > +       .val = GENMASK(63, 0), },
>> >       ID_HIDDEN(ID_AFR0_EL1),
>> >       AA32_ID_SANITISED(ID_MMFR0_EL1),
>> >       AA32_ID_SANITISED(ID_MMFR1_EL1),
>> > @@ -2057,7 +2057,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>> >         .get_user = get_id_reg,
>> >         .set_user = set_id_aa64dfr0_el1,
>> >         .reset = read_sanitised_id_aa64dfr0_el1,
>> > -       .val = ID_AA64DFR0_EL1_PMUVer_MASK, },
>> > +       .val = GENMASK(63, 0), },
>> >       ID_SANITISED(ID_AA64DFR1_EL1),
>> >       ID_UNALLOCATED(5,2),
>> >       ID_UNALLOCATED(5,3),
>>
>> How does userspace find out whether a given id reg is actually writable,
>> other than trying to write to it?
>>
> No mechanism was provided to userspace to discover if a given idreg or
> any fields of a given idreg is writable. The write to a readonly idreg
> can also succeed (write ignored) without any error if what's written
> is exactly the same as what the idreg holds or if it is a write to
> AArch32 idregs on an AArch64-only system.

Hm, I'm not sure that's a good thing for the cases where we want to
support mix-and-match userspace and kernels. Userspace may want to know
upfront whether it can actually tweak the contents of an idreg or not
(for example, in the context of using CPU models for compatibility), so
that it can reject or warn about certain configurations that may not
turn out as the user expects.

> Not sure if it is worth adding an API to return the writable mask for
> idregs, since we want to enable the writable for all allocated
> unhidden idregs eventually.

We'd enable any new idregs for writing from the start in the future, I
guess?

I see two approaches here:
- add an API to get a list of idregs with their writable masks
- add a capability "you can write to all idregs whatever you'd expect to
  be able to write there architecture wise", which would require to add
  support for all idregs prior to exposing that cap

The second option would be the easier one (if we don't manage to break
it in the future :)


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

Re: [PATCH v6 3/6] KVM: arm64: Enable writable for ID_AA64DFR0_EL1 and ID_DFR0_EL1

[Marc Zyngier]

On Fri, 21 Jul 2023 09:38:23 +0100,
Cornelia Huck <cohuck@redhat.com> wrote:
> 
> On Thu, Jul 20 2023, Jing Zhang <jingzhangos@google.com> wrote:
> 
> > Hi Cornelia,
> >
> > On Thu, Jul 20, 2023 at 1:52 AM Cornelia Huck <cohuck@redhat.com> wrote:
> >>
> >> On Tue, Jul 18 2023, Jing Zhang <jingzhangos@google.com> wrote:
> >>
> >> > All valid fields in ID_AA64DFR0_EL1 and ID_DFR0_EL1 are writable
> >> > from usrespace with this change.
> >>
> >> Typo: s/usrespace/userspace/
> > Thanks.
> >>
> >> >
> >> > Signed-off-by: Jing Zhang <jingzhangos@google.com>
> >> > ---
> >> >  arch/arm64/kvm/sys_regs.c | 4 ++--
> >> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> >> > index 053d8057ff1e..f33aec83f1b4 100644
> >> > --- a/arch/arm64/kvm/sys_regs.c
> >> > +++ b/arch/arm64/kvm/sys_regs.c
> >> > @@ -2008,7 +2008,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> >> >         .set_user = set_id_dfr0_el1,
> >> >         .visibility = aa32_id_visibility,
> >> >         .reset = read_sanitised_id_dfr0_el1,
> >> > -       .val = ID_DFR0_EL1_PerfMon_MASK, },
> >> > +       .val = GENMASK(63, 0), },
> >> >       ID_HIDDEN(ID_AFR0_EL1),
> >> >       AA32_ID_SANITISED(ID_MMFR0_EL1),
> >> >       AA32_ID_SANITISED(ID_MMFR1_EL1),
> >> > @@ -2057,7 +2057,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> >> >         .get_user = get_id_reg,
> >> >         .set_user = set_id_aa64dfr0_el1,
> >> >         .reset = read_sanitised_id_aa64dfr0_el1,
> >> > -       .val = ID_AA64DFR0_EL1_PMUVer_MASK, },
> >> > +       .val = GENMASK(63, 0), },
> >> >       ID_SANITISED(ID_AA64DFR1_EL1),
> >> >       ID_UNALLOCATED(5,2),
> >> >       ID_UNALLOCATED(5,3),
> >>
> >> How does userspace find out whether a given id reg is actually writable,
> >> other than trying to write to it?
> >>
> > No mechanism was provided to userspace to discover if a given idreg or
> > any fields of a given idreg is writable. The write to a readonly idreg
> > can also succeed (write ignored) without any error if what's written
> > is exactly the same as what the idreg holds or if it is a write to
> > AArch32 idregs on an AArch64-only system.
> 
> Hm, I'm not sure that's a good thing for the cases where we want to
> support mix-and-match userspace and kernels. Userspace may want to know
> upfront whether it can actually tweak the contents of an idreg or not
> (for example, in the context of using CPU models for compatibility), so
> that it can reject or warn about certain configurations that may not
> turn out as the user expects.
> 
> > Not sure if it is worth adding an API to return the writable mask for
> > idregs, since we want to enable the writable for all allocated
> > unhidden idregs eventually.
> 
> We'd enable any new idregs for writing from the start in the future, I
> guess?
> 
> I see two approaches here:
> - add an API to get a list of idregs with their writable masks
> - add a capability "you can write to all idregs whatever you'd expect to
>   be able to write there architecture wise", which would require to add
>   support for all idregs prior to exposing that cap
> 
> The second option would be the easier one (if we don't manage to break
> it in the future :)

I'm not sure the last option is even possible. The architecture keeps
allocating new ID registers in the op0==3, op1=={0, 1, 3}, CRn==0,
CRm=={0-7}, op2=={0-7} space, so fields that were RES0 until then
start having a non-0 value.

This could lead to a situation where you move from a system that
didn't know about ID_AA64MMFR6_EL1.XYZ to a system that advertises it,
and for which the XYZ instruction has another behaviour. Bad things
follow.

My preference would be a single ioctl that returns the full list of
writeable masks in the ID reg range. It is big, but not crazy big
(1536 bytes, if I haven't messed up), and includes the non ID_*_EL1
sysreg such as MPIDR_EL1, CTR_EL1, SMIDR_EL1.

It would allow the VMM to actively write zeroes to any writable ID
register it doesn't know about, or for which it doesn't have anything
to restore. It is also relatively future proof, as it covers
*everything* the architecture has provisioned for the future (by the
time that space is exhausted, I hope none of us will still be involved
with this crap).

Thanks,

	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 v6 3/6] KVM: arm64: Enable writable for ID_AA64DFR0_EL1 and ID_DFR0_EL1

[Cornelia Huck]

On Fri, Jul 21 2023, Marc Zyngier <maz@kernel.org> wrote:

> On Fri, 21 Jul 2023 09:38:23 +0100,
> Cornelia Huck <cohuck@redhat.com> wrote:
>> 
>> On Thu, Jul 20 2023, Jing Zhang <jingzhangos@google.com> wrote:
>> > No mechanism was provided to userspace to discover if a given idreg or
>> > any fields of a given idreg is writable. The write to a readonly idreg
>> > can also succeed (write ignored) without any error if what's written
>> > is exactly the same as what the idreg holds or if it is a write to
>> > AArch32 idregs on an AArch64-only system.
>> 
>> Hm, I'm not sure that's a good thing for the cases where we want to
>> support mix-and-match userspace and kernels. Userspace may want to know
>> upfront whether it can actually tweak the contents of an idreg or not
>> (for example, in the context of using CPU models for compatibility), so
>> that it can reject or warn about certain configurations that may not
>> turn out as the user expects.
>> 
>> > Not sure if it is worth adding an API to return the writable mask for
>> > idregs, since we want to enable the writable for all allocated
>> > unhidden idregs eventually.
>> 
>> We'd enable any new idregs for writing from the start in the future, I
>> guess?
>> 
>> I see two approaches here:
>> - add an API to get a list of idregs with their writable masks
>> - add a capability "you can write to all idregs whatever you'd expect to
>>   be able to write there architecture wise", which would require to add
>>   support for all idregs prior to exposing that cap
>> 
>> The second option would be the easier one (if we don't manage to break
>> it in the future :)
>
> I'm not sure the last option is even possible. The architecture keeps
> allocating new ID registers in the op0==3, op1=={0, 1, 3}, CRn==0,
> CRm=={0-7}, op2=={0-7} space, so fields that were RES0 until then
> start having a non-0 value.
>
> This could lead to a situation where you move from a system that
> didn't know about ID_AA64MMFR6_EL1.XYZ to a system that advertises it,
> and for which the XYZ instruction has another behaviour. Bad things
> follow.

Hrm :(

>
> My preference would be a single ioctl that returns the full list of
> writeable masks in the ID reg range. It is big, but not crazy big
> (1536 bytes, if I haven't messed up), and includes the non ID_*_EL1
> sysreg such as MPIDR_EL1, CTR_EL1, SMIDR_EL1.
>
> It would allow the VMM to actively write zeroes to any writable ID
> register it doesn't know about, or for which it doesn't have anything
> to restore. It is also relatively future proof, as it covers
> *everything* the architecture has provisioned for the future (by the
> time that space is exhausted, I hope none of us will still be involved
> with this crap).

Famous last words :)

But yes, that should work. This wouldn't be the first ioctl returning a
long list, and the VMM would just call it once on VM startup to figure
things out anyway.


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

Re: [PATCH v6 3/6] KVM: arm64: Enable writable for ID_AA64DFR0_EL1 and ID_DFR0_EL1

[Jing Zhang]

Hi Marc,

On Fri, Jul 21, 2023 at 2:31 AM Marc Zyngier <maz@kernel.org> wrote:
>
> On Fri, 21 Jul 2023 09:38:23 +0100,
> Cornelia Huck <cohuck@redhat.com> wrote:
> >
> > On Thu, Jul 20 2023, Jing Zhang <jingzhangos@google.com> wrote:
> >
> > > Hi Cornelia,
> > >
> > > On Thu, Jul 20, 2023 at 1:52 AM Cornelia Huck <cohuck@redhat.com> wrote:
> > >>
> > >> On Tue, Jul 18 2023, Jing Zhang <jingzhangos@google.com> wrote:
> > >>
> > >> > All valid fields in ID_AA64DFR0_EL1 and ID_DFR0_EL1 are writable
> > >> > from usrespace with this change.
> > >>
> > >> Typo: s/usrespace/userspace/
> > > Thanks.
> > >>
> > >> >
> > >> > Signed-off-by: Jing Zhang <jingzhangos@google.com>
> > >> > ---
> > >> >  arch/arm64/kvm/sys_regs.c | 4 ++--
> > >> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >> >
> > >> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > >> > index 053d8057ff1e..f33aec83f1b4 100644
> > >> > --- a/arch/arm64/kvm/sys_regs.c
> > >> > +++ b/arch/arm64/kvm/sys_regs.c
> > >> > @@ -2008,7 +2008,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> > >> >         .set_user = set_id_dfr0_el1,
> > >> >         .visibility = aa32_id_visibility,
> > >> >         .reset = read_sanitised_id_dfr0_el1,
> > >> > -       .val = ID_DFR0_EL1_PerfMon_MASK, },
> > >> > +       .val = GENMASK(63, 0), },
> > >> >       ID_HIDDEN(ID_AFR0_EL1),
> > >> >       AA32_ID_SANITISED(ID_MMFR0_EL1),
> > >> >       AA32_ID_SANITISED(ID_MMFR1_EL1),
> > >> > @@ -2057,7 +2057,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> > >> >         .get_user = get_id_reg,
> > >> >         .set_user = set_id_aa64dfr0_el1,
> > >> >         .reset = read_sanitised_id_aa64dfr0_el1,
> > >> > -       .val = ID_AA64DFR0_EL1_PMUVer_MASK, },
> > >> > +       .val = GENMASK(63, 0), },
> > >> >       ID_SANITISED(ID_AA64DFR1_EL1),
> > >> >       ID_UNALLOCATED(5,2),
> > >> >       ID_UNALLOCATED(5,3),
> > >>
> > >> How does userspace find out whether a given id reg is actually writable,
> > >> other than trying to write to it?
> > >>
> > > No mechanism was provided to userspace to discover if a given idreg or
> > > any fields of a given idreg is writable. The write to a readonly idreg
> > > can also succeed (write ignored) without any error if what's written
> > > is exactly the same as what the idreg holds or if it is a write to
> > > AArch32 idregs on an AArch64-only system.
> >
> > Hm, I'm not sure that's a good thing for the cases where we want to
> > support mix-and-match userspace and kernels. Userspace may want to know
> > upfront whether it can actually tweak the contents of an idreg or not
> > (for example, in the context of using CPU models for compatibility), so
> > that it can reject or warn about certain configurations that may not
> > turn out as the user expects.
> >
> > > Not sure if it is worth adding an API to return the writable mask for
> > > idregs, since we want to enable the writable for all allocated
> > > unhidden idregs eventually.
> >
> > We'd enable any new idregs for writing from the start in the future, I
> > guess?
> >
> > I see two approaches here:
> > - add an API to get a list of idregs with their writable masks
> > - add a capability "you can write to all idregs whatever you'd expect to
> >   be able to write there architecture wise", which would require to add
> >   support for all idregs prior to exposing that cap
> >
> > The second option would be the easier one (if we don't manage to break
> > it in the future :)
>
> I'm not sure the last option is even possible. The architecture keeps
> allocating new ID registers in the op0==3, op1=={0, 1, 3}, CRn==0,
> CRm=={0-7}, op2=={0-7} space, so fields that were RES0 until then
> start having a non-0 value.
For now, the per VM ID emulated ID registers support only covers space
for op0==3, op1==0, CRn==0, CRm=={1-7}, op2=={0-8}. For others, mask
value of 0 would be returned in the new ioctl.
>
> This could lead to a situation where you move from a system that
> didn't know about ID_AA64MMFR6_EL1.XYZ to a system that advertises it,
> and for which the XYZ instruction has another behaviour. Bad things
> follow.
>
> My preference would be a single ioctl that returns the full list of
> writeable masks in the ID reg range. It is big, but not crazy big
> (1536 bytes, if I haven't messed up), and includes the non ID_*_EL1
> sysreg such as MPIDR_EL1, CTR_EL1, SMIDR_EL1.
Just want to double confirm that would the ioclt return the list of
only writable masks, not the list of {idreg_name, mask} pair? So, the
VMM will need to index idreg's writable mask by op1, CRm, op2?
>
> It would allow the VMM to actively write zeroes to any writable ID
> register it doesn't know about, or for which it doesn't have anything
> to restore. It is also relatively future proof, as it covers
> *everything* the architecture has provisioned for the future (by the
> time that space is exhausted, I hope none of us will still be involved
> with this crap).
>
> Thanks,
>
>         M.
>
> --
> Without deviation from the norm, progress is not possible.
>
Thanks,
Jing

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

Re: [PATCH v6 3/6] KVM: arm64: Enable writable for ID_AA64DFR0_EL1 and ID_DFR0_EL1

[Oliver Upton]

On Fri, Jul 21, 2023 at 11:22:35AM -0700, Jing Zhang wrote:
> On Fri, Jul 21, 2023 at 2:31 AM Marc Zyngier <maz@kernel.org> wrote:
> > My preference would be a single ioctl that returns the full list of
> > writeable masks in the ID reg range. It is big, but not crazy big
> > (1536 bytes, if I haven't messed up), and includes the non ID_*_EL1
> > sysreg such as MPIDR_EL1, CTR_EL1, SMIDR_EL1.
> Just want to double confirm that would the ioclt return the list of
> only writable masks, not the list of {idreg_name, mask} pair? So, the
> VMM will need to index idreg's writable mask by op1, CRm, op2?

I generally agree with the approach Marc is proposing, but I wonder if
it makes sense to have userspace ask the kernel for this information on
a per-register basis.

What I had in mind was something similar to the KVM_GET_ONE_REG ioctl,
but instead of returning the register value it'd return the mask of the
register. This would keep the kernel implementation dead simple (I'm
lazy) and more easily allow for future expansion in case we want to
start describing more registers this way. Userspace would iterate the ID
register space and ask the kernel for the mask of registers it wants to
change.

Thoughts?

-- 
Thanks,
Oliver

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

Re: [PATCH v6 2/6] KVM: arm64: Reject attempts to set invalid debug arch version

[Oliver Upton]

On Tue, Jul 18, 2023 at 04:45:18PM +0000, Jing Zhang wrote:
> From: Oliver Upton <oliver.upton@linux.dev>
> 
> The debug architecture is mandatory in ARMv8, so KVM should not allow
> userspace to configure a vCPU with less than that. Of course, this isn't
> handled elegantly by the generic ID register plumbing, as the respective
> ID register fields have a nonzero starting value.
> 
> Add an explicit check for debug versions less than v8 of the
> architecture.
> 
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> Signed-off-by: Jing Zhang <jingzhangos@google.com>

This patch needs to be broken up. You're doing a couple things:

 1) Forcing the behavior of the DebugVer field to be FTR_LOWER_SAFE, and
   adding the necessary check for a valid version

 2) Changing KVM's value for the field to expose up to Debugv8p8 to the
   guest.

The latter isn't described in the changelog at all, and worse yet the
ordering of the series is not bisectable. Changing the default value of
the field w/o allowing writes breaks migration.

So, please split this patch in two and consider stacking like so:

 - Change #1 above (field sanitization)

 - "KVM: arm64: Enable writable for ID_AA64DFR0_EL1 and ID_DFR0_EL1"

 - Change #2 above (advertise up to v8p8)

-- 
Thanks,
Oliver

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

Re: [PATCH v6 2/6] KVM: arm64: Reject attempts to set invalid debug arch version

[Jing Zhang]

Hi Oliver,

On Fri, Jul 21, 2023 at 2:18 PM Oliver Upton <oliver.upton@linux.dev> wrote:
>
> On Tue, Jul 18, 2023 at 04:45:18PM +0000, Jing Zhang wrote:
> > From: Oliver Upton <oliver.upton@linux.dev>
> >
> > The debug architecture is mandatory in ARMv8, so KVM should not allow
> > userspace to configure a vCPU with less than that. Of course, this isn't
> > handled elegantly by the generic ID register plumbing, as the respective
> > ID register fields have a nonzero starting value.
> >
> > Add an explicit check for debug versions less than v8 of the
> > architecture.
> >
> > Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> > Signed-off-by: Jing Zhang <jingzhangos@google.com>
>
> This patch needs to be broken up. You're doing a couple things:
>
>  1) Forcing the behavior of the DebugVer field to be FTR_LOWER_SAFE, and
>    adding the necessary check for a valid version
>
>  2) Changing KVM's value for the field to expose up to Debugv8p8 to the
>    guest.
>
> The latter isn't described in the changelog at all, and worse yet the
> ordering of the series is not bisectable. Changing the default value of
> the field w/o allowing writes breaks migration.
>
> So, please split this patch in two and consider stacking like so:
>
>  - Change #1 above (field sanitization)
>
>  - "KVM: arm64: Enable writable for ID_AA64DFR0_EL1 and ID_DFR0_EL1"
>
>  - Change #2 above (advertise up to v8p8)
>
> --
> Thanks,
> Oliver
Sure, I'll split it as you suggested.

Thanks,
Jing

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

Re: [PATCH v6 3/6] KVM: arm64: Enable writable for ID_AA64DFR0_EL1 and ID_DFR0_EL1

[Cornelia Huck]

On Wed, Jul 26 2023, Oliver Upton <oliver.upton@linux.dev> wrote:

> Hi Cornelia,
>
> On Mon, Jul 24, 2023 at 10:45:44AM +0200, Cornelia Huck wrote:
>> On Fri, Jul 21 2023, Oliver Upton <oliver.upton@linux.dev> wrote:
>> > What I had in mind was something similar to the KVM_GET_ONE_REG ioctl,
>> > but instead of returning the register value it'd return the mask of the
>> > register. This would keep the kernel implementation dead simple (I'm
>> > lazy) and more easily allow for future expansion in case we want to
>> > start describing more registers this way. Userspace would iterate the ID
>> > register space and ask the kernel for the mask of registers it wants to
>> > change.
>> 
>> Hm... for userspace it might be easier to get one big list and then
>> parse it afterwards? Similar to what GET_REG_LIST does today.
>
> Possibly, but I felt like it was a bit different from GET_REG_LIST since
> this would actually be a list of key-value pairs (reg_id, mask) instead
> of a pure enumeration of IDs. My worry is that if/when we wind up describing
> more registers in this list-based ioctl then userspace is going to wind
> up traversing that structure a lot to find the register masks it actually
> cares about.

Depends on how userspace actually digests it, but point taken.

>
>> Are you thinking more of a KVM_GET_REG_INFO or so ioctl, that could
>> support different kinds of extra info (and might also make sense for
>> other architectures?) If we end up with something more versatile, it
>> might make sense going that route.
>
> TBH, I hadn't considered the extensibililty of a per-register ioctl, but
> that does seem like a good point.

Maybe smth like

/* available with KVM_CAP_GET_REG_INFO */
struct kvm_reg_info {
	__u64 id;
	__u32 op;
	__u32 len;
	__u8 data[];
};

/* operations for kvm_reg_info->op */
#define KVM_REG_INFO_ARM_ID_REG 0

#define KVM_GET_REG_INFO _IOW(KVMIO, 0xd2, struct kvm_reg_info)

and returning sys_reg_desc->val in data if id points to a valid id reg.


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

Re: [PATCH v6 3/6] KVM: arm64: Enable writable for ID_AA64DFR0_EL1 and ID_DFR0_EL1

[Marc Zyngier]

On Fri, 21 Jul 2023 10:48:27 +0100,
Cornelia Huck <cohuck@redhat.com> wrote:
> 
> On Fri, Jul 21 2023, Marc Zyngier <maz@kernel.org> wrote:
> 
> > On Fri, 21 Jul 2023 09:38:23 +0100,
> > Cornelia Huck <cohuck@redhat.com> wrote:
> >> 
> >> On Thu, Jul 20 2023, Jing Zhang <jingzhangos@google.com> wrote:
> >> > No mechanism was provided to userspace to discover if a given idreg or
> >> > any fields of a given idreg is writable. The write to a readonly idreg
> >> > can also succeed (write ignored) without any error if what's written
> >> > is exactly the same as what the idreg holds or if it is a write to
> >> > AArch32 idregs on an AArch64-only system.
> >> 
> >> Hm, I'm not sure that's a good thing for the cases where we want to
> >> support mix-and-match userspace and kernels. Userspace may want to know
> >> upfront whether it can actually tweak the contents of an idreg or not
> >> (for example, in the context of using CPU models for compatibility), so
> >> that it can reject or warn about certain configurations that may not
> >> turn out as the user expects.
> >> 
> >> > Not sure if it is worth adding an API to return the writable mask for
> >> > idregs, since we want to enable the writable for all allocated
> >> > unhidden idregs eventually.
> >> 
> >> We'd enable any new idregs for writing from the start in the future, I
> >> guess?
> >> 
> >> I see two approaches here:
> >> - add an API to get a list of idregs with their writable masks
> >> - add a capability "you can write to all idregs whatever you'd expect to
> >>   be able to write there architecture wise", which would require to add
> >>   support for all idregs prior to exposing that cap
> >> 
> >> The second option would be the easier one (if we don't manage to break
> >> it in the future :)
> >
> > I'm not sure the last option is even possible. The architecture keeps
> > allocating new ID registers in the op0==3, op1=={0, 1, 3}, CRn==0,
> > CRm=={0-7}, op2=={0-7} space, so fields that were RES0 until then
> > start having a non-0 value.
> >
> > This could lead to a situation where you move from a system that
> > didn't know about ID_AA64MMFR6_EL1.XYZ to a system that advertises it,
> > and for which the XYZ instruction has another behaviour. Bad things
> > follow.
> 
> Hrm :(
> 
> >
> > My preference would be a single ioctl that returns the full list of
> > writeable masks in the ID reg range. It is big, but not crazy big
> > (1536 bytes, if I haven't messed up), and includes the non ID_*_EL1
> > sysreg such as MPIDR_EL1, CTR_EL1, SMIDR_EL1.
> >
> > It would allow the VMM to actively write zeroes to any writable ID
> > register it doesn't know about, or for which it doesn't have anything
> > to restore. It is also relatively future proof, as it covers
> > *everything* the architecture has provisioned for the future (by the
> > time that space is exhausted, I hope none of us will still be involved
> > with this crap).
> 
> Famous last words :)
> 
> But yes, that should work. This wouldn't be the first ioctl returning a
> long list, and the VMM would just call it once on VM startup to figure
> things out anyway.

To be clear, see below for what I had in mind. It is of course
untested, and is probably overlooking a number of details, but you'll
hopefully get my drift. I think this has some benefit over the
per-sysreg ioctl, as it covers everything in one go, and is guaranteed
to be exhaustive (until the architecture grows another range of ID
crap).

Note that we don't necessarily need to restrict ourselves to a single
range either. We could also return some other ranges depending on
additional parameters (Oliver mentioned offline the case of the
PCMEIDx_EL0 registers).

Thank,

	M.

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 2ca2973abe66..fa79f3651423 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -3589,3 +3589,91 @@ int __init kvm_sys_reg_table_init(void)
 
 	return 0;
 }
+
+/*
+ * From DDI0487J.a, D19.2.66 ("ID_AA64MMFR2_EL1, AArch64 Memory Model
+ * Feature Register 2"):
+ *
+ * "The Feature ID space is defined as the System register space in
+ * AArch64 with op0==3, op1=={0, 1, 3}, CRn==0, CRm=={0-7},
+ * op2=={0-7}."
+ *
+ * This covers all R/O registers that indicate anything useful feature
+ * wise, including the ID registers.
+ */
+
+/* Userspace-visible definitions */
+#define ARM64_FEATURE_ID_SPACE_SIZE	(3 * 8 * 8)
+#define __ARM64_FEATURE_ID_SPACE_IDX(op0, op1, crn, crm, op2)		\
+	({								\
+		__u64 __op1 = op1 & 3;					\
+		__op1 -= (__op1 == 3);					\
+		((ARM64_SYS_REG_SHIFT_MASK(3, OP0) |			\
+		  ARM64_SYS_REG_SHIFT_MASK(__op1, OP1) |		\
+		  ARM64_SYS_REG_SHIFT_MASK(0, CRN) |			\
+		  ARM64_SYS_REG_SHIFT_MASK(crm & 7, CRM) |		\
+		  ARM64_SYS_REG_SHIFT_MASK(op2, OP2)) -			\
+		 (ARM64_SYS_REG_SHIFT_MASK(3, OP0) |			\
+		  ARM64_SYS_REG_SHIFT_MASK(0, OP1) |			\
+		  ARM64_SYS_REG_SHIFT_MASK(0, CRN) |			\
+		  ARM64_SYS_REG_SHIFT_MASK(0, CRM) |			\
+		  ARM64_SYS_REG_SHIFT_MASK(0, OP2)));			\
+	})
+
+#define ARM64_FEATURE_ID_SPACE_INDEX(r)					\
+	__ARM64_FEATURE_ID_SPACE_IDX(sys_reg_Op0(r),			\
+				     sys_reg_Op1(r),			\
+				     sys_reg_CRn(r),			\
+				     sys_reg_CRm(r),			\
+				     sys_reg_Op2(r))
+
+struct feature_id_writeable_masks {
+	u64	mask[ARM64_FEATURE_ID_SPACE_SIZE];
+};
+
+static bool is_feature_id_reg(u32 encoding)
+{
+	return (sys_reg_Op0(encoding) == 3 &&
+		(sys_reg_Op1(encoding) < 2 || sys_reg_Op1(encoding) == 3) &&
+		sys_reg_CRn(encoding) == 0 &&
+		sys_reg_CRm(encoding) <= 7);
+}
+
+int kvm_get_writeable_feature_regs(struct kvm *kvm, u64 __user *masks)
+{
+	/* Wipe the whole thing first */
+	for (int i = 0; i < ARM64_FEATURE_ID_SPACE_SIZE; i++)
+		if (put_user(0, masks + i))
+			return -EFAULT;
+
+	for (int i = 0; i < ARRAY_SIZE(sys_reg_descs); i++) {
+		const struct sys_reg_desc *reg = &sys_reg_descs[i];
+		u32 encoding = reg_to_encoding(reg);
+		u64 val;
+
+		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 necessarily 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)
+				continue;
+
+			val = reg->val;
+		} else {
+			val = ~0UL;
+		}
+
+		if (put_user(val,
+			     (masks + ARM64_FEATURE_ID_SPACE_INDEX(encoding))))
+			return -EFAULT;
+	}
+
+	return 0;
+}



-- 
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 v6 3/6] KVM: arm64: Enable writable for ID_AA64DFR0_EL1 and ID_DFR0_EL1

[Jing Zhang]

On Sat, Jul 29, 2023 at 3:36 AM Marc Zyngier <maz@kernel.org> wrote:
>
> On Fri, 21 Jul 2023 10:48:27 +0100,
> Cornelia Huck <cohuck@redhat.com> wrote:
> >
> > On Fri, Jul 21 2023, Marc Zyngier <maz@kernel.org> wrote:
> >
> > > On Fri, 21 Jul 2023 09:38:23 +0100,
> > > Cornelia Huck <cohuck@redhat.com> wrote:
> > >>
> > >> On Thu, Jul 20 2023, Jing Zhang <jingzhangos@google.com> wrote:
> > >> > No mechanism was provided to userspace to discover if a given idreg or
> > >> > any fields of a given idreg is writable. The write to a readonly idreg
> > >> > can also succeed (write ignored) without any error if what's written
> > >> > is exactly the same as what the idreg holds or if it is a write to
> > >> > AArch32 idregs on an AArch64-only system.
> > >>
> > >> Hm, I'm not sure that's a good thing for the cases where we want to
> > >> support mix-and-match userspace and kernels. Userspace may want to know
> > >> upfront whether it can actually tweak the contents of an idreg or not
> > >> (for example, in the context of using CPU models for compatibility), so
> > >> that it can reject or warn about certain configurations that may not
> > >> turn out as the user expects.
> > >>
> > >> > Not sure if it is worth adding an API to return the writable mask for
> > >> > idregs, since we want to enable the writable for all allocated
> > >> > unhidden idregs eventually.
> > >>
> > >> We'd enable any new idregs for writing from the start in the future, I
> > >> guess?
> > >>
> > >> I see two approaches here:
> > >> - add an API to get a list of idregs with their writable masks
> > >> - add a capability "you can write to all idregs whatever you'd expect to
> > >>   be able to write there architecture wise", which would require to add
> > >>   support for all idregs prior to exposing that cap
> > >>
> > >> The second option would be the easier one (if we don't manage to break
> > >> it in the future :)
> > >
> > > I'm not sure the last option is even possible. The architecture keeps
> > > allocating new ID registers in the op0==3, op1=={0, 1, 3}, CRn==0,
> > > CRm=={0-7}, op2=={0-7} space, so fields that were RES0 until then
> > > start having a non-0 value.
> > >
> > > This could lead to a situation where you move from a system that
> > > didn't know about ID_AA64MMFR6_EL1.XYZ to a system that advertises it,
> > > and for which the XYZ instruction has another behaviour. Bad things
> > > follow.
> >
> > Hrm :(
> >
> > >
> > > My preference would be a single ioctl that returns the full list of
> > > writeable masks in the ID reg range. It is big, but not crazy big
> > > (1536 bytes, if I haven't messed up), and includes the non ID_*_EL1
> > > sysreg such as MPIDR_EL1, CTR_EL1, SMIDR_EL1.
> > >
> > > It would allow the VMM to actively write zeroes to any writable ID
> > > register it doesn't know about, or for which it doesn't have anything
> > > to restore. It is also relatively future proof, as it covers
> > > *everything* the architecture has provisioned for the future (by the
> > > time that space is exhausted, I hope none of us will still be involved
> > > with this crap).
> >
> > Famous last words :)
> >
> > But yes, that should work. This wouldn't be the first ioctl returning a
> > long list, and the VMM would just call it once on VM startup to figure
> > things out anyway.
>
> To be clear, see below for what I had in mind. It is of course
> untested, and is probably overlooking a number of details, but you'll
> hopefully get my drift. I think this has some benefit over the
> per-sysreg ioctl, as it covers everything in one go, and is guaranteed
> to be exhaustive (until the architecture grows another range of ID
> crap).
>
> Note that we don't necessarily need to restrict ourselves to a single
> range either. We could also return some other ranges depending on
> additional parameters (Oliver mentioned offline the case of the
> PCMEIDx_EL0 registers).
>
> Thank,
>
>         M.
>
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 2ca2973abe66..fa79f3651423 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -3589,3 +3589,91 @@ int __init kvm_sys_reg_table_init(void)
>
>         return 0;
>  }
> +
> +/*
> + * From DDI0487J.a, D19.2.66 ("ID_AA64MMFR2_EL1, AArch64 Memory Model
> + * Feature Register 2"):
> + *
> + * "The Feature ID space is defined as the System register space in
> + * AArch64 with op0==3, op1=={0, 1, 3}, CRn==0, CRm=={0-7},
> + * op2=={0-7}."
> + *
> + * This covers all R/O registers that indicate anything useful feature
> + * wise, including the ID registers.
> + */
> +
> +/* Userspace-visible definitions */
> +#define ARM64_FEATURE_ID_SPACE_SIZE    (3 * 8 * 8)
> +#define __ARM64_FEATURE_ID_SPACE_IDX(op0, op1, crn, crm, op2)          \
> +       ({                                                              \
> +               __u64 __op1 = op1 & 3;                                  \
> +               __op1 -= (__op1 == 3);                                  \
> +               ((ARM64_SYS_REG_SHIFT_MASK(3, OP0) |                    \
> +                 ARM64_SYS_REG_SHIFT_MASK(__op1, OP1) |                \
> +                 ARM64_SYS_REG_SHIFT_MASK(0, CRN) |                    \
> +                 ARM64_SYS_REG_SHIFT_MASK(crm & 7, CRM) |              \
> +                 ARM64_SYS_REG_SHIFT_MASK(op2, OP2)) -                 \
> +                (ARM64_SYS_REG_SHIFT_MASK(3, OP0) |                    \
> +                 ARM64_SYS_REG_SHIFT_MASK(0, OP1) |                    \
> +                 ARM64_SYS_REG_SHIFT_MASK(0, CRN) |                    \
> +                 ARM64_SYS_REG_SHIFT_MASK(0, CRM) |                    \
> +                 ARM64_SYS_REG_SHIFT_MASK(0, OP2)));                   \
> +       })
> +
> +#define ARM64_FEATURE_ID_SPACE_INDEX(r)                                        \
> +       __ARM64_FEATURE_ID_SPACE_IDX(sys_reg_Op0(r),                    \
> +                                    sys_reg_Op1(r),                    \
> +                                    sys_reg_CRn(r),                    \
> +                                    sys_reg_CRm(r),                    \
> +                                    sys_reg_Op2(r))
> +
> +struct feature_id_writeable_masks {
> +       u64     mask[ARM64_FEATURE_ID_SPACE_SIZE];
> +};
> +
> +static bool is_feature_id_reg(u32 encoding)
> +{
> +       return (sys_reg_Op0(encoding) == 3 &&
> +               (sys_reg_Op1(encoding) < 2 || sys_reg_Op1(encoding) == 3) &&
> +               sys_reg_CRn(encoding) == 0 &&
> +               sys_reg_CRm(encoding) <= 7);
> +}
> +
> +int kvm_get_writeable_feature_regs(struct kvm *kvm, u64 __user *masks)
> +{
> +       /* Wipe the whole thing first */
> +       for (int i = 0; i < ARM64_FEATURE_ID_SPACE_SIZE; i++)
> +               if (put_user(0, masks + i))
> +                       return -EFAULT;
> +
> +       for (int i = 0; i < ARRAY_SIZE(sys_reg_descs); i++) {
> +               const struct sys_reg_desc *reg = &sys_reg_descs[i];
> +               u32 encoding = reg_to_encoding(reg);
> +               u64 val;
> +
> +               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 necessarily 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)
> +                               continue;
> +
> +                       val = reg->val;
> +               } else {
> +                       val = ~0UL;
> +               }
> +
> +               if (put_user(val,
> +                            (masks + ARM64_FEATURE_ID_SPACE_INDEX(encoding))))
> +                       return -EFAULT;
> +       }
> +
> +       return 0;
> +}
Thanks Marc.
The whole idea is clear to me now. I'll implement this in the next version.

Jing
>
>
>
> --
> 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