Re: [PATCH v2 08/19] KVM: arm64: Save/restore TCR2_EL1

From: Marc Zyngier <maz@kernel.org>
To: Joey Gouly <joey.gouly@arm.com>
Cc: <linux-arm-kernel@lists.infradead.org>, <nd@arm.com>,
	<broonie@kernel.org>, <catalin.marinas@arm.com>,
	<james.morse@arm.com>, <mark.rutland@arm.com>,
	<oliver.upton@linux.dev>, <suzuki.poulose@arm.com>,
	<will@kernel.org>, <yuzenghui@huawei.com>
Subject: Re: [PATCH v2 08/19] KVM: arm64: Save/restore TCR2_EL1
Date: Thu, 20 Apr 2023 10:13:38 +0100	[thread overview]
Message-ID: <86v8hqkjb1.wl-maz@kernel.org> (raw)
In-Reply-To: <20230413110513.243326-9-joey.gouly@arm.com>

On Thu, 13 Apr 2023 12:05:02 +0100,
Joey Gouly <joey.gouly@arm.com> wrote:
> 
> Define the new system register TCR2_EL1 and context switch it.
> 
> Signed-off-by: Joey Gouly <joey.gouly@arm.com>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Oliver Upton <oliver.upton@linux.dev>
> Cc: James Morse <james.morse@arm.com>
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> Cc: Zenghui Yu <yuzenghui@huawei.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
> ---
>  arch/arm64/include/asm/kvm_host.h          | 1 +
>  arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h | 4 ++++
>  arch/arm64/kvm/sys_regs.c                  | 1 +
>  3 files changed, 6 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index bcd774d74f34..e1137832a01f 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -269,6 +269,7 @@ enum vcpu_sysreg {
>  	TTBR0_EL1,	/* Translation Table Base Register 0 */
>  	TTBR1_EL1,	/* Translation Table Base Register 1 */
>  	TCR_EL1,	/* Translation Control Register */
> +	TCR2_EL1,	/* Extended Translation Control Register */
>  	ESR_EL1,	/* Exception Syndrome Register */
>  	AFSR0_EL1,	/* Auxiliary Fault Status Register 0 */
>  	AFSR1_EL1,	/* Auxiliary Fault Status Register 1 */
> diff --git a/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h b/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
> index 699ea1f8d409..16199a107a47 100644
> --- a/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
> +++ b/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
> @@ -44,6 +44,8 @@ static inline void __sysreg_save_el1_state(struct kvm_cpu_context *ctxt)
>  	ctxt_sys_reg(ctxt, TTBR0_EL1)	= read_sysreg_el1(SYS_TTBR0);
>  	ctxt_sys_reg(ctxt, TTBR1_EL1)	= read_sysreg_el1(SYS_TTBR1);
>  	ctxt_sys_reg(ctxt, TCR_EL1)	= read_sysreg_el1(SYS_TCR);
> +	if (cpus_have_final_cap(ARM64_HAS_TCR2))
> +		ctxt_sys_reg(ctxt, TCR2_EL1)	= read_sysreg_el1(SYS_TCR2);
>  	ctxt_sys_reg(ctxt, ESR_EL1)	= read_sysreg_el1(SYS_ESR);
>  	ctxt_sys_reg(ctxt, AFSR0_EL1)	= read_sysreg_el1(SYS_AFSR0);
>  	ctxt_sys_reg(ctxt, AFSR1_EL1)	= read_sysreg_el1(SYS_AFSR1);
> @@ -114,6 +116,8 @@ static inline void __sysreg_restore_el1_state(struct kvm_cpu_context *ctxt)
>  	write_sysreg_el1(ctxt_sys_reg(ctxt, CPACR_EL1),	SYS_CPACR);
>  	write_sysreg_el1(ctxt_sys_reg(ctxt, TTBR0_EL1),	SYS_TTBR0);
>  	write_sysreg_el1(ctxt_sys_reg(ctxt, TTBR1_EL1),	SYS_TTBR1);
> +	if (cpus_have_final_cap(ARM64_HAS_TCR2))
> +		write_sysreg_el1(ctxt_sys_reg(ctxt, TCR2_EL1),	SYS_TCR2);
>  	write_sysreg_el1(ctxt_sys_reg(ctxt, ESR_EL1),	SYS_ESR);
>  	write_sysreg_el1(ctxt_sys_reg(ctxt, AFSR0_EL1),	SYS_AFSR0);
>  	write_sysreg_el1(ctxt_sys_reg(ctxt, AFSR1_EL1),	SYS_AFSR1);
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 53749d3a0996..5e7e4a433035 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1871,6 +1871,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>  	{ SYS_DESC(SYS_TTBR0_EL1), access_vm_reg, reset_unknown, TTBR0_EL1 },
>  	{ SYS_DESC(SYS_TTBR1_EL1), access_vm_reg, reset_unknown, TTBR1_EL1 },
>  	{ SYS_DESC(SYS_TCR_EL1), access_vm_reg, reset_val, TCR_EL1, 0 },
> +	{ SYS_DESC(SYS_TCR2_EL1), NULL, reset_unknown, TCR2_EL1 },

I'm not convinced reset_unknown is the right thing, at least for the
bits that are defined as "If EL2 and EL3 is not implemented, this bit
resets to 0b0 on a reset."

Given that an EL1 guest isn't in control of EL2, I'm a bit wary that
we start execution of the guest in a context that isn't well defined.
My strong preference would be to reset TCR2 just like TCR, unless you
can provide a explanation of why UNKNOWN is actually more correct.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel