Re: [PATCH early-RFC 3/5] xen/arm: mm: Introduce helpers to prepare/enable/disable the identity mapping

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Julien,

> On 9 Mar 2022, at 12:20, Julien Grall <julien@xxxxxxx> wrote:
> 
> From: Julien GralL <jgrall@xxxxxxxxxx>
> 
> In follow-up patches we will need to have part of Xen identity mapped in
> order to safely switch the TTBR.
> 
> On some platform, the identity mapping may have to start at 0. If we always
> keep the identity region mapped, NULL pointer ference would lead to access
> to valid mapping.
> 
> It would be possible to relocate Xen to avoid clashing with address 0.
> However the identity mapping is only meant to be used in very limited
> places. Therefore it would be better to keep the identity region invalid
> for most of the time.
> 
> Two new helpers are introduced:
>    - prepare_identity_mapping() will setup the page-tables so it is
>      easy to create the mapping afterwards.
>    - update_identity_mapping() will create/remove the identity mapping

Nit: Would be better to first say what the patch is doing and then explaining
the NULL pointer possible issue.

> 
> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> ---
> xen/arch/arm/include/asm/mm.h |  2 +
> xen/arch/arm/mm.c             | 73 +++++++++++++++++++++++++++++++++++
> 2 files changed, 75 insertions(+)
> 
> diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h
> index 045a8ba4bb63..76973ea9a0ff 100644
> --- a/xen/arch/arm/include/asm/mm.h
> +++ b/xen/arch/arm/include/asm/mm.h
> @@ -177,6 +177,8 @@ extern unsigned long total_pages;
> 
> /* Boot-time pagetable setup */
> extern void setup_pagetables(unsigned long boot_phys_offset);
> +/* Enable/disable the identity mapping */
> +extern void update_identity_mapping(bool enable);
> /* Map FDT in boot pagetable */
> extern void *early_fdt_map(paddr_t fdt_paddr);
> /* Remove early mappings */
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 75ed9a3ce249..5c4dece16f7f 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -138,6 +138,12 @@ static DEFINE_PAGE_TABLE(cpu0_pgtable);
> static DEFINE_PAGE_TABLES(cpu0_dommap, DOMHEAP_SECOND_PAGES);
> #endif
> 
> +#ifdef CONFIG_ARM_64
> +static DEFINE_PAGE_TABLE(xen_first_id);
> +static DEFINE_PAGE_TABLE(xen_second_id);
> +static DEFINE_PAGE_TABLE(xen_third_id);
> +#endif
> +
> /* Common pagetable leaves */
> /* Second level page tables.
>  *
> @@ -573,6 +579,70 @@ void __init remove_early_mappings(void)
>     BUG_ON(rc);
> }
> 
> +/*
> + * The identity mapping may start at physical address 0. So don't want
> + * to keep it mapped longer than necessary.
> + *
> + * When this is called, we are still using the boot_pgtable.
> + *
> + * XXX: Handle Arm32 properly.
> + */
> +static void prepare_identity_mapping(void)
> +{
> +    paddr_t id_addr = virt_to_maddr(_start);
> +    lpae_t pte;
> +    DECLARE_OFFSETS(id_offsets, id_addr);
> +
> +    printk("id_addr 0x%lx\n", id_addr);

Debug print that should be removed.

> +#ifdef CONFIG_ARM_64
> +    if ( id_offsets[0] != 0 )
> +        panic("Cannot handled ID mapping above 512GB\n");

The error message here might not be really helpful for the user.
How about saying that Xen cannot be loaded in memory with
a physical address above 512GB ?

> +#endif
> +
> +    /* Link first ID table */
> +    pte = pte_of_xenaddr((vaddr_t)xen_first_id);
> +    pte.pt.table = 1;
> +    pte.pt.xn = 0;
> +
> +    write_pte(&boot_pgtable[id_offsets[0]], pte);
> +
> +    /* Link second ID table */
> +    pte = pte_of_xenaddr((vaddr_t)xen_second_id);
> +    pte.pt.table = 1;
> +    pte.pt.xn = 0;
> +
> +    write_pte(&xen_first_id[id_offsets[1]], pte);
> +
> +    /* Link third ID table */
> +    pte = pte_of_xenaddr((vaddr_t)xen_third_id);
> +    pte.pt.table = 1;
> +    pte.pt.xn = 0;
> +
> +    write_pte(&xen_second_id[id_offsets[2]], pte);
> +
> +    /* The mapping in the third table will be created at a later stage */
> +
> +    /*
> +     * Link the identity mapping in the runtime Xen page tables. No need to
> +     * use write_pte here as they are not live yet.
> +     */
> +    xen_pgtable[id_offsets[0]] = boot_pgtable[id_offsets[0]];
> +}
> +
> +void update_identity_mapping(bool enable)

You probably want an __init attribute here.

> +{
> +    paddr_t id_addr = virt_to_maddr(_start);
> +    int rc;
> +
> +    if ( enable )
> +        rc = map_pages_to_xen(id_addr, maddr_to_mfn(id_addr), 1,
> +                              PAGE_HYPERVISOR_RX);
> +    else
> +        rc = destroy_xen_mappings(id_addr, id_addr + PAGE_SIZE);
> +
> +    BUG_ON(rc);
> +}
> +
> /*
>  * After boot, Xen page-tables should not contain mapping that are both
>  * Writable and eXecutables.
> @@ -609,6 +679,9 @@ void __init setup_pagetables(unsigned long 
> boot_phys_offset)
> 
>     phys_offset = boot_phys_offset;
> 
> +    /* XXX: Find a better place to call it */

Why do you think this place is not right ?

> +    prepare_identity_mapping();
> +
> #ifdef CONFIG_ARM_64
>     pte = pte_of_xenaddr((uintptr_t)xen_first);
>     pte.pt.table = 1;
> -- 
> 2.32.0

Cheers
Bertrand