Re: [PATCH v3 3/3] xen/riscv: introduce identity mapping

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.07.2023 16:40, Oleksii Kurochko wrote:
> The way how switch to virtual address was implemented in the
> commit e66003e7be ("xen/riscv: introduce setup_initial_pages")
> isn't safe enough as:
> * enable_mmu() depends on hooking all exceptions
>   and pagefault.
> * Any exception other than pagefault, or not taking a pagefault
>   causes it to malfunction, which means you will fail to boot
>   depending on where Xen was loaded into memory.
> 
> Instead of the proposed way of switching to virtual addresses was
> decided to use identity mapping of the entrire Xen and after
> switching to virtual addresses identity mapping is removed from
> page-tables.
> Since it is not easy to keep track where the identity map was mapped,
> so we will look for the top-most entry exclusive to the identity
> map and remove it.

Doesn't this paragraph need adjustment now?

> --- a/xen/arch/riscv/mm.c
> +++ b/xen/arch/riscv/mm.c
> @@ -25,6 +25,12 @@ unsigned long __ro_after_init phys_offset;
>  #define LOAD_TO_LINK(addr) ((unsigned long)(addr) - phys_offset)
>  #define LINK_TO_LOAD(addr) ((unsigned long)(addr) + phys_offset)
>  
> +/*
> + * Should be removed as soon as enough headers will be merged for inclusion 
> of
> + * <xen/lib.h>.
> + */
> +#define ARRAY_SIZE(arr)              (sizeof(arr) / sizeof((arr)[0]))

Like said to Shawn for PPC in [1], there's now a pretty easy way to
get this macro available for use here without needing to include
xen/lib.h.

[1] https://lists.xen.org/archives/html/xen-devel/2023-07/msg01081.html

> @@ -35,8 +41,10 @@ unsigned long __ro_after_init phys_offset;
>   *
>   * It might be needed one more page table in case when Xen load address
>   * isn't 2 MB aligned.
> + *
> + * CONFIG_PAGING_LEVELS page tables are needed for identity mapping.
>   */
> -#define PGTBL_INITIAL_COUNT ((CONFIG_PAGING_LEVELS - 1) + 1)
> +#define PGTBL_INITIAL_COUNT (CONFIG_PAGING_LEVELS * 2 + 1)

Where did the "- 1" go?

> @@ -255,25 +266,40 @@ void __init noreturn noinline enable_mmu()
>      csr_write(CSR_SATP,
>                PFN_DOWN((unsigned long)stage1_pgtbl_root) |
>                RV_STAGE1_MODE << SATP_MODE_SHIFT);
> +}
>  
> -    asm volatile ( ".p2align 2" );
> - mmu_is_enabled:
> -    /*
> -     * Stack should be re-inited as:
> -     * 1. Right now an address of the stack is relative to load time
> -     *    addresses what will cause an issue in case of load start address
> -     *    isn't equal to linker start address.
> -     * 2. Addresses in stack are all load time relative which can be an
> -     *    issue in case when load start address isn't equal to linker
> -     *    start address.
> -     *
> -     * We can't return to the caller because the stack was reseted
> -     * and it may have stash some variable on the stack.
> -     * Jump to a brand new function as the stack was reseted
> -     */
> +void __init remove_identity_mapping(void)
> +{
> +    unsigned int i;
> +    pte_t *pgtbl;
> +    unsigned int index, xen_index;
> +    unsigned long load_start = LINK_TO_LOAD(_start);
> +
> +    for ( pgtbl = stage1_pgtbl_root, i = CONFIG_PAGING_LEVELS; i; i-- )
> +    {
> +        index = pt_index(i - 1, load_start);
> +        xen_index = pt_index(i - 1, XEN_VIRT_START);
> +
> +        if ( index != xen_index )
> +        {
> +            /* remove after it will be possible to include <xen/lib.h> */
> +            #define ROUNDUP(x, a) (((x) + (a) - 1) & ~((a) - 1))

ROUNDUP() is even part of the patch that I've submitted already.

> +            unsigned long load_end = LINK_TO_LOAD(_end);
> +            unsigned long pt_level_size = XEN_PT_LEVEL_SIZE(i - 1);
> +            unsigned long xen_size = ROUNDUP(load_end - load_start, 
> pt_level_size);
> +            unsigned long page_entries_num = xen_size / pt_level_size;
> +
> +            while ( page_entries_num-- )
> +                pgtbl[index++].pte = 0;
> +
> +            break;

Unless there's a "not crossing a 2Mb boundary" guarantee somewhere
that I've missed, this "break" is still too early afaict.

Jan