Re: [RFC 3/4] Add xen superpage splitting support to arm

[Julien Grall <julien@xxxxxxx>]

Hi,

On 13/12/2022 19:54, Smith, Jackson wrote:
> Updates xen_pt_update_entry function from xen/arch/arm/mm.c to
> automatically split superpages as needed.
Your signed-off-by is missing.

> ---
>   xen/arch/arm/mm.c | 91 +++++++++++++++++++++++++++++++++++++++++++++++--------
>   1 file changed, 78 insertions(+), 13 deletions(-)
>
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 6301752..91b9c2b 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -753,8 +753,78 @@ static int create_xen_table(lpae_t *entry)
>   }
>   
>   #define XEN_TABLE_MAP_FAILED 0
> -#define XEN_TABLE_SUPER_PAGE 1
> -#define XEN_TABLE_NORMAL_PAGE 2
> +#define XEN_TABLE_NORMAL_PAGE 1
> +
> +/* More or less taken from p2m_split_superpage, without the p2m stuff */
> +static bool xen_split_superpage(lpae_t *entry, unsigned int level,
> +                                unsigned int target, const unsigned int 
> *offsets)
> +{
> +    struct page_info *page;
> +    lpae_t pte, *table;
> +    unsigned int i;
> +    bool rv = true;
> +
> +    mfn_t mfn = lpae_get_mfn(*entry);
> +    unsigned int next_level = level + 1;
> +    unsigned int level_order = XEN_PT_LEVEL_ORDER(next_level);
> +
> +    ASSERT(level < target);
> +    ASSERT(lpae_is_superpage(*entry, level));
> +
> +    page = alloc_domheap_page(NULL, 0);
Page-table may be allocated from the boot allocator. So you want to use 
create_xen_table().
> +    if ( !page )
> +        return false;
> +
> +    table = __map_domain_page(page);
You want to use xen_map_table().

> +
> +    /*
> +     * We are either splitting a first level 1G page into 512 second level
> +     * 2M pages, or a second level 2M page into 512 third level 4K pages.
> +     */
> +    for ( i = 0; i < XEN_PT_LPAE_ENTRIES; i++ )
> +    {
> +        lpae_t *new_entry = table + i;
> +
> +        /*
> +         * Use the content of the superpage entry and override
> +         * the necessary fields. So the correct permission are kept.
> +         */
> +        pte = *entry;
> +        lpae_set_mfn(pte, mfn_add(mfn, i << level_order));
> +
> +        /*
> +         * First and second level pages set walk.table = 0, but third
> +         * level entries set walk.table = 1.
> +         */
> +        pte.walk.table = (next_level == 3);
> +
> +        write_pte(new_entry, pte);
> +    }
> +
> +    /*
> +     * Shatter superpage in the page to the level we want to make the
> +     * changes.
> +     * This is done outside the loop to avoid checking the offset to
> +     * know whether the entry should be shattered for every entry.
> +     */
> +    if ( next_level != target )
> +        rv = xen_split_superpage(table + offsets[next_level],
> +                                 level + 1, target, offsets);
> +
> +    clean_dcache_va_range(table, PAGE_SIZE);
Cleaning the cache is not necessary. This is done in the P2M case 
because it is shared with the IOMMU which may not support coherent access.
> +    unmap_domain_page(table);
This would be xen_map

> +
> +    /*
> +     * Generate the entry for this new table we created,
> +     * and write it back in place of the superpage entry.
> +     */
I am afraid this is not compliant with the Arm Arm. If you want to 
update valid entry (e.g. shattering a superpage), then you need to 
follow the break-before-make sequence. This means that:
  1. Replace the valid entry with an entry with an invalid one
  2. Flush the TLBs
  3. Write the new entry

Those steps will make your code compliant but it also means that a 
virtual address will be temporarily invalid so you could take a fault in 
the middle of your split if your stack or the table was part of the 
region. The same could happen for the other running CPUs but this is 
less problematic as they could spin on the page-table lock.
This is the main reason why we never implemented super-page shattering 
for the hypervisor.
So I would rather prefer if we can avoid shattering (I have made some 
suggestion in the cover letter). If we really need to shatter, then we 
should make sure this is only used in very limited use case by 
introducing a flag. So the caller will be reponsible to acknowledge it 
doesn't modify a region that may be used by itself or another CPU.
> +    pte = mfn_to_xen_entry(page_to_mfn(page), MT_NORMAL);
> +    pte.pt.table = 1;
> +    write_pte(entry, pte);
> +    clean_dcache(*entry);
Ditto about the cache cleaning.

Cheers,

--
Julien Grall