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

[Oliver Upton <oliver.upton@linux.dev>]

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