RE: [PATCH v5 3/5] xen/arm64: mm: Introduce helpers to prepare/enable/disable the identity mapping

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi Julien,

> -----Original Message-----
> Subject: [PATCH v5 3/5] xen/arm64: mm: Introduce helpers to
> prepare/enable/disable the identity mapping
> 
> 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 dereference 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 external helpers are introduced:
>     - arch_setup_page_tables() will setup the page-tables so it is
>       easy to create the mapping afterwards.
>     - update_identity_mapping() will create/remove the identity mapping
> 
> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>

Reviewed-by: Henry Wang <Henry.Wang@xxxxxxx>
With some nits below that can definitely be fixed on commit.

Tested on FVP in Arm64 mode, so:
Tested-by: Henry Wang <Henry.Wang@xxxxxxx>

> +static void __init prepare_boot_identity_mapping(void)
> +{
> +    paddr_t id_addr = virt_to_maddr(_start);
> +    lpae_t pte;
> +    DECLARE_OFFSETS(id_offsets, id_addr);
> +
> +    /*
> +     * We will be re-using the boot ID tables. They may not have been
> +     * zeroed but they should be unlinked. So it is fine to use
> +     * clear_page().
> +     */
> +    clear_page(boot_first_id);
> +    clear_page(boot_second_id);
> +    clear_page(boot_third_id);
> +
> +    if ( id_offsets[0] != 0 )
> +        panic("Cannot handled ID mapping above 512GB\n");

Nit: s/handled/handle/

> +
> +static void __init prepare_runtime_identity_mapping(void)
> +{
> +    paddr_t id_addr = virt_to_maddr(_start);
> +    lpae_t pte;
> +    DECLARE_OFFSETS(id_offsets, id_addr);
> +
> +    if ( id_offsets[0] >= IDENTITY_MAPPING_AREA_NR_L0 )
> +        panic("Cannot handled ID mapping above 512GB\n");

Same here.

> +void arch_setup_page_tables(void);
> +
> +/*
> + * Enable/disable the identity mapping in the live page-tables (i.e.
> + * the one pointer by TTBR_EL2).

Nit: I might be wrong but I think s/pointer/pointed/.

> + *
> + * Note that nested a call (e.g. enable=true, enable=true) is not

Nit: s/nested/nesting/ or "a nested call"?

> + * supported.
> + */
> +void update_identity_mapping(bool enable);

Kind regards,
Henry