Re: [PATCH] xen/arm: skip holes in physical address space when setting up frametable

["Orzel, Michal" <michal.orzel@xxxxxxx>]

On 22/04/2026 09:01, Julien Grall wrote:
> Hi Michal,
> 
> On 17/04/2026 10:11, Michal Orzel wrote:
>> Refactor setup_frametable_mappings() into init_frametable(), modeled
>> after x86's implementation. Instead of mapping one contiguous frametable
>> covering ram_start to ram_end (including holes), iterate the
>> pdx_group_valid bitmap to allocate and map frametable memory only for
>> valid PDX groups, skipping gaps in the physical address space. At the
>> moment we don't really take into account pdx_group_valid bitmap.
>>
>> This reduces memory consumption on systems with sparse RAM layouts by
>> not allocating frametable entries for non-existent memory regions.
>>
>> A file-local pdx_to_page() override is needed because the generic macro
>> in xen/include/xen/pdx.h does not account for ARM's non-zero
>> frametable_base_pdx.
> 
> Can you provide a bit more details? I am a bit concerned that this could 
> result to subttle bug in the future if code within mm.c is expecting the 
> original behavior. It would be preferable if the change is either for 
> everyone on Arm or the function is renamed to avoid any clash.
The generic pdx_to_page macro does not account for offset which is something I
mentioned in the footer and I'm willing to work on in the future. As of today,
this macro is *unused* on Arm. It's only used by x86 in some special big mem
related scenario. Using generic pdx_to_page on Arm would be wrong, so a future
patch doing that would be wrong (the fact that this patch adds a local redefine
does not change anything). Do we need a rename for a local redefine in a file
that is only related to frametable? Maybe a comment and a TODO would be ok?

> 
> [...]
> 
>> +void __init init_frametable(paddr_t ram_start)
>> +{
>> +    unsigned int sidx, nidx, max_idx;
>>   
>>       /*
>>        * The size of paddr_t should be sufficient for the complete range of
>> @@ -26,24 +47,34 @@ void __init setup_frametable_mappings(paddr_t ps, 
>> paddr_t pe)
>>       BUILD_BUG_ON((sizeof(paddr_t) * BITS_PER_BYTE) < PADDR_BITS);
>>       BUILD_BUG_ON(sizeof(struct page_info) != PAGE_INFO_SIZE);
>>   
>> -    if ( frametable_size > FRAMETABLE_SIZE )
>> -        panic("The frametable cannot cover the physical region %#"PRIpaddr" 
>> - %#"PRIpaddr"\n",
>> -              ps, pe);
>> +    max_idx = DIV_ROUND_UP(max_pdx, PDX_GROUP_COUNT);
>> +    frametable_base_pdx = mfn_to_pdx(maddr_to_mfn(ram_start));
>>   
>> -    frametable_base_pdx = mfn_to_pdx(maddr_to_mfn(ps));
>> -    /* Round up to 2M or 32M boundary, as appropriate. */
>> -    frametable_size = ROUNDUP(frametable_size, mapping_size);
>> -    base_mfn = alloc_boot_pages(frametable_size >> PAGE_SHIFT, 32<<(20-12));
>> +    /*
>> +     * pdx_to_page(pdx_s) in init_frametable_chunk must be page-aligned
>> +     * for map_pages_to_xen(). Aligning to PDX_GROUP_COUNT guarantees this
>> +     * because PDX_GROUP_COUNT * sizeof(page_info) is always a multiple of
>> +     * PAGE_SIZE by construction.
>> +     */
>> +    frametable_base_pdx = ROUNDDOWN(frametable_base_pdx, PDX_GROUP_COUNT);
>>   
>> -    rc = map_pages_to_xen(FRAMETABLE_VIRT_START, base_mfn,
>> -                          frametable_size >> PAGE_SHIFT,
>> -                          PAGE_HYPERVISOR_RW | _PAGE_BLOCK);
>> -    if ( rc )
>> -        panic("Unable to setup the frametable mappings.\n");
>> +    if ( (max_pdx - frametable_base_pdx) > FRAMETABLE_NR )
>> +        panic("Frametable too small\n");
>> +
>> +    for ( sidx = (frametable_base_pdx / PDX_GROUP_COUNT); ; sidx = nidx )
>> +    {
>> +        unsigned int eidx;
>> +
>> +        eidx = find_next_zero_bit(pdx_group_valid, max_idx, sidx);
>> +        nidx = find_next_bit(pdx_group_valid, max_idx, eidx);
>> +
>> +        if ( nidx >= max_idx )
>> +            break;
>> +
>> +        init_frametable_chunk(sidx * PDX_GROUP_COUNT, eidx * 
>> PDX_GROUP_COUNT);
> 
> The function will do a round-up the mapping to either a 2MiB or 32MiB 
> aligned size. This means we could potentially cover the previous mapped 
> region or the next one. I can't seem to find any code to cover this 
> use-case. What did I miss?
Hmm, I think I calculated something wrong here. Anyway, how about using 2MB
mapping size all the time? PDX group size is 2MB, in-loop chunks are multiple of
2MB, there is no roundup needed - zero overshoot. The last chunk may have ~2MB
overshoot but it does not matter as there is nothing after it to conflict with.
The downside is more TLB pressure.

Alternatively, we could reduce the mapping size closer to boundaries (x86
choice) but that would require a bit more work.

~Michal