On Tue, 13 Aug 2024 16:24:52 +0100,
Joey Gouly [off-list ref] wrote:

On Tue, Aug 13, 2024 at 03:47:37PM +0100, Marc Zyngier wrote:

quoted

Add the FEAT_S1PIE EL2 registers the sysreg descriptor array so that
they can be handled as a trap.

Access to these registers is conditionned on ID_AA64MMFR3_EL1.S1PIE
being advertised.

Similarly to other other changes, PIRE0_EL2 is guaranteed to trap
thanks to the D22677 update to the architecture..

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/sys_regs.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 52250db3c122..a5f604e24e05 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c

@@ -346,6 +346,18 @@ static bool access_rw(struct kvm_vcpu *vcpu,
 	return true;
 }
 
+static bool check_s1pie_access_rw(struct kvm_vcpu *vcpu,
+				  struct sys_reg_params *p,
+				  const struct sys_reg_desc *r)
+{
+	if (!kvm_has_feat(vcpu->kvm, ID_AA64MMFR3_EL1, S1PIE, IMP)) {
+		kvm_inject_undefined(vcpu);
+		return false;
+	}
+
+	return access_rw(vcpu, p, r);
+}
+
 /*
  * See note at ARMv7 ARM B1.14.4 (TL;DR: S/W ops are not easily virtualized).
  */

@@ -2827,6 +2839,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	EL2_REG(HPFAR_EL2, access_rw, reset_val, 0),
 
 	EL2_REG(MAIR_EL2, access_rw, reset_val, 0),
+	EL2_REG(PIRE0_EL2, check_s1pie_access_rw, reset_val, 0),
+	EL2_REG(PIR_EL2, check_s1pie_access_rw, reset_val, 0),
 	EL2_REG(AMAIR_EL2, access_rw, reset_val, 0),
 
 	EL2_REG(VBAR_EL2, access_rw, reset_val, 0),

I think we should also use this for PIR_EL1 / PIRE0_EL1? We have NULL for their access field.

	{ SYS_DESC(SYS_PIR_EL1), NULL, reset_unknown, PIR_EL1 },

I don't think we need this. In general, the EL1 FEAT_S1PIE registers
are directly accessed by the VM, and do not trap.

However, if the VM has been configured to not expose S1PIE, then we
set the corresponding FGU bits in kvm_calculate_traps():

	if (!kvm_has_feat(kvm, ID_AA64MMFR3_EL1, S1PIE, IMP))
		kvm->arch.fgu[HFGxTR_GROUP] |= (HFGxTR_EL2_nPIRE0_EL1 |
						HFGxTR_EL2_nPIR_EL1);

The effect of this is that we don't even make to the sysreg array, and
inject an UNDEF directly from the point of decoding the trap (see the
beginning of triage_sysreg_trap()).

For EL2 registers, there is no concept of FGT since they always trap,
so no architectural trick we can play to shortcut the handling.
Therefore we make it to the handler and have to triage things there.

Does it make sense?

	M.

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