lkml.org 
[lkml]   [2023]   [Oct]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
SubjectRe: [PATCH] arm64: mm: Validate CONFIG_PGTABLE_LEVELS conditionally
From
Hi Mark,

On 10/17/23 21:05, Mark Rutland wrote:
> 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.
>

It's all about code inspection. When CONFIG_PGTABLE_LEVELS == 2, PGD/PUD/PMD
are equivalent. The following two macros are interchangeable. The forthcoming
static_assert() enforces that the fixmap virtual space can't span multiple
PMD entries, meaning the space is limited to 32MB with above configuration.

#define NR_BM_PMD_TABLES \
SPAN_NR_ENTRIES(FIXADDR_TOT_START, FIXADDR_TOP, PUD_SHIFT)
#define NR_BM_PMD_TABLES \
SPAN_NR_ENTRIES(FIXADDR_TOT_START, FIXADDR_TOP, PMD_SHIFT)

static_assert(NR_BM_PMD_TABLES == 1);

However, multiple PTE tables are allowed. It means the fixmap virtual space
can span multiple PMD entries, which is controversial to the above enforcement
from the code level. Hopefully, I understood everything correctly.

#define NR_BM_PTE_TABLES \
SPAN_NR_ENTRIES(FIXADDR_TOT_START, FIXADDR_TOP, PMD_SHIFT)
static pte_t bm_pte[NR_BM_PTE_TABLES][PTRS_PER_PTE] __page_aligned_bss;


You're correct that the following edition is needed to trigger the assert.
The point is to have the fixmap virtual space larger than 32MB.

enum fixed_addresses {
FIX_HOLE,
:
FIX_PTE,
FIX_PMD,
FIX_PUD,
FIX_PGD,

FIX_DUMMY = FIX_PGD + 2048,

__end_of_fixed_addresses
};


> 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.
>

As I can see, multiple PMD entries can be handled well in early_fixmap_init().
However, multiple PMD entries aren't handled in fixmap_copy(), as you said.

early_fixmap_init
early_fixmap_init_pud
early_fixmap_init_pmd // multiple PMD entries handled in the loop

Thanks,
Gavin

>>
>> 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
>>
>

\
 
 \ /
  Last update: 2023-10-18 08:34    [W:0.094 / U:0.084 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site