Re: [RFC 5/8] KVM: arm64: Explicitly handle MDSELR_EL1 traps as UNDEFINED

From: Anshuman Khandual <anshuman.khandual@arm.com>
To: Marc Zyngier <maz@kernel.org>
Cc: linux-arm-kernel@lists.infradead.org,
	Jonathan Corbet <corbet@lwn.net>,
	Oliver Upton <oliver.upton@linux.dev>,
	James Morse <james.morse@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>, Mark Brown <broonie@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	kvmarm@lists.linux.dev, linux-kernel@vger.kernel.org
Subject: Re: [RFC 5/8] KVM: arm64: Explicitly handle MDSELR_EL1 traps as UNDEFINED
Date: Fri, 12 Apr 2024 08:11:23 +0530	[thread overview]
Message-ID: <5a2a74b3-f6cd-4cb6-8ee8-5dd7dc2bd686@arm.com> (raw)
In-Reply-To: <86a5m8t8s6.wl-maz@kernel.org>



On 4/5/24 15:45, Marc Zyngier wrote:
> On Fri, 05 Apr 2024 09:00:05 +0100,
> Anshuman Khandual <anshuman.khandual@arm.com> wrote:
>>
>> Currently read_sanitised_id_aa64dfr0_el1() caps the ID_AA64DFR0.DebugVer to
>> ID_AA64DFR0_DebugVer_V8P8, resulting in FEAT_Debugv8p9 not being exposed to
>> the guest. MDSELR_EL1 register access in the guest, is currently trapped by
>> the existing configuration of the fine-grained traps.
> 
> Please add support for the HDFGxTR2_EL2 registers in the trap routing
> arrays, add support for the corresponding FGUs in the corresponding

Afraid that I might not have enough background here to sufficiently understand
your suggestion above, but nonetheless here is an attempt in this regard.

- Add HDFGRTR2_EL2/HDFGWTR2_EL2 to enum vcpu_sysreg
	enum vcpu_sysreg {
		..........
		VNCR(HDFGRTR2_EL2),
		VNCR(HDFGWTR2_EL2),
		..........
	}

- Add their VNCR mappings addresses

	#define VNCR_HDFGRTR2_EL2      0x1A0
	#define VNCR_HDFGWTR2_EL2      0x1B0

- Add HDFGRTR2_EL2/HDFGWTR2_EL2 to sys_reg_descs[]

static const struct sys_reg_desc sys_reg_descs[] = {
	..........
	EL2_REG_VNCR(HDFGRTR2_EL2, reset_val, 0),
	EL2_REG_VNCR(HDFGWTR2_EL2, reset_val, 0),
	..........
}

- Add HDFGRTR2_GROUP to enum fgt_group_id
- Add HDFGRTR2_GROUP to reg_to_fgt_group_id()
- Update triage_sysreg_trap() for HDFGRTR2_GROUP
- Update __activate_traps_hfgxtr() both for HDFGRTR2_EL2 and HDFGWTR2_EL2
- Updated __deactivate_traps_hfgxtr() both for HDFGRTR2_EL2 and HDFGWTR2_EL2

> structure, and condition the UNDEF on the lack of *guest* support for
> the feature.

Does something like the following looks OK for preventing guest access into
MDSELR_EL1 instead ?

--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1711,6 +1711,19 @@ static u64 read_sanitised_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
        return val;
 }
 
+static bool trap_mdselr_el1(struct kvm_vcpu *vcpu,
+                          struct sys_reg_params *p,
+                          const struct sys_reg_desc *r)
+{
+       u64 dfr0 = read_sanitised_id_aa64dfr0_el1(vcpu, r);
+       int dver = cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_EL1_DebugVer_SHIFT);
+
+       if (dver != ID_AA64DFR0_EL1_DebugVer_V8P9)
+               return undef_access(vcpu, p, r);
+
+       return true;
+}
+
 static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
                               const struct sys_reg_desc *rd,
                               u64 val)
@@ -2203,7 +2216,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
        { SYS_DESC(SYS_MDSCR_EL1), trap_debug_regs, reset_val, MDSCR_EL1, 0 },
        DBG_BCR_BVR_WCR_WVR_EL1(2),
        DBG_BCR_BVR_WCR_WVR_EL1(3),
-       { SYS_DESC(SYS_MDSELR_EL1), undef_access },
+       { SYS_DESC(SYS_MDSELR_EL1), trap_mdselr_el1 },
        DBG_BCR_BVR_WCR_WVR_EL1(4),
        DBG_BCR_BVR_WCR_WVR_EL1(5),
        DBG_BCR_BVR_WCR_WVR_EL1(6),

I am sure this is rather incomplete, but will really appreciate if you could
provide some details and pointers.

> 
> In short, implement the architecture as described in the pseudocode,
> and not a cheap shortcut.
> 
> Thanks,
> 
> 	M.
> 
