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
