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

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

On 29-Apr-26 16:42, Julien Grall wrote:
> Hi Michal,
> 
> On 22/04/2026 09:25, Orzel, Michal wrote:
>>
>>
>> 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.
> 
> Sorry I missed the comment in the footer. But if the function is broken, 
> then why can't we implement pdx_to_page() correctly now? I understand 
> that ...
I wanted to do this in the future but ok, will do in v2.

> 
>   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?
> 
> ... this is not meant to be used by Arm today. But given this is used in 
> the page list, it is definitely not obvious that it is broken.
> 
> The alternative is to protect/move pdx_to_page() in x86. But I don't 
> know much churn this would involve.
> 
>>
>>>
>>> [...]
>>>
>>>> +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,
> 
> Looking at the code, it seems to be based on SECOND_SHIFT which 
> technically depends on the page granularity. Even though Xen supports 
> only 4KiB, we are trying to avoid making such assumption or add least 
> adding a BUILD_BUG_ON() (in this case, I would consider that 
> PDX_GROUP_COUNT is always 2MiB or SECOND_SHIFT).
> 
>> 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.
> 
> I am a bit warry to modify the frametable allocation method because it 
> is used fairly often in Xen. Would it be possible to hence the loop to 
> detect contiguous chunk and decide the size allocation based on the chunk?
To eliminate overshoot I'll round up to page size. In-loop chunks are already
page aligned so the round up is a no op except for the last chunk.

What we care about is to reduce TLB pressure by setting wherever possible
contiguous bit. It's not lost because map_pages_to_xen with block flag handles
contiguous internally via xen_pt_check_contig. Will do in v2.

~Michal