Received-SPF: pass (google.com: domain of linux-kernel+bounces-215406-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Fri, 14 Jun 2024 18:32:37 +0000
From: Oliver Upton <oliver.upton@linux.dev>
To: Sebastian Ott <sebott@redhat.com>
Cc: linux-arm-kernel@lists.infradead.org, kvmarm@lists.linux.dev,
	linux-kernel@vger.kernel.org, Marc Zyngier <maz@kernel.org>,
	James Morse <james.morse@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>, Shaoqin Huang <shahuang@redhat.com>,
	Eric Auger <eric.auger@redhat.com>
Subject: Re: [PATCH v4 3/6] KVM: arm64: add emulation for CTR_EL0 register
Message-ID: <ZmyMxRoKN5VhUW7J@linux.dev>
References: <20240603130507.17597-1-sebott@redhat.com>
 <20240603130507.17597-4-sebott@redhat.com>
 <ZmtwjLbP283ra0Xq@linux.dev>
 <Zmty99X4hnYwtRS4@linux.dev>
 <0b148e21-1714-f4f7-bc77-2a12b990572d@redhat.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <0b148e21-1714-f4f7-bc77-2a12b990572d@redhat.com>

On Fri, Jun 14, 2024 at 05:31:37PM +0200, Sebastian Ott wrote:

[...]

> Hm, but in that case we'd use reset_vm_ftr_id_reg() meaning we would have
> to make IDREG() work for this reg. Either by adding special handling to
> that macro or by increasing kvm->arch.id_regs[] a lot - both options don't
> sound very appealing.

Hiding some of the ugly details behind IDREG() isn't the worst thing,
IMO. The feature ID registers are not laid out contiguously in the
architecture, so it'd make sense that the corresponding KVM code not be
brittle to this.

The other benefit is we initialize kvm->arch.ctr_el0 exactly once, just
like the other ID registers. I believe there's a quirk with this patch
where an initialization that happens after a KVM_SET_ONE_REG on CTR_EL0
will clobber the userspace value.

So, here's where I'm at locally, I'll work it a bit more and try to
densely pack CTR_EL0 into the id_regs array. I also have some (untested)
changes to get CTR_EL0 to show up in the debugfs interface we now have.

Mind if I post what I have afterwards?

commit 6bf81bd50dc16309a627863948d49cfeeb00897e
Author: Sebastian Ott <sebott@redhat.com>
Date:   Mon Jun 3 15:05:03 2024 +0200

    KVM: arm64: Treat CTR_EL0 as a VM feature ID register
    
    Signed-off-by: Sebastian Ott <sebott@redhat.com>
    Reviewed-by: Shaoqin Huang <shahuang@redhat.com>
    Signed-off-by: Oliver Upton <oliver.upton@linux.dev>

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 212ae77eefaf..e5b8cdd70914 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -327,10 +327,20 @@ struct kvm_arch {
 	 */
 #define IDREG_IDX(id)		(((sys_reg_CRm(id) - 1) << 3) | sys_reg_Op2(id))
 #define IDX_IDREG(idx)		sys_reg(3, 0, 0, ((idx) >> 3) + 1, (idx) & Op2_mask)
-#define IDREG(kvm, id)		((kvm)->arch.id_regs[IDREG_IDX(id)])
+#define IDREG(kvm, id)								\
+(*({										\
+	u64 *__reg;								\
+	if ((id) == SYS_CTR_EL0)						\
+		__reg = &(kvm)->arch.ctr_el0;					\
+	else									\
+		__reg = &((kvm)->arch.id_regs[IDREG_IDX(id)]);			\
+	__reg;									\
+}))
 #define KVM_ARM_ID_REG_NUM	(IDREG_IDX(sys_reg(3, 0, 0, 7, 7)) + 1)
 	u64 id_regs[KVM_ARM_ID_REG_NUM];
 
+	u64 ctr_el0;
+
 	/* Masks for VNCR-baked sysregs */
 	struct kvm_sysreg_masks	*sysreg_masks;
 
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index dfabf7aec2c7..1ab2cbbc7a76 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1583,6 +1583,9 @@ static bool is_feature_id_reg(u32 encoding)
  */
 static inline bool is_vm_ftr_id_reg(u32 id)
 {
+	if (id == SYS_CTR_EL0)
+		return true;
+
 	return (sys_reg_Op0(id) == 3 && sys_reg_Op1(id) == 0 &&
 		sys_reg_CRn(id) == 0 && sys_reg_CRm(id) >= 1 &&
 		sys_reg_CRm(id) < 8);
@@ -1886,7 +1889,7 @@ static bool access_ctr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 	if (p->is_write)
 		return write_to_read_only(vcpu, p, r);
 
-	p->regval = read_sanitised_ftr_reg(SYS_CTR_EL0);
+	p->regval = IDREG(vcpu->kvm, SYS_CTR_EL0);
 	return true;
 }
 
@@ -2475,7 +2478,10 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	{ SYS_DESC(SYS_CCSIDR2_EL1), undef_access },
 	{ SYS_DESC(SYS_SMIDR_EL1), undef_access },
 	{ SYS_DESC(SYS_CSSELR_EL1), access_csselr, reset_unknown, CSSELR_EL1 },
-	{ SYS_DESC(SYS_CTR_EL0), access_ctr },
+	ID_WRITABLE(CTR_EL0, CTR_EL0_DIC_MASK |
+			     CTR_EL0_IDC_MASK |
+			     CTR_EL0_DminLine_MASK |
+			     CTR_EL0_IminLine_MASK),
 	{ SYS_DESC(SYS_SVCR), undef_access },
 
 	{ PMU_SYS_REG(PMCR_EL0), .access = access_pmcr, .reset = reset_pmcr,
@@ -3714,18 +3720,11 @@ FUNCTION_INVARIANT(midr_el1)
 FUNCTION_INVARIANT(revidr_el1)
 FUNCTION_INVARIANT(aidr_el1)
 
-static u64 get_ctr_el0(struct kvm_vcpu *v, const struct sys_reg_desc *r)
-{
-	((struct sys_reg_desc *)r)->val = read_sanitised_ftr_reg(SYS_CTR_EL0);
-	return ((struct sys_reg_desc *)r)->val;
-}
-
 /* ->val is filled in by kvm_sys_reg_table_init() */
 static struct sys_reg_desc invariant_sys_regs[] __ro_after_init = {
 	{ SYS_DESC(SYS_MIDR_EL1), NULL, get_midr_el1 },
 	{ SYS_DESC(SYS_REVIDR_EL1), NULL, get_revidr_el1 },
 	{ SYS_DESC(SYS_AIDR_EL1), NULL, get_aidr_el1 },
-	{ SYS_DESC(SYS_CTR_EL0), NULL, get_ctr_el0 },
 };
 
 static int get_invariant_sys_reg(u64 id, u64 __user *uaddr)

-- 
Thanks,
Oliver