linux-kernel - Re: [PATCH] arm64/mm: Define PTE

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]

Message-ID: <Z8saM94ixmDNjZzV@J2N7QTR9R3.cambridge.arm.com>
Date: Fri, 7 Mar 2025 16:09:23 +0000
From: Mark Rutland <mark.rutland@....com>
To: Anshuman Khandual <anshuman.khandual@....com>
Cc: Ryan Roberts <ryan.roberts@....com>,
	linux-arm-kernel@...ts.infradead.org,
	Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will@...nel.org>,
	Andrey Ryabinin <ryabinin.a.a@...il.com>,
	Alexander Potapenko <glider@...gle.com>,
	Andrey Konovalov <andreyknvl@...il.com>,
	Dmitry Vyukov <dvyukov@...gle.com>,
	Ard Biesheuvel <ardb@...nel.org>, linux-kernel@...r.kernel.org,
	kasan-dev@...glegroups.com
Subject: Re: [PATCH] arm64/mm: Define PTE_SHIFT

On Fri, Mar 07, 2025 at 02:50:56PM +0530, Anshuman Khandual wrote:
> On 3/7/25 14:37, Ryan Roberts wrote:
> > On 07/03/2025 05:08, Anshuman Khandual wrote:

> >>  #define EARLY_LEVEL(lvl, lvls, vstart, vend, add)	\
> >> -	(lvls > lvl ? EARLY_ENTRIES(vstart, vend, SWAPPER_BLOCK_SHIFT + lvl * (PAGE_SHIFT - 3), add) : 0)
> >> +	(lvls > lvl ? EARLY_ENTRIES(vstart, vend, SWAPPER_BLOCK_SHIFT + \
> >> +	lvl * (PAGE_SHIFT - PTE_SHIFT), add) : 0)
> > 
> > nit: not sure what style guide says, but I would indent this continuation an
> > extra level.
> 
> IIUC - An indentation is not normally required with a line continuation although
> the starting letter should match the starting letter in the line above but after
> the '(' (if any).

Regardless of indenttation, the existing code is fairly hard to read,
and I reckon it'd be better to split up, e.g.

| /* Number of VA bits resolved by a single translation table level */
| #define PTDESC_TABLE_SHIFT	(PAGE_SHIFT - PTDESC_ORDER)
| 
| #define __EARLY_LEVEL(lvl, vstart, vend, add) \
| 	EARLY_ENTRIES(vstart, vend, SWAPPER_BLOCK_SHIFT + lvl * PTDESC_TABLE_SHIFT, add)
| 
| #define EARLY_LEVEL(lvl, lvls, vstart, vend, add) \
| 	((lvls) > (lvl) ? __EARLY_LEVEL(lvl, vstart, vend, add) : 0)

... and ignoring the use of _SHIFT vs _ORDER, I think that structure is
far more legible.

With that, we can fold EARLY_ENTRIES() and __EARLY_LEVEL() together and
move the 'add' into EARLY_LEVEL(), e.g.

| /* Number of VA bits resolved by a single translation table level */
| #define PTDESC_TABLE_SHIFT	(PAGE_SHIFT - PTDESC_ORDER)
| 
| #define EARLY_ENTRIES(lvl, vstart, vend) \
| 	(SPAN_NR_ENTRIES(vstart, vend, SWAPPER_BLOCK_SHIFT + lvl * PTDESC_TABLE_SHIFT))
| 
| #define EARLY_LEVEL(lvl, lvls, vstart, vend, add) \
| 	((lvls) > (lvl) ? EARLY_ENTRIES(lvl, vstart, vend) + (add) : 0)

... which I think makes the 'add' a bit easier to understand too.

Mark.