[PATCH 0/5] KVM: arm64: NV trap forwarding fixes

[PATCH 0/5] KVM: arm64: NV trap forwarding fixes

[Marc Zyngier]

As Miguel was reworking some of the NV trap list, it became clear that
the 32bit handling didn't get much love. So I've taken Miguel's
series, massaged it a bit, and added my own stuff.

Apart from the last patch, the all have been on the list and reviewed.
I was hoping to take it into 6.6, but some of the late rework and the
required testing have made it impossible.

Oliver, if you're happy with the shape of it, I'd appreciate it if you
could take it into 6.7.

Thanks,

	M.

Marc Zyngier (2):
  KVM: arm64: Do not let a L1 hypervisor access the *32_EL2 sysregs
  KVM: arm64: Handle AArch32 SPSR_{irq,abt,und,fiq} as RAZ/WI

Miguel Luis (3):
  arm64: Add missing _EL12 encodings
  arm64: Add missing _EL2 encodings
  KVM: arm64: Refine _EL2 system register list that require trap
    reinjection

 arch/arm64/include/asm/sysreg.h | 45 +++++++++++++++++++
 arch/arm64/kvm/emulate-nested.c | 77 ++++++++++++++++++++++++++++++---
 arch/arm64/kvm/sys_regs.c       | 24 +++++++---
 3 files changed, 133 insertions(+), 13 deletions(-)

-- 
2.39.2


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

[PATCH 1/5] arm64: Add missing _EL12 encodings

[Marc Zyngier]

From: Miguel Luis <miguel.luis@oracle.com>

Some _EL12 encodings are missing. Add them.

Reviewed-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Miguel Luis <miguel.luis@oracle.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
Link: https://lore.kernel.org/r/20231016111743.30331-2-miguel.luis@oracle.com
---
 arch/arm64/include/asm/sysreg.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
index 38296579a4fd..ba5db50effec 100644
--- a/arch/arm64/include/asm/sysreg.h
+++ b/arch/arm64/include/asm/sysreg.h
@@ -567,19 +567,30 @@
 #define SYS_CNTHCTL_EL2			sys_reg(3, 4, 14, 1, 0)
 
 /* VHE encodings for architectural EL0/1 system registers */
+#define SYS_BRBCR_EL12			sys_reg(2, 5, 9, 0, 0)
 #define SYS_SCTLR_EL12			sys_reg(3, 5, 1, 0, 0)
+#define SYS_CPACR_EL12			sys_reg(3, 5, 1, 0, 2)
+#define SYS_SCTLR2_EL12			sys_reg(3, 5, 1, 0, 3)
+#define SYS_ZCR_EL12			sys_reg(3, 5, 1, 2, 0)
+#define SYS_TRFCR_EL12			sys_reg(3, 5, 1, 2, 1)
+#define SYS_SMCR_EL12			sys_reg(3, 5, 1, 2, 6)
 #define SYS_TTBR0_EL12			sys_reg(3, 5, 2, 0, 0)
 #define SYS_TTBR1_EL12			sys_reg(3, 5, 2, 0, 1)
 #define SYS_TCR_EL12			sys_reg(3, 5, 2, 0, 2)
+#define SYS_TCR2_EL12			sys_reg(3, 5, 2, 0, 3)
 #define SYS_SPSR_EL12			sys_reg(3, 5, 4, 0, 0)
 #define SYS_ELR_EL12			sys_reg(3, 5, 4, 0, 1)
 #define SYS_AFSR0_EL12			sys_reg(3, 5, 5, 1, 0)
 #define SYS_AFSR1_EL12			sys_reg(3, 5, 5, 1, 1)
 #define SYS_ESR_EL12			sys_reg(3, 5, 5, 2, 0)
 #define SYS_TFSR_EL12			sys_reg(3, 5, 5, 6, 0)
+#define SYS_FAR_EL12			sys_reg(3, 5, 6, 0, 0)
+#define SYS_PMSCR_EL12			sys_reg(3, 5, 9, 9, 0)
 #define SYS_MAIR_EL12			sys_reg(3, 5, 10, 2, 0)
 #define SYS_AMAIR_EL12			sys_reg(3, 5, 10, 3, 0)
 #define SYS_VBAR_EL12			sys_reg(3, 5, 12, 0, 0)
+#define SYS_CONTEXTIDR_EL12		sys_reg(3, 5, 13, 0, 1)
+#define SYS_SCXTNUM_EL12		sys_reg(3, 5, 13, 0, 7)
 #define SYS_CNTKCTL_EL12		sys_reg(3, 5, 14, 1, 0)
 #define SYS_CNTP_TVAL_EL02		sys_reg(3, 5, 14, 2, 0)
 #define SYS_CNTP_CTL_EL02		sys_reg(3, 5, 14, 2, 1)
-- 
2.39.2


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

[PATCH 2/5] arm64: Add missing _EL2 encodings

[Marc Zyngier]

From: Miguel Luis <miguel.luis@oracle.com>

Some _EL2 encodings are missing. Add them.

Signed-off-by: Miguel Luis <miguel.luis@oracle.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
[maz: dropped secure encodings]
Signed-off-by: Marc Zyngier <maz@kernel.org>
Link: https://lore.kernel.org/r/20231016111743.30331-3-miguel.luis@oracle.com
---
 arch/arm64/include/asm/sysreg.h | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
index ba5db50effec..4a20a7dc5bc4 100644
--- a/arch/arm64/include/asm/sysreg.h
+++ b/arch/arm64/include/asm/sysreg.h
@@ -270,6 +270,8 @@
 /* ETM */
 #define SYS_TRCOSLAR			sys_reg(2, 1, 1, 0, 4)
 
+#define SYS_BRBCR_EL2			sys_reg(2, 4, 9, 0, 0)
+
 #define SYS_MIDR_EL1			sys_reg(3, 0, 0, 0, 0)
 #define SYS_MPIDR_EL1			sys_reg(3, 0, 0, 0, 5)
 #define SYS_REVIDR_EL1			sys_reg(3, 0, 0, 0, 6)
@@ -484,6 +486,7 @@
 
 #define SYS_SCTLR_EL2			sys_reg(3, 4, 1, 0, 0)
 #define SYS_ACTLR_EL2			sys_reg(3, 4, 1, 0, 1)
+#define SYS_SCTLR2_EL2			sys_reg(3, 4, 1, 0, 3)
 #define SYS_HCR_EL2			sys_reg(3, 4, 1, 1, 0)
 #define SYS_MDCR_EL2			sys_reg(3, 4, 1, 1, 1)
 #define SYS_CPTR_EL2			sys_reg(3, 4, 1, 1, 2)
@@ -497,6 +500,7 @@
 #define SYS_VTCR_EL2			sys_reg(3, 4, 2, 1, 2)
 
 #define SYS_TRFCR_EL2			sys_reg(3, 4, 1, 2, 1)
+#define SYS_VNCR_EL2			sys_reg(3, 4, 2, 2, 0)
 #define SYS_HAFGRTR_EL2			sys_reg(3, 4, 3, 1, 6)
 #define SYS_SPSR_EL2			sys_reg(3, 4, 4, 0, 0)
 #define SYS_ELR_EL2			sys_reg(3, 4, 4, 0, 1)
@@ -514,6 +518,18 @@
 
 #define SYS_MAIR_EL2			sys_reg(3, 4, 10, 2, 0)
 #define SYS_AMAIR_EL2			sys_reg(3, 4, 10, 3, 0)
+#define SYS_MPAMHCR_EL2			sys_reg(3, 4, 10, 4, 0)
+#define SYS_MPAMVPMV_EL2		sys_reg(3, 4, 10, 4, 1)
+#define SYS_MPAM2_EL2			sys_reg(3, 4, 10, 5, 0)
+#define __SYS__MPAMVPMx_EL2(x)		sys_reg(3, 4, 10, 6, x)
+#define SYS_MPAMVPM0_EL2		__SYS__MPAMVPMx_EL2(0)
+#define SYS_MPAMVPM1_EL2		__SYS__MPAMVPMx_EL2(1)
+#define SYS_MPAMVPM2_EL2		__SYS__MPAMVPMx_EL2(2)
+#define SYS_MPAMVPM3_EL2		__SYS__MPAMVPMx_EL2(3)
+#define SYS_MPAMVPM4_EL2		__SYS__MPAMVPMx_EL2(4)
+#define SYS_MPAMVPM5_EL2		__SYS__MPAMVPMx_EL2(5)
+#define SYS_MPAMVPM6_EL2		__SYS__MPAMVPMx_EL2(6)
+#define SYS_MPAMVPM7_EL2		__SYS__MPAMVPMx_EL2(7)
 
 #define SYS_VBAR_EL2			sys_reg(3, 4, 12, 0, 0)
 #define SYS_RVBAR_EL2			sys_reg(3, 4, 12, 0, 1)
@@ -562,9 +578,23 @@
 
 #define SYS_CONTEXTIDR_EL2		sys_reg(3, 4, 13, 0, 1)
 #define SYS_TPIDR_EL2			sys_reg(3, 4, 13, 0, 2)
+#define SYS_SCXTNUM_EL2			sys_reg(3, 4, 13, 0, 7)
+
+#define __AMEV_op2(m)			(m & 0x7)
+#define __AMEV_CRm(n, m)		(n | ((m & 0x8) >> 3))
+#define __SYS__AMEVCNTVOFF0n_EL2(m)	sys_reg(3, 4, 13, __AMEV_CRm(0x8, m), __AMEV_op2(m))
+#define SYS_AMEVCNTVOFF0n_EL2(m)	__SYS__AMEVCNTVOFF0n_EL2(m)
+#define __SYS__AMEVCNTVOFF1n_EL2(m)	sys_reg(3, 4, 13, __AMEV_CRm(0xA, m), __AMEV_op2(m))
+#define SYS_AMEVCNTVOFF1n_EL2(m)	__SYS__AMEVCNTVOFF1n_EL2(m)
 
 #define SYS_CNTVOFF_EL2			sys_reg(3, 4, 14, 0, 3)
 #define SYS_CNTHCTL_EL2			sys_reg(3, 4, 14, 1, 0)
+#define SYS_CNTHP_TVAL_EL2		sys_reg(3, 4, 14, 2, 0)
+#define SYS_CNTHP_CTL_EL2		sys_reg(3, 4, 14, 2, 1)
+#define SYS_CNTHP_CVAL_EL2		sys_reg(3, 4, 14, 2, 2)
+#define SYS_CNTHV_TVAL_EL2		sys_reg(3, 4, 14, 3, 0)
+#define SYS_CNTHV_CTL_EL2		sys_reg(3, 4, 14, 3, 1)
+#define SYS_CNTHV_CVAL_EL2		sys_reg(3, 4, 14, 3, 2)
 
 /* VHE encodings for architectural EL0/1 system registers */
 #define SYS_BRBCR_EL12			sys_reg(2, 5, 9, 0, 0)
-- 
2.39.2


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

[PATCH 3/5] KVM: arm64: Refine _EL2 system register list that require trap reinjection

[Marc Zyngier]

From: Miguel Luis <miguel.luis@oracle.com>

Implement a fine grained approach in the _EL2 sysreg range instead of
the current wide cast trap. This ensures that we don't mistakenly
inject the wrong exception into the guest.

Fixes: d0fc0a2519a6 ("KVM: arm64: nv: Add trap forwarding for HCR_EL2")
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Miguel Luis <miguel.luis@oracle.com>
[maz: commit message massaging, dropped secure and AArch32 registers
      from the list]
Signed-off-by: Marc Zyngier <maz@kernel.org>
Link: https://lore.kernel.org/r/20231016111743.30331-4-miguel.luis@oracle.com
---
 arch/arm64/kvm/emulate-nested.c | 77 ++++++++++++++++++++++++++++++---
 1 file changed, 71 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/kvm/emulate-nested.c b/arch/arm64/kvm/emulate-nested.c
index ee902ff2a50f..06185216a297 100644
--- a/arch/arm64/kvm/emulate-nested.c
+++ b/arch/arm64/kvm/emulate-nested.c
@@ -648,15 +648,80 @@ static const struct encoding_to_trap_config encoding_to_cgt[] __initconst = {
 	SR_TRAP(SYS_APGAKEYLO_EL1,	CGT_HCR_APK),
 	SR_TRAP(SYS_APGAKEYHI_EL1,	CGT_HCR_APK),
 	/* All _EL2 registers */
-	SR_RANGE_TRAP(sys_reg(3, 4, 0, 0, 0),
-		      sys_reg(3, 4, 3, 15, 7), CGT_HCR_NV),
+	SR_TRAP(SYS_BRBCR_EL2,		CGT_HCR_NV),
+	SR_TRAP(SYS_VPIDR_EL2,		CGT_HCR_NV),
+	SR_TRAP(SYS_VMPIDR_EL2,		CGT_HCR_NV),
+	SR_TRAP(SYS_SCTLR_EL2,		CGT_HCR_NV),
+	SR_TRAP(SYS_ACTLR_EL2,		CGT_HCR_NV),
+	SR_TRAP(SYS_SCTLR2_EL2,		CGT_HCR_NV),
+	SR_RANGE_TRAP(SYS_HCR_EL2,
+		      SYS_HCRX_EL2,	CGT_HCR_NV),
+	SR_TRAP(SYS_SMPRIMAP_EL2,	CGT_HCR_NV),
+	SR_TRAP(SYS_SMCR_EL2,		CGT_HCR_NV),
+	SR_RANGE_TRAP(SYS_TTBR0_EL2,
+		      SYS_TCR2_EL2,	CGT_HCR_NV),
+	SR_TRAP(SYS_VTTBR_EL2,		CGT_HCR_NV),
+	SR_TRAP(SYS_VTCR_EL2,		CGT_HCR_NV),
+	SR_TRAP(SYS_VNCR_EL2,		CGT_HCR_NV),
+	SR_RANGE_TRAP(SYS_HDFGRTR_EL2,
+		      SYS_HAFGRTR_EL2,	CGT_HCR_NV),
 	/* Skip the SP_EL1 encoding... */
 	SR_TRAP(SYS_SPSR_EL2,		CGT_HCR_NV),
 	SR_TRAP(SYS_ELR_EL2,		CGT_HCR_NV),
-	SR_RANGE_TRAP(sys_reg(3, 4, 4, 1, 1),
-		      sys_reg(3, 4, 10, 15, 7), CGT_HCR_NV),
-	SR_RANGE_TRAP(sys_reg(3, 4, 12, 0, 0),
-		      sys_reg(3, 4, 14, 15, 7), CGT_HCR_NV),
+	/* Skip SPSR_irq, SPSR_abt, SPSR_und, SPSR_fiq */
+	SR_TRAP(SYS_AFSR0_EL2,		CGT_HCR_NV),
+	SR_TRAP(SYS_AFSR1_EL2,		CGT_HCR_NV),
+	SR_TRAP(SYS_ESR_EL2,		CGT_HCR_NV),
+	SR_TRAP(SYS_VSESR_EL2,		CGT_HCR_NV),
+	SR_TRAP(SYS_TFSR_EL2,		CGT_HCR_NV),
+	SR_TRAP(SYS_FAR_EL2,		CGT_HCR_NV),
+	SR_TRAP(SYS_HPFAR_EL2,		CGT_HCR_NV),
+	SR_TRAP(SYS_PMSCR_EL2,		CGT_HCR_NV),
+	SR_TRAP(SYS_MAIR_EL2,		CGT_HCR_NV),
+	SR_TRAP(SYS_AMAIR_EL2,		CGT_HCR_NV),
+	SR_TRAP(SYS_MPAMHCR_EL2,	CGT_HCR_NV),
+	SR_TRAP(SYS_MPAMVPMV_EL2,	CGT_HCR_NV),
+	SR_TRAP(SYS_MPAM2_EL2,		CGT_HCR_NV),
+	SR_RANGE_TRAP(SYS_MPAMVPM0_EL2,
+		      SYS_MPAMVPM7_EL2,	CGT_HCR_NV),
+	/*
+	 * Note that the spec. describes a group of MEC registers
+	 * whose access should not trap, therefore skip the following:
+	 * MECID_A0_EL2, MECID_A1_EL2, MECID_P0_EL2,
+	 * MECID_P1_EL2, MECIDR_EL2, VMECID_A_EL2,
+	 * VMECID_P_EL2.
+	 */
+	SR_RANGE_TRAP(SYS_VBAR_EL2,
+		      SYS_RMR_EL2,	CGT_HCR_NV),
+	SR_TRAP(SYS_VDISR_EL2,		CGT_HCR_NV),
+	/* ICH_AP0R<m>_EL2 */
+	SR_RANGE_TRAP(SYS_ICH_AP0R0_EL2,
+		      SYS_ICH_AP0R3_EL2, CGT_HCR_NV),
+	/* ICH_AP1R<m>_EL2 */
+	SR_RANGE_TRAP(SYS_ICH_AP1R0_EL2,
+		      SYS_ICH_AP1R3_EL2, CGT_HCR_NV),
+	SR_TRAP(SYS_ICC_SRE_EL2,	CGT_HCR_NV),
+	SR_RANGE_TRAP(SYS_ICH_HCR_EL2,
+		      SYS_ICH_EISR_EL2,	CGT_HCR_NV),
+	SR_TRAP(SYS_ICH_ELRSR_EL2,	CGT_HCR_NV),
+	SR_TRAP(SYS_ICH_VMCR_EL2,	CGT_HCR_NV),
+	/* ICH_LR<m>_EL2 */
+	SR_RANGE_TRAP(SYS_ICH_LR0_EL2,
+		      SYS_ICH_LR15_EL2, CGT_HCR_NV),
+	SR_TRAP(SYS_CONTEXTIDR_EL2,	CGT_HCR_NV),
+	SR_TRAP(SYS_TPIDR_EL2,		CGT_HCR_NV),
+	SR_TRAP(SYS_SCXTNUM_EL2,	CGT_HCR_NV),
+	/* AMEVCNTVOFF0<n>_EL2, AMEVCNTVOFF1<n>_EL2  */
+	SR_RANGE_TRAP(SYS_AMEVCNTVOFF0n_EL2(0),
+		      SYS_AMEVCNTVOFF1n_EL2(15), CGT_HCR_NV),
+	/* CNT*_EL2 */
+	SR_TRAP(SYS_CNTVOFF_EL2,	CGT_HCR_NV),
+	SR_TRAP(SYS_CNTPOFF_EL2,	CGT_HCR_NV),
+	SR_TRAP(SYS_CNTHCTL_EL2,	CGT_HCR_NV),
+	SR_RANGE_TRAP(SYS_CNTHP_TVAL_EL2,
+		      SYS_CNTHP_CVAL_EL2, CGT_HCR_NV),
+	SR_RANGE_TRAP(SYS_CNTHV_TVAL_EL2,
+		      SYS_CNTHV_CVAL_EL2, CGT_HCR_NV),
 	/* All _EL02, _EL12 registers */
 	SR_RANGE_TRAP(sys_reg(3, 5, 0, 0, 0),
 		      sys_reg(3, 5, 10, 15, 7), CGT_HCR_NV),
-- 
2.39.2


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

[PATCH 4/5] KVM: arm64: Do not let a L1 hypervisor access the *32_EL2 sysregs

[Marc Zyngier]

DBGVCR32_EL2, DACR32_EL2, IFSR32_EL2 and FPEXC32_EL2 are required to
UNDEF when AArch32 isn't implemented, which is definitely the case when
running NV.

Given that this is the only case where these registers can trap,
unconditionally inject an UNDEF exception.

Signed-off-by: Marc Zyngier <maz@kernel.org>
Reviewed-by: Oliver Upton <oliver.upton@linux.dev>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Link: https://lore.kernel.org/r/20231013223311.3950585-1-maz@kernel.org
---
 arch/arm64/kvm/sys_regs.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 0afd6136e275..0071ccccaf00 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1961,7 +1961,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	// DBGDTR[TR]X_EL0 share the same encoding
 	{ SYS_DESC(SYS_DBGDTRTX_EL0), trap_raz_wi },
 
-	{ SYS_DESC(SYS_DBGVCR32_EL2), NULL, reset_val, DBGVCR32_EL2, 0 },
+	{ SYS_DESC(SYS_DBGVCR32_EL2), trap_undef, reset_val, DBGVCR32_EL2, 0 },
 
 	{ SYS_DESC(SYS_MPIDR_EL1), NULL, reset_mpidr, MPIDR_EL1 },
 
@@ -2380,18 +2380,18 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	EL2_REG(VTTBR_EL2, access_rw, reset_val, 0),
 	EL2_REG(VTCR_EL2, access_rw, reset_val, 0),
 
-	{ SYS_DESC(SYS_DACR32_EL2), NULL, reset_unknown, DACR32_EL2 },
+	{ SYS_DESC(SYS_DACR32_EL2), trap_undef, reset_unknown, DACR32_EL2 },
 	EL2_REG(HDFGRTR_EL2, access_rw, reset_val, 0),
 	EL2_REG(HDFGWTR_EL2, access_rw, reset_val, 0),
 	EL2_REG(SPSR_EL2, access_rw, reset_val, 0),
 	EL2_REG(ELR_EL2, access_rw, reset_val, 0),
 	{ SYS_DESC(SYS_SP_EL1), access_sp_el1},
 
-	{ SYS_DESC(SYS_IFSR32_EL2), NULL, reset_unknown, IFSR32_EL2 },
+	{ SYS_DESC(SYS_IFSR32_EL2), trap_undef, reset_unknown, IFSR32_EL2 },
 	EL2_REG(AFSR0_EL2, access_rw, reset_val, 0),
 	EL2_REG(AFSR1_EL2, access_rw, reset_val, 0),
 	EL2_REG(ESR_EL2, access_rw, reset_val, 0),
-	{ SYS_DESC(SYS_FPEXC32_EL2), NULL, reset_val, FPEXC32_EL2, 0x700 },
+	{ SYS_DESC(SYS_FPEXC32_EL2), trap_undef, reset_val, FPEXC32_EL2, 0x700 },
 
 	EL2_REG(FAR_EL2, access_rw, reset_val, 0),
 	EL2_REG(HPFAR_EL2, access_rw, reset_val, 0),
-- 
2.39.2


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

[PATCH 5/5] KVM: arm64: Handle AArch32 SPSR_{irq,abt,und,fiq} as RAZ/WI

[Marc Zyngier]

When trapping accesses from a NV guest that tries to access
SPSR_{irq,abt,und,fiq}, make sure we handle them as RAZ/WI,
as if AArch32 wasn't implemented.

This involves a bit of repainting to make the visibility
handler more generic.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/include/asm/sysreg.h |  4 ++++
 arch/arm64/kvm/sys_regs.c       | 16 +++++++++++++---
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
index 4a20a7dc5bc4..5e65f51c10d2 100644
--- a/arch/arm64/include/asm/sysreg.h
+++ b/arch/arm64/include/asm/sysreg.h
@@ -505,6 +505,10 @@
 #define SYS_SPSR_EL2			sys_reg(3, 4, 4, 0, 0)
 #define SYS_ELR_EL2			sys_reg(3, 4, 4, 0, 1)
 #define SYS_SP_EL1			sys_reg(3, 4, 4, 1, 0)
+#define SYS_SPSR_irq			sys_reg(3, 4, 4, 3, 0)
+#define SYS_SPSR_abt			sys_reg(3, 4, 4, 3, 1)
+#define SYS_SPSR_und			sys_reg(3, 4, 4, 3, 2)
+#define SYS_SPSR_fiq			sys_reg(3, 4, 4, 3, 3)
 #define SYS_IFSR32_EL2			sys_reg(3, 4, 5, 0, 1)
 #define SYS_AFSR0_EL2			sys_reg(3, 4, 5, 1, 0)
 #define SYS_AFSR1_EL2			sys_reg(3, 4, 5, 1, 1)
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 0071ccccaf00..be1ebd2c5ba0 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1791,8 +1791,8 @@ static unsigned int el2_visibility(const struct kvm_vcpu *vcpu,
  * HCR_EL2.E2H==1, and only in the sysreg table for convenience of
  * handling traps. Given that, they are always hidden from userspace.
  */
-static unsigned int elx2_visibility(const struct kvm_vcpu *vcpu,
-				    const struct sys_reg_desc *rd)
+static unsigned int hidden_user_visibility(const struct kvm_vcpu *vcpu,
+					   const struct sys_reg_desc *rd)
 {
 	return REG_HIDDEN_USER;
 }
@@ -1803,7 +1803,7 @@ static unsigned int elx2_visibility(const struct kvm_vcpu *vcpu,
 	.reset = rst,				\
 	.reg = name##_EL1,			\
 	.val = v,				\
-	.visibility = elx2_visibility,		\
+	.visibility = hidden_user_visibility,	\
 }
 
 /*
@@ -2387,6 +2387,16 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	EL2_REG(ELR_EL2, access_rw, reset_val, 0),
 	{ SYS_DESC(SYS_SP_EL1), access_sp_el1},
 
+	/* AArch32 SPSR_* are RES0 if trapped from a NV guest */
+	{ SYS_DESC(SYS_SPSR_irq), .access = trap_raz_wi,
+	  .visibility = hidden_user_visibility },
+	{ SYS_DESC(SYS_SPSR_abt), .access = trap_raz_wi,
+	  .visibility = hidden_user_visibility },
+	{ SYS_DESC(SYS_SPSR_und), .access = trap_raz_wi,
+	  .visibility = hidden_user_visibility },
+	{ SYS_DESC(SYS_SPSR_fiq), .access = trap_raz_wi,
+	  .visibility = hidden_user_visibility },
+
 	{ SYS_DESC(SYS_IFSR32_EL2), trap_undef, reset_unknown, IFSR32_EL2 },
 	EL2_REG(AFSR0_EL2, access_rw, reset_val, 0),
 	EL2_REG(AFSR1_EL2, access_rw, reset_val, 0),
-- 
2.39.2


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

Re: [PATCH 5/5] KVM: arm64: Handle AArch32 SPSR_{irq,abt,und,fiq} as RAZ/WI

[Miguel Luis]

Hi Marc,

> On 23 Oct 2023, at 09:54, Marc Zyngier <maz@kernel.org> wrote:
> 
> When trapping accesses from a NV guest that tries to access
> SPSR_{irq,abt,und,fiq}, make sure we handle them as RAZ/WI,
> as if AArch32 wasn't implemented.
> 
> This involves a bit of repainting to make the visibility
> handler more generic.
> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
> arch/arm64/include/asm/sysreg.h |  4 ++++
> arch/arm64/kvm/sys_regs.c       | 16 +++++++++++++---
> 2 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> index 4a20a7dc5bc4..5e65f51c10d2 100644
> --- a/arch/arm64/include/asm/sysreg.h
> +++ b/arch/arm64/include/asm/sysreg.h
> @@ -505,6 +505,10 @@
> #define SYS_SPSR_EL2 sys_reg(3, 4, 4, 0, 0)
> #define SYS_ELR_EL2 sys_reg(3, 4, 4, 0, 1)
> #define SYS_SP_EL1 sys_reg(3, 4, 4, 1, 0)
> +#define SYS_SPSR_irq sys_reg(3, 4, 4, 3, 0)
> +#define SYS_SPSR_abt sys_reg(3, 4, 4, 3, 1)
> +#define SYS_SPSR_und sys_reg(3, 4, 4, 3, 2)
> +#define SYS_SPSR_fiq sys_reg(3, 4, 4, 3, 3)
> #define SYS_IFSR32_EL2 sys_reg(3, 4, 5, 0, 1)
> #define SYS_AFSR0_EL2 sys_reg(3, 4, 5, 1, 0)
> #define SYS_AFSR1_EL2 sys_reg(3, 4, 5, 1, 1)
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 0071ccccaf00..be1ebd2c5ba0 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1791,8 +1791,8 @@ static unsigned int el2_visibility(const struct kvm_vcpu *vcpu,
>  * HCR_EL2.E2H==1, and only in the sysreg table for convenience of
>  * handling traps. Given that, they are always hidden from userspace.
>  */
> -static unsigned int elx2_visibility(const struct kvm_vcpu *vcpu,
> -    const struct sys_reg_desc *rd)
> +static unsigned int hidden_user_visibility(const struct kvm_vcpu *vcpu,
> +   const struct sys_reg_desc *rd)
> {
> return REG_HIDDEN_USER;
> }
> @@ -1803,7 +1803,7 @@ static unsigned int elx2_visibility(const struct kvm_vcpu *vcpu,
> .reset = rst, \
> .reg = name##_EL1, \
> .val = v, \
> - .visibility = elx2_visibility, \
> + .visibility = hidden_user_visibility, \
> }
> 
> /*
> @@ -2387,6 +2387,16 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> EL2_REG(ELR_EL2, access_rw, reset_val, 0),
> { SYS_DESC(SYS_SP_EL1), access_sp_el1},
> 
> + /* AArch32 SPSR_* are RES0 if trapped from a NV guest */
> + { SYS_DESC(SYS_SPSR_irq), .access = trap_raz_wi,
> +  .visibility = hidden_user_visibility },
> + { SYS_DESC(SYS_SPSR_abt), .access = trap_raz_wi,
> +  .visibility = hidden_user_visibility },
> + { SYS_DESC(SYS_SPSR_und), .access = trap_raz_wi,
> +  .visibility = hidden_user_visibility },
> + { SYS_DESC(SYS_SPSR_fiq), .access = trap_raz_wi,
> +  .visibility = hidden_user_visibility },
> +

I’m trying to understand this patch and its surroundings.

Those SPSR_* registers UNDEF at EL0. I do not understand
why use REG_HIDDEN_USER instead of REG_HIDDEN.

Also, could you please explain what is happening at PSTATE.EL == EL1
and if EL2Enabled() && HCR_EL2.NV == ‘1’  ?

For my education on this subject, I would be very grateful if you could
help me understand this.

Thank you

Miguel


> { SYS_DESC(SYS_IFSR32_EL2), trap_undef, reset_unknown, IFSR32_EL2 },
> EL2_REG(AFSR0_EL2, access_rw, reset_val, 0),
> EL2_REG(AFSR1_EL2, access_rw, reset_val, 0),
> -- 
> 2.39.2
> 

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

Re: [PATCH 5/5] KVM: arm64: Handle AArch32 SPSR_{irq,abt,und,fiq} as RAZ/WI

[Marc Zyngier]

On Mon, 23 Oct 2023 19:55:10 +0100,
Miguel Luis <miguel.luis@oracle.com> wrote:
> 
> Hi Marc,
> 
> > On 23 Oct 2023, at 09:54, Marc Zyngier <maz@kernel.org> wrote:
> > 
> > When trapping accesses from a NV guest that tries to access
> > SPSR_{irq,abt,und,fiq}, make sure we handle them as RAZ/WI,
> > as if AArch32 wasn't implemented.
> > 
> > This involves a bit of repainting to make the visibility
> > handler more generic.
> > 
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> > arch/arm64/include/asm/sysreg.h |  4 ++++
> > arch/arm64/kvm/sys_regs.c       | 16 +++++++++++++---
> > 2 files changed, 17 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> > index 4a20a7dc5bc4..5e65f51c10d2 100644
> > --- a/arch/arm64/include/asm/sysreg.h
> > +++ b/arch/arm64/include/asm/sysreg.h
> > @@ -505,6 +505,10 @@
> > #define SYS_SPSR_EL2 sys_reg(3, 4, 4, 0, 0)
> > #define SYS_ELR_EL2 sys_reg(3, 4, 4, 0, 1)
> > #define SYS_SP_EL1 sys_reg(3, 4, 4, 1, 0)
> > +#define SYS_SPSR_irq sys_reg(3, 4, 4, 3, 0)
> > +#define SYS_SPSR_abt sys_reg(3, 4, 4, 3, 1)
> > +#define SYS_SPSR_und sys_reg(3, 4, 4, 3, 2)
> > +#define SYS_SPSR_fiq sys_reg(3, 4, 4, 3, 3)
> > #define SYS_IFSR32_EL2 sys_reg(3, 4, 5, 0, 1)
> > #define SYS_AFSR0_EL2 sys_reg(3, 4, 5, 1, 0)
> > #define SYS_AFSR1_EL2 sys_reg(3, 4, 5, 1, 1)
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index 0071ccccaf00..be1ebd2c5ba0 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -1791,8 +1791,8 @@ static unsigned int el2_visibility(const struct kvm_vcpu *vcpu,
> >  * HCR_EL2.E2H==1, and only in the sysreg table for convenience of
> >  * handling traps. Given that, they are always hidden from userspace.
> >  */
> > -static unsigned int elx2_visibility(const struct kvm_vcpu *vcpu,
> > -    const struct sys_reg_desc *rd)
> > +static unsigned int hidden_user_visibility(const struct kvm_vcpu *vcpu,
> > +   const struct sys_reg_desc *rd)
> > {
> > return REG_HIDDEN_USER;
> > }
> > @@ -1803,7 +1803,7 @@ static unsigned int elx2_visibility(const struct kvm_vcpu *vcpu,
> > .reset = rst, \
> > .reg = name##_EL1, \
> > .val = v, \
> > - .visibility = elx2_visibility, \
> > + .visibility = hidden_user_visibility, \
> > }
> > 
> > /*
> > @@ -2387,6 +2387,16 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> > EL2_REG(ELR_EL2, access_rw, reset_val, 0),
> > { SYS_DESC(SYS_SP_EL1), access_sp_el1},
> > 
> > + /* AArch32 SPSR_* are RES0 if trapped from a NV guest */
> > + { SYS_DESC(SYS_SPSR_irq), .access = trap_raz_wi,
> > +  .visibility = hidden_user_visibility },
> > + { SYS_DESC(SYS_SPSR_abt), .access = trap_raz_wi,
> > +  .visibility = hidden_user_visibility },
> > + { SYS_DESC(SYS_SPSR_und), .access = trap_raz_wi,
> > +  .visibility = hidden_user_visibility },
> > + { SYS_DESC(SYS_SPSR_fiq), .access = trap_raz_wi,
> > +  .visibility = hidden_user_visibility },
> > +
> 
> I’m trying to understand this patch and its surroundings.
> 
> Those SPSR_* registers UNDEF at EL0. I do not understand
> why use REG_HIDDEN_USER instead of REG_HIDDEN.

USER here means host userspace, not guest EL0. That's because the
various SPSR_* registers are already visible from userspace as
KVM_REG_ARM_CORE_REG(spsr[KVM_SPSR_*]), and the above entries are
solely for the purpose of handling a trap (and thus must not be
exposed in the list of available sysregs).

This is similar to what we are doing for the ELx2 registers, which are
already exposed as EL0/EL1 registers.

> Also, could you please explain what is happening at PSTATE.EL == EL1
> and if EL2Enabled() && HCR_EL2.NV == ‘1’  ?

We directly take the trap and not forward it. This isn't exactly the
letter of the architecture, but at the same time, treating these
registers as RAZ/WI is the only valid implementation. I don't
immediately see a problem with taking this shortcut.

	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 5/5] KVM: arm64: Handle AArch32 SPSR_{irq,abt,und,fiq} as RAZ/WI

[Oliver Upton]

On Tue, Oct 24, 2023 at 06:25:33PM +0100, Marc Zyngier wrote:
> On Mon, 23 Oct 2023 19:55:10 +0100, Miguel Luis <miguel.luis@oracle.com> wrote:
> > Also, could you please explain what is happening at PSTATE.EL == EL1
> > and if EL2Enabled() && HCR_EL2.NV == ‘1’  ?
> 
> We directly take the trap and not forward it. This isn't exactly the
> letter of the architecture, but at the same time, treating these
> registers as RAZ/WI is the only valid implementation. I don't
> immediately see a problem with taking this shortcut.

Ugh, that's annoying. The other EL2 views of AArch32 state UNDEF if EL1
doesn't implement AArch32. It'd be nice to get a relaxation in the
architecture to allow an UNDEF here.

Broadening the scope to KVM's emulation of AArch64-only behavior, I
think we should be a bit more aggressive in sanitising AArch32 support
from the ID registers. That way any AA64-only behavior in KVM is
architectural from the guest POV.

Maybe something like:

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 57c8190d5438..045f41900433 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1447,6 +1447,16 @@ static unsigned int sve_visibility(const struct kvm_vcpu *vcpu,
 	return REG_HIDDEN;
 }
 
+#define ID_REG_LIMIT_FIELD_ENUM(val, reg, field, limit)			\
+({									\
+	u64 __f_val = FIELD_GET(reg##_##field##_MASK, val);		\
+	(val) &= ~reg##_##field##_MASK;					\
+	(val) |= FIELD_PREP(reg##_##field##_MASK,			\
+			min(__f_val, (u64)reg##_##field##_##limit));	\
+	(val);								\
+})
+
+
 static u64 read_sanitised_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
 					  const struct sys_reg_desc *rd)
 {
@@ -1477,19 +1487,38 @@ static u64 read_sanitised_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
 		val |= SYS_FIELD_PREP_ENUM(ID_AA64PFR0_EL1, GIC, IMP);
 	}
 
+	/*
+	 * Hide AArch32 support if the vCPU wasn't configured for it. The
+	 * architecture requires all higher ELs to be AArch64-only in this
+	 * situation as well.
+	 */
+	if (!vcpu_el1_is_32bit(vcpu)) {
+		val = ID_REG_LIMIT_FIELD_ENUM(val, ID_AA64PFR0_EL1, EL1, IMP);
+		val = ID_REG_LIMIT_FIELD_ENUM(val, ID_AA64PFR0_EL1, EL2, IMP);
+		val = ID_REG_LIMIT_FIELD_ENUM(val, ID_AA64PFR0_EL1, EL3, IMP);
+	}
+
 	val &= ~ID_AA64PFR0_EL1_AMU_MASK;
 
 	return val;
 }
 
-#define ID_REG_LIMIT_FIELD_ENUM(val, reg, field, limit)			       \
-({									       \
-	u64 __f_val = FIELD_GET(reg##_##field##_MASK, val);		       \
-	(val) &= ~reg##_##field##_MASK;					       \
-	(val) |= FIELD_PREP(reg##_##field##_MASK,			       \
-			min(__f_val, (u64)reg##_##field##_##limit));	       \
-	(val);								       \
-})
+static u64 set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
+			       const struct sys_reg_desc *rd,
+			       u64 val)
+{
+	/*
+	 * Older versions of KVM freely reported AArch32 support, even if the
+	 * vCPU was configured for AArch64.
+	 */
+	if (!vcpu_el1_is_32bit(vcpu)) {
+		val = ID_REG_LIMIT_FIELD_ENUM(val, ID_AA64PFR0_EL1, EL1, IMP);
+		val = ID_REG_LIMIT_FIELD_ENUM(val, ID_AA64PFR0_EL1, EL2, IMP);
+		val = ID_REG_LIMIT_FIELD_ENUM(val, ID_AA64PFR0_EL1, EL3, IMP);
+	}
+
+	return set_id_reg(vcpu, rd, val);
+}
 
 static u64 read_sanitised_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
 					  const struct sys_reg_desc *rd)
@@ -2055,7 +2084,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	{ SYS_DESC(SYS_ID_AA64PFR0_EL1),
 	  .access = access_id_reg,
 	  .get_user = get_id_reg,
-	  .set_user = set_id_reg,
+	  .set_user = set_id_aa64pfr0_el1,
 	  .reset = read_sanitised_id_aa64pfr0_el1,
 	  .val = ~(ID_AA64PFR0_EL1_AMU |
 		   ID_AA64PFR0_EL1_MPAM |

-- 
Thanks,
Oliver

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

Re: [PATCH 5/5] KVM: arm64: Handle AArch32 SPSR_{irq,abt,und,fiq} as RAZ/WI

[Oliver Upton]

On Tue, Oct 24, 2023 at 10:41:57PM +0000, Oliver Upton wrote:
> On Tue, Oct 24, 2023 at 06:25:33PM +0100, Marc Zyngier wrote:
> > On Mon, 23 Oct 2023 19:55:10 +0100, Miguel Luis <miguel.luis@oracle.com> wrote:
> > > Also, could you please explain what is happening at PSTATE.EL == EL1
> > > and if EL2Enabled() && HCR_EL2.NV == ‘1’  ?
> > 
> > We directly take the trap and not forward it. This isn't exactly the
> > letter of the architecture, but at the same time, treating these
> > registers as RAZ/WI is the only valid implementation. I don't
> > immediately see a problem with taking this shortcut.
> 
> Ugh, that's annoying. The other EL2 views of AArch32 state UNDEF if EL1
> doesn't implement AArch32. It'd be nice to get a relaxation in the
> architecture to allow an UNDEF here.

Correction (I wasn't thinking): RES0 behavior should be invariant, much
like the UNDEF behavior of the other AA32-specific registers.

-- 
Thanks,
Oliver

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

Re: [PATCH 0/5] KVM: arm64: NV trap forwarding fixes

[Oliver Upton]

On Mon, 23 Oct 2023 10:54:39 +0100, Marc Zyngier wrote:
> As Miguel was reworking some of the NV trap list, it became clear that
> the 32bit handling didn't get much love. So I've taken Miguel's
> series, massaged it a bit, and added my own stuff.
> 
> Apart from the last patch, the all have been on the list and reviewed.
> I was hoping to take it into 6.6, but some of the late rework and the
> required testing have made it impossible.
> 
> [...]

Applied to kvmarm/next, thanks!

[1/5] arm64: Add missing _EL12 encodings
      https://git.kernel.org/kvmarm/kvmarm/c/d5cb781b7741
[2/5] arm64: Add missing _EL2 encodings
      https://git.kernel.org/kvmarm/kvmarm/c/41f6c9344713
[3/5] KVM: arm64: Refine _EL2 system register list that require trap reinjection
      https://git.kernel.org/kvmarm/kvmarm/c/04cf54650554
[4/5] KVM: arm64: Do not let a L1 hypervisor access the *32_EL2 sysregs
      https://git.kernel.org/kvmarm/kvmarm/c/c7d11a61c7f7
[5/5] KVM: arm64: Handle AArch32 SPSR_{irq,abt,und,fiq} as RAZ/WI
      https://git.kernel.org/kvmarm/kvmarm/c/3f7915ccc902

--
Best,
Oliver

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

Re: [PATCH 5/5] KVM: arm64: Handle AArch32 SPSR_{irq,abt,und,fiq} as RAZ/WI

[Marc Zyngier]

On Wed, 25 Oct 2023 00:04:27 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> On Tue, Oct 24, 2023 at 10:41:57PM +0000, Oliver Upton wrote:
> > On Tue, Oct 24, 2023 at 06:25:33PM +0100, Marc Zyngier wrote:
> > > On Mon, 23 Oct 2023 19:55:10 +0100, Miguel Luis <miguel.luis@oracle.com> wrote:
> > > > Also, could you please explain what is happening at PSTATE.EL == EL1
> > > > and if EL2Enabled() && HCR_EL2.NV == ‘1’  ?
> > > 
> > > We directly take the trap and not forward it. This isn't exactly the
> > > letter of the architecture, but at the same time, treating these
> > > registers as RAZ/WI is the only valid implementation. I don't
> > > immediately see a problem with taking this shortcut.
> > 
> > Ugh, that's annoying. The other EL2 views of AArch32 state UNDEF if EL1
> > doesn't implement AArch32. It'd be nice to get a relaxation in the
> > architecture to allow an UNDEF here.
> 
> Correction (I wasn't thinking): RES0 behavior should be invariant, much
> like the UNDEF behavior of the other AA32-specific registers.

I'm not sure what you're asking for exactly here, so let me explain my
understanding of the architecture on this point, which is that the
*32_EL2 registers are different in nature from the SPSR_* registers.

IFAR32_EL2 and co are accessors for the equivalent AArch32 registers.
If AArch32 isn't implemented, then these registers should UNDEF,
because there is nothing to access at all.

The status of SPSR_* is more subtle: the AArch32 exception model is
banked (irq, fiq, abt, und), and for each bank we have a triplet
(LR_*, SP_*, SPSR_*), plus the extra R[8-12]_fiq. On taking an
exception from AArch32 EL1 to AArch64 EL2, all the (LR_*, SP_*,
R*_fiq) are stored as part of the AArch64 GPRs (X16-X30, see I_PYKVS).

And what about SPSR_*? Well, they live as extra registers that are
populated on exception entry. But they are similar in nature to the
GPRs that store the rest of the stuff. When AArch32 isn't implemented,
the natural choice is to keep them around, only as RES0, because they
are just GPRs.

Of course, all of this is an architectural choice. If I had to change
anything, I'd rather have everything to UNDEF. But there is some logic
in the madness. And frankly, we will never see an AArch32-capable,
NV-capable HW implementation ever, so this is all fairly academic.

Cheers,

	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 5/5] KVM: arm64: Handle AArch32 SPSR_{irq,abt,und,fiq} as RAZ/WI

[Oliver Upton]

On Wed, Oct 25, 2023 at 09:28:03AM +0100, Marc Zyngier wrote:
> On Wed, 25 Oct 2023 00:04:27 +0100, Oliver Upton <oliver.upton@linux.dev> wrote:
> > Correction (I wasn't thinking): RES0 behavior should be invariant, much
> > like the UNDEF behavior of the other AA32-specific registers.
> 
> I'm not sure what you're asking for exactly here, so let me explain my
> understanding of the architecture on this point, which is that the
> *32_EL2 registers are different in nature from the SPSR_* registers.

Damn, I still didn't manage to get my point across!

> IFAR32_EL2 and co are accessors for the equivalent AArch32 registers.
> If AArch32 isn't implemented, then these registers should UNDEF,
> because there is nothing to access at all.
> 
> The status of SPSR_* is more subtle: the AArch32 exception model is
> banked (irq, fiq, abt, und), and for each bank we have a triplet
> (LR_*, SP_*, SPSR_*), plus the extra R[8-12]_fiq. On taking an
> exception from AArch32 EL1 to AArch64 EL2, all the (LR_*, SP_*,
> R*_fiq) are stored as part of the AArch64 GPRs (X16-X30, see I_PYKVS).

Thanks. Yeah, I've got a pretty good handle on what's going on here.
What I really was trying to compare is the way these aliases into AA32
state are handled, and the annoying difference between the two sets.

IFAR32 and friends UNDEF unconditionally w/o AArch32, which I quite
like.

To your point, the SPSR_* accessors still trap even if AArch32 is not
implemented. I was suggesting in passing that it'd be nice if the
architecture alternatively allowed for these to read as RES0 (no trap)
if NV==1 and AArch32 is not implemented, which aligns with your change.

But after all...

> we will never see an AArch32-capable, NV-capable HW implementation
> ever, so this is all fairly academic.

None of this matters in the first place :)

-- 
Thanks,
Oliver

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

Re: [PATCH 5/5] KVM: arm64: Handle AArch32 SPSR_{irq,abt,und,fiq} as RAZ/WI

[Marc Zyngier]

On Wed, 25 Oct 2023 09:46:13 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> To your point, the SPSR_* accessors still trap even if AArch32 is not
> implemented. I was suggesting in passing that it'd be nice if the
> architecture alternatively allowed for these to read as RES0 (no trap)
> if NV==1 and AArch32 is not implemented, which aligns with your change.

Ah, I see what you mean now. Yeah, I'll put that on the bucket list of
things I need to write up. It has quite a few entries already, and I
really should get to it.

Thanks,

	M.

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

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

Re: [PATCH 5/5] KVM: arm64: Handle AArch32 SPSR_{irq,abt,und,fiq} as RAZ/WI

[Miguel Luis]

Hi Marc, Oliver,

> On 24 Oct 2023, at 17:25, Marc Zyngier <maz@kernel.org> wrote:
> 
> On Mon, 23 Oct 2023 19:55:10 +0100,
> Miguel Luis <miguel.luis@oracle.com> wrote:
>> 
>> Hi Marc,
>> 
>>> On 23 Oct 2023, at 09:54, Marc Zyngier <maz@kernel.org> wrote:
>>> 
>>> When trapping accesses from a NV guest that tries to access
>>> SPSR_{irq,abt,und,fiq}, make sure we handle them as RAZ/WI,
>>> as if AArch32 wasn't implemented.
>>> 
>>> This involves a bit of repainting to make the visibility
>>> handler more generic.
>>> 
>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>> ---
>>> arch/arm64/include/asm/sysreg.h |  4 ++++
>>> arch/arm64/kvm/sys_regs.c       | 16 +++++++++++++---
>>> 2 files changed, 17 insertions(+), 3 deletions(-)
>>> 
>>> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
>>> index 4a20a7dc5bc4..5e65f51c10d2 100644
>>> --- a/arch/arm64/include/asm/sysreg.h
>>> +++ b/arch/arm64/include/asm/sysreg.h
>>> @@ -505,6 +505,10 @@
>>> #define SYS_SPSR_EL2 sys_reg(3, 4, 4, 0, 0)
>>> #define SYS_ELR_EL2 sys_reg(3, 4, 4, 0, 1)
>>> #define SYS_SP_EL1 sys_reg(3, 4, 4, 1, 0)
>>> +#define SYS_SPSR_irq sys_reg(3, 4, 4, 3, 0)
>>> +#define SYS_SPSR_abt sys_reg(3, 4, 4, 3, 1)
>>> +#define SYS_SPSR_und sys_reg(3, 4, 4, 3, 2)
>>> +#define SYS_SPSR_fiq sys_reg(3, 4, 4, 3, 3)
>>> #define SYS_IFSR32_EL2 sys_reg(3, 4, 5, 0, 1)
>>> #define SYS_AFSR0_EL2 sys_reg(3, 4, 5, 1, 0)
>>> #define SYS_AFSR1_EL2 sys_reg(3, 4, 5, 1, 1)
>>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>>> index 0071ccccaf00..be1ebd2c5ba0 100644
>>> --- a/arch/arm64/kvm/sys_regs.c
>>> +++ b/arch/arm64/kvm/sys_regs.c
>>> @@ -1791,8 +1791,8 @@ static unsigned int el2_visibility(const struct kvm_vcpu *vcpu,
>>> * HCR_EL2.E2H==1, and only in the sysreg table for convenience of
>>> * handling traps. Given that, they are always hidden from userspace.
>>> */
>>> -static unsigned int elx2_visibility(const struct kvm_vcpu *vcpu,
>>> -    const struct sys_reg_desc *rd)
>>> +static unsigned int hidden_user_visibility(const struct kvm_vcpu *vcpu,
>>> +   const struct sys_reg_desc *rd)
>>> {
>>> return REG_HIDDEN_USER;
>>> }
>>> @@ -1803,7 +1803,7 @@ static unsigned int elx2_visibility(const struct kvm_vcpu *vcpu,
>>> .reset = rst, \
>>> .reg = name##_EL1, \
>>> .val = v, \
>>> - .visibility = elx2_visibility, \
>>> + .visibility = hidden_user_visibility, \
>>> }
>>> 
>>> /*
>>> @@ -2387,6 +2387,16 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>>> EL2_REG(ELR_EL2, access_rw, reset_val, 0),
>>> { SYS_DESC(SYS_SP_EL1), access_sp_el1},
>>> 
>>> + /* AArch32 SPSR_* are RES0 if trapped from a NV guest */
>>> + { SYS_DESC(SYS_SPSR_irq), .access = trap_raz_wi,
>>> +  .visibility = hidden_user_visibility },
>>> + { SYS_DESC(SYS_SPSR_abt), .access = trap_raz_wi,
>>> +  .visibility = hidden_user_visibility },
>>> + { SYS_DESC(SYS_SPSR_und), .access = trap_raz_wi,
>>> +  .visibility = hidden_user_visibility },
>>> + { SYS_DESC(SYS_SPSR_fiq), .access = trap_raz_wi,
>>> +  .visibility = hidden_user_visibility },
>>> +
>> 
>> I’m trying to understand this patch and its surroundings.
>> 
>> Those SPSR_* registers UNDEF at EL0. I do not understand
>> why use REG_HIDDEN_USER instead of REG_HIDDEN.
> 
> USER here means host userspace, not guest EL0. That's because the
> various SPSR_* registers are already visible from userspace as
> KVM_REG_ARM_CORE_REG(spsr[KVM_SPSR_*]), and the above entries are
> solely for the purpose of handling a trap (and thus must not be
> exposed in the list of available sysregs).
> 
> This is similar to what we are doing for the ELx2 registers, which are
> already exposed as EL0/EL1 registers.
> 
>> Also, could you please explain what is happening at PSTATE.EL == EL1
>> and if EL2Enabled() && HCR_EL2.NV == ‘1’  ?
> 
> We directly take the trap and not forward it. This isn't exactly the
> letter of the architecture, but at the same time, treating these
> registers as RAZ/WI is the only valid implementation. I don't
> immediately see a problem with taking this shortcut.
> 

Thank you for explaining and expanding on this topic.

I will take some time to absorb all your comments.

Thank you

Miguel


> 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