Re: [Qemu-devel] [PATCH v2 0/5] target/arm: KVM vs ARMISARegisters

[Marc Zyngier <marc.zyngier@arm.com>]

Hi Richard,

On Sun, 04 Nov 2018 09:50:29 +0000,
Richard Henderson <richard.henderson@linaro.org> wrote:
> 
> On 11/3/18 12:32 PM, Marc Zyngier wrote:
> > We actively hide the LORegion feature from the guest since
> > cc33c4e20185a391766ed5e78e2acc97e17ba511 (in the 4.17 time frame), so
> > you shouldn't be able to obtain these on a recent host.
> 
> I don't think that patch is ideal.
> 
> In particular, LOR is a mandatory requirement of ARMv8.1.
> The OS can rightly presume it is present in context.
> 
> Better, I think, is to trap the LOR registers, as you are doing,
> and have them act as RAZ/WI.  This indicates, via LORID_EL1, that
> there are zero supported LO regions, which is a valid ARMv8.1
> configuration.

I agree this looks like a much better solution. I seem to remember
Mark (now cc'd) battling some half baked implementation that didn't
expose the LOR feature, and yet allowed the various LOR registers to
be freely used, with unknown consequences.

Since we unfortunately need to handle that sorry excuse for a CPU (and
assuming I remember the case correctly), I'd suggest the following
(untested) change:

- We leave the LOR feature visible
- We handle the LOR registers as RAZ/WI
- We still delivering an UNDEF when we're in the aforementioned case.

It would also solve the pathological cases that would result from
mixing 8.0 and 8.1+ CPUs in the same system (yeah, we all foolishly
thought it would never happen...).

Thoughts?

	M.

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 22fbbdbece3c..d50f912d3f4a 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -314,10 +314,15 @@ static bool trap_raz_wi(struct kvm_vcpu *vcpu,
 		return read_zero(vcpu, p);
 }
 
-static bool trap_undef(struct kvm_vcpu *vcpu,
-		       struct sys_reg_params *p,
-		       const struct sys_reg_desc *r)
+static bool trap_loregion(struct kvm_vcpu *vcpu,
+			  struct sys_reg_params *p,
+			  const struct sys_reg_desc *r)
 {
+	u64 val = read_sanitised_ftr_reg(SYS_ID_AA64MMFR1_EL1);
+
+	if (val & (0xfUL << ID_AA64MMFR1_LOR_SHIFT))
+		return trap_raz_wi(vcpu, p, r);
+
 	kvm_inject_undefined(vcpu);
 	return false;
 }
@@ -1040,11 +1045,6 @@ static u64 read_id_reg(struct sys_reg_desc const *r, bool raz)
 			kvm_debug("SVE unsupported for guests, suppressing\n");
 
 		val &= ~(0xfUL << ID_AA64PFR0_SVE_SHIFT);
-	} else if (id == SYS_ID_AA64MMFR1_EL1) {
-		if (val & (0xfUL << ID_AA64MMFR1_LOR_SHIFT))
-			kvm_debug("LORegions unsupported for guests, suppressing\n");
-
-		val &= ~(0xfUL << ID_AA64MMFR1_LOR_SHIFT);
 	}
 
 	return val;
@@ -1330,11 +1330,11 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	{ SYS_DESC(SYS_MAIR_EL1), access_vm_reg, reset_unknown, MAIR_EL1 },
 	{ SYS_DESC(SYS_AMAIR_EL1), access_vm_reg, reset_amair_el1, AMAIR_EL1 },
 
-	{ SYS_DESC(SYS_LORSA_EL1), trap_undef },
-	{ SYS_DESC(SYS_LOREA_EL1), trap_undef },
-	{ SYS_DESC(SYS_LORN_EL1), trap_undef },
-	{ SYS_DESC(SYS_LORC_EL1), trap_undef },
-	{ SYS_DESC(SYS_LORID_EL1), trap_undef },
+	{ SYS_DESC(SYS_LORSA_EL1), trap_loregion },
+	{ SYS_DESC(SYS_LOREA_EL1), trap_loregion },
+	{ SYS_DESC(SYS_LORN_EL1), trap_loregion },
+	{ SYS_DESC(SYS_LORC_EL1), trap_loregion },
+	{ SYS_DESC(SYS_LORID_EL1), trap_loregion },
 
 	{ SYS_DESC(SYS_VBAR_EL1), NULL, reset_val, VBAR_EL1, 0 },
 	{ SYS_DESC(SYS_DISR_EL1), NULL, reset_val, DISR_EL1, 0 },


-- 
Jazz is not dead, it just smell funny.