linux-kernel - Re: [PATCH 2/4] arm64/sysreg: Replace TCR

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] [thread-next>] [day] [month] [year] [list]
Message-ID: <87v7mjk9we.wl-maz@kernel.org>
Date: Tue, 19 Aug 2025 09:12:17 +0100
From: Marc Zyngier <maz@...nel.org>
To: Anshuman Khandual <anshuman.khandual@....com>
Cc: linux-arm-kernel@...ts.infradead.org,
	Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will@...nel.org>,
	Oliver Upton <oliver.upton@...ux.dev>,
	Mark Brown <broonie@...nel.org>,
	Ryan Roberts <ryan.roberts@....com>,
	kvmarm@...ts.linux.dev,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/4] arm64/sysreg: Replace TCR_EL1 field macros

On Tue, 19 Aug 2025 07:46:50 +0100,
Anshuman Khandual <anshuman.khandual@....com> wrote:
> 
> 
> 
> On 18/08/25 9:16 PM, Marc Zyngier wrote:
> > On Mon, 18 Aug 2025 05:57:57 +0100,
> > Anshuman Khandual <anshuman.khandual@....com> wrote:
> >>
> >> This just replaces all used TCR_EL1 field macros with tools sysreg variant
> >> based fields and subsequently drops them from the header (pgtable-hwdef.h).
> >> While here, also drop all the unused TCR_XXX macros from the header.
> >>
> >> Cc: Catalin Marinas <catalin.marinas@....com>
> >> Cc: Will Deacon <will@...nel.org>
> >> Cc: Marc Zyngier <maz@...nel.org>
> >> Cc: Mark Brown <broonie@...nel.org>
> >> Cc: kvmarm@...ts.linux.dev
> >> Cc: linux-arm-kernel@...ts.infradead.org
> >> Cc: linux-kernel@...r.kernel.org
> >> Signed-off-by: Anshuman Khandual <anshuman.khandual@....com>
> >> ---
> >>  arch/arm64/include/asm/assembler.h         |   6 +-
> >>  arch/arm64/include/asm/cputype.h           |   2 +-
> >>  arch/arm64/include/asm/kvm_arm.h           |  28 +++---
> >>  arch/arm64/include/asm/kvm_nested.h        |   6 +-
> >>  arch/arm64/include/asm/mmu_context.h       |   4 +-
> >>  arch/arm64/include/asm/pgtable-hwdef.h     | 107 +++------------------
> >>  arch/arm64/include/asm/pgtable-prot.h      |   2 +-
> >>  arch/arm64/kernel/cpufeature.c             |   4 +-
> >>  arch/arm64/kernel/pi/map_kernel.c          |   8 +-
> >>  arch/arm64/kernel/vmcore_info.c            |   2 +-
> >>  arch/arm64/kvm/arm.c                       |   6 +-
> >>  arch/arm64/kvm/at.c                        |  48 ++++-----
> >>  arch/arm64/kvm/hyp/include/hyp/switch.h    |   2 +-
> >>  arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h |   2 +-
> >>  arch/arm64/kvm/hyp/nvhe/switch.c           |   2 +-
> >>  arch/arm64/kvm/hyp/nvhe/tlb.c              |   2 +-
> >>  arch/arm64/kvm/hyp/vhe/tlb.c               |   2 +-
> >>  arch/arm64/kvm/nested.c                    |   8 +-
> >>  arch/arm64/kvm/pauth.c                     |  12 +--
> >>  arch/arm64/mm/proc.S                       |  29 +++---
> >>  tools/arch/arm64/include/asm/cputype.h     |   2 +-
> >>  21 files changed, 101 insertions(+), 183 deletions(-)
> > 
> > [...]
> > 
> >> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> >> index 888f7c7abf54..b47d6d530e57 100644
> >> --- a/arch/arm64/kvm/arm.c
> >> +++ b/arch/arm64/kvm/arm.c
> >> @@ -2000,10 +2000,10 @@ static void __init cpu_prepare_hyp_mode(int cpu, u32 hyp_va_bits)
> >>  
> >>  	tcr = read_sysreg(tcr_el1);
> >>  	if (cpus_have_final_cap(ARM64_KVM_HVHE)) {
> >> -		tcr &= ~(TCR_HD | TCR_HA | TCR_A1 | TCR_T0SZ_MASK);
> >> -		tcr |= TCR_EPD1_MASK;
> >> +		tcr &= ~(TCR_EL1_HD | TCR_EL1_HA | TCR_EL1_A1 | TCR_EL1_T0SZ_MASK);
> >> +		tcr |= TCR_EL1_EPD1_MASK;
> > 
> > Except that none of that code is about EL1. At all.
> > 
> >>  	} else {
> >> -		unsigned long ips = FIELD_GET(TCR_IPS_MASK, tcr);
> >> +		unsigned long ips = FIELD_GET(TCR_EL1_IPS_MASK, tcr);
> >>  
> >>  		tcr &= TCR_EL2_MASK;
> >>  		tcr |= TCR_EL2_RES1 | FIELD_PREP(TCR_EL2_PS_MASK, ips);
> >> diff --git a/arch/arm64/kvm/at.c b/arch/arm64/kvm/at.c
> >> index 0e5610533949..5f0f10ef38f0 100644
> >> --- a/arch/arm64/kvm/at.c
> >> +++ b/arch/arm64/kvm/at.c
> >> @@ -134,8 +134,8 @@ static int setup_s1_walk(struct kvm_vcpu *vcpu, struct s1_walk_info *wi,
> >>  	tbi = (wi->regime == TR_EL2 ?
> >>  	       FIELD_GET(TCR_EL2_TBI, tcr) :
> >>  	       (va55 ?
> >> -		FIELD_GET(TCR_TBI1, tcr) :
> >> -		FIELD_GET(TCR_TBI0, tcr)));
> >> +		FIELD_GET(TCR_EL1_TBI1, tcr) :
> >> +		FIELD_GET(TCR_EL1_TBI0, tcr)));
> > 
> > This is the reason number one why I dislike this patch.
> > 
> > Here, we deal with both the EL1&0 *and* the EL2&0 translation
> > regimes. And I left the original definition *on purpose* so that
> > nobody would read this code as being EL1-only. Now, you will glance
> > over it with warm fuzzy feeling that you know what this is about --
> > purely EL1. And that's what bugs are made of.
> > 
> > Of course, nothing changed functionally. But is it better? No.
> 
> Just wondering - will it be better to use TCR_EL1/TCR_EL2 definitions
> conditionally for EL1&0 and EL2&0 translation regimes as applicable
> ?

Write the code, look at the result, realise this is totally
useless. Because TCR_EL1 and TCR_EL2 *WHEN E2H==1* are designed to
have the same layout.

> Could there any other better method here ? Because the current
> situation where there are some custom TCR macros, some tools sysreg
> generated macros, and then those macros getting used in an adhoc
> manner in different places, is not very consistent either.

The better way is to leave this stuff alone. Honestly, I don't see any
improvement in repainting the KVM code to make it less readable.

If anything, define the old macros in terms of the new ones, and move
them to be KVM-private.

	M.

-- 
Jazz isn't dead. It just smells funny.