[PATCH v2 0/3] KVM: arm64: Limit feature register reads from AArch32

[PATCH v2 0/3] KVM: arm64: Limit feature register reads from AArch32

[Oliver Upton]

KVM/arm64 does not restrict the guest's view of the AArch32 feature
registers when read from AArch32. HCR_EL2.TID3 is cleared for AArch32
guests, meaning that register reads come straight from hardware. This is
problematic as KVM relies on read_sanitised_ftr_reg() to expose a set of
features consistent for a particular system.

Appropriate handlers must first be put in place for CP10 and CP15 ID
register accesses before setting TID3. Rather than exhaustively
enumerating each of the encodings for CP10 and CP15 registers, take the
lazy route and aim the register accesses at the AArch64 system register
table.

Patch 1 reroutes the CP15 registers into the AArch64 table, taking care
to immediately RAZ undefined ranges of registers. This is done to avoid
possibly conflicting with encodings for future AArch64 registers.

Patch 2 installs an exit handler for the CP10 ID registers and also
relies on the general AArch64 register handler to implement reads.

Finally, patch 3 actually sets TID3 for AArch32 guests, providing
known-safe values for feature register accesses.

Series applies cleanly to kvmarm/fixes at commit:

  8872d9b3e35a ("KVM: arm64: Drop unneeded minor version check from PSCI v1.x handler")

There is an argument that the series is in fact a bug fix for running
AArch32 VMs on heterogeneous systems. To that end, it could be
blamed/backported to when we first knew better:

  93390c0a1b20 ("arm64: KVM: Hide unsupported AArch64 CPU features from guests")

But I left that tag off as in the aforementioned change skipping
AArch32 was intentional.

Tested with AArch32 kvm-unit-tests and booting an AArch32 debian guest
on a Raspberry Pi 4.

v1: https://lore.kernel.org/kvmarm/20220329011301.1166265-1-oupton@google.com/

v1 -> v2:
 - Actually set TID3! Oops.
 - Refactor kvm_emulate_cp15_id_reg() to check preconditions before
   proceeding to emulation (Reiji)
 - Tighten up comment on kvm_is_cp15_id_reg() to indicate that the only
   other trapped ID register (CTR) is already handled in the cp15
   register table (Reiji)

Oliver Upton (3):
  KVM: arm64: Wire up CP15 feature registers to their AArch64
    equivalents
  KVM: arm64: Plumb cp10 ID traps through the AArch64 sysreg handler
  KVM: arm64: Start trapping ID registers for 32 bit guests

 arch/arm64/include/asm/kvm_arm.h     |   3 +-
 arch/arm64/include/asm/kvm_emulate.h |   8 --
 arch/arm64/include/asm/kvm_host.h    |   1 +
 arch/arm64/kvm/handle_exit.c         |   1 +
 arch/arm64/kvm/sys_regs.c            | 129 +++++++++++++++++++++++++++
 5 files changed, 133 insertions(+), 9 deletions(-)

-- 
2.35.1.1094.g7c7d902a7c-goog

[PATCH v2 0/3] KVM: arm64: Limit feature register reads from AArch32

[Oliver Upton]

KVM/arm64 does not restrict the guest's view of the AArch32 feature
registers when read from AArch32. HCR_EL2.TID3 is cleared for AArch32
guests, meaning that register reads come straight from hardware. This is
problematic as KVM relies on read_sanitised_ftr_reg() to expose a set of
features consistent for a particular system.

Appropriate handlers must first be put in place for CP10 and CP15 ID
register accesses before setting TID3. Rather than exhaustively
enumerating each of the encodings for CP10 and CP15 registers, take the
lazy route and aim the register accesses at the AArch64 system register
table.

Patch 1 reroutes the CP15 registers into the AArch64 table, taking care
to immediately RAZ undefined ranges of registers. This is done to avoid
possibly conflicting with encodings for future AArch64 registers.

Patch 2 installs an exit handler for the CP10 ID registers and also
relies on the general AArch64 register handler to implement reads.

Finally, patch 3 actually sets TID3 for AArch32 guests, providing
known-safe values for feature register accesses.

Series applies cleanly to kvmarm/fixes at commit:

  8872d9b3e35a ("KVM: arm64: Drop unneeded minor version check from PSCI v1.x handler")

There is an argument that the series is in fact a bug fix for running
AArch32 VMs on heterogeneous systems. To that end, it could be
blamed/backported to when we first knew better:

  93390c0a1b20 ("arm64: KVM: Hide unsupported AArch64 CPU features from guests")

But I left that tag off as in the aforementioned change skipping
AArch32 was intentional.

Tested with AArch32 kvm-unit-tests and booting an AArch32 debian guest
on a Raspberry Pi 4.

v1: https://lore.kernel.org/kvmarm/20220329011301.1166265-1-oupton@google.com/

v1 -> v2:
 - Actually set TID3! Oops.
 - Refactor kvm_emulate_cp15_id_reg() to check preconditions before
   proceeding to emulation (Reiji)
 - Tighten up comment on kvm_is_cp15_id_reg() to indicate that the only
   other trapped ID register (CTR) is already handled in the cp15
   register table (Reiji)

Oliver Upton (3):
  KVM: arm64: Wire up CP15 feature registers to their AArch64
    equivalents
  KVM: arm64: Plumb cp10 ID traps through the AArch64 sysreg handler
  KVM: arm64: Start trapping ID registers for 32 bit guests

 arch/arm64/include/asm/kvm_arm.h     |   3 +-
 arch/arm64/include/asm/kvm_emulate.h |   8 --
 arch/arm64/include/asm/kvm_host.h    |   1 +
 arch/arm64/kvm/handle_exit.c         |   1 +
 arch/arm64/kvm/sys_regs.c            | 129 +++++++++++++++++++++++++++
 5 files changed, 133 insertions(+), 9 deletions(-)

-- 
2.35.1.1094.g7c7d902a7c-goog

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

[PATCH v2 0/3] KVM: arm64: Limit feature register reads from AArch32

[Oliver Upton]

KVM/arm64 does not restrict the guest's view of the AArch32 feature
registers when read from AArch32. HCR_EL2.TID3 is cleared for AArch32
guests, meaning that register reads come straight from hardware. This is
problematic as KVM relies on read_sanitised_ftr_reg() to expose a set of
features consistent for a particular system.

Appropriate handlers must first be put in place for CP10 and CP15 ID
register accesses before setting TID3. Rather than exhaustively
enumerating each of the encodings for CP10 and CP15 registers, take the
lazy route and aim the register accesses at the AArch64 system register
table.

Patch 1 reroutes the CP15 registers into the AArch64 table, taking care
to immediately RAZ undefined ranges of registers. This is done to avoid
possibly conflicting with encodings for future AArch64 registers.

Patch 2 installs an exit handler for the CP10 ID registers and also
relies on the general AArch64 register handler to implement reads.

Finally, patch 3 actually sets TID3 for AArch32 guests, providing
known-safe values for feature register accesses.

Series applies cleanly to kvmarm/fixes at commit:

  8872d9b3e35a ("KVM: arm64: Drop unneeded minor version check from PSCI v1.x handler")

There is an argument that the series is in fact a bug fix for running
AArch32 VMs on heterogeneous systems. To that end, it could be
blamed/backported to when we first knew better:

  93390c0a1b20 ("arm64: KVM: Hide unsupported AArch64 CPU features from guests")

But I left that tag off as in the aforementioned change skipping
AArch32 was intentional.

Tested with AArch32 kvm-unit-tests and booting an AArch32 debian guest
on a Raspberry Pi 4.

v1: https://lore.kernel.org/kvmarm/20220329011301.1166265-1-oupton@google.com/

v1 -> v2:
 - Actually set TID3! Oops.
 - Refactor kvm_emulate_cp15_id_reg() to check preconditions before
   proceeding to emulation (Reiji)
 - Tighten up comment on kvm_is_cp15_id_reg() to indicate that the only
   other trapped ID register (CTR) is already handled in the cp15
   register table (Reiji)

Oliver Upton (3):
  KVM: arm64: Wire up CP15 feature registers to their AArch64
    equivalents
  KVM: arm64: Plumb cp10 ID traps through the AArch64 sysreg handler
  KVM: arm64: Start trapping ID registers for 32 bit guests

 arch/arm64/include/asm/kvm_arm.h     |   3 +-
 arch/arm64/include/asm/kvm_emulate.h |   8 --
 arch/arm64/include/asm/kvm_host.h    |   1 +
 arch/arm64/kvm/handle_exit.c         |   1 +
 arch/arm64/kvm/sys_regs.c            | 129 +++++++++++++++++++++++++++
 5 files changed, 133 insertions(+), 9 deletions(-)

-- 
2.35.1.1094.g7c7d902a7c-goog


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

[PATCH v2 1/3] KVM: arm64: Wire up CP15 feature registers to their AArch64 equivalents

[Oliver Upton]

KVM currently does not trap ID register accesses from an AArch32 EL1.
This is painful for a couple of reasons. Certain unimplemented features
are visible to AArch32 EL1, as we limit PMU to version 3 and the debug
architecture to v8.0. Additionally, we attempt to paper over
heterogeneous systems by using register values that are safe
system-wide. All this hard work is completely sidestepped because KVM
does not set TID3 for AArch32 guests.

Fix up handling of CP15 feature registers by simply rerouting to their
AArch64 aliases. Punt setting HCR_EL2.TID3 to a later change, as we need
to fix up the oddball CP10 feature registers still.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 arch/arm64/kvm/sys_regs.c | 68 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index dd34b5ab51d4..8b791256a5b4 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -2339,6 +2339,67 @@ static int kvm_handle_cp_64(struct kvm_vcpu *vcpu,
 	return 1;
 }
 
+static int emulate_sys_reg(struct kvm_vcpu *vcpu, struct sys_reg_params *params);
+
+/**
+ * kvm_emulate_cp15_id_reg() - Handles an MRC trap on a guest CP15 access where
+ *			       CRn=0, which corresponds to the AArch32 feature
+ *			       registers.
+ * @vcpu: the vCPU pointer
+ * @params: the system register access parameters.
+ *
+ * Our cp15 system register tables do not enumerate the AArch32 feature
+ * registers. Conveniently, our AArch64 table does, and the AArch32 system
+ * register encoding can be trivially remapped into the AArch64 for the feature
+ * registers: Append op0=3, leaving op1, CRn, CRm, and op2 the same.
+ *
+ * According to DDI0487G.b G7.3.1, paragraph "Behavior of VMSAv8-32 32-bit
+ * System registers with (coproc=0b1111, CRn==c0)", read accesses from this
+ * range are either UNKNOWN or RES0. Rerouting remains architectural as we
+ * treat undefined registers in this range as RAZ.
+ */
+static int kvm_emulate_cp15_id_reg(struct kvm_vcpu *vcpu,
+				   struct sys_reg_params *params)
+{
+	int Rt = kvm_vcpu_sys_get_rt(vcpu);
+	int ret = 1;
+
+	/* Treat impossible writes to RO registers as UNDEFINED */
+	if (params->is_write) {
+		unhandled_cp_access(vcpu, params);
+		return 1;
+	}
+
+	params->Op0 = 3;
+
+	/*
+	 * All registers where CRm > 3 are known to be UNKNOWN/RAZ from AArch32.
+	 * Avoid conflicting with future expansion of AArch64 feature registers
+	 * and simply treat them as RAZ here.
+	 */
+	if (params->CRm > 3)
+		params->regval = 0;
+	else
+		ret = emulate_sys_reg(vcpu, params);
+
+	vcpu_set_reg(vcpu, Rt, params->regval);
+	return ret;
+}
+
+/**
+ * kvm_is_cp15_id_reg() - Returns true if the specified CP15 register is an
+ *			  AArch32 ID register.
+ * @params: the system register access parameters
+ *
+ * Note that CP15 ID registers where CRm=0 are excluded from this check. The
+ * only register trapped in the CRm=0 range is CTR, which is already handled in
+ * the cp15 register table.
+ */
+static inline bool kvm_is_cp15_id_reg(struct sys_reg_params *params)
+{
+	return params->CRn == 0 && params->Op1 == 0 && params->CRm != 0;
+}
+
 /**
  * kvm_handle_cp_32 -- handles a mrc/mcr trap on a guest CP14/CP15 access
  * @vcpu: The VCPU pointer
@@ -2360,6 +2421,13 @@ static int kvm_handle_cp_32(struct kvm_vcpu *vcpu,
 	params.Op1 = (esr >> 14) & 0x7;
 	params.Op2 = (esr >> 17) & 0x7;
 
+	/*
+	 * Certain AArch32 ID registers are handled by rerouting to the AArch64
+	 * system register table.
+	 */
+	if (ESR_ELx_EC(esr) == ESR_ELx_EC_CP15_32 && kvm_is_cp15_id_reg(&params))
+		return kvm_emulate_cp15_id_reg(vcpu, &params);
+
 	if (!emulate_cp(vcpu, &params, global, nr_global)) {
 		if (!params.is_write)
 			vcpu_set_reg(vcpu, Rt, params.regval);
-- 
2.35.1.1094.g7c7d902a7c-goog

[PATCH v2 1/3] KVM: arm64: Wire up CP15 feature registers to their AArch64 equivalents

[Oliver Upton]

KVM currently does not trap ID register accesses from an AArch32 EL1.
This is painful for a couple of reasons. Certain unimplemented features
are visible to AArch32 EL1, as we limit PMU to version 3 and the debug
architecture to v8.0. Additionally, we attempt to paper over
heterogeneous systems by using register values that are safe
system-wide. All this hard work is completely sidestepped because KVM
does not set TID3 for AArch32 guests.

Fix up handling of CP15 feature registers by simply rerouting to their
AArch64 aliases. Punt setting HCR_EL2.TID3 to a later change, as we need
to fix up the oddball CP10 feature registers still.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 arch/arm64/kvm/sys_regs.c | 68 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index dd34b5ab51d4..8b791256a5b4 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -2339,6 +2339,67 @@ static int kvm_handle_cp_64(struct kvm_vcpu *vcpu,
 	return 1;
 }
 
+static int emulate_sys_reg(struct kvm_vcpu *vcpu, struct sys_reg_params *params);
+
+/**
+ * kvm_emulate_cp15_id_reg() - Handles an MRC trap on a guest CP15 access where
+ *			       CRn=0, which corresponds to the AArch32 feature
+ *			       registers.
+ * @vcpu: the vCPU pointer
+ * @params: the system register access parameters.
+ *
+ * Our cp15 system register tables do not enumerate the AArch32 feature
+ * registers. Conveniently, our AArch64 table does, and the AArch32 system
+ * register encoding can be trivially remapped into the AArch64 for the feature
+ * registers: Append op0=3, leaving op1, CRn, CRm, and op2 the same.
+ *
+ * According to DDI0487G.b G7.3.1, paragraph "Behavior of VMSAv8-32 32-bit
+ * System registers with (coproc=0b1111, CRn==c0)", read accesses from this
+ * range are either UNKNOWN or RES0. Rerouting remains architectural as we
+ * treat undefined registers in this range as RAZ.
+ */
+static int kvm_emulate_cp15_id_reg(struct kvm_vcpu *vcpu,
+				   struct sys_reg_params *params)
+{
+	int Rt = kvm_vcpu_sys_get_rt(vcpu);
+	int ret = 1;
+
+	/* Treat impossible writes to RO registers as UNDEFINED */
+	if (params->is_write) {
+		unhandled_cp_access(vcpu, params);
+		return 1;
+	}
+
+	params->Op0 = 3;
+
+	/*
+	 * All registers where CRm > 3 are known to be UNKNOWN/RAZ from AArch32.
+	 * Avoid conflicting with future expansion of AArch64 feature registers
+	 * and simply treat them as RAZ here.
+	 */
+	if (params->CRm > 3)
+		params->regval = 0;
+	else
+		ret = emulate_sys_reg(vcpu, params);
+
+	vcpu_set_reg(vcpu, Rt, params->regval);
+	return ret;
+}
+
+/**
+ * kvm_is_cp15_id_reg() - Returns true if the specified CP15 register is an
+ *			  AArch32 ID register.
+ * @params: the system register access parameters
+ *
+ * Note that CP15 ID registers where CRm=0 are excluded from this check. The
+ * only register trapped in the CRm=0 range is CTR, which is already handled in
+ * the cp15 register table.
+ */
+static inline bool kvm_is_cp15_id_reg(struct sys_reg_params *params)
+{
+	return params->CRn == 0 && params->Op1 == 0 && params->CRm != 0;
+}
+
 /**
  * kvm_handle_cp_32 -- handles a mrc/mcr trap on a guest CP14/CP15 access
  * @vcpu: The VCPU pointer
@@ -2360,6 +2421,13 @@ static int kvm_handle_cp_32(struct kvm_vcpu *vcpu,
 	params.Op1 = (esr >> 14) & 0x7;
 	params.Op2 = (esr >> 17) & 0x7;
 
+	/*
+	 * Certain AArch32 ID registers are handled by rerouting to the AArch64
+	 * system register table.
+	 */
+	if (ESR_ELx_EC(esr) == ESR_ELx_EC_CP15_32 && kvm_is_cp15_id_reg(&params))
+		return kvm_emulate_cp15_id_reg(vcpu, &params);
+
 	if (!emulate_cp(vcpu, &params, global, nr_global)) {
 		if (!params.is_write)
 			vcpu_set_reg(vcpu, Rt, params.regval);
-- 
2.35.1.1094.g7c7d902a7c-goog

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

[PATCH v2 1/3] KVM: arm64: Wire up CP15 feature registers to their AArch64 equivalents

[Oliver Upton]

KVM currently does not trap ID register accesses from an AArch32 EL1.
This is painful for a couple of reasons. Certain unimplemented features
are visible to AArch32 EL1, as we limit PMU to version 3 and the debug
architecture to v8.0. Additionally, we attempt to paper over
heterogeneous systems by using register values that are safe
system-wide. All this hard work is completely sidestepped because KVM
does not set TID3 for AArch32 guests.

Fix up handling of CP15 feature registers by simply rerouting to their
AArch64 aliases. Punt setting HCR_EL2.TID3 to a later change, as we need
to fix up the oddball CP10 feature registers still.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 arch/arm64/kvm/sys_regs.c | 68 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index dd34b5ab51d4..8b791256a5b4 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -2339,6 +2339,67 @@ static int kvm_handle_cp_64(struct kvm_vcpu *vcpu,
 	return 1;
 }
 
+static int emulate_sys_reg(struct kvm_vcpu *vcpu, struct sys_reg_params *params);
+
+/**
+ * kvm_emulate_cp15_id_reg() - Handles an MRC trap on a guest CP15 access where
+ *			       CRn=0, which corresponds to the AArch32 feature
+ *			       registers.
+ * @vcpu: the vCPU pointer
+ * @params: the system register access parameters.
+ *
+ * Our cp15 system register tables do not enumerate the AArch32 feature
+ * registers. Conveniently, our AArch64 table does, and the AArch32 system
+ * register encoding can be trivially remapped into the AArch64 for the feature
+ * registers: Append op0=3, leaving op1, CRn, CRm, and op2 the same.
+ *
+ * According to DDI0487G.b G7.3.1, paragraph "Behavior of VMSAv8-32 32-bit
+ * System registers with (coproc=0b1111, CRn==c0)", read accesses from this
+ * range are either UNKNOWN or RES0. Rerouting remains architectural as we
+ * treat undefined registers in this range as RAZ.
+ */
+static int kvm_emulate_cp15_id_reg(struct kvm_vcpu *vcpu,
+				   struct sys_reg_params *params)
+{
+	int Rt = kvm_vcpu_sys_get_rt(vcpu);
+	int ret = 1;
+
+	/* Treat impossible writes to RO registers as UNDEFINED */
+	if (params->is_write) {
+		unhandled_cp_access(vcpu, params);
+		return 1;
+	}
+
+	params->Op0 = 3;
+
+	/*
+	 * All registers where CRm > 3 are known to be UNKNOWN/RAZ from AArch32.
+	 * Avoid conflicting with future expansion of AArch64 feature registers
+	 * and simply treat them as RAZ here.
+	 */
+	if (params->CRm > 3)
+		params->regval = 0;
+	else
+		ret = emulate_sys_reg(vcpu, params);
+
+	vcpu_set_reg(vcpu, Rt, params->regval);
+	return ret;
+}
+
+/**
+ * kvm_is_cp15_id_reg() - Returns true if the specified CP15 register is an
+ *			  AArch32 ID register.
+ * @params: the system register access parameters
+ *
+ * Note that CP15 ID registers where CRm=0 are excluded from this check. The
+ * only register trapped in the CRm=0 range is CTR, which is already handled in
+ * the cp15 register table.
+ */
+static inline bool kvm_is_cp15_id_reg(struct sys_reg_params *params)
+{
+	return params->CRn == 0 && params->Op1 == 0 && params->CRm != 0;
+}
+
 /**
  * kvm_handle_cp_32 -- handles a mrc/mcr trap on a guest CP14/CP15 access
  * @vcpu: The VCPU pointer
@@ -2360,6 +2421,13 @@ static int kvm_handle_cp_32(struct kvm_vcpu *vcpu,
 	params.Op1 = (esr >> 14) & 0x7;
 	params.Op2 = (esr >> 17) & 0x7;
 
+	/*
+	 * Certain AArch32 ID registers are handled by rerouting to the AArch64
+	 * system register table.
+	 */
+	if (ESR_ELx_EC(esr) == ESR_ELx_EC_CP15_32 && kvm_is_cp15_id_reg(&params))
+		return kvm_emulate_cp15_id_reg(vcpu, &params);
+
 	if (!emulate_cp(vcpu, &params, global, nr_global)) {
 		if (!params.is_write)
 			vcpu_set_reg(vcpu, Rt, params.regval);
-- 
2.35.1.1094.g7c7d902a7c-goog


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

[PATCH v2 2/3] KVM: arm64: Plumb cp10 ID traps through the AArch64 sysreg handler

[Oliver Upton]

In order to enable HCR_EL2.TID3 for AArch32 guests KVM needs to handle
traps where ESR_EL2.EC=0x8, which corresponds to an attempted VMRS
access from an ID group register. Specifically, the MVFR{0-2} registers
are accessed this way from AArch32. Conveniently, these registers are
architecturally mapped to MVFR{0-2}_EL1 in AArch64. Furthermore, KVM
already handles reads to these aliases in AArch64.

Plumb VMRS read traps through to the general AArch64 system register
handler.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 arch/arm64/include/asm/kvm_host.h |  1 +
 arch/arm64/kvm/handle_exit.c      |  1 +
 arch/arm64/kvm/sys_regs.c         | 61 +++++++++++++++++++++++++++++++
 3 files changed, 63 insertions(+)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 0e96087885fe..7a65ac268a22 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -673,6 +673,7 @@ int kvm_handle_cp14_64(struct kvm_vcpu *vcpu);
 int kvm_handle_cp15_32(struct kvm_vcpu *vcpu);
 int kvm_handle_cp15_64(struct kvm_vcpu *vcpu);
 int kvm_handle_sys_reg(struct kvm_vcpu *vcpu);
+int kvm_handle_cp10_id(struct kvm_vcpu *vcpu);
 
 void kvm_reset_sys_regs(struct kvm_vcpu *vcpu);
 
diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
index 97fe14aab1a3..5088a86ace5b 100644
--- a/arch/arm64/kvm/handle_exit.c
+++ b/arch/arm64/kvm/handle_exit.c
@@ -167,6 +167,7 @@ static exit_handle_fn arm_exit_handlers[] = {
 	[ESR_ELx_EC_CP15_64]	= kvm_handle_cp15_64,
 	[ESR_ELx_EC_CP14_MR]	= kvm_handle_cp14_32,
 	[ESR_ELx_EC_CP14_LS]	= kvm_handle_cp14_load_store,
+	[ESR_ELx_EC_CP10_ID]	= kvm_handle_cp10_id,
 	[ESR_ELx_EC_CP14_64]	= kvm_handle_cp14_64,
 	[ESR_ELx_EC_HVC32]	= handle_hvc,
 	[ESR_ELx_EC_SMC32]	= handle_smc,
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 8b791256a5b4..4863592d060d 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -2341,6 +2341,67 @@ static int kvm_handle_cp_64(struct kvm_vcpu *vcpu,
 
 static int emulate_sys_reg(struct kvm_vcpu *vcpu, struct sys_reg_params *params);
 
+/*
+ * The CP10 ID registers are architecturally mapped to AArch64 feature
+ * registers. Abuse that fact so we can rely on the AArch64 handler for accesses
+ * from AArch32.
+ */
+static bool kvm_esr_cp10_id_to_sys64(u32 esr, struct sys_reg_params *params)
+{
+	params->is_write = ((esr & 1) == 0);
+	params->Op0 = 3;
+	params->Op1 = 0;
+	params->CRn = 0;
+	params->CRm = 3;
+
+	switch ((esr >> 10) & 0xf) {
+	/* MVFR0 */
+	case 0b0111:
+		params->Op2 = 0;
+		break;
+	/* MVFR1 */
+	case 0b0110:
+		params->Op2 = 1;
+		break;
+	/* MVFR2 */
+	case 0b0101:
+		params->Op2 = 2;
+		break;
+	default:
+		return false;
+	}
+
+	return true;
+}
+
+/**
+ * kvm_handle_cp10_id() - Handles a VMRS trap on guest access to a 'Media and
+ *			  VFP Register' from AArch32.
+ * @vcpu: The vCPU pointer
+ *
+ * MVFR{0-2} are architecturally mapped to the AArch64 MVFR{0-2}_EL1 registers.
+ * Work out the correct AArch64 system register encoding and reroute to the
+ * AArch64 system register emulation.
+ */
+int kvm_handle_cp10_id(struct kvm_vcpu *vcpu)
+{
+	int Rt = kvm_vcpu_sys_get_rt(vcpu);
+	u32 esr = kvm_vcpu_get_esr(vcpu);
+	struct sys_reg_params params;
+	int ret;
+
+	/* UNDEF on any unhandled register or an attempted write */
+	if (!kvm_esr_cp10_id_to_sys64(esr, &params) || params.is_write) {
+		kvm_inject_undefined(vcpu);
+		return 1;
+	}
+
+	ret = emulate_sys_reg(vcpu, &params);
+
+	vcpu_set_reg(vcpu, Rt, params.regval);
+	return ret;
+}
+
 /**
  * kvm_emulate_cp15_id_reg() - Handles an MRC trap on a guest CP15 access where
  *			       CRn=0, which corresponds to the AArch32 feature
-- 
2.35.1.1094.g7c7d902a7c-goog

[PATCH v2 2/3] KVM: arm64: Plumb cp10 ID traps through the AArch64 sysreg handler

[Oliver Upton]

In order to enable HCR_EL2.TID3 for AArch32 guests KVM needs to handle
traps where ESR_EL2.EC=0x8, which corresponds to an attempted VMRS
access from an ID group register. Specifically, the MVFR{0-2} registers
are accessed this way from AArch32. Conveniently, these registers are
architecturally mapped to MVFR{0-2}_EL1 in AArch64. Furthermore, KVM
already handles reads to these aliases in AArch64.

Plumb VMRS read traps through to the general AArch64 system register
handler.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 arch/arm64/include/asm/kvm_host.h |  1 +
 arch/arm64/kvm/handle_exit.c      |  1 +
 arch/arm64/kvm/sys_regs.c         | 61 +++++++++++++++++++++++++++++++
 3 files changed, 63 insertions(+)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 0e96087885fe..7a65ac268a22 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -673,6 +673,7 @@ int kvm_handle_cp14_64(struct kvm_vcpu *vcpu);
 int kvm_handle_cp15_32(struct kvm_vcpu *vcpu);
 int kvm_handle_cp15_64(struct kvm_vcpu *vcpu);
 int kvm_handle_sys_reg(struct kvm_vcpu *vcpu);
+int kvm_handle_cp10_id(struct kvm_vcpu *vcpu);
 
 void kvm_reset_sys_regs(struct kvm_vcpu *vcpu);
 
diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
index 97fe14aab1a3..5088a86ace5b 100644
--- a/arch/arm64/kvm/handle_exit.c
+++ b/arch/arm64/kvm/handle_exit.c
@@ -167,6 +167,7 @@ static exit_handle_fn arm_exit_handlers[] = {
 	[ESR_ELx_EC_CP15_64]	= kvm_handle_cp15_64,
 	[ESR_ELx_EC_CP14_MR]	= kvm_handle_cp14_32,
 	[ESR_ELx_EC_CP14_LS]	= kvm_handle_cp14_load_store,
+	[ESR_ELx_EC_CP10_ID]	= kvm_handle_cp10_id,
 	[ESR_ELx_EC_CP14_64]	= kvm_handle_cp14_64,
 	[ESR_ELx_EC_HVC32]	= handle_hvc,
 	[ESR_ELx_EC_SMC32]	= handle_smc,
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 8b791256a5b4..4863592d060d 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -2341,6 +2341,67 @@ static int kvm_handle_cp_64(struct kvm_vcpu *vcpu,
 
 static int emulate_sys_reg(struct kvm_vcpu *vcpu, struct sys_reg_params *params);
 
+/*
+ * The CP10 ID registers are architecturally mapped to AArch64 feature
+ * registers. Abuse that fact so we can rely on the AArch64 handler for accesses
+ * from AArch32.
+ */
+static bool kvm_esr_cp10_id_to_sys64(u32 esr, struct sys_reg_params *params)
+{
+	params->is_write = ((esr & 1) == 0);
+	params->Op0 = 3;
+	params->Op1 = 0;
+	params->CRn = 0;
+	params->CRm = 3;
+
+	switch ((esr >> 10) & 0xf) {
+	/* MVFR0 */
+	case 0b0111:
+		params->Op2 = 0;
+		break;
+	/* MVFR1 */
+	case 0b0110:
+		params->Op2 = 1;
+		break;
+	/* MVFR2 */
+	case 0b0101:
+		params->Op2 = 2;
+		break;
+	default:
+		return false;
+	}
+
+	return true;
+}
+
+/**
+ * kvm_handle_cp10_id() - Handles a VMRS trap on guest access to a 'Media and
+ *			  VFP Register' from AArch32.
+ * @vcpu: The vCPU pointer
+ *
+ * MVFR{0-2} are architecturally mapped to the AArch64 MVFR{0-2}_EL1 registers.
+ * Work out the correct AArch64 system register encoding and reroute to the
+ * AArch64 system register emulation.
+ */
+int kvm_handle_cp10_id(struct kvm_vcpu *vcpu)
+{
+	int Rt = kvm_vcpu_sys_get_rt(vcpu);
+	u32 esr = kvm_vcpu_get_esr(vcpu);
+	struct sys_reg_params params;
+	int ret;
+
+	/* UNDEF on any unhandled register or an attempted write */
+	if (!kvm_esr_cp10_id_to_sys64(esr, &params) || params.is_write) {
+		kvm_inject_undefined(vcpu);
+		return 1;
+	}
+
+	ret = emulate_sys_reg(vcpu, &params);
+
+	vcpu_set_reg(vcpu, Rt, params.regval);
+	return ret;
+}
+
 /**
  * kvm_emulate_cp15_id_reg() - Handles an MRC trap on a guest CP15 access where
  *			       CRn=0, which corresponds to the AArch32 feature
-- 
2.35.1.1094.g7c7d902a7c-goog

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

[PATCH v2 2/3] KVM: arm64: Plumb cp10 ID traps through the AArch64 sysreg handler

[Oliver Upton]

In order to enable HCR_EL2.TID3 for AArch32 guests KVM needs to handle
traps where ESR_EL2.EC=0x8, which corresponds to an attempted VMRS
access from an ID group register. Specifically, the MVFR{0-2} registers
are accessed this way from AArch32. Conveniently, these registers are
architecturally mapped to MVFR{0-2}_EL1 in AArch64. Furthermore, KVM
already handles reads to these aliases in AArch64.

Plumb VMRS read traps through to the general AArch64 system register
handler.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 arch/arm64/include/asm/kvm_host.h |  1 +
 arch/arm64/kvm/handle_exit.c      |  1 +
 arch/arm64/kvm/sys_regs.c         | 61 +++++++++++++++++++++++++++++++
 3 files changed, 63 insertions(+)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 0e96087885fe..7a65ac268a22 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -673,6 +673,7 @@ int kvm_handle_cp14_64(struct kvm_vcpu *vcpu);
 int kvm_handle_cp15_32(struct kvm_vcpu *vcpu);
 int kvm_handle_cp15_64(struct kvm_vcpu *vcpu);
 int kvm_handle_sys_reg(struct kvm_vcpu *vcpu);
+int kvm_handle_cp10_id(struct kvm_vcpu *vcpu);
 
 void kvm_reset_sys_regs(struct kvm_vcpu *vcpu);
 
diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
index 97fe14aab1a3..5088a86ace5b 100644
--- a/arch/arm64/kvm/handle_exit.c
+++ b/arch/arm64/kvm/handle_exit.c
@@ -167,6 +167,7 @@ static exit_handle_fn arm_exit_handlers[] = {
 	[ESR_ELx_EC_CP15_64]	= kvm_handle_cp15_64,
 	[ESR_ELx_EC_CP14_MR]	= kvm_handle_cp14_32,
 	[ESR_ELx_EC_CP14_LS]	= kvm_handle_cp14_load_store,
+	[ESR_ELx_EC_CP10_ID]	= kvm_handle_cp10_id,
 	[ESR_ELx_EC_CP14_64]	= kvm_handle_cp14_64,
 	[ESR_ELx_EC_HVC32]	= handle_hvc,
 	[ESR_ELx_EC_SMC32]	= handle_smc,
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 8b791256a5b4..4863592d060d 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -2341,6 +2341,67 @@ static int kvm_handle_cp_64(struct kvm_vcpu *vcpu,
 
 static int emulate_sys_reg(struct kvm_vcpu *vcpu, struct sys_reg_params *params);
 
+/*
+ * The CP10 ID registers are architecturally mapped to AArch64 feature
+ * registers. Abuse that fact so we can rely on the AArch64 handler for accesses
+ * from AArch32.
+ */
+static bool kvm_esr_cp10_id_to_sys64(u32 esr, struct sys_reg_params *params)
+{
+	params->is_write = ((esr & 1) == 0);
+	params->Op0 = 3;
+	params->Op1 = 0;
+	params->CRn = 0;
+	params->CRm = 3;
+
+	switch ((esr >> 10) & 0xf) {
+	/* MVFR0 */
+	case 0b0111:
+		params->Op2 = 0;
+		break;
+	/* MVFR1 */
+	case 0b0110:
+		params->Op2 = 1;
+		break;
+	/* MVFR2 */
+	case 0b0101:
+		params->Op2 = 2;
+		break;
+	default:
+		return false;
+	}
+
+	return true;
+}
+
+/**
+ * kvm_handle_cp10_id() - Handles a VMRS trap on guest access to a 'Media and
+ *			  VFP Register' from AArch32.
+ * @vcpu: The vCPU pointer
+ *
+ * MVFR{0-2} are architecturally mapped to the AArch64 MVFR{0-2}_EL1 registers.
+ * Work out the correct AArch64 system register encoding and reroute to the
+ * AArch64 system register emulation.
+ */
+int kvm_handle_cp10_id(struct kvm_vcpu *vcpu)
+{
+	int Rt = kvm_vcpu_sys_get_rt(vcpu);
+	u32 esr = kvm_vcpu_get_esr(vcpu);
+	struct sys_reg_params params;
+	int ret;
+
+	/* UNDEF on any unhandled register or an attempted write */
+	if (!kvm_esr_cp10_id_to_sys64(esr, &params) || params.is_write) {
+		kvm_inject_undefined(vcpu);
+		return 1;
+	}
+
+	ret = emulate_sys_reg(vcpu, &params);
+
+	vcpu_set_reg(vcpu, Rt, params.regval);
+	return ret;
+}
+
 /**
  * kvm_emulate_cp15_id_reg() - Handles an MRC trap on a guest CP15 access where
  *			       CRn=0, which corresponds to the AArch32 feature
-- 
2.35.1.1094.g7c7d902a7c-goog


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

[PATCH v2 3/3] KVM: arm64: Start trapping ID registers for 32 bit guests

[Oliver Upton]

To date KVM has not trapped ID register accesses from AArch32, meaning
that guests get an unconstrained view of what hardware supports. This
can be a serious problem because we try to base the guest's feature
registers on values that are safe system-wide. Furthermore, KVM does not
implement the latest ISA in the PMU and Debug architecture, so we
constrain these fields to supported values.

Since KVM now correctly handles CP15 and CP10 register traps, we no
longer need to clear HCR_EL2.TID3 for 32 bit guests and will instead
emulate reads with their safe values.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 arch/arm64/include/asm/kvm_arm.h     | 3 ++-
 arch/arm64/include/asm/kvm_emulate.h | 8 --------
 2 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
index 01d47c5886dc..2fc2d995c10a 100644
--- a/arch/arm64/include/asm/kvm_arm.h
+++ b/arch/arm64/include/asm/kvm_arm.h
@@ -80,11 +80,12 @@
  * FMO:		Override CPSR.F and enable signaling with VF
  * SWIO:	Turn set/way invalidates into set/way clean+invalidate
  * PTW:		Take a stage2 fault if a stage1 walk steps in device memory
+ * TID3:	Trap EL1 reads of group 3 ID registers
  */
 #define HCR_GUEST_FLAGS (HCR_TSC | HCR_TSW | HCR_TWE | HCR_TWI | HCR_VM | \
 			 HCR_BSU_IS | HCR_FB | HCR_TACR | \
 			 HCR_AMO | HCR_SWIO | HCR_TIDCP | HCR_RW | HCR_TLOR | \
-			 HCR_FMO | HCR_IMO | HCR_PTW )
+			 HCR_FMO | HCR_IMO | HCR_PTW | HCR_TID3 )
 #define HCR_VIRT_EXCP_MASK (HCR_VSE | HCR_VI | HCR_VF)
 #define HCR_HOST_NVHE_FLAGS (HCR_RW | HCR_API | HCR_APK | HCR_ATA)
 #define HCR_HOST_NVHE_PROTECTED_FLAGS (HCR_HOST_NVHE_FLAGS | HCR_TSC)
diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
index d62405ce3e6d..fe32b4c8b35b 100644
--- a/arch/arm64/include/asm/kvm_emulate.h
+++ b/arch/arm64/include/asm/kvm_emulate.h
@@ -75,14 +75,6 @@ static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
 	if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features))
 		vcpu->arch.hcr_el2 &= ~HCR_RW;
 
-	/*
-	 * TID3: trap feature register accesses that we virtualise.
-	 * For now this is conditional, since no AArch32 feature regs
-	 * are currently virtualised.
-	 */
-	if (!vcpu_el1_is_32bit(vcpu))
-		vcpu->arch.hcr_el2 |= HCR_TID3;
-
 	if (cpus_have_const_cap(ARM64_MISMATCHED_CACHE_TYPE) ||
 	    vcpu_el1_is_32bit(vcpu))
 		vcpu->arch.hcr_el2 |= HCR_TID2;
-- 
2.35.1.1094.g7c7d902a7c-goog

[PATCH v2 3/3] KVM: arm64: Start trapping ID registers for 32 bit guests

[Oliver Upton]

To date KVM has not trapped ID register accesses from AArch32, meaning
that guests get an unconstrained view of what hardware supports. This
can be a serious problem because we try to base the guest's feature
registers on values that are safe system-wide. Furthermore, KVM does not
implement the latest ISA in the PMU and Debug architecture, so we
constrain these fields to supported values.

Since KVM now correctly handles CP15 and CP10 register traps, we no
longer need to clear HCR_EL2.TID3 for 32 bit guests and will instead
emulate reads with their safe values.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 arch/arm64/include/asm/kvm_arm.h     | 3 ++-
 arch/arm64/include/asm/kvm_emulate.h | 8 --------
 2 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
index 01d47c5886dc..2fc2d995c10a 100644
--- a/arch/arm64/include/asm/kvm_arm.h
+++ b/arch/arm64/include/asm/kvm_arm.h
@@ -80,11 +80,12 @@
  * FMO:		Override CPSR.F and enable signaling with VF
  * SWIO:	Turn set/way invalidates into set/way clean+invalidate
  * PTW:		Take a stage2 fault if a stage1 walk steps in device memory
+ * TID3:	Trap EL1 reads of group 3 ID registers
  */
 #define HCR_GUEST_FLAGS (HCR_TSC | HCR_TSW | HCR_TWE | HCR_TWI | HCR_VM | \
 			 HCR_BSU_IS | HCR_FB | HCR_TACR | \
 			 HCR_AMO | HCR_SWIO | HCR_TIDCP | HCR_RW | HCR_TLOR | \
-			 HCR_FMO | HCR_IMO | HCR_PTW )
+			 HCR_FMO | HCR_IMO | HCR_PTW | HCR_TID3 )
 #define HCR_VIRT_EXCP_MASK (HCR_VSE | HCR_VI | HCR_VF)
 #define HCR_HOST_NVHE_FLAGS (HCR_RW | HCR_API | HCR_APK | HCR_ATA)
 #define HCR_HOST_NVHE_PROTECTED_FLAGS (HCR_HOST_NVHE_FLAGS | HCR_TSC)
diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
index d62405ce3e6d..fe32b4c8b35b 100644
--- a/arch/arm64/include/asm/kvm_emulate.h
+++ b/arch/arm64/include/asm/kvm_emulate.h
@@ -75,14 +75,6 @@ static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
 	if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features))
 		vcpu->arch.hcr_el2 &= ~HCR_RW;
 
-	/*
-	 * TID3: trap feature register accesses that we virtualise.
-	 * For now this is conditional, since no AArch32 feature regs
-	 * are currently virtualised.
-	 */
-	if (!vcpu_el1_is_32bit(vcpu))
-		vcpu->arch.hcr_el2 |= HCR_TID3;
-
 	if (cpus_have_const_cap(ARM64_MISMATCHED_CACHE_TYPE) ||
 	    vcpu_el1_is_32bit(vcpu))
 		vcpu->arch.hcr_el2 |= HCR_TID2;
-- 
2.35.1.1094.g7c7d902a7c-goog

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

[PATCH v2 3/3] KVM: arm64: Start trapping ID registers for 32 bit guests

[Oliver Upton]

To date KVM has not trapped ID register accesses from AArch32, meaning
that guests get an unconstrained view of what hardware supports. This
can be a serious problem because we try to base the guest's feature
registers on values that are safe system-wide. Furthermore, KVM does not
implement the latest ISA in the PMU and Debug architecture, so we
constrain these fields to supported values.

Since KVM now correctly handles CP15 and CP10 register traps, we no
longer need to clear HCR_EL2.TID3 for 32 bit guests and will instead
emulate reads with their safe values.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 arch/arm64/include/asm/kvm_arm.h     | 3 ++-
 arch/arm64/include/asm/kvm_emulate.h | 8 --------
 2 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
index 01d47c5886dc..2fc2d995c10a 100644
--- a/arch/arm64/include/asm/kvm_arm.h
+++ b/arch/arm64/include/asm/kvm_arm.h
@@ -80,11 +80,12 @@
  * FMO:		Override CPSR.F and enable signaling with VF
  * SWIO:	Turn set/way invalidates into set/way clean+invalidate
  * PTW:		Take a stage2 fault if a stage1 walk steps in device memory
+ * TID3:	Trap EL1 reads of group 3 ID registers
  */
 #define HCR_GUEST_FLAGS (HCR_TSC | HCR_TSW | HCR_TWE | HCR_TWI | HCR_VM | \
 			 HCR_BSU_IS | HCR_FB | HCR_TACR | \
 			 HCR_AMO | HCR_SWIO | HCR_TIDCP | HCR_RW | HCR_TLOR | \
-			 HCR_FMO | HCR_IMO | HCR_PTW )
+			 HCR_FMO | HCR_IMO | HCR_PTW | HCR_TID3 )
 #define HCR_VIRT_EXCP_MASK (HCR_VSE | HCR_VI | HCR_VF)
 #define HCR_HOST_NVHE_FLAGS (HCR_RW | HCR_API | HCR_APK | HCR_ATA)
 #define HCR_HOST_NVHE_PROTECTED_FLAGS (HCR_HOST_NVHE_FLAGS | HCR_TSC)
diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
index d62405ce3e6d..fe32b4c8b35b 100644
--- a/arch/arm64/include/asm/kvm_emulate.h
+++ b/arch/arm64/include/asm/kvm_emulate.h
@@ -75,14 +75,6 @@ static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
 	if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features))
 		vcpu->arch.hcr_el2 &= ~HCR_RW;
 
-	/*
-	 * TID3: trap feature register accesses that we virtualise.
-	 * For now this is conditional, since no AArch32 feature regs
-	 * are currently virtualised.
-	 */
-	if (!vcpu_el1_is_32bit(vcpu))
-		vcpu->arch.hcr_el2 |= HCR_TID3;
-
 	if (cpus_have_const_cap(ARM64_MISMATCHED_CACHE_TYPE) ||
 	    vcpu_el1_is_32bit(vcpu))
 		vcpu->arch.hcr_el2 |= HCR_TID2;
-- 
2.35.1.1094.g7c7d902a7c-goog


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

Re: [PATCH v2 1/3] KVM: arm64: Wire up CP15 feature registers to their AArch64 equivalents

[Reiji Watanabe]

On Thu, Mar 31, 2022 at 6:08 PM Oliver Upton <oupton@google.com> wrote:
>
> KVM currently does not trap ID register accesses from an AArch32 EL1.
> This is painful for a couple of reasons. Certain unimplemented features
> are visible to AArch32 EL1, as we limit PMU to version 3 and the debug
> architecture to v8.0. Additionally, we attempt to paper over
> heterogeneous systems by using register values that are safe
> system-wide. All this hard work is completely sidestepped because KVM
> does not set TID3 for AArch32 guests.
>
> Fix up handling of CP15 feature registers by simply rerouting to their
> AArch64 aliases. Punt setting HCR_EL2.TID3 to a later change, as we need
> to fix up the oddball CP10 feature registers still.
>
> Signed-off-by: Oliver Upton <oupton@google.com>

Reviewed-by: Reiji Watanabe <reijiw@google.com>

Re: [PATCH v2 1/3] KVM: arm64: Wire up CP15 feature registers to their AArch64 equivalents

[Reiji Watanabe]

On Thu, Mar 31, 2022 at 6:08 PM Oliver Upton <oupton@google.com> wrote:
>
> KVM currently does not trap ID register accesses from an AArch32 EL1.
> This is painful for a couple of reasons. Certain unimplemented features
> are visible to AArch32 EL1, as we limit PMU to version 3 and the debug
> architecture to v8.0. Additionally, we attempt to paper over
> heterogeneous systems by using register values that are safe
> system-wide. All this hard work is completely sidestepped because KVM
> does not set TID3 for AArch32 guests.
>
> Fix up handling of CP15 feature registers by simply rerouting to their
> AArch64 aliases. Punt setting HCR_EL2.TID3 to a later change, as we need
> to fix up the oddball CP10 feature registers still.
>
> Signed-off-by: Oliver Upton <oupton@google.com>

Reviewed-by: Reiji Watanabe <reijiw@google.com>
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v2 1/3] KVM: arm64: Wire up CP15 feature registers to their AArch64 equivalents

[Reiji Watanabe]

On Thu, Mar 31, 2022 at 6:08 PM Oliver Upton <oupton@google.com> wrote:
>
> KVM currently does not trap ID register accesses from an AArch32 EL1.
> This is painful for a couple of reasons. Certain unimplemented features
> are visible to AArch32 EL1, as we limit PMU to version 3 and the debug
> architecture to v8.0. Additionally, we attempt to paper over
> heterogeneous systems by using register values that are safe
> system-wide. All this hard work is completely sidestepped because KVM
> does not set TID3 for AArch32 guests.
>
> Fix up handling of CP15 feature registers by simply rerouting to their
> AArch64 aliases. Punt setting HCR_EL2.TID3 to a later change, as we need
> to fix up the oddball CP10 feature registers still.
>
> Signed-off-by: Oliver Upton <oupton@google.com>

Reviewed-by: Reiji Watanabe <reijiw@google.com>

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

Re: [PATCH v2 2/3] KVM: arm64: Plumb cp10 ID traps through the AArch64 sysreg handler

[Reiji Watanabe]

On Thu, Mar 31, 2022 at 6:08 PM Oliver Upton <oupton@google.com> wrote:
>
> In order to enable HCR_EL2.TID3 for AArch32 guests KVM needs to handle
> traps where ESR_EL2.EC=0x8, which corresponds to an attempted VMRS
> access from an ID group register. Specifically, the MVFR{0-2} registers
> are accessed this way from AArch32. Conveniently, these registers are
> architecturally mapped to MVFR{0-2}_EL1 in AArch64. Furthermore, KVM
> already handles reads to these aliases in AArch64.
>
> Plumb VMRS read traps through to the general AArch64 system register
> handler.
>
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  arch/arm64/include/asm/kvm_host.h |  1 +
>  arch/arm64/kvm/handle_exit.c      |  1 +
>  arch/arm64/kvm/sys_regs.c         | 61 +++++++++++++++++++++++++++++++
>  3 files changed, 63 insertions(+)
>
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 0e96087885fe..7a65ac268a22 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -673,6 +673,7 @@ int kvm_handle_cp14_64(struct kvm_vcpu *vcpu);
>  int kvm_handle_cp15_32(struct kvm_vcpu *vcpu);
>  int kvm_handle_cp15_64(struct kvm_vcpu *vcpu);
>  int kvm_handle_sys_reg(struct kvm_vcpu *vcpu);
> +int kvm_handle_cp10_id(struct kvm_vcpu *vcpu);
>
>  void kvm_reset_sys_regs(struct kvm_vcpu *vcpu);
>
> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> index 97fe14aab1a3..5088a86ace5b 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
> @@ -167,6 +167,7 @@ static exit_handle_fn arm_exit_handlers[] = {
>         [ESR_ELx_EC_CP15_64]    = kvm_handle_cp15_64,
>         [ESR_ELx_EC_CP14_MR]    = kvm_handle_cp14_32,
>         [ESR_ELx_EC_CP14_LS]    = kvm_handle_cp14_load_store,
> +       [ESR_ELx_EC_CP10_ID]    = kvm_handle_cp10_id,
>         [ESR_ELx_EC_CP14_64]    = kvm_handle_cp14_64,
>         [ESR_ELx_EC_HVC32]      = handle_hvc,
>         [ESR_ELx_EC_SMC32]      = handle_smc,
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 8b791256a5b4..4863592d060d 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -2341,6 +2341,67 @@ static int kvm_handle_cp_64(struct kvm_vcpu *vcpu,
>
>  static int emulate_sys_reg(struct kvm_vcpu *vcpu, struct sys_reg_params *params);
>
> +/*
> + * The CP10 ID registers are architecturally mapped to AArch64 feature
> + * registers. Abuse that fact so we can rely on the AArch64 handler for accesses
> + * from AArch32.
> + */
> +static bool kvm_esr_cp10_id_to_sys64(u32 esr, struct sys_reg_params *params)
> +{
> +       params->is_write = ((esr & 1) == 0);
> +       params->Op0 = 3;
> +       params->Op1 = 0;
> +       params->CRn = 0;
> +       params->CRm = 3;
> +
> +       switch ((esr >> 10) & 0xf) {
> +       /* MVFR0 */
> +       case 0b0111:
> +               params->Op2 = 0;
> +               break;
> +       /* MVFR1 */
> +       case 0b0110:
> +               params->Op2 = 1;
> +               break;
> +       /* MVFR2 */
> +       case 0b0101:
> +               params->Op2 = 2;
> +               break;
> +       default:
> +               return false;
> +       }
> +
> +       return true;
> +}
> +
> +/**
> + * kvm_handle_cp10_id() - Handles a VMRS trap on guest access to a 'Media and
> + *                       VFP Register' from AArch32.
> + * @vcpu: The vCPU pointer
> + *
> + * MVFR{0-2} are architecturally mapped to the AArch64 MVFR{0-2}_EL1 registers.
> + * Work out the correct AArch64 system register encoding and reroute to the
> + * AArch64 system register emulation.
> + */
> +int kvm_handle_cp10_id(struct kvm_vcpu *vcpu)
> +{
> +       int Rt = kvm_vcpu_sys_get_rt(vcpu);
> +       u32 esr = kvm_vcpu_get_esr(vcpu);
> +       struct sys_reg_params params;
> +       int ret;
> +
> +       /* UNDEF on any unhandled register or an attempted write */
> +       if (!kvm_esr_cp10_id_to_sys64(esr, &params) || params.is_write) {
> +               kvm_inject_undefined(vcpu);

Nit: For debugging, it might be more useful to use unhandled_cp_access()
(, which needs to be changed to support ESR_ELx_EC_CP10_ID though)
rather than directly calling kvm_inject_undefined().

Reviewed-by: Reiji Watanabe <reijiw@google.com>

Thanks,
Reiji



> +               return 1;
> +       }
> +
> +       ret = emulate_sys_reg(vcpu, &params);
> +
> +       vcpu_set_reg(vcpu, Rt, params.regval);
> +       return ret;
> +}
> +
>  /**
>   * kvm_emulate_cp15_id_reg() - Handles an MRC trap on a guest CP15 access where
>   *                            CRn=0, which corresponds to the AArch32 feature
> --
> 2.35.1.1094.g7c7d902a7c-goog
>

Re: [PATCH v2 2/3] KVM: arm64: Plumb cp10 ID traps through the AArch64 sysreg handler

[Reiji Watanabe]

On Thu, Mar 31, 2022 at 6:08 PM Oliver Upton <oupton@google.com> wrote:
>
> In order to enable HCR_EL2.TID3 for AArch32 guests KVM needs to handle
> traps where ESR_EL2.EC=0x8, which corresponds to an attempted VMRS
> access from an ID group register. Specifically, the MVFR{0-2} registers
> are accessed this way from AArch32. Conveniently, these registers are
> architecturally mapped to MVFR{0-2}_EL1 in AArch64. Furthermore, KVM
> already handles reads to these aliases in AArch64.
>
> Plumb VMRS read traps through to the general AArch64 system register
> handler.
>
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  arch/arm64/include/asm/kvm_host.h |  1 +
>  arch/arm64/kvm/handle_exit.c      |  1 +
>  arch/arm64/kvm/sys_regs.c         | 61 +++++++++++++++++++++++++++++++
>  3 files changed, 63 insertions(+)
>
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 0e96087885fe..7a65ac268a22 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -673,6 +673,7 @@ int kvm_handle_cp14_64(struct kvm_vcpu *vcpu);
>  int kvm_handle_cp15_32(struct kvm_vcpu *vcpu);
>  int kvm_handle_cp15_64(struct kvm_vcpu *vcpu);
>  int kvm_handle_sys_reg(struct kvm_vcpu *vcpu);
> +int kvm_handle_cp10_id(struct kvm_vcpu *vcpu);
>
>  void kvm_reset_sys_regs(struct kvm_vcpu *vcpu);
>
> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> index 97fe14aab1a3..5088a86ace5b 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
> @@ -167,6 +167,7 @@ static exit_handle_fn arm_exit_handlers[] = {
>         [ESR_ELx_EC_CP15_64]    = kvm_handle_cp15_64,
>         [ESR_ELx_EC_CP14_MR]    = kvm_handle_cp14_32,
>         [ESR_ELx_EC_CP14_LS]    = kvm_handle_cp14_load_store,
> +       [ESR_ELx_EC_CP10_ID]    = kvm_handle_cp10_id,
>         [ESR_ELx_EC_CP14_64]    = kvm_handle_cp14_64,
>         [ESR_ELx_EC_HVC32]      = handle_hvc,
>         [ESR_ELx_EC_SMC32]      = handle_smc,
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 8b791256a5b4..4863592d060d 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -2341,6 +2341,67 @@ static int kvm_handle_cp_64(struct kvm_vcpu *vcpu,
>
>  static int emulate_sys_reg(struct kvm_vcpu *vcpu, struct sys_reg_params *params);
>
> +/*
> + * The CP10 ID registers are architecturally mapped to AArch64 feature
> + * registers. Abuse that fact so we can rely on the AArch64 handler for accesses
> + * from AArch32.
> + */
> +static bool kvm_esr_cp10_id_to_sys64(u32 esr, struct sys_reg_params *params)
> +{
> +       params->is_write = ((esr & 1) == 0);
> +       params->Op0 = 3;
> +       params->Op1 = 0;
> +       params->CRn = 0;
> +       params->CRm = 3;
> +
> +       switch ((esr >> 10) & 0xf) {
> +       /* MVFR0 */
> +       case 0b0111:
> +               params->Op2 = 0;
> +               break;
> +       /* MVFR1 */
> +       case 0b0110:
> +               params->Op2 = 1;
> +               break;
> +       /* MVFR2 */
> +       case 0b0101:
> +               params->Op2 = 2;
> +               break;
> +       default:
> +               return false;
> +       }
> +
> +       return true;
> +}
> +
> +/**
> + * kvm_handle_cp10_id() - Handles a VMRS trap on guest access to a 'Media and
> + *                       VFP Register' from AArch32.
> + * @vcpu: The vCPU pointer
> + *
> + * MVFR{0-2} are architecturally mapped to the AArch64 MVFR{0-2}_EL1 registers.
> + * Work out the correct AArch64 system register encoding and reroute to the
> + * AArch64 system register emulation.
> + */
> +int kvm_handle_cp10_id(struct kvm_vcpu *vcpu)
> +{
> +       int Rt = kvm_vcpu_sys_get_rt(vcpu);
> +       u32 esr = kvm_vcpu_get_esr(vcpu);
> +       struct sys_reg_params params;
> +       int ret;
> +
> +       /* UNDEF on any unhandled register or an attempted write */
> +       if (!kvm_esr_cp10_id_to_sys64(esr, &params) || params.is_write) {
> +               kvm_inject_undefined(vcpu);

Nit: For debugging, it might be more useful to use unhandled_cp_access()
(, which needs to be changed to support ESR_ELx_EC_CP10_ID though)
rather than directly calling kvm_inject_undefined().

Reviewed-by: Reiji Watanabe <reijiw@google.com>

Thanks,
Reiji



> +               return 1;
> +       }
> +
> +       ret = emulate_sys_reg(vcpu, &params);
> +
> +       vcpu_set_reg(vcpu, Rt, params.regval);
> +       return ret;
> +}
> +
>  /**
>   * kvm_emulate_cp15_id_reg() - Handles an MRC trap on a guest CP15 access where
>   *                            CRn=0, which corresponds to the AArch32 feature
> --
> 2.35.1.1094.g7c7d902a7c-goog
>
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v2 2/3] KVM: arm64: Plumb cp10 ID traps through the AArch64 sysreg handler

[Reiji Watanabe]

On Thu, Mar 31, 2022 at 6:08 PM Oliver Upton <oupton@google.com> wrote:
>
> In order to enable HCR_EL2.TID3 for AArch32 guests KVM needs to handle
> traps where ESR_EL2.EC=0x8, which corresponds to an attempted VMRS
> access from an ID group register. Specifically, the MVFR{0-2} registers
> are accessed this way from AArch32. Conveniently, these registers are
> architecturally mapped to MVFR{0-2}_EL1 in AArch64. Furthermore, KVM
> already handles reads to these aliases in AArch64.
>
> Plumb VMRS read traps through to the general AArch64 system register
> handler.
>
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  arch/arm64/include/asm/kvm_host.h |  1 +
>  arch/arm64/kvm/handle_exit.c      |  1 +
>  arch/arm64/kvm/sys_regs.c         | 61 +++++++++++++++++++++++++++++++
>  3 files changed, 63 insertions(+)
>
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 0e96087885fe..7a65ac268a22 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -673,6 +673,7 @@ int kvm_handle_cp14_64(struct kvm_vcpu *vcpu);
>  int kvm_handle_cp15_32(struct kvm_vcpu *vcpu);
>  int kvm_handle_cp15_64(struct kvm_vcpu *vcpu);
>  int kvm_handle_sys_reg(struct kvm_vcpu *vcpu);
> +int kvm_handle_cp10_id(struct kvm_vcpu *vcpu);
>
>  void kvm_reset_sys_regs(struct kvm_vcpu *vcpu);
>
> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> index 97fe14aab1a3..5088a86ace5b 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
> @@ -167,6 +167,7 @@ static exit_handle_fn arm_exit_handlers[] = {
>         [ESR_ELx_EC_CP15_64]    = kvm_handle_cp15_64,
>         [ESR_ELx_EC_CP14_MR]    = kvm_handle_cp14_32,
>         [ESR_ELx_EC_CP14_LS]    = kvm_handle_cp14_load_store,
> +       [ESR_ELx_EC_CP10_ID]    = kvm_handle_cp10_id,
>         [ESR_ELx_EC_CP14_64]    = kvm_handle_cp14_64,
>         [ESR_ELx_EC_HVC32]      = handle_hvc,
>         [ESR_ELx_EC_SMC32]      = handle_smc,
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 8b791256a5b4..4863592d060d 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -2341,6 +2341,67 @@ static int kvm_handle_cp_64(struct kvm_vcpu *vcpu,
>
>  static int emulate_sys_reg(struct kvm_vcpu *vcpu, struct sys_reg_params *params);
>
> +/*
> + * The CP10 ID registers are architecturally mapped to AArch64 feature
> + * registers. Abuse that fact so we can rely on the AArch64 handler for accesses
> + * from AArch32.
> + */
> +static bool kvm_esr_cp10_id_to_sys64(u32 esr, struct sys_reg_params *params)
> +{
> +       params->is_write = ((esr & 1) == 0);
> +       params->Op0 = 3;
> +       params->Op1 = 0;
> +       params->CRn = 0;
> +       params->CRm = 3;
> +
> +       switch ((esr >> 10) & 0xf) {
> +       /* MVFR0 */
> +       case 0b0111:
> +               params->Op2 = 0;
> +               break;
> +       /* MVFR1 */
> +       case 0b0110:
> +               params->Op2 = 1;
> +               break;
> +       /* MVFR2 */
> +       case 0b0101:
> +               params->Op2 = 2;
> +               break;
> +       default:
> +               return false;
> +       }
> +
> +       return true;
> +}
> +
> +/**
> + * kvm_handle_cp10_id() - Handles a VMRS trap on guest access to a 'Media and
> + *                       VFP Register' from AArch32.
> + * @vcpu: The vCPU pointer
> + *
> + * MVFR{0-2} are architecturally mapped to the AArch64 MVFR{0-2}_EL1 registers.
> + * Work out the correct AArch64 system register encoding and reroute to the
> + * AArch64 system register emulation.
> + */
> +int kvm_handle_cp10_id(struct kvm_vcpu *vcpu)
> +{
> +       int Rt = kvm_vcpu_sys_get_rt(vcpu);
> +       u32 esr = kvm_vcpu_get_esr(vcpu);
> +       struct sys_reg_params params;
> +       int ret;
> +
> +       /* UNDEF on any unhandled register or an attempted write */
> +       if (!kvm_esr_cp10_id_to_sys64(esr, &params) || params.is_write) {
> +               kvm_inject_undefined(vcpu);

Nit: For debugging, it might be more useful to use unhandled_cp_access()
(, which needs to be changed to support ESR_ELx_EC_CP10_ID though)
rather than directly calling kvm_inject_undefined().

Reviewed-by: Reiji Watanabe <reijiw@google.com>

Thanks,
Reiji



> +               return 1;
> +       }
> +
> +       ret = emulate_sys_reg(vcpu, &params);
> +
> +       vcpu_set_reg(vcpu, Rt, params.regval);
> +       return ret;
> +}
> +
>  /**
>   * kvm_emulate_cp15_id_reg() - Handles an MRC trap on a guest CP15 access where
>   *                            CRn=0, which corresponds to the AArch32 feature
> --
> 2.35.1.1094.g7c7d902a7c-goog
>

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

Re: [PATCH v2 3/3] KVM: arm64: Start trapping ID registers for 32 bit guests

[Reiji Watanabe]

On Thu, Mar 31, 2022 at 6:08 PM Oliver Upton <oupton@google.com> wrote:
>
> To date KVM has not trapped ID register accesses from AArch32, meaning
> that guests get an unconstrained view of what hardware supports. This
> can be a serious problem because we try to base the guest's feature
> registers on values that are safe system-wide. Furthermore, KVM does not
> implement the latest ISA in the PMU and Debug architecture, so we
> constrain these fields to supported values.
>
> Since KVM now correctly handles CP15 and CP10 register traps, we no
> longer need to clear HCR_EL2.TID3 for 32 bit guests and will instead
> emulate reads with their safe values.
>
> Signed-off-by: Oliver Upton <oupton@google.com>

Reviewed-by: Reiji Watanabe <reijiw@google.com>

BTW, due to this, on a system that supports PMUv3, ID_DFR0_E1 value will
become 0 for the aarch32 guest without PMUv3. This is the correct behavior,
but it affects migration.  I'm not sure how much we should care about
migration of the aarch32 guest though (and it will be resolved once ID
registers become configurable anyway).

Thanks,
Reiji

Re: [PATCH v2 3/3] KVM: arm64: Start trapping ID registers for 32 bit guests

[Reiji Watanabe]

On Thu, Mar 31, 2022 at 6:08 PM Oliver Upton <oupton@google.com> wrote:
>
> To date KVM has not trapped ID register accesses from AArch32, meaning
> that guests get an unconstrained view of what hardware supports. This
> can be a serious problem because we try to base the guest's feature
> registers on values that are safe system-wide. Furthermore, KVM does not
> implement the latest ISA in the PMU and Debug architecture, so we
> constrain these fields to supported values.
>
> Since KVM now correctly handles CP15 and CP10 register traps, we no
> longer need to clear HCR_EL2.TID3 for 32 bit guests and will instead
> emulate reads with their safe values.
>
> Signed-off-by: Oliver Upton <oupton@google.com>

Reviewed-by: Reiji Watanabe <reijiw@google.com>

BTW, due to this, on a system that supports PMUv3, ID_DFR0_E1 value will
become 0 for the aarch32 guest without PMUv3. This is the correct behavior,
but it affects migration.  I'm not sure how much we should care about
migration of the aarch32 guest though (and it will be resolved once ID
registers become configurable anyway).

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

Re: [PATCH v2 3/3] KVM: arm64: Start trapping ID registers for 32 bit guests

[Reiji Watanabe]

On Thu, Mar 31, 2022 at 6:08 PM Oliver Upton <oupton@google.com> wrote:
>
> To date KVM has not trapped ID register accesses from AArch32, meaning
> that guests get an unconstrained view of what hardware supports. This
> can be a serious problem because we try to base the guest's feature
> registers on values that are safe system-wide. Furthermore, KVM does not
> implement the latest ISA in the PMU and Debug architecture, so we
> constrain these fields to supported values.
>
> Since KVM now correctly handles CP15 and CP10 register traps, we no
> longer need to clear HCR_EL2.TID3 for 32 bit guests and will instead
> emulate reads with their safe values.
>
> Signed-off-by: Oliver Upton <oupton@google.com>

Reviewed-by: Reiji Watanabe <reijiw@google.com>

BTW, due to this, on a system that supports PMUv3, ID_DFR0_E1 value will
become 0 for the aarch32 guest without PMUv3. This is the correct behavior,
but it affects migration.  I'm not sure how much we should care about
migration of the aarch32 guest though (and it will be resolved once ID
registers become configurable anyway).

Thanks,
Reiji

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

Re: [PATCH v2 2/3] KVM: arm64: Plumb cp10 ID traps through the AArch64 sysreg handler

[Oliver Upton]

Hi Reiji,

On Sun, Apr 03, 2022 at 08:57:47PM -0700, Reiji Watanabe wrote:
> > +int kvm_handle_cp10_id(struct kvm_vcpu *vcpu)
> > +{
> > +       int Rt = kvm_vcpu_sys_get_rt(vcpu);
> > +       u32 esr = kvm_vcpu_get_esr(vcpu);
> > +       struct sys_reg_params params;
> > +       int ret;
> > +
> > +       /* UNDEF on any unhandled register or an attempted write */
> > +       if (!kvm_esr_cp10_id_to_sys64(esr, &params) || params.is_write) {
> > +               kvm_inject_undefined(vcpu);
> 
> Nit: For debugging, it might be more useful to use unhandled_cp_access()
> (, which needs to be changed to support ESR_ELx_EC_CP10_ID though)
> rather than directly calling kvm_inject_undefined().

A very worthy nit, you spotted my laziness in shunting straight to
kvm_inject_undefined() :)

Thinking about this a bit more deeply, this code should be dead. The
only time either of these conditions would happen is on a broken
implementation. Probably should still handle it gracefully in case the
CP10 handling in KVM becomes (or is in my own patch!) busted.

> Reviewed-by: Reiji Watanabe <reijiw@google.com>

Appreciated!

--
Thanks,
Oliver

Re: [PATCH v2 2/3] KVM: arm64: Plumb cp10 ID traps through the AArch64 sysreg handler

[Oliver Upton]

Hi Reiji,

On Sun, Apr 03, 2022 at 08:57:47PM -0700, Reiji Watanabe wrote:
> > +int kvm_handle_cp10_id(struct kvm_vcpu *vcpu)
> > +{
> > +       int Rt = kvm_vcpu_sys_get_rt(vcpu);
> > +       u32 esr = kvm_vcpu_get_esr(vcpu);
> > +       struct sys_reg_params params;
> > +       int ret;
> > +
> > +       /* UNDEF on any unhandled register or an attempted write */
> > +       if (!kvm_esr_cp10_id_to_sys64(esr, &params) || params.is_write) {
> > +               kvm_inject_undefined(vcpu);
> 
> Nit: For debugging, it might be more useful to use unhandled_cp_access()
> (, which needs to be changed to support ESR_ELx_EC_CP10_ID though)
> rather than directly calling kvm_inject_undefined().

A very worthy nit, you spotted my laziness in shunting straight to
kvm_inject_undefined() :)

Thinking about this a bit more deeply, this code should be dead. The
only time either of these conditions would happen is on a broken
implementation. Probably should still handle it gracefully in case the
CP10 handling in KVM becomes (or is in my own patch!) busted.

> Reviewed-by: Reiji Watanabe <reijiw@google.com>

Appreciated!

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

Re: [PATCH v2 2/3] KVM: arm64: Plumb cp10 ID traps through the AArch64 sysreg handler

[Oliver Upton]

Hi Reiji,

On Sun, Apr 03, 2022 at 08:57:47PM -0700, Reiji Watanabe wrote:
> > +int kvm_handle_cp10_id(struct kvm_vcpu *vcpu)
> > +{
> > +       int Rt = kvm_vcpu_sys_get_rt(vcpu);
> > +       u32 esr = kvm_vcpu_get_esr(vcpu);
> > +       struct sys_reg_params params;
> > +       int ret;
> > +
> > +       /* UNDEF on any unhandled register or an attempted write */
> > +       if (!kvm_esr_cp10_id_to_sys64(esr, &params) || params.is_write) {
> > +               kvm_inject_undefined(vcpu);
> 
> Nit: For debugging, it might be more useful to use unhandled_cp_access()
> (, which needs to be changed to support ESR_ELx_EC_CP10_ID though)
> rather than directly calling kvm_inject_undefined().

A very worthy nit, you spotted my laziness in shunting straight to
kvm_inject_undefined() :)

Thinking about this a bit more deeply, this code should be dead. The
only time either of these conditions would happen is on a broken
implementation. Probably should still handle it gracefully in case the
CP10 handling in KVM becomes (or is in my own patch!) busted.

> Reviewed-by: Reiji Watanabe <reijiw@google.com>

Appreciated!

--
Thanks,
Oliver

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

Re: [PATCH v2 3/3] KVM: arm64: Start trapping ID registers for 32 bit guests

[Oliver Upton]

Hi Reiji,

On Sun, Apr 03, 2022 at 09:45:15PM -0700, Reiji Watanabe wrote:
> On Thu, Mar 31, 2022 at 6:08 PM Oliver Upton <oupton@google.com> wrote:
> >
> > To date KVM has not trapped ID register accesses from AArch32, meaning
> > that guests get an unconstrained view of what hardware supports. This
> > can be a serious problem because we try to base the guest's feature
> > registers on values that are safe system-wide. Furthermore, KVM does not
> > implement the latest ISA in the PMU and Debug architecture, so we
> > constrain these fields to supported values.
> >
> > Since KVM now correctly handles CP15 and CP10 register traps, we no
> > longer need to clear HCR_EL2.TID3 for 32 bit guests and will instead
> > emulate reads with their safe values.
> >
> > Signed-off-by: Oliver Upton <oupton@google.com>
> 
> Reviewed-by: Reiji Watanabe <reijiw@google.com>
> 
> BTW, due to this, on a system that supports PMUv3, ID_DFR0_E1 value will
> become 0 for the aarch32 guest without PMUv3. This is the correct behavior,
> but it affects migration.  I'm not sure how much we should care about
> migration of the aarch32 guest though (and it will be resolved once ID
> registers become configurable anyway).

I believe userspace has been accessing the sanitised values of these
feature registers the entire time, so we should be OK on the UAPI side.

From the guest's perspective, I don't believe there is a meaningful
change. Even if the guest were to believe the value it sees in
ID_DFR0.PerfMon, it'll crash and burn on the first attempt to poke a PMU
register as we synthesize an UNDEF, right? At least now we cover our
tracks and ensure the vCPU correctly identifies itself to the guest.

This is, of course, unless I missed something painfully obvious :)

--
Thanks,
Oliver

Re: [PATCH v2 3/3] KVM: arm64: Start trapping ID registers for 32 bit guests

[Oliver Upton]

Hi Reiji,

On Sun, Apr 03, 2022 at 09:45:15PM -0700, Reiji Watanabe wrote:
> On Thu, Mar 31, 2022 at 6:08 PM Oliver Upton <oupton@google.com> wrote:
> >
> > To date KVM has not trapped ID register accesses from AArch32, meaning
> > that guests get an unconstrained view of what hardware supports. This
> > can be a serious problem because we try to base the guest's feature
> > registers on values that are safe system-wide. Furthermore, KVM does not
> > implement the latest ISA in the PMU and Debug architecture, so we
> > constrain these fields to supported values.
> >
> > Since KVM now correctly handles CP15 and CP10 register traps, we no
> > longer need to clear HCR_EL2.TID3 for 32 bit guests and will instead
> > emulate reads with their safe values.
> >
> > Signed-off-by: Oliver Upton <oupton@google.com>
> 
> Reviewed-by: Reiji Watanabe <reijiw@google.com>
> 
> BTW, due to this, on a system that supports PMUv3, ID_DFR0_E1 value will
> become 0 for the aarch32 guest without PMUv3. This is the correct behavior,
> but it affects migration.  I'm not sure how much we should care about
> migration of the aarch32 guest though (and it will be resolved once ID
> registers become configurable anyway).

I believe userspace has been accessing the sanitised values of these
feature registers the entire time, so we should be OK on the UAPI side.

From the guest's perspective, I don't believe there is a meaningful
change. Even if the guest were to believe the value it sees in
ID_DFR0.PerfMon, it'll crash and burn on the first attempt to poke a PMU
register as we synthesize an UNDEF, right? At least now we cover our
tracks and ensure the vCPU correctly identifies itself to the guest.

This is, of course, unless I missed something painfully obvious :)

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

Re: [PATCH v2 3/3] KVM: arm64: Start trapping ID registers for 32 bit guests

[Oliver Upton]

Hi Reiji,

On Sun, Apr 03, 2022 at 09:45:15PM -0700, Reiji Watanabe wrote:
> On Thu, Mar 31, 2022 at 6:08 PM Oliver Upton <oupton@google.com> wrote:
> >
> > To date KVM has not trapped ID register accesses from AArch32, meaning
> > that guests get an unconstrained view of what hardware supports. This
> > can be a serious problem because we try to base the guest's feature
> > registers on values that are safe system-wide. Furthermore, KVM does not
> > implement the latest ISA in the PMU and Debug architecture, so we
> > constrain these fields to supported values.
> >
> > Since KVM now correctly handles CP15 and CP10 register traps, we no
> > longer need to clear HCR_EL2.TID3 for 32 bit guests and will instead
> > emulate reads with their safe values.
> >
> > Signed-off-by: Oliver Upton <oupton@google.com>
> 
> Reviewed-by: Reiji Watanabe <reijiw@google.com>
> 
> BTW, due to this, on a system that supports PMUv3, ID_DFR0_E1 value will
> become 0 for the aarch32 guest without PMUv3. This is the correct behavior,
> but it affects migration.  I'm not sure how much we should care about
> migration of the aarch32 guest though (and it will be resolved once ID
> registers become configurable anyway).

I believe userspace has been accessing the sanitised values of these
feature registers the entire time, so we should be OK on the UAPI side.

From the guest's perspective, I don't believe there is a meaningful
change. Even if the guest were to believe the value it sees in
ID_DFR0.PerfMon, it'll crash and burn on the first attempt to poke a PMU
register as we synthesize an UNDEF, right? At least now we cover our
tracks and ensure the vCPU correctly identifies itself to the guest.

This is, of course, unless I missed something painfully obvious :)

--
Thanks,
Oliver

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

Re: [PATCH v2 2/3] KVM: arm64: Plumb cp10 ID traps through the AArch64 sysreg handler

[Oliver Upton]

On Mon, Apr 04, 2022 at 05:28:33AM +0000, Oliver Upton wrote:
> Hi Reiji,
> 
> On Sun, Apr 03, 2022 at 08:57:47PM -0700, Reiji Watanabe wrote:
> > > +int kvm_handle_cp10_id(struct kvm_vcpu *vcpu)
> > > +{
> > > +       int Rt = kvm_vcpu_sys_get_rt(vcpu);
> > > +       u32 esr = kvm_vcpu_get_esr(vcpu);
> > > +       struct sys_reg_params params;
> > > +       int ret;
> > > +
> > > +       /* UNDEF on any unhandled register or an attempted write */
> > > +       if (!kvm_esr_cp10_id_to_sys64(esr, &params) || params.is_write) {
> > > +               kvm_inject_undefined(vcpu);
> > 
> > Nit: For debugging, it might be more useful to use unhandled_cp_access()
> > (, which needs to be changed to support ESR_ELx_EC_CP10_ID though)
> > rather than directly calling kvm_inject_undefined().
> 
> A very worthy nit, you spotted my laziness in shunting straight to
> kvm_inject_undefined() :)
> 
> Thinking about this a bit more deeply, this code should be dead. The
> only time either of these conditions would happen is on a broken
> implementation. Probably should still handle it gracefully in case the
> CP10 handling in KVM becomes (or is in my own patch!) busted.

Actually, on second thought: any objections to leaving this as-is?
kvm_esr_cp10_id_to_sys64() spits out sys_reg_params that point at the
MRS alias for the VMRS register. Even if that call succeeds, the params
that get printed out by unhandled_cp_access() do not match the actual
register the guest was accessing. And if the call fails, ->Op2 is
uninitialized.

Sorry for backtracking here.

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

Re: [PATCH v2 2/3] KVM: arm64: Plumb cp10 ID traps through the AArch64 sysreg handler

[Oliver Upton]

On Mon, Apr 04, 2022 at 05:28:33AM +0000, Oliver Upton wrote:
> Hi Reiji,
> 
> On Sun, Apr 03, 2022 at 08:57:47PM -0700, Reiji Watanabe wrote:
> > > +int kvm_handle_cp10_id(struct kvm_vcpu *vcpu)
> > > +{
> > > +       int Rt = kvm_vcpu_sys_get_rt(vcpu);
> > > +       u32 esr = kvm_vcpu_get_esr(vcpu);
> > > +       struct sys_reg_params params;
> > > +       int ret;
> > > +
> > > +       /* UNDEF on any unhandled register or an attempted write */
> > > +       if (!kvm_esr_cp10_id_to_sys64(esr, &params) || params.is_write) {
> > > +               kvm_inject_undefined(vcpu);
> > 
> > Nit: For debugging, it might be more useful to use unhandled_cp_access()
> > (, which needs to be changed to support ESR_ELx_EC_CP10_ID though)
> > rather than directly calling kvm_inject_undefined().
> 
> A very worthy nit, you spotted my laziness in shunting straight to
> kvm_inject_undefined() :)
> 
> Thinking about this a bit more deeply, this code should be dead. The
> only time either of these conditions would happen is on a broken
> implementation. Probably should still handle it gracefully in case the
> CP10 handling in KVM becomes (or is in my own patch!) busted.

Actually, on second thought: any objections to leaving this as-is?
kvm_esr_cp10_id_to_sys64() spits out sys_reg_params that point at the
MRS alias for the VMRS register. Even if that call succeeds, the params
that get printed out by unhandled_cp_access() do not match the actual
register the guest was accessing. And if the call fails, ->Op2 is
uninitialized.

Sorry for backtracking here.

--
Thanks,
Oliver

Re: [PATCH v2 2/3] KVM: arm64: Plumb cp10 ID traps through the AArch64 sysreg handler

[Oliver Upton]

On Mon, Apr 04, 2022 at 05:28:33AM +0000, Oliver Upton wrote:
> Hi Reiji,
> 
> On Sun, Apr 03, 2022 at 08:57:47PM -0700, Reiji Watanabe wrote:
> > > +int kvm_handle_cp10_id(struct kvm_vcpu *vcpu)
> > > +{
> > > +       int Rt = kvm_vcpu_sys_get_rt(vcpu);
> > > +       u32 esr = kvm_vcpu_get_esr(vcpu);
> > > +       struct sys_reg_params params;
> > > +       int ret;
> > > +
> > > +       /* UNDEF on any unhandled register or an attempted write */
> > > +       if (!kvm_esr_cp10_id_to_sys64(esr, &params) || params.is_write) {
> > > +               kvm_inject_undefined(vcpu);
> > 
> > Nit: For debugging, it might be more useful to use unhandled_cp_access()
> > (, which needs to be changed to support ESR_ELx_EC_CP10_ID though)
> > rather than directly calling kvm_inject_undefined().
> 
> A very worthy nit, you spotted my laziness in shunting straight to
> kvm_inject_undefined() :)
> 
> Thinking about this a bit more deeply, this code should be dead. The
> only time either of these conditions would happen is on a broken
> implementation. Probably should still handle it gracefully in case the
> CP10 handling in KVM becomes (or is in my own patch!) busted.

Actually, on second thought: any objections to leaving this as-is?
kvm_esr_cp10_id_to_sys64() spits out sys_reg_params that point at the
MRS alias for the VMRS register. Even if that call succeeds, the params
that get printed out by unhandled_cp_access() do not match the actual
register the guest was accessing. And if the call fails, ->Op2 is
uninitialized.

Sorry for backtracking here.

--
Thanks,
Oliver

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

Re: [PATCH v2 2/3] KVM: arm64: Plumb cp10 ID traps through the AArch64 sysreg handler

[Reiji Watanabe]

Hi Oliver,

On Mon, Apr 4, 2022 at 4:19 PM Oliver Upton <oupton@google.com> wrote:
>
> On Mon, Apr 04, 2022 at 05:28:33AM +0000, Oliver Upton wrote:
> > Hi Reiji,
> >
> > On Sun, Apr 03, 2022 at 08:57:47PM -0700, Reiji Watanabe wrote:
> > > > +int kvm_handle_cp10_id(struct kvm_vcpu *vcpu)
> > > > +{
> > > > +       int Rt = kvm_vcpu_sys_get_rt(vcpu);
> > > > +       u32 esr = kvm_vcpu_get_esr(vcpu);
> > > > +       struct sys_reg_params params;
> > > > +       int ret;
> > > > +
> > > > +       /* UNDEF on any unhandled register or an attempted write */
> > > > +       if (!kvm_esr_cp10_id_to_sys64(esr, &params) || params.is_write) {
> > > > +               kvm_inject_undefined(vcpu);
> > >
> > > Nit: For debugging, it might be more useful to use unhandled_cp_access()
> > > (, which needs to be changed to support ESR_ELx_EC_CP10_ID though)
> > > rather than directly calling kvm_inject_undefined().
> >
> > A very worthy nit, you spotted my laziness in shunting straight to
> > kvm_inject_undefined() :)
> >
> > Thinking about this a bit more deeply, this code should be dead. The
> > only time either of these conditions would happen is on a broken
> > implementation. Probably should still handle it gracefully in case the
> > CP10 handling in KVM becomes (or is in my own patch!) busted.
>
> Actually, on second thought: any objections to leaving this as-is?
> kvm_esr_cp10_id_to_sys64() spits out sys_reg_params that point at the
> MRS alias for the VMRS register. Even if that call succeeds, the params
> that get printed out by unhandled_cp_access() do not match the actual
> register the guest was accessing. And if the call fails, ->Op2 is
> uninitialized.

I understand a few additional changes are needed for this.
Or simply use WARN_ON_ONCE ? (since this cannot be created by
the guest but by a KVM or CPU problem)

I'm fine with the current code as-is though:)

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

Re: [PATCH v2 2/3] KVM: arm64: Plumb cp10 ID traps through the AArch64 sysreg handler

[Reiji Watanabe]

Hi Oliver,

On Mon, Apr 4, 2022 at 4:19 PM Oliver Upton <oupton@google.com> wrote:
>
> On Mon, Apr 04, 2022 at 05:28:33AM +0000, Oliver Upton wrote:
> > Hi Reiji,
> >
> > On Sun, Apr 03, 2022 at 08:57:47PM -0700, Reiji Watanabe wrote:
> > > > +int kvm_handle_cp10_id(struct kvm_vcpu *vcpu)
> > > > +{
> > > > +       int Rt = kvm_vcpu_sys_get_rt(vcpu);
> > > > +       u32 esr = kvm_vcpu_get_esr(vcpu);
> > > > +       struct sys_reg_params params;
> > > > +       int ret;
> > > > +
> > > > +       /* UNDEF on any unhandled register or an attempted write */
> > > > +       if (!kvm_esr_cp10_id_to_sys64(esr, &params) || params.is_write) {
> > > > +               kvm_inject_undefined(vcpu);
> > >
> > > Nit: For debugging, it might be more useful to use unhandled_cp_access()
> > > (, which needs to be changed to support ESR_ELx_EC_CP10_ID though)
> > > rather than directly calling kvm_inject_undefined().
> >
> > A very worthy nit, you spotted my laziness in shunting straight to
> > kvm_inject_undefined() :)
> >
> > Thinking about this a bit more deeply, this code should be dead. The
> > only time either of these conditions would happen is on a broken
> > implementation. Probably should still handle it gracefully in case the
> > CP10 handling in KVM becomes (or is in my own patch!) busted.
>
> Actually, on second thought: any objections to leaving this as-is?
> kvm_esr_cp10_id_to_sys64() spits out sys_reg_params that point at the
> MRS alias for the VMRS register. Even if that call succeeds, the params
> that get printed out by unhandled_cp_access() do not match the actual
> register the guest was accessing. And if the call fails, ->Op2 is
> uninitialized.

I understand a few additional changes are needed for this.
Or simply use WARN_ON_ONCE ? (since this cannot be created by
the guest but by a KVM or CPU problem)

I'm fine with the current code as-is though:)

Thanks,
Reiji

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

Re: [PATCH v2 2/3] KVM: arm64: Plumb cp10 ID traps through the AArch64 sysreg handler

[Reiji Watanabe]

Hi Oliver,

On Mon, Apr 4, 2022 at 4:19 PM Oliver Upton <oupton@google.com> wrote:
>
> On Mon, Apr 04, 2022 at 05:28:33AM +0000, Oliver Upton wrote:
> > Hi Reiji,
> >
> > On Sun, Apr 03, 2022 at 08:57:47PM -0700, Reiji Watanabe wrote:
> > > > +int kvm_handle_cp10_id(struct kvm_vcpu *vcpu)
> > > > +{
> > > > +       int Rt = kvm_vcpu_sys_get_rt(vcpu);
> > > > +       u32 esr = kvm_vcpu_get_esr(vcpu);
> > > > +       struct sys_reg_params params;
> > > > +       int ret;
> > > > +
> > > > +       /* UNDEF on any unhandled register or an attempted write */
> > > > +       if (!kvm_esr_cp10_id_to_sys64(esr, &params) || params.is_write) {
> > > > +               kvm_inject_undefined(vcpu);
> > >
> > > Nit: For debugging, it might be more useful to use unhandled_cp_access()
> > > (, which needs to be changed to support ESR_ELx_EC_CP10_ID though)
> > > rather than directly calling kvm_inject_undefined().
> >
> > A very worthy nit, you spotted my laziness in shunting straight to
> > kvm_inject_undefined() :)
> >
> > Thinking about this a bit more deeply, this code should be dead. The
> > only time either of these conditions would happen is on a broken
> > implementation. Probably should still handle it gracefully in case the
> > CP10 handling in KVM becomes (or is in my own patch!) busted.
>
> Actually, on second thought: any objections to leaving this as-is?
> kvm_esr_cp10_id_to_sys64() spits out sys_reg_params that point at the
> MRS alias for the VMRS register. Even if that call succeeds, the params
> that get printed out by unhandled_cp_access() do not match the actual
> register the guest was accessing. And if the call fails, ->Op2 is
> uninitialized.

I understand a few additional changes are needed for this.
Or simply use WARN_ON_ONCE ? (since this cannot be created by
the guest but by a KVM or CPU problem)

I'm fine with the current code as-is though:)

Thanks,
Reiji

Re: [PATCH v2 3/3] KVM: arm64: Start trapping ID registers for 32 bit guests

[Reiji Watanabe]

Hi Oliver,

On Sun, Apr 3, 2022 at 10:46 PM Oliver Upton <oupton@google.com> wrote:
>
> Hi Reiji,
>
> On Sun, Apr 03, 2022 at 09:45:15PM -0700, Reiji Watanabe wrote:
> > On Thu, Mar 31, 2022 at 6:08 PM Oliver Upton <oupton@google.com> wrote:
> > >
> > > To date KVM has not trapped ID register accesses from AArch32, meaning
> > > that guests get an unconstrained view of what hardware supports. This
> > > can be a serious problem because we try to base the guest's feature
> > > registers on values that are safe system-wide. Furthermore, KVM does not
> > > implement the latest ISA in the PMU and Debug architecture, so we
> > > constrain these fields to supported values.
> > >
> > > Since KVM now correctly handles CP15 and CP10 register traps, we no
> > > longer need to clear HCR_EL2.TID3 for 32 bit guests and will instead
> > > emulate reads with their safe values.
> > >
> > > Signed-off-by: Oliver Upton <oupton@google.com>
> >
> > Reviewed-by: Reiji Watanabe <reijiw@google.com>
> >
> > BTW, due to this, on a system that supports PMUv3, ID_DFR0_E1 value will
> > become 0 for the aarch32 guest without PMUv3. This is the correct behavior,
> > but it affects migration.  I'm not sure how much we should care about
> > migration of the aarch32 guest though (and it will be resolved once ID
> > registers become configurable anyway).
>
> I believe userspace has been accessing the sanitised values of these
> feature registers the entire time, so we should be OK on the UAPI side.

You are right:)
I totally forgot this was just about trapping. Sorry for the noise.

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

Re: [PATCH v2 3/3] KVM: arm64: Start trapping ID registers for 32 bit guests

[Reiji Watanabe]

Hi Oliver,

On Sun, Apr 3, 2022 at 10:46 PM Oliver Upton <oupton@google.com> wrote:
>
> Hi Reiji,
>
> On Sun, Apr 03, 2022 at 09:45:15PM -0700, Reiji Watanabe wrote:
> > On Thu, Mar 31, 2022 at 6:08 PM Oliver Upton <oupton@google.com> wrote:
> > >
> > > To date KVM has not trapped ID register accesses from AArch32, meaning
> > > that guests get an unconstrained view of what hardware supports. This
> > > can be a serious problem because we try to base the guest's feature
> > > registers on values that are safe system-wide. Furthermore, KVM does not
> > > implement the latest ISA in the PMU and Debug architecture, so we
> > > constrain these fields to supported values.
> > >
> > > Since KVM now correctly handles CP15 and CP10 register traps, we no
> > > longer need to clear HCR_EL2.TID3 for 32 bit guests and will instead
> > > emulate reads with their safe values.
> > >
> > > Signed-off-by: Oliver Upton <oupton@google.com>
> >
> > Reviewed-by: Reiji Watanabe <reijiw@google.com>
> >
> > BTW, due to this, on a system that supports PMUv3, ID_DFR0_E1 value will
> > become 0 for the aarch32 guest without PMUv3. This is the correct behavior,
> > but it affects migration.  I'm not sure how much we should care about
> > migration of the aarch32 guest though (and it will be resolved once ID
> > registers become configurable anyway).
>
> I believe userspace has been accessing the sanitised values of these
> feature registers the entire time, so we should be OK on the UAPI side.

You are right:)
I totally forgot this was just about trapping. Sorry for the noise.

Thanks,
Reiji

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

Re: [PATCH v2 3/3] KVM: arm64: Start trapping ID registers for 32 bit guests

[Reiji Watanabe]

Hi Oliver,

On Sun, Apr 3, 2022 at 10:46 PM Oliver Upton <oupton@google.com> wrote:
>
> Hi Reiji,
>
> On Sun, Apr 03, 2022 at 09:45:15PM -0700, Reiji Watanabe wrote:
> > On Thu, Mar 31, 2022 at 6:08 PM Oliver Upton <oupton@google.com> wrote:
> > >
> > > To date KVM has not trapped ID register accesses from AArch32, meaning
> > > that guests get an unconstrained view of what hardware supports. This
> > > can be a serious problem because we try to base the guest's feature
> > > registers on values that are safe system-wide. Furthermore, KVM does not
> > > implement the latest ISA in the PMU and Debug architecture, so we
> > > constrain these fields to supported values.
> > >
> > > Since KVM now correctly handles CP15 and CP10 register traps, we no
> > > longer need to clear HCR_EL2.TID3 for 32 bit guests and will instead
> > > emulate reads with their safe values.
> > >
> > > Signed-off-by: Oliver Upton <oupton@google.com>
> >
> > Reviewed-by: Reiji Watanabe <reijiw@google.com>
> >
> > BTW, due to this, on a system that supports PMUv3, ID_DFR0_E1 value will
> > become 0 for the aarch32 guest without PMUv3. This is the correct behavior,
> > but it affects migration.  I'm not sure how much we should care about
> > migration of the aarch32 guest though (and it will be resolved once ID
> > registers become configurable anyway).
>
> I believe userspace has been accessing the sanitised values of these
> feature registers the entire time, so we should be OK on the UAPI side.

You are right:)
I totally forgot this was just about trapping. Sorry for the noise.

Thanks,
Reiji

Re: [PATCH v2 1/3] KVM: arm64: Wire up CP15 feature registers to their AArch64 equivalents

[Marc Zyngier]

On Fri, 01 Apr 2022 02:08:30 +0100,
Oliver Upton <oupton@google.com> wrote:
> 
> KVM currently does not trap ID register accesses from an AArch32 EL1.
> This is painful for a couple of reasons. Certain unimplemented features
> are visible to AArch32 EL1, as we limit PMU to version 3 and the debug
> architecture to v8.0. Additionally, we attempt to paper over
> heterogeneous systems by using register values that are safe
> system-wide. All this hard work is completely sidestepped because KVM
> does not set TID3 for AArch32 guests.
> 
> Fix up handling of CP15 feature registers by simply rerouting to their
> AArch64 aliases. Punt setting HCR_EL2.TID3 to a later change, as we need
> to fix up the oddball CP10 feature registers still.
> 
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  arch/arm64/kvm/sys_regs.c | 68 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 68 insertions(+)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index dd34b5ab51d4..8b791256a5b4 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -2339,6 +2339,67 @@ static int kvm_handle_cp_64(struct kvm_vcpu *vcpu,
>  	return 1;
>  }
>  
> +static int emulate_sys_reg(struct kvm_vcpu *vcpu, struct sys_reg_params *params);
> +
> +/**
> + * kvm_emulate_cp15_id_reg() - Handles an MRC trap on a guest CP15 access where
> + *			       CRn=0, which corresponds to the AArch32 feature
> + *			       registers.
> + * @vcpu: the vCPU pointer
> + * @params: the system register access parameters.
> + *
> + * Our cp15 system register tables do not enumerate the AArch32 feature
> + * registers. Conveniently, our AArch64 table does, and the AArch32 system
> + * register encoding can be trivially remapped into the AArch64 for the feature
> + * registers: Append op0=3, leaving op1, CRn, CRm, and op2 the same.
> + *
> + * According to DDI0487G.b G7.3.1, paragraph "Behavior of VMSAv8-32 32-bit
> + * System registers with (coproc=0b1111, CRn==c0)", read accesses from this
> + * range are either UNKNOWN or RES0. Rerouting remains architectural as we
> + * treat undefined registers in this range as RAZ.
> + */
> +static int kvm_emulate_cp15_id_reg(struct kvm_vcpu *vcpu,
> +				   struct sys_reg_params *params)
> +{
> +	int Rt = kvm_vcpu_sys_get_rt(vcpu);
> +	int ret = 1;
> +
> +	/* Treat impossible writes to RO registers as UNDEFINED */
> +	if (params->is_write) {
> +		unhandled_cp_access(vcpu, params);
> +		return 1;
> +	}
> +
> +	params->Op0 = 3;
> +
> +	/*
> +	 * All registers where CRm > 3 are known to be UNKNOWN/RAZ from AArch32.
> +	 * Avoid conflicting with future expansion of AArch64 feature registers
> +	 * and simply treat them as RAZ here.
> +	 */
> +	if (params->CRm > 3)
> +		params->regval = 0;
> +	else
> +		ret = emulate_sys_reg(vcpu, params);
> +
> +	vcpu_set_reg(vcpu, Rt, params->regval);

It feels odd to update Rt without checking whether the read has
succeeded. In your case, this is harmless, but would break with the
approach I'm outlining below.

> +	return ret;
> +}
> +
> +/**
> + * kvm_is_cp15_id_reg() - Returns true if the specified CP15 register is an
> + *			  AArch32 ID register.
> + * @params: the system register access parameters
> + *
> + * Note that CP15 ID registers where CRm=0 are excluded from this check. The
> + * only register trapped in the CRm=0 range is CTR, which is already handled in
> + * the cp15 register table.

There is also the fact that CTR_EL0 has Op1=3 while CTR has Op1=0,
which prevents it from fitting in your scheme.

> + */
> +static inline bool kvm_is_cp15_id_reg(struct sys_reg_params *params)
> +{
> +	return params->CRn == 0 && params->Op1 == 0 && params->CRm != 0;
> +}
> +
>  /**
>   * kvm_handle_cp_32 -- handles a mrc/mcr trap on a guest CP14/CP15 access
>   * @vcpu: The VCPU pointer
> @@ -2360,6 +2421,13 @@ static int kvm_handle_cp_32(struct kvm_vcpu *vcpu,
>  	params.Op1 = (esr >> 14) & 0x7;
>  	params.Op2 = (esr >> 17) & 0x7;
>  
> +	/*
> +	 * Certain AArch32 ID registers are handled by rerouting to the AArch64
> +	 * system register table.
> +	 */
> +	if (ESR_ELx_EC(esr) == ESR_ELx_EC_CP15_32 && kvm_is_cp15_id_reg(&params))
> +		return kvm_emulate_cp15_id_reg(vcpu, &params);

I think this is a bit ugly. We reach this point from a function that
was cp15-specific, and now we are reconstructing the context. I'd
rather this is moved to kvm_handle_cp15_32(), and treated there
(untested):

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 7b45c040cc27..a071d89ace92 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -2350,28 +2350,21 @@ static int kvm_handle_cp_64(struct kvm_vcpu *vcpu,
  * @run:  The kvm_run struct
  */
 static int kvm_handle_cp_32(struct kvm_vcpu *vcpu,
+			    struct sys_reg_params *params,
 			    const struct sys_reg_desc *global,
 			    size_t nr_global)
 {
-	struct sys_reg_params params;
-	u32 esr = kvm_vcpu_get_esr(vcpu);
 	int Rt  = kvm_vcpu_sys_get_rt(vcpu);
 
-	params.CRm = (esr >> 1) & 0xf;
-	params.regval = vcpu_get_reg(vcpu, Rt);
-	params.is_write = ((esr & 1) == 0);
-	params.CRn = (esr >> 10) & 0xf;
-	params.Op0 = 0;
-	params.Op1 = (esr >> 14) & 0x7;
-	params.Op2 = (esr >> 17) & 0x7;
+	params->regval = vcpu_get_reg(vcpu, Rt);
 
-	if (!emulate_cp(vcpu, &params, global, nr_global)) {
-		if (!params.is_write)
-			vcpu_set_reg(vcpu, Rt, params.regval);
+	if (!emulate_cp(vcpu, params, global, nr_global)) {
+		if (!params->is_write)
+			vcpu_set_reg(vcpu, Rt, params->regval);
 		return 1;
 	}
 
-	unhandled_cp_access(vcpu, &params);
+	unhandled_cp_access(vcpu, params);
 	return 1;
 }
 
@@ -2382,7 +2375,14 @@ int kvm_handle_cp15_64(struct kvm_vcpu *vcpu)
 
 int kvm_handle_cp15_32(struct kvm_vcpu *vcpu)
 {
-	return kvm_handle_cp_32(vcpu, cp15_regs, ARRAY_SIZE(cp15_regs));
+	struct sys_reg_params params;
+
+	params = esr_cp1x_32_to_params(kvm_vcpu_get_esr(vcpu));
+
+	if (params.Op1 == 0 && params.CRn == 0 && params.CRm)
+		return kvm_emulate_cp15_id_reg(vcpu, &params);
+
+	return kvm_handle_cp_32(vcpu, &params, cp15_regs, ARRAY_SIZE(cp15_regs));
 }
 
 int kvm_handle_cp14_64(struct kvm_vcpu *vcpu)
@@ -2392,7 +2392,11 @@ int kvm_handle_cp14_64(struct kvm_vcpu *vcpu)
 
 int kvm_handle_cp14_32(struct kvm_vcpu *vcpu)
 {
-	return kvm_handle_cp_32(vcpu, cp14_regs, ARRAY_SIZE(cp14_regs));
+	struct sys_reg_params params;
+
+	params = esr_cp1x_32_to_params(kvm_vcpu_get_esr(vcpu));
+
+	return kvm_handle_cp_32(vcpu, &params, cp14_regs, ARRAY_SIZE(cp14_regs));
 }
 
 static bool is_imp_def_sys_reg(struct sys_reg_params *params)
diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
index cc0cc95a0280..fd4b2bb8c782 100644
--- a/arch/arm64/kvm/sys_regs.h
+++ b/arch/arm64/kvm/sys_regs.h
@@ -35,6 +35,13 @@ struct sys_reg_params {
 				  .Op2 = ((esr) >> 17) & 0x7,                  \
 				  .is_write = !((esr) & 1) })
 
+#define esr_cp1x_32_to_params(esr)					       \
+	((struct sys_reg_params){ .Op1 = ((esr) >> 14) & 0x7,                  \
+				  .CRn = ((esr) >> 10) & 0xf,                  \
+				  .CRm = ((esr) >> 1) & 0xf,                   \
+				  .Op2 = ((esr) >> 17) & 0x7,                  \
+				  .is_write = !((esr) & 1) })
+
 struct sys_reg_desc {
 	/* Sysreg string for debug */
 	const char *name;


What do you think?

	M.

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v2 1/3] KVM: arm64: Wire up CP15 feature registers to their AArch64 equivalents

[Marc Zyngier]

On Fri, 01 Apr 2022 02:08:30 +0100,
Oliver Upton <oupton@google.com> wrote:
> 
> KVM currently does not trap ID register accesses from an AArch32 EL1.
> This is painful for a couple of reasons. Certain unimplemented features
> are visible to AArch32 EL1, as we limit PMU to version 3 and the debug
> architecture to v8.0. Additionally, we attempt to paper over
> heterogeneous systems by using register values that are safe
> system-wide. All this hard work is completely sidestepped because KVM
> does not set TID3 for AArch32 guests.
> 
> Fix up handling of CP15 feature registers by simply rerouting to their
> AArch64 aliases. Punt setting HCR_EL2.TID3 to a later change, as we need
> to fix up the oddball CP10 feature registers still.
> 
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  arch/arm64/kvm/sys_regs.c | 68 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 68 insertions(+)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index dd34b5ab51d4..8b791256a5b4 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -2339,6 +2339,67 @@ static int kvm_handle_cp_64(struct kvm_vcpu *vcpu,
>  	return 1;
>  }
>  
> +static int emulate_sys_reg(struct kvm_vcpu *vcpu, struct sys_reg_params *params);
> +
> +/**
> + * kvm_emulate_cp15_id_reg() - Handles an MRC trap on a guest CP15 access where
> + *			       CRn=0, which corresponds to the AArch32 feature
> + *			       registers.
> + * @vcpu: the vCPU pointer
> + * @params: the system register access parameters.
> + *
> + * Our cp15 system register tables do not enumerate the AArch32 feature
> + * registers. Conveniently, our AArch64 table does, and the AArch32 system
> + * register encoding can be trivially remapped into the AArch64 for the feature
> + * registers: Append op0=3, leaving op1, CRn, CRm, and op2 the same.
> + *
> + * According to DDI0487G.b G7.3.1, paragraph "Behavior of VMSAv8-32 32-bit
> + * System registers with (coproc=0b1111, CRn==c0)", read accesses from this
> + * range are either UNKNOWN or RES0. Rerouting remains architectural as we
> + * treat undefined registers in this range as RAZ.
> + */
> +static int kvm_emulate_cp15_id_reg(struct kvm_vcpu *vcpu,
> +				   struct sys_reg_params *params)
> +{
> +	int Rt = kvm_vcpu_sys_get_rt(vcpu);
> +	int ret = 1;
> +
> +	/* Treat impossible writes to RO registers as UNDEFINED */
> +	if (params->is_write) {
> +		unhandled_cp_access(vcpu, params);
> +		return 1;
> +	}
> +
> +	params->Op0 = 3;
> +
> +	/*
> +	 * All registers where CRm > 3 are known to be UNKNOWN/RAZ from AArch32.
> +	 * Avoid conflicting with future expansion of AArch64 feature registers
> +	 * and simply treat them as RAZ here.
> +	 */
> +	if (params->CRm > 3)
> +		params->regval = 0;
> +	else
> +		ret = emulate_sys_reg(vcpu, params);
> +
> +	vcpu_set_reg(vcpu, Rt, params->regval);

It feels odd to update Rt without checking whether the read has
succeeded. In your case, this is harmless, but would break with the
approach I'm outlining below.

> +	return ret;
> +}
> +
> +/**
> + * kvm_is_cp15_id_reg() - Returns true if the specified CP15 register is an
> + *			  AArch32 ID register.
> + * @params: the system register access parameters
> + *
> + * Note that CP15 ID registers where CRm=0 are excluded from this check. The
> + * only register trapped in the CRm=0 range is CTR, which is already handled in
> + * the cp15 register table.

There is also the fact that CTR_EL0 has Op1=3 while CTR has Op1=0,
which prevents it from fitting in your scheme.

> + */
> +static inline bool kvm_is_cp15_id_reg(struct sys_reg_params *params)
> +{
> +	return params->CRn == 0 && params->Op1 == 0 && params->CRm != 0;
> +}
> +
>  /**
>   * kvm_handle_cp_32 -- handles a mrc/mcr trap on a guest CP14/CP15 access
>   * @vcpu: The VCPU pointer
> @@ -2360,6 +2421,13 @@ static int kvm_handle_cp_32(struct kvm_vcpu *vcpu,
>  	params.Op1 = (esr >> 14) & 0x7;
>  	params.Op2 = (esr >> 17) & 0x7;
>  
> +	/*
> +	 * Certain AArch32 ID registers are handled by rerouting to the AArch64
> +	 * system register table.
> +	 */
> +	if (ESR_ELx_EC(esr) == ESR_ELx_EC_CP15_32 && kvm_is_cp15_id_reg(&params))
> +		return kvm_emulate_cp15_id_reg(vcpu, &params);

I think this is a bit ugly. We reach this point from a function that
was cp15-specific, and now we are reconstructing the context. I'd
rather this is moved to kvm_handle_cp15_32(), and treated there
(untested):

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 7b45c040cc27..a071d89ace92 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -2350,28 +2350,21 @@ static int kvm_handle_cp_64(struct kvm_vcpu *vcpu,
  * @run:  The kvm_run struct
  */
 static int kvm_handle_cp_32(struct kvm_vcpu *vcpu,
+			    struct sys_reg_params *params,
 			    const struct sys_reg_desc *global,
 			    size_t nr_global)
 {
-	struct sys_reg_params params;
-	u32 esr = kvm_vcpu_get_esr(vcpu);
 	int Rt  = kvm_vcpu_sys_get_rt(vcpu);
 
-	params.CRm = (esr >> 1) & 0xf;
-	params.regval = vcpu_get_reg(vcpu, Rt);
-	params.is_write = ((esr & 1) == 0);
-	params.CRn = (esr >> 10) & 0xf;
-	params.Op0 = 0;
-	params.Op1 = (esr >> 14) & 0x7;
-	params.Op2 = (esr >> 17) & 0x7;
+	params->regval = vcpu_get_reg(vcpu, Rt);
 
-	if (!emulate_cp(vcpu, &params, global, nr_global)) {
-		if (!params.is_write)
-			vcpu_set_reg(vcpu, Rt, params.regval);
+	if (!emulate_cp(vcpu, params, global, nr_global)) {
+		if (!params->is_write)
+			vcpu_set_reg(vcpu, Rt, params->regval);
 		return 1;
 	}
 
-	unhandled_cp_access(vcpu, &params);
+	unhandled_cp_access(vcpu, params);
 	return 1;
 }
 
@@ -2382,7 +2375,14 @@ int kvm_handle_cp15_64(struct kvm_vcpu *vcpu)
 
 int kvm_handle_cp15_32(struct kvm_vcpu *vcpu)
 {
-	return kvm_handle_cp_32(vcpu, cp15_regs, ARRAY_SIZE(cp15_regs));
+	struct sys_reg_params params;
+
+	params = esr_cp1x_32_to_params(kvm_vcpu_get_esr(vcpu));
+
+	if (params.Op1 == 0 && params.CRn == 0 && params.CRm)
+		return kvm_emulate_cp15_id_reg(vcpu, &params);
+
+	return kvm_handle_cp_32(vcpu, &params, cp15_regs, ARRAY_SIZE(cp15_regs));
 }
 
 int kvm_handle_cp14_64(struct kvm_vcpu *vcpu)
@@ -2392,7 +2392,11 @@ int kvm_handle_cp14_64(struct kvm_vcpu *vcpu)
 
 int kvm_handle_cp14_32(struct kvm_vcpu *vcpu)
 {
-	return kvm_handle_cp_32(vcpu, cp14_regs, ARRAY_SIZE(cp14_regs));
+	struct sys_reg_params params;
+
+	params = esr_cp1x_32_to_params(kvm_vcpu_get_esr(vcpu));
+
+	return kvm_handle_cp_32(vcpu, &params, cp14_regs, ARRAY_SIZE(cp14_regs));
 }
 
 static bool is_imp_def_sys_reg(struct sys_reg_params *params)
diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
index cc0cc95a0280..fd4b2bb8c782 100644
--- a/arch/arm64/kvm/sys_regs.h
+++ b/arch/arm64/kvm/sys_regs.h
@@ -35,6 +35,13 @@ struct sys_reg_params {
 				  .Op2 = ((esr) >> 17) & 0x7,                  \
 				  .is_write = !((esr) & 1) })
 
+#define esr_cp1x_32_to_params(esr)					       \
+	((struct sys_reg_params){ .Op1 = ((esr) >> 14) & 0x7,                  \
+				  .CRn = ((esr) >> 10) & 0xf,                  \
+				  .CRm = ((esr) >> 1) & 0xf,                   \
+				  .Op2 = ((esr) >> 17) & 0x7,                  \
+				  .is_write = !((esr) & 1) })
+
 struct sys_reg_desc {
 	/* Sysreg string for debug */
 	const char *name;


What do you think?

	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 v2 1/3] KVM: arm64: Wire up CP15 feature registers to their AArch64 equivalents

[Marc Zyngier]

On Fri, 01 Apr 2022 02:08:30 +0100,
Oliver Upton <oupton@google.com> wrote:
> 
> KVM currently does not trap ID register accesses from an AArch32 EL1.
> This is painful for a couple of reasons. Certain unimplemented features
> are visible to AArch32 EL1, as we limit PMU to version 3 and the debug
> architecture to v8.0. Additionally, we attempt to paper over
> heterogeneous systems by using register values that are safe
> system-wide. All this hard work is completely sidestepped because KVM
> does not set TID3 for AArch32 guests.
> 
> Fix up handling of CP15 feature registers by simply rerouting to their
> AArch64 aliases. Punt setting HCR_EL2.TID3 to a later change, as we need
> to fix up the oddball CP10 feature registers still.
> 
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  arch/arm64/kvm/sys_regs.c | 68 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 68 insertions(+)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index dd34b5ab51d4..8b791256a5b4 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -2339,6 +2339,67 @@ static int kvm_handle_cp_64(struct kvm_vcpu *vcpu,
>  	return 1;
>  }
>  
> +static int emulate_sys_reg(struct kvm_vcpu *vcpu, struct sys_reg_params *params);
> +
> +/**
> + * kvm_emulate_cp15_id_reg() - Handles an MRC trap on a guest CP15 access where
> + *			       CRn=0, which corresponds to the AArch32 feature
> + *			       registers.
> + * @vcpu: the vCPU pointer
> + * @params: the system register access parameters.
> + *
> + * Our cp15 system register tables do not enumerate the AArch32 feature
> + * registers. Conveniently, our AArch64 table does, and the AArch32 system
> + * register encoding can be trivially remapped into the AArch64 for the feature
> + * registers: Append op0=3, leaving op1, CRn, CRm, and op2 the same.
> + *
> + * According to DDI0487G.b G7.3.1, paragraph "Behavior of VMSAv8-32 32-bit
> + * System registers with (coproc=0b1111, CRn==c0)", read accesses from this
> + * range are either UNKNOWN or RES0. Rerouting remains architectural as we
> + * treat undefined registers in this range as RAZ.
> + */
> +static int kvm_emulate_cp15_id_reg(struct kvm_vcpu *vcpu,
> +				   struct sys_reg_params *params)
> +{
> +	int Rt = kvm_vcpu_sys_get_rt(vcpu);
> +	int ret = 1;
> +
> +	/* Treat impossible writes to RO registers as UNDEFINED */
> +	if (params->is_write) {
> +		unhandled_cp_access(vcpu, params);
> +		return 1;
> +	}
> +
> +	params->Op0 = 3;
> +
> +	/*
> +	 * All registers where CRm > 3 are known to be UNKNOWN/RAZ from AArch32.
> +	 * Avoid conflicting with future expansion of AArch64 feature registers
> +	 * and simply treat them as RAZ here.
> +	 */
> +	if (params->CRm > 3)
> +		params->regval = 0;
> +	else
> +		ret = emulate_sys_reg(vcpu, params);
> +
> +	vcpu_set_reg(vcpu, Rt, params->regval);

It feels odd to update Rt without checking whether the read has
succeeded. In your case, this is harmless, but would break with the
approach I'm outlining below.

> +	return ret;
> +}
> +
> +/**
> + * kvm_is_cp15_id_reg() - Returns true if the specified CP15 register is an
> + *			  AArch32 ID register.
> + * @params: the system register access parameters
> + *
> + * Note that CP15 ID registers where CRm=0 are excluded from this check. The
> + * only register trapped in the CRm=0 range is CTR, which is already handled in
> + * the cp15 register table.

There is also the fact that CTR_EL0 has Op1=3 while CTR has Op1=0,
which prevents it from fitting in your scheme.

> + */
> +static inline bool kvm_is_cp15_id_reg(struct sys_reg_params *params)
> +{
> +	return params->CRn == 0 && params->Op1 == 0 && params->CRm != 0;
> +}
> +
>  /**
>   * kvm_handle_cp_32 -- handles a mrc/mcr trap on a guest CP14/CP15 access
>   * @vcpu: The VCPU pointer
> @@ -2360,6 +2421,13 @@ static int kvm_handle_cp_32(struct kvm_vcpu *vcpu,
>  	params.Op1 = (esr >> 14) & 0x7;
>  	params.Op2 = (esr >> 17) & 0x7;
>  
> +	/*
> +	 * Certain AArch32 ID registers are handled by rerouting to the AArch64
> +	 * system register table.
> +	 */
> +	if (ESR_ELx_EC(esr) == ESR_ELx_EC_CP15_32 && kvm_is_cp15_id_reg(&params))
> +		return kvm_emulate_cp15_id_reg(vcpu, &params);

I think this is a bit ugly. We reach this point from a function that
was cp15-specific, and now we are reconstructing the context. I'd
rather this is moved to kvm_handle_cp15_32(), and treated there
(untested):

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 7b45c040cc27..a071d89ace92 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -2350,28 +2350,21 @@ static int kvm_handle_cp_64(struct kvm_vcpu *vcpu,
  * @run:  The kvm_run struct
  */
 static int kvm_handle_cp_32(struct kvm_vcpu *vcpu,
+			    struct sys_reg_params *params,
 			    const struct sys_reg_desc *global,
 			    size_t nr_global)
 {
-	struct sys_reg_params params;
-	u32 esr = kvm_vcpu_get_esr(vcpu);
 	int Rt  = kvm_vcpu_sys_get_rt(vcpu);
 
-	params.CRm = (esr >> 1) & 0xf;
-	params.regval = vcpu_get_reg(vcpu, Rt);
-	params.is_write = ((esr & 1) == 0);
-	params.CRn = (esr >> 10) & 0xf;
-	params.Op0 = 0;
-	params.Op1 = (esr >> 14) & 0x7;
-	params.Op2 = (esr >> 17) & 0x7;
+	params->regval = vcpu_get_reg(vcpu, Rt);
 
-	if (!emulate_cp(vcpu, &params, global, nr_global)) {
-		if (!params.is_write)
-			vcpu_set_reg(vcpu, Rt, params.regval);
+	if (!emulate_cp(vcpu, params, global, nr_global)) {
+		if (!params->is_write)
+			vcpu_set_reg(vcpu, Rt, params->regval);
 		return 1;
 	}
 
-	unhandled_cp_access(vcpu, &params);
+	unhandled_cp_access(vcpu, params);
 	return 1;
 }
 
@@ -2382,7 +2375,14 @@ int kvm_handle_cp15_64(struct kvm_vcpu *vcpu)
 
 int kvm_handle_cp15_32(struct kvm_vcpu *vcpu)
 {
-	return kvm_handle_cp_32(vcpu, cp15_regs, ARRAY_SIZE(cp15_regs));
+	struct sys_reg_params params;
+
+	params = esr_cp1x_32_to_params(kvm_vcpu_get_esr(vcpu));
+
+	if (params.Op1 == 0 && params.CRn == 0 && params.CRm)
+		return kvm_emulate_cp15_id_reg(vcpu, &params);
+
+	return kvm_handle_cp_32(vcpu, &params, cp15_regs, ARRAY_SIZE(cp15_regs));
 }
 
 int kvm_handle_cp14_64(struct kvm_vcpu *vcpu)
@@ -2392,7 +2392,11 @@ int kvm_handle_cp14_64(struct kvm_vcpu *vcpu)
 
 int kvm_handle_cp14_32(struct kvm_vcpu *vcpu)
 {
-	return kvm_handle_cp_32(vcpu, cp14_regs, ARRAY_SIZE(cp14_regs));
+	struct sys_reg_params params;
+
+	params = esr_cp1x_32_to_params(kvm_vcpu_get_esr(vcpu));
+
+	return kvm_handle_cp_32(vcpu, &params, cp14_regs, ARRAY_SIZE(cp14_regs));
 }
 
 static bool is_imp_def_sys_reg(struct sys_reg_params *params)
diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
index cc0cc95a0280..fd4b2bb8c782 100644
--- a/arch/arm64/kvm/sys_regs.h
+++ b/arch/arm64/kvm/sys_regs.h
@@ -35,6 +35,13 @@ struct sys_reg_params {
 				  .Op2 = ((esr) >> 17) & 0x7,                  \
 				  .is_write = !((esr) & 1) })
 
+#define esr_cp1x_32_to_params(esr)					       \
+	((struct sys_reg_params){ .Op1 = ((esr) >> 14) & 0x7,                  \
+				  .CRn = ((esr) >> 10) & 0xf,                  \
+				  .CRm = ((esr) >> 1) & 0xf,                   \
+				  .Op2 = ((esr) >> 17) & 0x7,                  \
+				  .is_write = !((esr) & 1) })
+
 struct sys_reg_desc {
 	/* Sysreg string for debug */
 	const char *name;


What do you think?

	M.

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

Re: [PATCH v2 1/3] KVM: arm64: Wire up CP15 feature registers to their AArch64 equivalents

[Oliver Upton]

Hi Marc,

On Wed, Apr 06, 2022 at 04:07:28PM +0100, Marc Zyngier wrote:
> > +	/*
> > +	 * All registers where CRm > 3 are known to be UNKNOWN/RAZ from AArch32.
> > +	 * Avoid conflicting with future expansion of AArch64 feature registers
> > +	 * and simply treat them as RAZ here.
> > +	 */
> > +	if (params->CRm > 3)
> > +		params->regval = 0;
> > +	else
> > +		ret = emulate_sys_reg(vcpu, params);
> > +
> > +	vcpu_set_reg(vcpu, Rt, params->regval);
> 
> It feels odd to update Rt without checking whether the read has
> succeeded. In your case, this is harmless, but would break with the
> approach I'm outlining below.
> 

A total kludge to avoid yet another level of indentation :) I'll go
about this the right way next spin.

> > +	return ret;
> > +}
> > +
> > +/**
> > + * kvm_is_cp15_id_reg() - Returns true if the specified CP15 register is an
> > + *			  AArch32 ID register.
> > + * @params: the system register access parameters
> > + *
> > + * Note that CP15 ID registers where CRm=0 are excluded from this check. The
> > + * only register trapped in the CRm=0 range is CTR, which is already handled in
> > + * the cp15 register table.
> 
> There is also the fact that CTR_EL0 has Op1=3 while CTR has Op1=0,
> which prevents it from fitting in your scheme.
> 
> > + */
> > +static inline bool kvm_is_cp15_id_reg(struct sys_reg_params *params)
> > +{
> > +	return params->CRn == 0 && params->Op1 == 0 && params->CRm != 0;
> > +}
> > +
> >  /**
> >   * kvm_handle_cp_32 -- handles a mrc/mcr trap on a guest CP14/CP15 access
> >   * @vcpu: The VCPU pointer
> > @@ -2360,6 +2421,13 @@ static int kvm_handle_cp_32(struct kvm_vcpu *vcpu,
> >  	params.Op1 = (esr >> 14) & 0x7;
> >  	params.Op2 = (esr >> 17) & 0x7;
> >  
> > +	/*
> > +	 * Certain AArch32 ID registers are handled by rerouting to the AArch64
> > +	 * system register table.
> > +	 */
> > +	if (ESR_ELx_EC(esr) == ESR_ELx_EC_CP15_32 && kvm_is_cp15_id_reg(&params))
> > +		return kvm_emulate_cp15_id_reg(vcpu, &params);
> 
> I think this is a bit ugly. We reach this point from a function that
> was cp15-specific, and now we are reconstructing the context. I'd
> rather this is moved to kvm_handle_cp15_32(), and treated there
> (untested):
>

Completely agree, hoisting this would be much more elegant.

> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 7b45c040cc27..a071d89ace92 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -2350,28 +2350,21 @@ static int kvm_handle_cp_64(struct kvm_vcpu *vcpu,
>   * @run:  The kvm_run struct
>   */
>  static int kvm_handle_cp_32(struct kvm_vcpu *vcpu,
> +			    struct sys_reg_params *params,
>  			    const struct sys_reg_desc *global,
>  			    size_t nr_global)
>  {
> -	struct sys_reg_params params;
> -	u32 esr = kvm_vcpu_get_esr(vcpu);
>  	int Rt  = kvm_vcpu_sys_get_rt(vcpu);
>  
> -	params.CRm = (esr >> 1) & 0xf;
> -	params.regval = vcpu_get_reg(vcpu, Rt);
> -	params.is_write = ((esr & 1) == 0);
> -	params.CRn = (esr >> 10) & 0xf;
> -	params.Op0 = 0;
> -	params.Op1 = (esr >> 14) & 0x7;
> -	params.Op2 = (esr >> 17) & 0x7;
> +	params->regval = vcpu_get_reg(vcpu, Rt);
>  
> -	if (!emulate_cp(vcpu, &params, global, nr_global)) {
> -		if (!params.is_write)
> -			vcpu_set_reg(vcpu, Rt, params.regval);
> +	if (!emulate_cp(vcpu, params, global, nr_global)) {
> +		if (!params->is_write)
> +			vcpu_set_reg(vcpu, Rt, params->regval);
>  		return 1;
>  	}
>  
> -	unhandled_cp_access(vcpu, &params);
> +	unhandled_cp_access(vcpu, params);
>  	return 1;
>  }
>  
> @@ -2382,7 +2375,14 @@ int kvm_handle_cp15_64(struct kvm_vcpu *vcpu)
>  
>  int kvm_handle_cp15_32(struct kvm_vcpu *vcpu)
>  {
> -	return kvm_handle_cp_32(vcpu, cp15_regs, ARRAY_SIZE(cp15_regs));
> +	struct sys_reg_params params;
> +
> +	params = esr_cp1x_32_to_params(kvm_vcpu_get_esr(vcpu));
> +
> +	if (params.Op1 == 0 && params.CRn == 0 && params.CRm)
> +		return kvm_emulate_cp15_id_reg(vcpu, &params);
> +
> +	return kvm_handle_cp_32(vcpu, &params, cp15_regs, ARRAY_SIZE(cp15_regs));
>  }
>  
>  int kvm_handle_cp14_64(struct kvm_vcpu *vcpu)
> @@ -2392,7 +2392,11 @@ int kvm_handle_cp14_64(struct kvm_vcpu *vcpu)
>  
>  int kvm_handle_cp14_32(struct kvm_vcpu *vcpu)
>  {
> -	return kvm_handle_cp_32(vcpu, cp14_regs, ARRAY_SIZE(cp14_regs));
> +	struct sys_reg_params params;
> +
> +	params = esr_cp1x_32_to_params(kvm_vcpu_get_esr(vcpu));
> +
> +	return kvm_handle_cp_32(vcpu, &params, cp14_regs, ARRAY_SIZE(cp14_regs));
>  }
>  
>  static bool is_imp_def_sys_reg(struct sys_reg_params *params)
> diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
> index cc0cc95a0280..fd4b2bb8c782 100644
> --- a/arch/arm64/kvm/sys_regs.h
> +++ b/arch/arm64/kvm/sys_regs.h
> @@ -35,6 +35,13 @@ struct sys_reg_params {
>  				  .Op2 = ((esr) >> 17) & 0x7,                  \
>  				  .is_write = !((esr) & 1) })
>  
> +#define esr_cp1x_32_to_params(esr)					       \
> +	((struct sys_reg_params){ .Op1 = ((esr) >> 14) & 0x7,                  \
> +				  .CRn = ((esr) >> 10) & 0xf,                  \
> +				  .CRm = ((esr) >> 1) & 0xf,                   \
> +				  .Op2 = ((esr) >> 17) & 0x7,                  \
> +				  .is_write = !((esr) & 1) })
> +
>  struct sys_reg_desc {
>  	/* Sysreg string for debug */
>  	const char *name;
> 
> 
> What do you think?

Way better. Your suggested patch looks correct, I'll fold all of this
together and test it out. Thanks for the suggestions :)

--
Best,
Oliver
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v2 1/3] KVM: arm64: Wire up CP15 feature registers to their AArch64 equivalents

[Oliver Upton]

Hi Marc,

On Wed, Apr 06, 2022 at 04:07:28PM +0100, Marc Zyngier wrote:
> > +	/*
> > +	 * All registers where CRm > 3 are known to be UNKNOWN/RAZ from AArch32.
> > +	 * Avoid conflicting with future expansion of AArch64 feature registers
> > +	 * and simply treat them as RAZ here.
> > +	 */
> > +	if (params->CRm > 3)
> > +		params->regval = 0;
> > +	else
> > +		ret = emulate_sys_reg(vcpu, params);
> > +
> > +	vcpu_set_reg(vcpu, Rt, params->regval);
> 
> It feels odd to update Rt without checking whether the read has
> succeeded. In your case, this is harmless, but would break with the
> approach I'm outlining below.
> 

A total kludge to avoid yet another level of indentation :) I'll go
about this the right way next spin.

> > +	return ret;
> > +}
> > +
> > +/**
> > + * kvm_is_cp15_id_reg() - Returns true if the specified CP15 register is an
> > + *			  AArch32 ID register.
> > + * @params: the system register access parameters
> > + *
> > + * Note that CP15 ID registers where CRm=0 are excluded from this check. The
> > + * only register trapped in the CRm=0 range is CTR, which is already handled in
> > + * the cp15 register table.
> 
> There is also the fact that CTR_EL0 has Op1=3 while CTR has Op1=0,
> which prevents it from fitting in your scheme.
> 
> > + */
> > +static inline bool kvm_is_cp15_id_reg(struct sys_reg_params *params)
> > +{
> > +	return params->CRn == 0 && params->Op1 == 0 && params->CRm != 0;
> > +}
> > +
> >  /**
> >   * kvm_handle_cp_32 -- handles a mrc/mcr trap on a guest CP14/CP15 access
> >   * @vcpu: The VCPU pointer
> > @@ -2360,6 +2421,13 @@ static int kvm_handle_cp_32(struct kvm_vcpu *vcpu,
> >  	params.Op1 = (esr >> 14) & 0x7;
> >  	params.Op2 = (esr >> 17) & 0x7;
> >  
> > +	/*
> > +	 * Certain AArch32 ID registers are handled by rerouting to the AArch64
> > +	 * system register table.
> > +	 */
> > +	if (ESR_ELx_EC(esr) == ESR_ELx_EC_CP15_32 && kvm_is_cp15_id_reg(&params))
> > +		return kvm_emulate_cp15_id_reg(vcpu, &params);
> 
> I think this is a bit ugly. We reach this point from a function that
> was cp15-specific, and now we are reconstructing the context. I'd
> rather this is moved to kvm_handle_cp15_32(), and treated there
> (untested):
>

Completely agree, hoisting this would be much more elegant.

> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 7b45c040cc27..a071d89ace92 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -2350,28 +2350,21 @@ static int kvm_handle_cp_64(struct kvm_vcpu *vcpu,
>   * @run:  The kvm_run struct
>   */
>  static int kvm_handle_cp_32(struct kvm_vcpu *vcpu,
> +			    struct sys_reg_params *params,
>  			    const struct sys_reg_desc *global,
>  			    size_t nr_global)
>  {
> -	struct sys_reg_params params;
> -	u32 esr = kvm_vcpu_get_esr(vcpu);
>  	int Rt  = kvm_vcpu_sys_get_rt(vcpu);
>  
> -	params.CRm = (esr >> 1) & 0xf;
> -	params.regval = vcpu_get_reg(vcpu, Rt);
> -	params.is_write = ((esr & 1) == 0);
> -	params.CRn = (esr >> 10) & 0xf;
> -	params.Op0 = 0;
> -	params.Op1 = (esr >> 14) & 0x7;
> -	params.Op2 = (esr >> 17) & 0x7;
> +	params->regval = vcpu_get_reg(vcpu, Rt);
>  
> -	if (!emulate_cp(vcpu, &params, global, nr_global)) {
> -		if (!params.is_write)
> -			vcpu_set_reg(vcpu, Rt, params.regval);
> +	if (!emulate_cp(vcpu, params, global, nr_global)) {
> +		if (!params->is_write)
> +			vcpu_set_reg(vcpu, Rt, params->regval);
>  		return 1;
>  	}
>  
> -	unhandled_cp_access(vcpu, &params);
> +	unhandled_cp_access(vcpu, params);
>  	return 1;
>  }
>  
> @@ -2382,7 +2375,14 @@ int kvm_handle_cp15_64(struct kvm_vcpu *vcpu)
>  
>  int kvm_handle_cp15_32(struct kvm_vcpu *vcpu)
>  {
> -	return kvm_handle_cp_32(vcpu, cp15_regs, ARRAY_SIZE(cp15_regs));
> +	struct sys_reg_params params;
> +
> +	params = esr_cp1x_32_to_params(kvm_vcpu_get_esr(vcpu));
> +
> +	if (params.Op1 == 0 && params.CRn == 0 && params.CRm)
> +		return kvm_emulate_cp15_id_reg(vcpu, &params);
> +
> +	return kvm_handle_cp_32(vcpu, &params, cp15_regs, ARRAY_SIZE(cp15_regs));
>  }
>  
>  int kvm_handle_cp14_64(struct kvm_vcpu *vcpu)
> @@ -2392,7 +2392,11 @@ int kvm_handle_cp14_64(struct kvm_vcpu *vcpu)
>  
>  int kvm_handle_cp14_32(struct kvm_vcpu *vcpu)
>  {
> -	return kvm_handle_cp_32(vcpu, cp14_regs, ARRAY_SIZE(cp14_regs));
> +	struct sys_reg_params params;
> +
> +	params = esr_cp1x_32_to_params(kvm_vcpu_get_esr(vcpu));
> +
> +	return kvm_handle_cp_32(vcpu, &params, cp14_regs, ARRAY_SIZE(cp14_regs));
>  }
>  
>  static bool is_imp_def_sys_reg(struct sys_reg_params *params)
> diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
> index cc0cc95a0280..fd4b2bb8c782 100644
> --- a/arch/arm64/kvm/sys_regs.h
> +++ b/arch/arm64/kvm/sys_regs.h
> @@ -35,6 +35,13 @@ struct sys_reg_params {
>  				  .Op2 = ((esr) >> 17) & 0x7,                  \
>  				  .is_write = !((esr) & 1) })
>  
> +#define esr_cp1x_32_to_params(esr)					       \
> +	((struct sys_reg_params){ .Op1 = ((esr) >> 14) & 0x7,                  \
> +				  .CRn = ((esr) >> 10) & 0xf,                  \
> +				  .CRm = ((esr) >> 1) & 0xf,                   \
> +				  .Op2 = ((esr) >> 17) & 0x7,                  \
> +				  .is_write = !((esr) & 1) })
> +
>  struct sys_reg_desc {
>  	/* Sysreg string for debug */
>  	const char *name;
> 
> 
> What do you think?

Way better. Your suggested patch looks correct, I'll fold all of this
together and test it out. Thanks for the suggestions :)

--
Best,
Oliver

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

Re: [PATCH v2 1/3] KVM: arm64: Wire up CP15 feature registers to their AArch64 equivalents

[Oliver Upton]

Hi Marc,

On Wed, Apr 06, 2022 at 04:07:28PM +0100, Marc Zyngier wrote:
> > +	/*
> > +	 * All registers where CRm > 3 are known to be UNKNOWN/RAZ from AArch32.
> > +	 * Avoid conflicting with future expansion of AArch64 feature registers
> > +	 * and simply treat them as RAZ here.
> > +	 */
> > +	if (params->CRm > 3)
> > +		params->regval = 0;
> > +	else
> > +		ret = emulate_sys_reg(vcpu, params);
> > +
> > +	vcpu_set_reg(vcpu, Rt, params->regval);
> 
> It feels odd to update Rt without checking whether the read has
> succeeded. In your case, this is harmless, but would break with the
> approach I'm outlining below.
> 

A total kludge to avoid yet another level of indentation :) I'll go
about this the right way next spin.

> > +	return ret;
> > +}
> > +
> > +/**
> > + * kvm_is_cp15_id_reg() - Returns true if the specified CP15 register is an
> > + *			  AArch32 ID register.
> > + * @params: the system register access parameters
> > + *
> > + * Note that CP15 ID registers where CRm=0 are excluded from this check. The
> > + * only register trapped in the CRm=0 range is CTR, which is already handled in
> > + * the cp15 register table.
> 
> There is also the fact that CTR_EL0 has Op1=3 while CTR has Op1=0,
> which prevents it from fitting in your scheme.
> 
> > + */
> > +static inline bool kvm_is_cp15_id_reg(struct sys_reg_params *params)
> > +{
> > +	return params->CRn == 0 && params->Op1 == 0 && params->CRm != 0;
> > +}
> > +
> >  /**
> >   * kvm_handle_cp_32 -- handles a mrc/mcr trap on a guest CP14/CP15 access
> >   * @vcpu: The VCPU pointer
> > @@ -2360,6 +2421,13 @@ static int kvm_handle_cp_32(struct kvm_vcpu *vcpu,
> >  	params.Op1 = (esr >> 14) & 0x7;
> >  	params.Op2 = (esr >> 17) & 0x7;
> >  
> > +	/*
> > +	 * Certain AArch32 ID registers are handled by rerouting to the AArch64
> > +	 * system register table.
> > +	 */
> > +	if (ESR_ELx_EC(esr) == ESR_ELx_EC_CP15_32 && kvm_is_cp15_id_reg(&params))
> > +		return kvm_emulate_cp15_id_reg(vcpu, &params);
> 
> I think this is a bit ugly. We reach this point from a function that
> was cp15-specific, and now we are reconstructing the context. I'd
> rather this is moved to kvm_handle_cp15_32(), and treated there
> (untested):
>

Completely agree, hoisting this would be much more elegant.

> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 7b45c040cc27..a071d89ace92 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -2350,28 +2350,21 @@ static int kvm_handle_cp_64(struct kvm_vcpu *vcpu,
>   * @run:  The kvm_run struct
>   */
>  static int kvm_handle_cp_32(struct kvm_vcpu *vcpu,
> +			    struct sys_reg_params *params,
>  			    const struct sys_reg_desc *global,
>  			    size_t nr_global)
>  {
> -	struct sys_reg_params params;
> -	u32 esr = kvm_vcpu_get_esr(vcpu);
>  	int Rt  = kvm_vcpu_sys_get_rt(vcpu);
>  
> -	params.CRm = (esr >> 1) & 0xf;
> -	params.regval = vcpu_get_reg(vcpu, Rt);
> -	params.is_write = ((esr & 1) == 0);
> -	params.CRn = (esr >> 10) & 0xf;
> -	params.Op0 = 0;
> -	params.Op1 = (esr >> 14) & 0x7;
> -	params.Op2 = (esr >> 17) & 0x7;
> +	params->regval = vcpu_get_reg(vcpu, Rt);
>  
> -	if (!emulate_cp(vcpu, &params, global, nr_global)) {
> -		if (!params.is_write)
> -			vcpu_set_reg(vcpu, Rt, params.regval);
> +	if (!emulate_cp(vcpu, params, global, nr_global)) {
> +		if (!params->is_write)
> +			vcpu_set_reg(vcpu, Rt, params->regval);
>  		return 1;
>  	}
>  
> -	unhandled_cp_access(vcpu, &params);
> +	unhandled_cp_access(vcpu, params);
>  	return 1;
>  }
>  
> @@ -2382,7 +2375,14 @@ int kvm_handle_cp15_64(struct kvm_vcpu *vcpu)
>  
>  int kvm_handle_cp15_32(struct kvm_vcpu *vcpu)
>  {
> -	return kvm_handle_cp_32(vcpu, cp15_regs, ARRAY_SIZE(cp15_regs));
> +	struct sys_reg_params params;
> +
> +	params = esr_cp1x_32_to_params(kvm_vcpu_get_esr(vcpu));
> +
> +	if (params.Op1 == 0 && params.CRn == 0 && params.CRm)
> +		return kvm_emulate_cp15_id_reg(vcpu, &params);
> +
> +	return kvm_handle_cp_32(vcpu, &params, cp15_regs, ARRAY_SIZE(cp15_regs));
>  }
>  
>  int kvm_handle_cp14_64(struct kvm_vcpu *vcpu)
> @@ -2392,7 +2392,11 @@ int kvm_handle_cp14_64(struct kvm_vcpu *vcpu)
>  
>  int kvm_handle_cp14_32(struct kvm_vcpu *vcpu)
>  {
> -	return kvm_handle_cp_32(vcpu, cp14_regs, ARRAY_SIZE(cp14_regs));
> +	struct sys_reg_params params;
> +
> +	params = esr_cp1x_32_to_params(kvm_vcpu_get_esr(vcpu));
> +
> +	return kvm_handle_cp_32(vcpu, &params, cp14_regs, ARRAY_SIZE(cp14_regs));
>  }
>  
>  static bool is_imp_def_sys_reg(struct sys_reg_params *params)
> diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
> index cc0cc95a0280..fd4b2bb8c782 100644
> --- a/arch/arm64/kvm/sys_regs.h
> +++ b/arch/arm64/kvm/sys_regs.h
> @@ -35,6 +35,13 @@ struct sys_reg_params {
>  				  .Op2 = ((esr) >> 17) & 0x7,                  \
>  				  .is_write = !((esr) & 1) })
>  
> +#define esr_cp1x_32_to_params(esr)					       \
> +	((struct sys_reg_params){ .Op1 = ((esr) >> 14) & 0x7,                  \
> +				  .CRn = ((esr) >> 10) & 0xf,                  \
> +				  .CRm = ((esr) >> 1) & 0xf,                   \
> +				  .Op2 = ((esr) >> 17) & 0x7,                  \
> +				  .is_write = !((esr) & 1) })
> +
>  struct sys_reg_desc {
>  	/* Sysreg string for debug */
>  	const char *name;
> 
> 
> What do you think?

Way better. Your suggested patch looks correct, I'll fold all of this
together and test it out. Thanks for the suggestions :)

--
Best,
Oliver