Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
Date:   Tue, 17 Oct 2023 12:05:10 +0100
From:   Mark Rutland <mark.rutland@arm.com>
To:     Gavin Shan <gshan@redhat.com>
Cc:     linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        catalin.marinas@arm.com, will@kernel.org, ryan.roberts@arm.com,
        anshuman.khandual@arm.com, shan.gavin@gmail.com
Subject: Re: [PATCH] arm64: mm: Validate CONFIG_PGTABLE_LEVELS conditionally
Message-ID: <ZS5qZtcJAjtaKP3X@FVFF77S0Q05N>
References: <20231017005300.334140-1-gshan@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231017005300.334140-1-gshan@redhat.com>
Precedence: bulk

On Tue, Oct 17, 2023 at 10:53:00AM +1000, Gavin Shan wrote:
> It's allowed for the fixmap virtual address space to span multiple
> PMD entries. Instead, the address space isn't allowed to span multiple
> PUD entries. However, PMD entries are folded to PUD and PGD entries
> in the following combination. In this particular case, the validation
> on NR_BM_PMD_TABLES should be avoided.
> 
>   CONFIG_ARM64_PAGE_SHIFT = 14
>   CONFIG_ARM64_VA_BITS_36 = y
>   CONFIG_PGTABLE_LEVELS   = 2

Is this something you found by inspection, or are you hitting a real issue on a
particular config?

I built a kernel with:

  defconfig + CONFIG_ARM64_16K_PAGES=y + CONFIG_ARM64_VA_BITS_36=y

... which gives the CONFIG_* configuration you list above, and that works just
fine.

For 2-level 16K pages we'd need to reserve more than 32M of fixmap slots for
the assertion to fire, and we only reserve ~6M of slots in total today, so I
can't see how this would be a problem unless you have 26M+ of local additions
to the fixmap?

Regardless of that, I don't think it's right to elide the check entirely.

The point of the check is to make sure that the fixmap VA range doesn't span
across multiple PMD/PUD/P4D/PGD entries, as the early_fixmap_init() and
fixmap_copy() code don't handle that in general. When using 2-level 16K pages,
we still want to ensure the fixmap is contained within a single PGD, and
checking that it falls within a single folded PMD will check that.

See the message for commit:

  414c109bdf496195 ("arm64: mm: always map fixmap at page granularity")

... and the bits that deleted from early_fixmap_init().

AFAICT this is fine as-is.

Mark.

> 
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
>  arch/arm64/mm/fixmap.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/mm/fixmap.c b/arch/arm64/mm/fixmap.c
> index c0a3301203bd..5384e5c3aeaa 100644
> --- a/arch/arm64/mm/fixmap.c
> +++ b/arch/arm64/mm/fixmap.c
> @@ -18,10 +18,11 @@
>  
>  #define NR_BM_PTE_TABLES \
>  	SPAN_NR_ENTRIES(FIXADDR_TOT_START, FIXADDR_TOP, PMD_SHIFT)
> +#if CONFIG_PGTABLE_LEVELS > 2
>  #define NR_BM_PMD_TABLES \
>  	SPAN_NR_ENTRIES(FIXADDR_TOT_START, FIXADDR_TOP, PUD_SHIFT)
> -
>  static_assert(NR_BM_PMD_TABLES == 1);
> +#endif
>  
>  #define __BM_TABLE_IDX(addr, shift) \
>  	(((addr) >> (shift)) - (FIXADDR_TOT_START >> (shift)))
> -- 
> 2.41.0
>