2023-07-24 02:57:41

by Zhang Jianhua

Subject: [PATCH -next] arm64: fix -Wundef warning for PUD_SHIFT

When building with W=1, the following warning occurs.

arch/arm64/include/asm/kernel-pgtable.h:129:41: error: "PUD_SHIFT" is not defined, evaluates to 0 [-Werror=undef]
129 | #define ARM64_MEMSTART_SHIFT PUD_SHIFT
| ^~~~~~~~~
arch/arm64/include/asm/kernel-pgtable.h:142:5: note: in expansion of macro ‘ARM64_MEMSTART_SHIFT’
142 | #if ARM64_MEMSTART_SHIFT < SECTION_SIZE_BITS
| ^~~~~~~~~~~~~~~~~~~~

The reason is that PUD_SHIFT isn't defined if CONFIG_PGTABLE_LEVELS ==
3, and at this time PUD_SHIFT is equal to PGDIR_SHIFT, so define it.

Fixes: 06e9bf2fd9b3 ("arm64: choose memstart_addr based on minimum sparsemem section alignment")
Signed-off-by: Zhang Jianhua <[email protected]>
---
v2:
Define PUD_SHIFT before use it instead of judgement
---
---
arch/arm64/include/asm/kernel-pgtable.h | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/arch/arm64/include/asm/kernel-pgtable.h b/arch/arm64/include/asm/kernel-pgtable.h
index 577773870b66..996a144e2929 100644
--- a/arch/arm64/include/asm/kernel-pgtable.h
+++ b/arch/arm64/include/asm/kernel-pgtable.h
@@ -59,6 +59,10 @@
#define EARLY_KASLR (0)
#endif

+#ifndef PUD_SHIFT
+#define PUD_SHIFT PGDIR_SHIFT
+#endif
+
#define SPAN_NR_ENTRIES(vstart, vend, shift) \
((((vend) - 1) >> (shift)) - ((vstart) >> (shift)) + 1)

--
2.34.1

2023-07-24 03:46:08

by Anshuman Khandual

Subject: Re: [PATCH -next] arm64: fix -Wundef warning for PUD_SHIFT

On 7/24/23 16:00, Zhang Jianhua wrote:
> When building with W=1, the following warning occurs.
>
> arch/arm64/include/asm/kernel-pgtable.h:129:41: error: "PUD_SHIFT" is not defined, evaluates to 0 [-Werror=undef]
> 129 | #define ARM64_MEMSTART_SHIFT PUD_SHIFT
> | ^~~~~~~~~
> arch/arm64/include/asm/kernel-pgtable.h:142:5: note: in expansion of macro ‘ARM64_MEMSTART_SHIFT’
> 142 | #if ARM64_MEMSTART_SHIFT < SECTION_SIZE_BITS
> | ^~~~~~~~~~~~~~~~~~~~
>
> The reason is that PUD_SHIFT isn't defined if CONFIG_PGTABLE_LEVELS ==
> 3, and at this time PUD_SHIFT is equal to PGDIR_SHIFT, so define it.

CONFIG_PGTABLE_LEVELS equals 3 only with CONFIG_VA_BITS = 39.

>
> Fixes: 06e9bf2fd9b3 ("arm64: choose memstart_addr based on minimum sparsemem section alignment")

This is not a fix, ARM64_MEMSTART_ALIGN would fallback being (1UL << SECTION_SIZE_BITS)
when PUD_SHIFT is undefined (aka 0) but agreed that it's not clean.

> Signed-off-by: Zhang Jianhua <[email protected]>
> ---
> v2:
> Define PUD_SHIFT before use it instead of judgement
> ---
> ---
> arch/arm64/include/asm/kernel-pgtable.h | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/arch/arm64/include/asm/kernel-pgtable.h b/arch/arm64/include/asm/kernel-pgtable.h
> index 577773870b66..996a144e2929 100644
> --- a/arch/arm64/include/asm/kernel-pgtable.h
> +++ b/arch/arm64/include/asm/kernel-pgtable.h
> @@ -59,6 +59,10 @@
> #define EARLY_KASLR (0)
> #endif
>
> +#ifndef PUD_SHIFT
> +#define PUD_SHIFT PGDIR_SHIFT
> +#endif
Rather ARM64_MEMSTART_SHIFT block needs to be re-written in a more pgtable levels agnostic
manner ? OR maybe something like this.

diff --git a/arch/arm64/include/asm/kernel-pgtable.h b/arch/arm64/include/asm/kernel-pgtable.h
index 577773870b66..5a83b4b249e8 100644
--- a/arch/arm64/include/asm/kernel-pgtable.h
+++ b/arch/arm64/include/asm/kernel-pgtable.h
@@ -125,12 +125,14 @@
* (64k granule), or a multiple that can be mapped using contiguous bits
* in the page tables: 32 * PMD_SIZE (16k granule)
*/
-#if defined(CONFIG_ARM64_4K_PAGES)
+#if defined(CONFIG_ARM64_4K_PAGES) && defined(PUD_SHIFT)
#define ARM64_MEMSTART_SHIFT PUD_SHIFT
-#elif defined(CONFIG_ARM64_16K_PAGES)
+#elif defined(CONFIG_ARM64_16K_PAGES) && defined(CONT_PMD_SHIFT)
#define ARM64_MEMSTART_SHIFT CONT_PMD_SHIFT
-#else
+#elif defined(CONFIG_AR64_64K_PAGES) && defined(PMD_SHIFT)
#define ARM64_MEMSTART_SHIFT PMD_SHIFT
+#else
+#define ARM64_MEMSTART_SHIFT PGDIR_SHIFT
#endif

The only cases where PUD_SHIFT or PMD_SHIFT (along with CONT_PMD_SHIFT) would not be defined is
when XXX_SHIFT level itself matches PGDIR_SHIFT.

> +
> #define SPAN_NR_ENTRIES(vstart, vend, shift) \
> ((((vend) - 1) >> (shift)) - ((vstart) >> (shift)) + 1)
>

2023-07-24 06:48:32

by Zhang Jianhua

Subject: Re: [PATCH -next] arm64: fix -Wundef warning for PUD_SHIFT

在 2023/7/24 11:26, Anshuman Khandual 写道:
>
> On 7/24/23 16:00, Zhang Jianhua wrote:
>> When building with W=1, the following warning occurs.
>>
>> arch/arm64/include/asm/kernel-pgtable.h:129:41: error: "PUD_SHIFT" is not defined, evaluates to 0 [-Werror=undef]
>> 129 | #define ARM64_MEMSTART_SHIFT PUD_SHIFT
>> | ^~~~~~~~~
>> arch/arm64/include/asm/kernel-pgtable.h:142:5: note: in expansion of macro ‘ARM64_MEMSTART_SHIFT’
>> 142 | #if ARM64_MEMSTART_SHIFT < SECTION_SIZE_BITS
>> | ^~~~~~~~~~~~~~~~~~~~
>>
>> The reason is that PUD_SHIFT isn't defined if CONFIG_PGTABLE_LEVELS ==
>> 3, and at this time PUD_SHIFT is equal to PGDIR_SHIFT, so define it.
> CONFIG_PGTABLE_LEVELS equals 3 only with CONFIG_VA_BITS = 39.
>
>> Fixes: 06e9bf2fd9b3 ("arm64: choose memstart_addr based on minimum sparsemem section alignment")
> This is not a fix, ARM64_MEMSTART_ALIGN would fallback being (1UL << SECTION_SIZE_BITS)
> when PUD_SHIFT is undefined (aka 0) but agreed that it's not clean.
Yes, it wouldn't be wrong in the end.
>> Signed-off-by: Zhang Jianhua <[email protected]>
>> ---
>> v2:
>> Define PUD_SHIFT before use it instead of judgement
>> ---
>> ---
>> arch/arm64/include/asm/kernel-pgtable.h | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/arch/arm64/include/asm/kernel-pgtable.h b/arch/arm64/include/asm/kernel-pgtable.h
>> index 577773870b66..996a144e2929 100644
>> --- a/arch/arm64/include/asm/kernel-pgtable.h
>> +++ b/arch/arm64/include/asm/kernel-pgtable.h
>> @@ -59,6 +59,10 @@
>> #define EARLY_KASLR (0)
>> #endif
>>
>> +#ifndef PUD_SHIFT
>> +#define PUD_SHIFT PGDIR_SHIFT
>> +#endif
> Rather ARM64_MEMSTART_SHIFT block needs to be re-written in a more pgtable levels agnostic
> manner ? OR maybe something like this.
>
> diff --git a/arch/arm64/include/asm/kernel-pgtable.h b/arch/arm64/include/asm/kernel-pgtable.h
> index 577773870b66..5a83b4b249e8 100644
> --- a/arch/arm64/include/asm/kernel-pgtable.h
> +++ b/arch/arm64/include/asm/kernel-pgtable.h
> @@ -125,12 +125,14 @@
> * (64k granule), or a multiple that can be mapped using contiguous bits
> * in the page tables: 32 * PMD_SIZE (16k granule)
> */
> -#if defined(CONFIG_ARM64_4K_PAGES)
> +#if defined(CONFIG_ARM64_4K_PAGES) && defined(PUD_SHIFT)
> #define ARM64_MEMSTART_SHIFT PUD_SHIFT
> -#elif defined(CONFIG_ARM64_16K_PAGES)
> +#elif defined(CONFIG_ARM64_16K_PAGES) && defined(CONT_PMD_SHIFT)
> #define ARM64_MEMSTART_SHIFT CONT_PMD_SHIFT
> -#else
> +#elif defined(CONFIG_AR64_64K_PAGES) && defined(PMD_SHIFT)
> #define ARM64_MEMSTART_SHIFT PMD_SHIFT
> +#else
> +#define ARM64_MEMSTART_SHIFT PGDIR_SHIFT
> #endif
>
> The only cases where PUD_SHIFT or PMD_SHIFT (along with CONT_PMD_SHIFT) would not be defined is
> when XXX_SHIFT level itself matches PGDIR_SHIFT.
Thanks for your reply, this modification seems more comprehensive and
appropriate. I will send v2 patch soon.
>> +
>> #define SPAN_NR_ENTRIES(vstart, vend, shift) \
>> ((((vend) - 1) >> (shift)) - ((vstart) >> (shift)) + 1)
>>