[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Tue, 18 Jul 2023 17:03:06 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=j7XIIjbdxwU5oe6DH9OpvRRjlDoIeHwh7emp9bepywM=; b=DG5903gIt8rA6vMubNRnrDSb+AhwWwZ5A59BijA/egvSHpaUSrxUL8xktRJSdKXCSqOxt/I1QuPiAm5BIkK7A3EhgWbcSCVvdkvO4qY9hSbyZ1AMz0wxPGYYopZ8cEnQ0QsKbEebuxg5009o/Yv7Uvkm1wyRIiK+rbo81WOfVwOiiGm+ZYb7cQXvdPjrJYH92Oo+UuIhr6MGsDtf16SasKT86lRsHReBPj514ttsftMEVtgnLqsQXL0YSQsM/bc07ZP0hmzULHsF18tHOPSAMkLFgn5jnV0lmV4mPLwhqbFO9Q8cLhtas2lWMpERIf4STDKzMjFzJGoJNsUoeEMiog==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=mJXFRY/gKQFHqIuxQi0agpIFJ4I4EOEGzyhO8kHc8gKd9LoD3eSGZRhewYHdanlucmtks2Su58Rfe92QLeH9445FzlRIjachn7e2fBT6zbihWXZPnfZoWP27uDlXguSka9RBzWXaIJMfHF71zJIKc4MjY1dhQ36xukqPZXiTnF3pZGMR57ERHsqCdnsaSkGWOLLFwjPuJRpcVGX2FIAKgJWvJaxpeb30d7iTfQFOvLEi5XXd4+pXYlzYPNCYXGZe8lKlfWOfQ9Jt7/RQYGXu5ljTnnIMVuVg4Dmk0GpzCcB8nMUrrobxCLfGWHcqXR1x9DYGu5b6XVe7S6sBcv34PA==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Bob Eshleman <bobbyeshleman@xxxxxxxxx>, Alistair Francis <alistair.francis@xxxxxxx>, Connor Davis <connojdavis@xxxxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • Delivery-date: Tue, 18 Jul 2023 15:03:21 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.