Re: [PATCH v4] ELF: AT_PAGE_SHIFT_MASK -- supply userspace with available page shifts

On Tue, Feb 13, 2024 at 09:51:01PM +0300, Alexey Dobriyan wrote:

On Fri, Feb 09, 2024 at 04:41:36PM -0800, Kees Cook wrote:

quoted

On Fri, Feb 09, 2024 at 03:30:37PM +0300, Alexey Dobriyan wrote:

quoted

On Mon, Feb 05, 2024 at 04:48:08AM -0800, Kees Cook wrote:

quoted

On Mon, Feb 05, 2024 at 12:51:43PM +0300, Alexey Dobriyan wrote:

quoted

+#define ARCH_AT_PAGE_SHIFT_MASK					\
+	do {							\
+		u32 val = 1 << 12;				\
+		if (boot_cpu_has(X86_FEATURE_PSE)) {		\
+			val |= 1 << 21;				\
+		}						\
+		if (boot_cpu_has(X86_FEATURE_GBPAGES)) {	\
+			val |= 1 << 30;				\
+		}						\

Can we use something besides literal "12", "21", and "30" values here?

Ehh, no, why? Inside x86_64 the page shifts are very specific numbers,
they won't change.

Well, it's nicer to have meaningful words to describe these things.

Not really. Inside specific arch page shifts are fixed, so using names
is just more macros one need to remember.

If I were to invent names (which I wouldn't), the best names are

	PAGE_SHIFT
	PAGE_SHIFT2
	PAGE_SHIFT3
	...

with PAGE_SHIFT2, PAGE_SHIFT3 being optional macros if arch doesn't support
multiple page sizes.

quoted

In fact, PAGE_SHIFT already exists for 12, and HPAGE_SHIFT already exists
for 21. Please use those, and add another, perhaps GBPAGE_SHIFT, for 30.

HPAGE_SHIFT is bad name, H doesn't describe anything unless arch is
known. Hugepages is marketing name. If GBPAGE_SHIFT is good name,
then HPAGE_SHIFT is bad name, it should've been MBPAGE_SHIFT, which
wrong because it is 2 MiB not 1 MiB.

BTW parisc has REAL_HPAGE_SHIFT !

Sure, I mean, we've got an x86-specific function here, so let's use the
x86-specific macros we already have for 12 and 21, and then add the
missing one for 30.

-- 
Kees Cook