linux-kernel - Re: [PATCH v4 3/6] KVM: arm64: add emulation for CTR

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date: Mon, 17 Jun 2024 12:45:21 +0200 (CEST)
From: Sebastian Ott <sebott@...hat.com>
To: Oliver Upton <oliver.upton@...ux.dev>
cc: linux-arm-kernel@...ts.infradead.org, kvmarm@...ts.linux.dev, 
    linux-kernel@...r.kernel.org, Marc Zyngier <maz@...nel.org>, 
    James Morse <james.morse@....com>, 
    Suzuki K Poulose <suzuki.poulose@....com>, 
    Catalin Marinas <catalin.marinas@....com>, Will Deacon <will@...nel.org>, 
    Shaoqin Huang <shahuang@...hat.com>, Eric Auger <eric.auger@...hat.com>
Subject: Re: [PATCH v4 3/6] KVM: arm64: add emulation for CTR_EL0 register

On Fri, 14 Jun 2024, Oliver Upton wrote:
> 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?

Sure go ahead.

Thanks,
Sebastian


>
> commit 6bf81bd50dc16309a627863948d49cfeeb00897e
> Author: Sebastian Ott <sebott@...hat.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@...hat.com>
>    Reviewed-by: Shaoqin Huang <shahuang@...hat.com>
>    Signed-off-by: Oliver Upton <oliver.upton@...ux.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
>
>