Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <b31592db-d589-c425-1f5f-1c981ec72a65@arm.com>
Date:   Mon, 24 Jul 2023 08:56:02 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH -next] arm64: fix -Wundef warning for PUD_SHIFT
Content-Language: en-US
To:     Zhang Jianhua <chris.zjh@huawei.com>, catalin.marinas@arm.com,
        will@kernel.org, joey.gouly@arm.com, mark.rutland@arm.com,
        ardb@kernel.org
Cc:     linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
References: <20230724103055.2379274-1-chris.zjh@huawei.com>
From:   Anshuman Khandual <anshuman.khandual@arm.com>
In-Reply-To: <20230724103055.2379274-1-chris.zjh@huawei.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk


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 <chris.zjh@huawei.com>
> ---
> 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)
>