[PATCH v3 0/3] Fine grain sysregs allowed to trap for nested virtualization

[PATCH v3 0/3] Fine grain sysregs allowed to trap for nested virtualization

[Miguel Luis]

The current HCR_EL2 description defines ranges of system register encodings in
which accesses should trap for NV. These ranges include encodings which aren't
defined in the reference manual. In order avoid this, let's rather implement a
more fine grained approach excluding those undefined.

Changes v2 -> v3
 patch 1:
	Fix BRBCR_EL12. (Eric)
	Add Eric's R-b. Thanks!
 patch 2:
	New patch to add missing _EL2 encodings for patch 3.
 patch 3:
	Previously patch 2 in v2.
	Add Fixes tag. (Eric)
	Merge contiguous ranges and exclude undefined encodings. (Eric)

v1 -> v2
 patch 1:
	fix indentation
 patch 2:
	improve commit message (Marc)
	fix indentation (Marc)
	follow kernel comment format (Marc)
	describe LRs in ranges (Marc)
	include AMEVCNTVOFF0<n>_EL2 and AMEVCNTVOFF1<n>_EL2
 patch 3:
	drop. Excluded IMPDEF range is trapped by HCR_EL2.TIDCP

v2: https://lore.kernel.org/kvmarm/20230925162057.27548-1-miguel.luis@oracle.com/
v1: https://lore.kernel.org/kvmarm/20230913185209.32282-1-miguel.luis@oracle.com/

Miguel Luis (3):
  arm64: Add missing _EL12 encodings
  arm64: Add missing _EL2 encodings
  arm64/kvm: Fine grain _EL2 system registers list that affect nested
    virtualization

 arch/arm64/include/asm/sysreg.h | 48 ++++++++++++++++++
 arch/arm64/kvm/emulate-nested.c | 88 ++++++++++++++++++++++++++++++---
 2 files changed, 130 insertions(+), 6 deletions(-)

--
2.39.2

[PATCH v3 1/3] arm64: Add missing _EL12 encodings

[Miguel Luis]

Some _EL12 encodings are missing. Add them.

Reviewed-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Miguel Luis <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

[PATCH v3 2/3] arm64: Add missing _EL2 encodings

[Miguel Luis]

Some _EL2 encodings are missing. Add them.

Signed-off-by: Miguel Luis <miguel.luis@oracle.com>
---
 arch/arm64/include/asm/sysreg.h | 37 +++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
index ba5db50effec..d8e8607c9de8 100644
--- a/arch/arm64/include/asm/sysreg.h
+++ b/arch/arm64/include/asm/sysreg.h
@@ -484,6 +484,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 +498,10 @@
 #define SYS_VTCR_EL2			sys_reg(3, 4, 2, 1, 2)
 
 #define SYS_TRFCR_EL2			sys_reg(3, 4, 1, 2, 1)
+#define SYS_SDER32_EL2			sys_reg(3, 4, 1, 3, 1)
+#define SYS_VNCR_EL2			sys_reg(3, 4, 2, 2, 0)
+#define SYS_VSTTBR_EL2			sys_reg(3, 4, 2, 6, 0)
+#define SYS_VSTCR_EL2			sys_reg(3, 4, 2, 6, 2)
 #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 +519,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 +579,29 @@
 
 #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)
+#define SYS_CNTHVS_TVAL_EL2		sys_reg(3, 4, 14, 4, 0)
+#define SYS_CNTHVS_CTL_EL2		sys_reg(3, 4, 14, 4, 1)
+#define SYS_CNTHVS_CVAL_EL2		sys_reg(3, 4, 14, 4, 2)
+#define SYS_CNTHPS_TVAL_EL2		sys_reg(3, 4, 14, 5, 0)
+#define SYS_CNTHPS_CTL_EL2		sys_reg(3, 4, 14, 5, 1)
+#define SYS_CNTHPS_CVAL_EL2		sys_reg(3, 4, 14, 5, 2)
 
 /* VHE encodings for architectural EL0/1 system registers */
 #define SYS_BRBCR_EL12			sys_reg(2, 5, 9, 0, 0)
-- 
2.39.2

[PATCH v3 3/3] arm64/kvm: Fine grain _EL2 system registers list that affect nested virtualization

[Miguel Luis]

Implement a fine grained approach in the _EL2 sysreg ranges.

Fixes: d0fc0a2519a6 ("KVM: arm64: nv: Add trap forwarding for HCR_EL2")
Signed-off-by: Miguel Luis <miguel.luis@oracle.com>
---
 arch/arm64/kvm/emulate-nested.c | 88 ++++++++++++++++++++++++++++++---
 1 file changed, 82 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/kvm/emulate-nested.c b/arch/arm64/kvm/emulate-nested.c
index 9ced1bf0c2b7..3af49e130ee6 100644
--- a/arch/arm64/kvm/emulate-nested.c
+++ b/arch/arm64/kvm/emulate-nested.c
@@ -648,15 +648,91 @@ 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_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_TRAP(SYS_SDER32_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_TRAP(SYS_VSTTBR_EL2,		CGT_HCR_NV),
+	SR_TRAP(SYS_VSTCR_EL2,		CGT_HCR_NV),
+	SR_TRAP(SYS_DACR32_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),
+	/* SPSR_irq, SPSR_abt, SPSR_und, SPSR_fiq */
+	SR_RANGE_TRAP(sys_reg(3, 4, 4, 3, 0),
+		      sys_reg(3, 4, 4, 3, 3), CGT_HCR_NV),
+	SR_TRAP(SYS_IFSR32_EL2,		CGT_HCR_NV),
+	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_FPEXC32_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),
+	SR_RANGE_TRAP(SYS_CNTHVS_TVAL_EL2,
+		      SYS_CNTHVS_CVAL_EL2, CGT_HCR_NV),
+	SR_RANGE_TRAP(SYS_CNTHPS_TVAL_EL2,
+		      SYS_CNTHPS_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

Re: [PATCH v3 2/3] arm64: Add missing _EL2 encodings

[Eric Auger]

Hi Miguel,

On 10/11/23 20:01, Miguel Luis wrote:
> Some _EL2 encodings are missing. Add them.
>
> Signed-off-by: Miguel Luis <miguel.luis@oracle.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric
> ---
>  arch/arm64/include/asm/sysreg.h | 37 +++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
>
> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> index ba5db50effec..d8e8607c9de8 100644
> --- a/arch/arm64/include/asm/sysreg.h
> +++ b/arch/arm64/include/asm/sysreg.h
> @@ -484,6 +484,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 +498,10 @@
>  #define SYS_VTCR_EL2			sys_reg(3, 4, 2, 1, 2)
>  
>  #define SYS_TRFCR_EL2			sys_reg(3, 4, 1, 2, 1)
> +#define SYS_SDER32_EL2			sys_reg(3, 4, 1, 3, 1)
> +#define SYS_VNCR_EL2			sys_reg(3, 4, 2, 2, 0)
> +#define SYS_VSTTBR_EL2			sys_reg(3, 4, 2, 6, 0)
> +#define SYS_VSTCR_EL2			sys_reg(3, 4, 2, 6, 2)
>  #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 +519,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 +579,29 @@
>  
>  #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)
> +#define SYS_CNTHVS_TVAL_EL2		sys_reg(3, 4, 14, 4, 0)
> +#define SYS_CNTHVS_CTL_EL2		sys_reg(3, 4, 14, 4, 1)
> +#define SYS_CNTHVS_CVAL_EL2		sys_reg(3, 4, 14, 4, 2)
> +#define SYS_CNTHPS_TVAL_EL2		sys_reg(3, 4, 14, 5, 0)
> +#define SYS_CNTHPS_CTL_EL2		sys_reg(3, 4, 14, 5, 1)
> +#define SYS_CNTHPS_CVAL_EL2		sys_reg(3, 4, 14, 5, 2)
>  
>  /* VHE encodings for architectural EL0/1 system registers */
>  #define SYS_BRBCR_EL12			sys_reg(2, 5, 9, 0, 0)

Re: [PATCH v3 3/3] arm64/kvm: Fine grain _EL2 system registers list that affect nested virtualization

[Eric Auger]

Hi Miguel,

On 10/11/23 20:01, Miguel Luis wrote:
> Implement a fine grained approach in the _EL2 sysreg ranges.
>
> Fixes: d0fc0a2519a6 ("KVM: arm64: nv: Add trap forwarding for HCR_EL2")
> Signed-off-by: Miguel Luis <miguel.luis@oracle.com>
> ---
>  arch/arm64/kvm/emulate-nested.c | 88 ++++++++++++++++++++++++++++++---
>  1 file changed, 82 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm64/kvm/emulate-nested.c b/arch/arm64/kvm/emulate-nested.c
> index 9ced1bf0c2b7..3af49e130ee6 100644
> --- a/arch/arm64/kvm/emulate-nested.c
> +++ b/arch/arm64/kvm/emulate-nested.c
> @@ -648,15 +648,91 @@ 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_VPIDR_EL2,		CGT_HCR_NV),
I think you miss DBGVCR32_EL2
> +	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_TRAP(SYS_SDER32_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_TRAP(SYS_VSTTBR_EL2,		CGT_HCR_NV),
> +	SR_TRAP(SYS_VSTCR_EL2,		CGT_HCR_NV),
> +	SR_TRAP(SYS_DACR32_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),
> +	/* SPSR_irq, SPSR_abt, SPSR_und, SPSR_fiq */
> +	SR_RANGE_TRAP(sys_reg(3, 4, 4, 3, 0),
> +		      sys_reg(3, 4, 4, 3, 3), CGT_HCR_NV),
> +	SR_TRAP(SYS_IFSR32_EL2,		CGT_HCR_NV),
> +	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_FPEXC32_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),
you miss BRBCR_EL2
> +	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),
About the MPAM where did you find the pseudo code?
> +	/*
> +	 * 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),
> +	SR_RANGE_TRAP(SYS_CNTHVS_TVAL_EL2,
> +		      SYS_CNTHVS_CVAL_EL2, CGT_HCR_NV),
> +	SR_RANGE_TRAP(SYS_CNTHPS_TVAL_EL2,
> +		      SYS_CNTHPS_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),
Besides this looks good to me and to me this looks safer than the
previous large span approach but that's my taste ;-)

Thanks

Eric

 

Re: [PATCH v3 3/3] arm64/kvm: Fine grain _EL2 system registers list that affect nested virtualization

[Marc Zyngier]

On 2023-10-12 16:22, Eric Auger wrote:
> Hi Miguel,
> 
> On 10/11/23 20:01, Miguel Luis wrote:
>> Implement a fine grained approach in the _EL2 sysreg ranges.
>> 
>> Fixes: d0fc0a2519a6 ("KVM: arm64: nv: Add trap forwarding for 
>> HCR_EL2")
>> Signed-off-by: Miguel Luis <miguel.luis@oracle.com>
>> ---
>>  arch/arm64/kvm/emulate-nested.c | 88 
>> ++++++++++++++++++++++++++++++---
>>  1 file changed, 82 insertions(+), 6 deletions(-)
>> 
>> diff --git a/arch/arm64/kvm/emulate-nested.c 
>> b/arch/arm64/kvm/emulate-nested.c
>> index 9ced1bf0c2b7..3af49e130ee6 100644
>> --- a/arch/arm64/kvm/emulate-nested.c
>> +++ b/arch/arm64/kvm/emulate-nested.c
>> @@ -648,15 +648,91 @@ 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_VPIDR_EL2,		CGT_HCR_NV),
> I think you miss DBGVCR32_EL2

I don't think this register should be part of this list. We don't
support AArch32 with NV, and the spec says that DBGVCR32_EL2 UNDEFs
when EL1 doesn't support AArch32.

So the change that needs to happen is to inject an UNDEF when trapping
a DBGVCR32_EL2and not forward the trap to the guest.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v3 3/3] arm64/kvm: Fine grain _EL2 system registers list that affect nested virtualization

[Miguel Luis]

Hi Eric,

> On 12 Oct 2023, at 15:22, Eric Auger <eric.auger@redhat.com> wrote:
> 
> Hi Miguel,
> 
> On 10/11/23 20:01, Miguel Luis wrote:
>> Implement a fine grained approach in the _EL2 sysreg ranges.
>> 
>> Fixes: d0fc0a2519a6 ("KVM: arm64: nv: Add trap forwarding for HCR_EL2")
>> Signed-off-by: Miguel Luis <miguel.luis@oracle.com>
>> ---
>> arch/arm64/kvm/emulate-nested.c | 88 ++++++++++++++++++++++++++++++---
>> 1 file changed, 82 insertions(+), 6 deletions(-)
>> 
>> diff --git a/arch/arm64/kvm/emulate-nested.c b/arch/arm64/kvm/emulate-nested.c
>> index 9ced1bf0c2b7..3af49e130ee6 100644
>> --- a/arch/arm64/kvm/emulate-nested.c
>> +++ b/arch/arm64/kvm/emulate-nested.c
>> @@ -648,15 +648,91 @@ 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_VPIDR_EL2, CGT_HCR_NV),
> I think you miss DBGVCR32_EL2

That would be op0 == 10, which I indeed didn't considered given the ranges
previously defined. From its pseudocode I see it would make sense only if EL1
would support AArch32 but that seems not to be in the plans.

>> + 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_TRAP(SYS_SDER32_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_TRAP(SYS_VSTTBR_EL2, CGT_HCR_NV),
>> + SR_TRAP(SYS_VSTCR_EL2, CGT_HCR_NV),
>> + SR_TRAP(SYS_DACR32_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),
>> + /* SPSR_irq, SPSR_abt, SPSR_und, SPSR_fiq */
>> + SR_RANGE_TRAP(sys_reg(3, 4, 4, 3, 0),
>> +       sys_reg(3, 4, 4, 3, 3), CGT_HCR_NV),
>> + SR_TRAP(SYS_IFSR32_EL2, CGT_HCR_NV),
>> + 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_FPEXC32_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),
> you miss BRBCR_EL2

Yes, definitely. Same as above, didn't considered op0 == 10, (Table D18-1).
This one seems to me the only one missing too.

>> + 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),
> About the MPAM where did you find the pseudo code?

The pseucode isn't available on the document. I'm following the statement when
HCR_EL2.NV is 1, where:

"The System or Special-purpose registers for which accesses are trapped and
reported using EC syndrome value 0x18 are as follows:
— Registers accessed using MRS or MSR with a name ending in _EL2, except, [...]"

>> + /*
>> +  * 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),
>> + SR_RANGE_TRAP(SYS_CNTHVS_TVAL_EL2,
>> +       SYS_CNTHVS_CVAL_EL2, CGT_HCR_NV),
>> + SR_RANGE_TRAP(SYS_CNTHPS_TVAL_EL2,
>> +       SYS_CNTHPS_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),
> Besides this looks good to me and to me this looks safer than the
> previous large span approach but that's my taste ;-)
> 

Your suggestions made total sense to me.

Thanks!
Miguel

> Thanks
> 
> Eric

Re: [PATCH v3 2/3] arm64: Add missing _EL2 encodings

[Miguel Luis]

Hi Eric,

> On 12 Oct 2023, at 13:46, Eric Auger <eric.auger@redhat.com> wrote:
> 
> Hi Miguel,
> 
> On 10/11/23 20:01, Miguel Luis wrote:
>> Some _EL2 encodings are missing. Add them.
>> 
>> Signed-off-by: Miguel Luis <miguel.luis@oracle.com>
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> 

Thank you.

Miguel

> Eric
>> ---
>> arch/arm64/include/asm/sysreg.h | 37 +++++++++++++++++++++++++++++++++
>> 1 file changed, 37 insertions(+)
>> 
>> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
>> index ba5db50effec..d8e8607c9de8 100644
>> --- a/arch/arm64/include/asm/sysreg.h
>> +++ b/arch/arm64/include/asm/sysreg.h
>> @@ -484,6 +484,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 +498,10 @@
>> #define SYS_VTCR_EL2 sys_reg(3, 4, 2, 1, 2)
>> 
>> #define SYS_TRFCR_EL2 sys_reg(3, 4, 1, 2, 1)
>> +#define SYS_SDER32_EL2 sys_reg(3, 4, 1, 3, 1)
>> +#define SYS_VNCR_EL2 sys_reg(3, 4, 2, 2, 0)
>> +#define SYS_VSTTBR_EL2 sys_reg(3, 4, 2, 6, 0)
>> +#define SYS_VSTCR_EL2 sys_reg(3, 4, 2, 6, 2)
>> #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 +519,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 +579,29 @@
>> 
>> #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)
>> +#define SYS_CNTHVS_TVAL_EL2 sys_reg(3, 4, 14, 4, 0)
>> +#define SYS_CNTHVS_CTL_EL2 sys_reg(3, 4, 14, 4, 1)
>> +#define SYS_CNTHVS_CVAL_EL2 sys_reg(3, 4, 14, 4, 2)
>> +#define SYS_CNTHPS_TVAL_EL2 sys_reg(3, 4, 14, 5, 0)
>> +#define SYS_CNTHPS_CTL_EL2 sys_reg(3, 4, 14, 5, 1)
>> +#define SYS_CNTHPS_CVAL_EL2 sys_reg(3, 4, 14, 5, 2)
>> 
>> /* VHE encodings for architectural EL0/1 system registers */
>> #define SYS_BRBCR_EL12 sys_reg(2, 5, 9, 0, 0)
> 

Re: [PATCH v3 3/3] arm64/kvm: Fine grain _EL2 system registers list that affect nested virtualization

[Eric Auger]

Hi Marc,

On 10/12/23 22:36, Marc Zyngier wrote:
> On 2023-10-12 16:22, Eric Auger wrote:
>> Hi Miguel,
>>
>> On 10/11/23 20:01, Miguel Luis wrote:
>>> Implement a fine grained approach in the _EL2 sysreg ranges.
>>>
>>> Fixes: d0fc0a2519a6 ("KVM: arm64: nv: Add trap forwarding for HCR_EL2")
>>> Signed-off-by: Miguel Luis <miguel.luis@oracle.com>
>>> ---
>>>  arch/arm64/kvm/emulate-nested.c | 88 ++++++++++++++++++++++++++++++---
>>>  1 file changed, 82 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/arch/arm64/kvm/emulate-nested.c
>>> b/arch/arm64/kvm/emulate-nested.c
>>> index 9ced1bf0c2b7..3af49e130ee6 100644
>>> --- a/arch/arm64/kvm/emulate-nested.c
>>> +++ b/arch/arm64/kvm/emulate-nested.c
>>> @@ -648,15 +648,91 @@ 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_VPIDR_EL2,        CGT_HCR_NV),
>> I think you miss DBGVCR32_EL2
>
> I don't think this register should be part of this list. We don't
> support AArch32 with NV, and the spec says that DBGVCR32_EL2 UNDEFs
> when EL1 doesn't support AArch32.
>
> So the change that needs to happen is to inject an UNDEF when trapping
> a DBGVCR32_EL2and not forward the trap to the guest.

OK this makes sense

Thanks

Eric
>
> Thanks,
>
>         M.

Re: [PATCH v3 3/3] arm64/kvm: Fine grain _EL2 system registers list that affect nested virtualization

[Eric Auger]

Hi Miguel,

On 10/13/23 20:41, Miguel Luis wrote:
> Hi Eric,
>
>> On 12 Oct 2023, at 15:22, Eric Auger <eric.auger@redhat.com> wrote:
>>
>> Hi Miguel,
>>
>> On 10/11/23 20:01, Miguel Luis wrote:
>>> Implement a fine grained approach in the _EL2 sysreg ranges.
>>>
>>> Fixes: d0fc0a2519a6 ("KVM: arm64: nv: Add trap forwarding for HCR_EL2")
>>> Signed-off-by: Miguel Luis <miguel.luis@oracle.com>
>>> ---
>>> arch/arm64/kvm/emulate-nested.c | 88 ++++++++++++++++++++++++++++++---
>>> 1 file changed, 82 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/arch/arm64/kvm/emulate-nested.c b/arch/arm64/kvm/emulate-nested.c
>>> index 9ced1bf0c2b7..3af49e130ee6 100644
>>> --- a/arch/arm64/kvm/emulate-nested.c
>>> +++ b/arch/arm64/kvm/emulate-nested.c
>>> @@ -648,15 +648,91 @@ 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_VPIDR_EL2, CGT_HCR_NV),
>> I think you miss DBGVCR32_EL2
> That would be op0 == 10, which I indeed didn't considered given the ranges
> previously defined. From its pseudocode I see it would make sense only if EL1
> would support AArch32 but that seems not to be in the plans.
>
>>> + 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_TRAP(SYS_SDER32_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_TRAP(SYS_VSTTBR_EL2, CGT_HCR_NV),
>>> + SR_TRAP(SYS_VSTCR_EL2, CGT_HCR_NV),
>>> + SR_TRAP(SYS_DACR32_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),
>>> + /* SPSR_irq, SPSR_abt, SPSR_und, SPSR_fiq */
>>> + SR_RANGE_TRAP(sys_reg(3, 4, 4, 3, 0),
>>> +       sys_reg(3, 4, 4, 3, 3), CGT_HCR_NV),
>>> + SR_TRAP(SYS_IFSR32_EL2, CGT_HCR_NV),
>>> + 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_FPEXC32_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),
>> you miss BRBCR_EL2
> Yes, definitely. Same as above, didn't considered op0 == 10, (Table D18-1).
> This one seems to me the only one missing too.

yep
>
>>> + 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),
>> About the MPAM where did you find the pseudo code?
> The pseucode isn't available on the document. I'm following the statement when
> HCR_EL2.NV is 1, where:
>
> "The System or Special-purpose registers for which accesses are trapped and
> reported using EC syndrome value 0x18 are as follows:
> — Registers accessed using MRS or MSR with a name ending in _EL2, except, [...]"

ok thank you for the clarification. With

BRBCR_EL2 handling, feel free to add my

Reviewed-by: Eric Auger <eric.auger@redhat.com>

(I guess you will handle 
DBGVCR32_EL2 in a separate patch)

Eric

>
>>> + /*
>>> +  * 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),
>>> + SR_RANGE_TRAP(SYS_CNTHVS_TVAL_EL2,
>>> +       SYS_CNTHVS_CVAL_EL2, CGT_HCR_NV),
>>> + SR_RANGE_TRAP(SYS_CNTHPS_TVAL_EL2,
>>> +       SYS_CNTHPS_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),
>> Besides this looks good to me and to me this looks safer than the
>> previous large span approach but that's my taste ;-)
>>
> Your suggestions made total sense to me.
>
> Thanks!
> Miguel
>
>> Thanks
>>
>> Eric
>

Re: [PATCH v3 3/3] arm64/kvm: Fine grain _EL2 system registers list that affect nested virtualization

[Miguel Luis]

Hi Eric,

> On 16 Oct 2023, at 09:54, Eric Auger <eric.auger@redhat.com> wrote:
> 
> Hi Miguel,
> 
> On 10/13/23 20:41, Miguel Luis wrote:
>> Hi Eric,
>> 
>>> On 12 Oct 2023, at 15:22, Eric Auger <eric.auger@redhat.com> wrote:
>>> 
>>> Hi Miguel,
>>> 
>>> On 10/11/23 20:01, Miguel Luis wrote:
>>>> Implement a fine grained approach in the _EL2 sysreg ranges.
>>>> 
>>>> Fixes: d0fc0a2519a6 ("KVM: arm64: nv: Add trap forwarding for HCR_EL2")
>>>> Signed-off-by: Miguel Luis <miguel.luis@oracle.com>
>>>> ---
>>>> arch/arm64/kvm/emulate-nested.c | 88 ++++++++++++++++++++++++++++++---
>>>> 1 file changed, 82 insertions(+), 6 deletions(-)
>>>> 
>>>> diff --git a/arch/arm64/kvm/emulate-nested.c b/arch/arm64/kvm/emulate-nested.c
>>>> index 9ced1bf0c2b7..3af49e130ee6 100644
>>>> --- a/arch/arm64/kvm/emulate-nested.c
>>>> +++ b/arch/arm64/kvm/emulate-nested.c
>>>> @@ -648,15 +648,91 @@ 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_VPIDR_EL2, CGT_HCR_NV),
>>> I think you miss DBGVCR32_EL2
>> That would be op0 == 10, which I indeed didn't considered given the ranges
>> previously defined. From its pseudocode I see it would make sense only if EL1
>> would support AArch32 but that seems not to be in the plans.
>> 
>>>> + 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_TRAP(SYS_SDER32_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_TRAP(SYS_VSTTBR_EL2, CGT_HCR_NV),
>>>> + SR_TRAP(SYS_VSTCR_EL2, CGT_HCR_NV),
>>>> + SR_TRAP(SYS_DACR32_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),
>>>> + /* SPSR_irq, SPSR_abt, SPSR_und, SPSR_fiq */
>>>> + SR_RANGE_TRAP(sys_reg(3, 4, 4, 3, 0),
>>>> +       sys_reg(3, 4, 4, 3, 3), CGT_HCR_NV),
>>>> + SR_TRAP(SYS_IFSR32_EL2, CGT_HCR_NV),
>>>> + 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_FPEXC32_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),
>>> you miss BRBCR_EL2
>> Yes, definitely. Same as above, didn't considered op0 == 10, (Table D18-1).
>> This one seems to me the only one missing too.
> 
> yep

Thanks for confirming it!

>> 
>>>> + 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),
>>> About the MPAM where did you find the pseudo code?
>> The pseucode isn't available on the document. I'm following the statement when
>> HCR_EL2.NV is 1, where:
>> 
>> "The System or Special-purpose registers for which accesses are trapped and
>> reported using EC syndrome value 0x18 are as follows:
>> — Registers accessed using MRS or MSR with a name ending in _EL2, except, [...]"
> 
> ok thank you for the clarification. With
> 
> BRBCR_EL2 handling, feel free to add my
> 
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> 

Thank you Eric!

> (I guess you will handle 
> DBGVCR32_EL2 in a separate patch)
> 

I think that Marc is addressing it here: https://lore.kernel.org/kvmarm/20231013223311.3950585-1-maz@kernel.org/
but I can be wrong.

Marc, could you please confirm ?

( re: https://lore.kernel.org/kvmarm/e6f3002c10848e911c4bfee3a1d472aa@kernel.org/ )

Thank you both in advance.

Miguel

> Eric
> 
>> 
>>>> + /*
>>>> +  * 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),
>>>> + SR_RANGE_TRAP(SYS_CNTHVS_TVAL_EL2,
>>>> +       SYS_CNTHVS_CVAL_EL2, CGT_HCR_NV),
>>>> + SR_RANGE_TRAP(SYS_CNTHPS_TVAL_EL2,
>>>> +       SYS_CNTHPS_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),
>>> Besides this looks good to me and to me this looks safer than the
>>> previous large span approach but that's my taste ;-)
>>> 
>> Your suggestions made total sense to me.
>> 
>> Thanks!
>> Miguel
>> 
>>> Thanks
>>> 
>>> Eric

Re: [PATCH v3 3/3] arm64/kvm: Fine grain _EL2 system registers list that affect nested virtualization

[Marc Zyngier]

On 2023-10-16 11:11, Miguel Luis wrote:
> Hi Eric,
> 
>> On 16 Oct 2023, at 09:54, Eric Auger <eric.auger@redhat.com> wrote:
>> 
>> Hi Miguel,
>> 
>> On 10/13/23 20:41, Miguel Luis wrote:
>>> Hi Eric,
>>> 
>>>> On 12 Oct 2023, at 15:22, Eric Auger <eric.auger@redhat.com> wrote:
>>>> 
>>>> Hi Miguel,
>>>> 
>>>> On 10/11/23 20:01, Miguel Luis wrote:
>>>>> Implement a fine grained approach in the _EL2 sysreg ranges.
>>>>> 
>>>>> Fixes: d0fc0a2519a6 ("KVM: arm64: nv: Add trap forwarding for 
>>>>> HCR_EL2")
>>>>> Signed-off-by: Miguel Luis <miguel.luis@oracle.com>
>>>>> ---
>>>>> arch/arm64/kvm/emulate-nested.c | 88 
>>>>> ++++++++++++++++++++++++++++++---
>>>>> 1 file changed, 82 insertions(+), 6 deletions(-)
>>>>> 
>>>>> diff --git a/arch/arm64/kvm/emulate-nested.c 
>>>>> b/arch/arm64/kvm/emulate-nested.c
>>>>> index 9ced1bf0c2b7..3af49e130ee6 100644
>>>>> --- a/arch/arm64/kvm/emulate-nested.c
>>>>> +++ b/arch/arm64/kvm/emulate-nested.c
>>>>> @@ -648,15 +648,91 @@ 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_VPIDR_EL2, CGT_HCR_NV),
>>>> I think you miss DBGVCR32_EL2
>>> That would be op0 == 10, which I indeed didn't considered given the 
>>> ranges
>>> previously defined. From its pseudocode I see it would make sense 
>>> only if EL1
>>> would support AArch32 but that seems not to be in the plans.
>>> 
>>>>> + 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_TRAP(SYS_SDER32_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_TRAP(SYS_VSTTBR_EL2, CGT_HCR_NV),
>>>>> + SR_TRAP(SYS_VSTCR_EL2, CGT_HCR_NV),
>>>>> + SR_TRAP(SYS_DACR32_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),
>>>>> + /* SPSR_irq, SPSR_abt, SPSR_und, SPSR_fiq */
>>>>> + SR_RANGE_TRAP(sys_reg(3, 4, 4, 3, 0),
>>>>> +       sys_reg(3, 4, 4, 3, 3), CGT_HCR_NV),
>>>>> + SR_TRAP(SYS_IFSR32_EL2, CGT_HCR_NV),
>>>>> + 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_FPEXC32_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),
>>>> you miss BRBCR_EL2
>>> Yes, definitely. Same as above, didn't considered op0 == 10, (Table 
>>> D18-1).
>>> This one seems to me the only one missing too.
>> 
>> yep
> 
> Thanks for confirming it!
> 
>>> 
>>>>> + 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),
>>>> About the MPAM where did you find the pseudo code?
>>> The pseucode isn't available on the document. I'm following the 
>>> statement when
>>> HCR_EL2.NV is 1, where:
>>> 
>>> "The System or Special-purpose registers for which accesses are 
>>> trapped and
>>> reported using EC syndrome value 0x18 are as follows:
>>> — Registers accessed using MRS or MSR with a name ending in _EL2, 
>>> except, [...]"
>> 
>> ok thank you for the clarification. With
>> 
>> BRBCR_EL2 handling, feel free to add my
>> 
>> Reviewed-by: Eric Auger <eric.auger@redhat.com>
>> 
> 
> Thank you Eric!
> 
>> (I guess you will handle
>> DBGVCR32_EL2 in a separate patch)
>> 
> 
> I think that Marc is addressing it here:
> https://lore.kernel.org/kvmarm/20231013223311.3950585-1-maz@kernel.org/
> but I can be wrong.
> 
> Marc, could you please confirm ?

Indeed, that's the fix. If you respin this series *now*, I can take
this patch and your series as a set of fixes for 6.6.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...