Re: [PATCH v5 11/15] x86/setup: Do not create valid mappings when directmap=no

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Jan 08, 2025 at 03:18:18PM +0000, Alejandro Vallejo wrote:
> From: Hongyan Xia <hongyxia@xxxxxxxxxx>
> 
> Create empty mappings in the second e820 pass. Also, destroy existing
> direct map mappings created in the first pass.
> 
> To make xenheap pages visible in guests, it is necessary to create empty
> L3 tables in the direct map even when directmap=no, since guest cr3s
> copy idle domain's L4 entries, which means they will share mappings in
> the direct map if we pre-populate idle domain's L4 entries and L3
> tables. A helper is introduced for this.
> 
> Also, after the direct map is actually gone, we need to stop updating
> the direct map in update_xen_mappings().
> 
> Signed-off-by: Hongyan Xia <hongyxia@xxxxxxxxxx>
> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> Signed-off-by: Elias El Yandouzi <eliasely@xxxxxxxxxx>
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>
> ---
> v4->v5:
>   * No changes.
> ---
>  xen/arch/x86/setup.c | 73 +++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 66 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 609ec4cf07f2..23b77f13bc10 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1060,6 +1060,56 @@ static struct domain *__init create_dom0(struct 
> boot_info *bi)
>      return d;
>  }
>  
> +/*
> + * This either populates a valid direct map, or allocates empty L3 tables and
> + * creates the L4 entries for virtual address between [start, end) in the
> + * direct map depending on has_directmap();
> + *
> + * When directmap=no, we still need to populate empty L3 tables in the
> + * direct map region. The reason is that on-demand xenheap mappings are
> + * created in the idle domain's page table but must be seen by
> + * everyone. Since all domains share the direct map L4 entries, they
> + * will share xenheap mappings if we pre-populate the L4 entries and L3
> + * tables in the direct map region for all RAM. We also rely on the fact
> + * that L3 tables are never freed.
> + */
> +static void __init populate_directmap(paddr_t pstart, paddr_t pend,
> +                                      unsigned int flags)
> +{
> +    unsigned long vstart = (unsigned long)__va(pstart);
> +    unsigned long vend = (unsigned long)__va(pend);
> +
> +    if ( pstart >= pend )
> +        return;
> +
> +    BUG_ON(vstart < DIRECTMAP_VIRT_START);
> +    BUG_ON(vend > DIRECTMAP_VIRT_END);
> +
> +    if ( has_directmap() )
> +        /* Populate valid direct map. */
> +        BUG_ON(map_pages_to_xen(vstart, maddr_to_mfn(pstart),
> +                                PFN_DOWN(pend - pstart), flags));
> +    else
> +    {
> +        /* Create empty L3 tables. */
> +        unsigned long vaddr = vstart & ~((1UL << L4_PAGETABLE_SHIFT) - 1);
> +
> +        for ( unsigned long idx = l4_table_offset(vaddr);
> +              idx <= l4_table_offset(vend); idx++ )
> +        {
> +            l4_pgentry_t *pl4e = &idle_pg_table[l4_table_offset(idx)];

As we are attempting to integrate this series with the per-CPU
mappings work, there's an issue here.  l4_table_offset() call is
duplicated, as idx is already the L4 table index:

        for ( unsigned long idx = l4_table_offset(vaddr);
              idx <= l4_table_offset(vend); idx++ )
        {
            l4_pgentry_t *pl4e = &idle_pg_table[idx];

This probably went unnoticed in small systems that can fit all the
directmap in a single L4 entry, but does explode on bigger ones.

Leaving a note here in case anyone else picks this up before a new
version is sent.

Regards, Roger.