[PATCH v1 0/3] enable MMU for RISC-V

[PATCH v1 0/3] enable MMU for RISC-V

[Oleksii Kurochko]

The patch series introduces the following things:
1. Functionality to build the page tables for Xen that map the
   following:
     * The physical location of Xen (where the bootloader loaded it)
     * The link-time location of Xen (where the linker expected Xen's
       addresses to be.
2. Load the built page table into the SATP
3. Enables MMU.
4. Updates smoke test to grep message which should be printed after
   MMU is enabled.

Oleksii Kurochko (3):
  xen/riscv: introduce setup_initial_pages
  xen/riscv: setup initial pagetables
  automation: update RISC-V smoke test

 automation/scripts/qemu-smoke-riscv64.sh |   2 +-
 xen/arch/riscv/Makefile                  |   1 +
 xen/arch/riscv/include/asm/mm.h          |   9 +
 xen/arch/riscv/include/asm/page.h        |  90 +++++++++
 xen/arch/riscv/mm.c                      | 223 +++++++++++++++++++++++
 xen/arch/riscv/setup.c                   |  11 ++
 6 files changed, 335 insertions(+), 1 deletion(-)
 create mode 100644 xen/arch/riscv/include/asm/mm.h
 create mode 100644 xen/arch/riscv/include/asm/page.h
 create mode 100644 xen/arch/riscv/mm.c

-- 
2.39.0

[PATCH v1 1/3] xen/riscv: introduce setup_initial_pages

[Oleksii Kurochko]

Mostly the code for setup_initial_pages was taken from Bobby's
repo except for the following changes:
* Use only a minimal part of the code enough to enable MMU
* rename {_}setup_initial_pagetables functions
* add writable argument for _setup_initial_pagetables to have
  an opportunity to make some sections read-only
* update setup_initial_pagetables function to make some sections
  read-only
* change the order of _setup_inital_pagetables()
  in setup_initial_pagetable():
  * first it is called for text, init, rodata sections
  * after call it for ranges [link_addr_start : link_addr_end] and
    [load_addr_start : load_addr_end]
  Before it was done first for the ranges and after for sections but
  in that case read-only status will be equal to 'true' and
  as sections' addresses  can/are inside the ranges the read-only status
  won't be updated for them as it was set up before.

Origin: https://gitlab.com/xen-on-risc-v/xen/-/tree/riscv-rebase 4af165b468af
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/riscv/Makefile           |   1 +
 xen/arch/riscv/include/asm/mm.h   |   9 ++
 xen/arch/riscv/include/asm/page.h |  90 ++++++++++++
 xen/arch/riscv/mm.c               | 223 ++++++++++++++++++++++++++++++
 4 files changed, 323 insertions(+)
 create mode 100644 xen/arch/riscv/include/asm/mm.h
 create mode 100644 xen/arch/riscv/include/asm/page.h
 create mode 100644 xen/arch/riscv/mm.c

diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
index 443f6bf15f..956ceb02df 100644
--- a/xen/arch/riscv/Makefile
+++ b/xen/arch/riscv/Makefile
@@ -1,5 +1,6 @@
 obj-$(CONFIG_EARLY_PRINTK) += early_printk.o
 obj-y += entry.o
+obj-y += mm.o
 obj-$(CONFIG_RISCV_64) += riscv64/
 obj-y += sbi.o
 obj-y += setup.o
diff --git a/xen/arch/riscv/include/asm/mm.h b/xen/arch/riscv/include/asm/mm.h
new file mode 100644
index 0000000000..fc1866b1d8
--- /dev/null
+++ b/xen/arch/riscv/include/asm/mm.h
@@ -0,0 +1,9 @@
+#ifndef _ASM_RISCV_MM_H
+#define _ASM_RISCV_MM_H
+
+void setup_initial_pagetables(unsigned long load_addr_start,
+                              unsigned long load_addr_end,
+                              unsigned long linker_addr_start,
+                              unsigned long linker_addr_end);
+
+#endif /* _ASM_RISCV_MM_H */
diff --git a/xen/arch/riscv/include/asm/page.h b/xen/arch/riscv/include/asm/page.h
new file mode 100644
index 0000000000..fabbe1305f
--- /dev/null
+++ b/xen/arch/riscv/include/asm/page.h
@@ -0,0 +1,90 @@
+#ifndef _ASM_RISCV_PAGE_H
+#define _ASM_RISCV_PAGE_H
+
+#include <xen/const.h>
+#include <xen/types.h>
+
+#define PAGE_ENTRIES            512
+#define VPN_BITS                (9)
+#define VPN_MASK                ((unsigned long)((1 << VPN_BITS) - 1))
+
+#ifdef CONFIG_RISCV_64
+/* L3 index Bit[47:39] */
+#define THIRD_SHIFT             (39)
+#define THIRD_MASK              (VPN_MASK << THIRD_SHIFT)
+/* L2 index Bit[38:30] */
+#define SECOND_SHIFT            (30)
+#define SECOND_MASK             (VPN_MASK << SECOND_SHIFT)
+/* L1 index Bit[29:21] */
+#define FIRST_SHIFT             (21)
+#define FIRST_MASK              (VPN_MASK << FIRST_SHIFT)
+/* L0 index Bit[20:12] */
+#define ZEROETH_SHIFT           (12)
+#define ZEROETH_MASK            (VPN_MASK << ZEROETH_SHIFT)
+
+#else // CONFIG_RISCV_32
+
+/* L1 index Bit[31:22] */
+#define FIRST_SHIFT             (22)
+#define FIRST_MASK              (VPN_MASK << FIRST_SHIFT)
+
+/* L0 index Bit[21:12] */
+#define ZEROETH_SHIFT           (12)
+#define ZEROETH_MASK            (VPN_MASK << ZEROETH_SHIFT)
+#endif
+
+#define THIRD_SIZE              (1 << THIRD_SHIFT)
+#define THIRD_MAP_MASK          (~(THIRD_SIZE - 1))
+#define SECOND_SIZE             (1 << SECOND_SHIFT)
+#define SECOND_MAP_MASK         (~(SECOND_SIZE - 1))
+#define FIRST_SIZE              (1 << FIRST_SHIFT)
+#define FIRST_MAP_MASK          (~(FIRST_SIZE - 1))
+#define ZEROETH_SIZE            (1 << ZEROETH_SHIFT)
+#define ZEROETH_MAP_MASK        (~(ZEROETH_SIZE - 1))
+
+#define PTE_SHIFT               10
+
+#define PTE_VALID               BIT(0, UL)
+#define PTE_READABLE            BIT(1, UL)
+#define PTE_WRITABLE            BIT(2, UL)
+#define PTE_EXECUTABLE          BIT(3, UL)
+#define PTE_USER                BIT(4, UL)
+#define PTE_GLOBAL              BIT(5, UL)
+#define PTE_ACCESSED            BIT(6, UL)
+#define PTE_DIRTY               BIT(7, UL)
+#define PTE_RSW                 (BIT(8, UL) | BIT(9, UL))
+
+#define PTE_LEAF_DEFAULT        (PTE_VALID | PTE_READABLE | PTE_WRITABLE | PTE_EXECUTABLE)
+#define PTE_TABLE               (PTE_VALID)
+
+/* Calculate the offsets into the pagetables for a given VA */
+#define zeroeth_linear_offset(va)   ((va) >> ZEROETH_SHIFT)
+#define first_linear_offset(va)     ((va) >> FIRST_SHIFT)
+#define second_linear_offset(va)    ((va) >> SECOND_SHIFT)
+#define third_linear_offset(va)     ((va) >> THIRD_SHIFT)
+
+#define pagetable_zeroeth_index(va) zeroeth_linear_offset((va) & ZEROETH_MASK)
+#define pagetable_first_index(va)   first_linear_offset((va) & FIRST_MASK)
+#define pagetable_second_index(va)  second_linear_offset((va) & SECOND_MASK)
+#define pagetable_third_index(va)   third_linear_offset((va) & THIRD_MASK)
+
+/* Page Table entry */
+typedef struct {
+    uint64_t pte;
+} pte_t;
+
+/* Shift the VPN[x] or PPN[x] fields of a virtual or physical address
+ * to become the shifted PPN[x] fields of a page table entry */
+#define addr_to_ppn(x) (((x) >> PAGE_SHIFT) << PTE_SHIFT)
+
+static inline pte_t paddr_to_pte(unsigned long paddr)
+{
+    return (pte_t) { .pte = addr_to_ppn(paddr) };
+}
+
+static inline bool pte_is_valid(pte_t *p)
+{
+    return p->pte & PTE_VALID;
+}
+
+#endif /* _ASM_RISCV_PAGE_H */
diff --git a/xen/arch/riscv/mm.c b/xen/arch/riscv/mm.c
new file mode 100644
index 0000000000..6e172376eb
--- /dev/null
+++ b/xen/arch/riscv/mm.c
@@ -0,0 +1,223 @@
+#include <xen/init.h>
+#include <xen/lib.h>
+
+#include <asm/csr.h>
+#include <asm/mm.h>
+#include <asm/page.h>
+
+/*
+ * xen_second_pagetable is indexed with the VPN[2] page table entry field
+ * xen_first_pagetable is accessed from the VPN[1] page table entry field
+ * xen_zeroeth_pagetable is accessed from the VPN[0] page table entry field
+ */
+pte_t xen_second_pagetable[PAGE_ENTRIES] __attribute__((__aligned__(PAGE_SIZE)));
+static pte_t xen_first_pagetable[PAGE_ENTRIES]
+    __attribute__((__aligned__(PAGE_SIZE)));
+static pte_t xen_zeroeth_pagetable[PAGE_ENTRIES]
+    __attribute__((__aligned__(PAGE_SIZE)));
+
+extern unsigned long _stext;
+extern unsigned long _etext;
+extern unsigned long __init_begin;
+extern unsigned long __init_end;
+extern unsigned long _srodata;
+extern unsigned long _erodata;
+
+paddr_t phys_offset;
+
+#define resolve_early_addr(x) \
+    ({                                                                          \
+         unsigned long * __##x;                                                 \
+        if ( load_addr_start <= x && x < load_addr_end )                        \
+            __##x = (unsigned long *)x;                                         \
+        else                                                                    \
+            __##x = (unsigned long *)(x + load_addr_start - linker_addr_start); \
+        __##x;                                                                  \
+     })
+
+static void __init clear_pagetables(unsigned long load_addr_start,
+                             unsigned long load_addr_end,
+                             unsigned long linker_addr_start,
+                             unsigned long linker_addr_end)
+{
+    unsigned long *p;
+    unsigned long page;
+    unsigned long i;
+
+    page = (unsigned long)&xen_second_pagetable[0];
+
+    p = resolve_early_addr(page);
+    for ( i = 0; i < ARRAY_SIZE(xen_second_pagetable); i++ )
+    {
+        p[i] = 0ULL;
+    }
+
+    page = (unsigned long)&xen_first_pagetable[0];
+    p = resolve_early_addr(page);
+    for ( i = 0; i < ARRAY_SIZE(xen_first_pagetable); i++ )
+    {
+        p[i] = 0ULL;
+    }
+
+    page = (unsigned long)&xen_zeroeth_pagetable[0];
+    p = resolve_early_addr(page);
+    for ( i = 0; i < ARRAY_SIZE(xen_zeroeth_pagetable); i++ )
+    {
+        p[i] = 0ULL;
+    }
+}
+
+/*
+ * WARNING: load_addr() and linker_addr() are to be called only when the MMU is
+ * disabled and only when executed by the primary CPU.  They cannot refer to
+ * any global variable or functions.
+ */
+
+/*
+ * Convert an addressed layed out at link time to the address where it was loaded
+ * by the bootloader.
+ */
+#define load_addr(linker_address)                                              \
+    ({                                                                         \
+        unsigned long __linker_address = (unsigned long)(linker_address);      \
+        if ( linker_addr_start <= __linker_address &&                          \
+            __linker_address < linker_addr_end )                               \
+        {                                                                      \
+            __linker_address =                                                 \
+                __linker_address - linker_addr_start + load_addr_start;        \
+        }                                                                      \
+        __linker_address;                                                      \
+    })
+
+/* Convert boot-time Xen address from where it was loaded by the boot loader to the address it was layed out
+ * at link-time.
+ */
+#define linker_addr(load_address)                                              \
+    ({                                                                         \
+        unsigned long __load_address = (unsigned long)(load_address);          \
+        if ( load_addr_start <= __load_address &&                              \
+            __load_address < load_addr_end )                                   \
+        {                                                                      \
+            __load_address =                                                   \
+                __load_address - load_addr_start + linker_addr_start;          \
+        }                                                                      \
+        __load_address;                                                        \
+    })
+
+static void __attribute__((section(".entry")))
+_setup_initial_pagetables(pte_t *second, pte_t *first, pte_t *zeroeth,
+                         unsigned long map_start,
+                         unsigned long map_end,
+                         unsigned long pa_start,
+                         bool writable)
+{
+    unsigned long page_addr;
+    unsigned long index2;
+    unsigned long index1;
+    unsigned long index0;
+
+    /* align start addresses */
+    map_start &= ZEROETH_MAP_MASK;
+    pa_start &= ZEROETH_MAP_MASK;
+
+    page_addr = map_start;
+    while ( page_addr < map_end )
+    {
+        index2 = pagetable_second_index(page_addr);
+        index1 = pagetable_first_index(page_addr);
+        index0 = pagetable_zeroeth_index(page_addr);
+
+        /* Setup level2 table */
+        second[index2] = paddr_to_pte((unsigned long)first);
+        second[index2].pte |= PTE_TABLE;
+
+        /* Setup level1 table */
+        first[index1] = paddr_to_pte((unsigned long)zeroeth);
+        first[index1].pte |= PTE_TABLE;
+
+        /* Setup level0 table */
+        if ( !pte_is_valid(&zeroeth[index0]) )
+        {
+            /* Update level0 table */
+            zeroeth[index0] = paddr_to_pte((page_addr - map_start) + pa_start);
+            zeroeth[index0].pte |= PTE_LEAF_DEFAULT;
+            zeroeth[index0].pte &= ~((!writable) ? PTE_WRITABLE : 0);
+        }
+
+        /* Point to next page */
+        page_addr += ZEROETH_SIZE;
+    }
+}
+
+/*
+ * setup_initial_pagetables:
+ *
+ * 1) Build the page tables for Xen that map the following:
+ *   1.1)  The physical location of Xen (where the bootloader loaded it)
+ *   1.2)  The link-time location of Xen (where the linker expected Xen's
+ *         addresses to be)
+ * 2) Load the page table into the SATP and enable the MMU
+ */
+void __attribute__((section(".entry")))
+setup_initial_pagetables(unsigned long load_addr_start,
+                         unsigned long load_addr_end,
+                         unsigned long linker_addr_start,
+                         unsigned long linker_addr_end)
+{
+    pte_t *second;
+    pte_t *first;
+    pte_t *zeroeth;
+
+    clear_pagetables(load_addr_start, load_addr_end,
+                     linker_addr_start, linker_addr_end);
+
+    /* Get the addresses where the page tables were loaded */
+    second  = (pte_t *)load_addr(&xen_second_pagetable);
+    first   = (pte_t *)load_addr(&xen_first_pagetable);
+    zeroeth = (pte_t *)load_addr(&xen_zeroeth_pagetable);
+
+    /*
+     * Create a mapping from Xen's link-time addresses to where they were actually loaded.
+     */
+    _setup_initial_pagetables(second, first, zeroeth,
+                              linker_addr(&_stext),
+                              linker_addr(&_etext),
+                              load_addr(&_stext),
+                              false);
+    _setup_initial_pagetables(second, first, zeroeth,
+                              linker_addr(&__init_begin),
+                              linker_addr(&__init_end),
+                              load_addr(&__init_begin),
+                              true);
+    _setup_initial_pagetables(second, first, zeroeth,
+                              linker_addr(&_srodata),
+                              linker_addr(&_erodata),
+                              load_addr(&_srodata),
+                              false);
+    _setup_initial_pagetables(second, first, zeroeth,
+                              linker_addr_start,
+                              linker_addr_end,
+                              load_addr_start,
+                              true);
+
+    /*
+     * Create a mapping of the load time address range to... the load time address range.
+     * This mapping is used at boot time only.
+     */
+    _setup_initial_pagetables(second, first, zeroeth,
+                              load_addr_start,
+                              load_addr_end,
+                              load_addr_start,
+                              true);
+
+    /* Ensure page table writes precede loading the SATP */
+    asm volatile("sfence.vma");
+
+    /* Enable the MMU and load the new pagetable for Xen */
+    csr_write(CSR_SATP,
+              (load_addr(xen_second_pagetable) >> PAGE_SHIFT) | SATP_MODE_SV39 << SATP_MODE_SHIFT);
+
+    phys_offset = load_addr_start - linker_addr_start;
+
+    return;
+}
-- 
2.39.0

Re: [PATCH v1 1/3] xen/riscv: introduce setup_initial_pages

[Andrew Cooper]

On 24/02/2023 3:06 pm, Oleksii Kurochko wrote:
> diff --git a/xen/arch/riscv/include/asm/page.h b/xen/arch/riscv/include/asm/page.h
> new file mode 100644
> index 0000000000..fabbe1305f
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/page.h
> @@ -0,0 +1,90 @@
> +#ifndef _ASM_RISCV_PAGE_H
> +#define _ASM_RISCV_PAGE_H
> +
> +#include <xen/const.h>
> +#include <xen/types.h>
> +
> +#define PAGE_ENTRIES            512
> +#define VPN_BITS                (9)
> +#define VPN_MASK                ((unsigned long)((1 << VPN_BITS) - 1))
> +
> +#ifdef CONFIG_RISCV_64
> +/* L3 index Bit[47:39] */
> +#define THIRD_SHIFT             (39)
> +#define THIRD_MASK              (VPN_MASK << THIRD_SHIFT)
> +/* L2 index Bit[38:30] */
> +#define SECOND_SHIFT            (30)
> +#define SECOND_MASK             (VPN_MASK << SECOND_SHIFT)
> +/* L1 index Bit[29:21] */
> +#define FIRST_SHIFT             (21)
> +#define FIRST_MASK              (VPN_MASK << FIRST_SHIFT)
> +/* L0 index Bit[20:12] */
> +#define ZEROETH_SHIFT           (12)
> +#define ZEROETH_MASK            (VPN_MASK << ZEROETH_SHIFT)

Don't name these with words.  That's an error ultimately inherited from
an architectural mistake ARM.

These should be named L1 (4k) thru L4 (512T), and don't need separate
separate masks or shifts because it looks like RISC-V designed their
pagetables in a coherent and uniform way.

You'll find the code simplifies substantially if you have
PAGETABLE_ORDER 9 somewhere in here.

The shift is always (PAGE_ORDER + level * PAGETABLE_ORDER), and it's
rare that you need something other than "(addr >> shift) & mask".  About
the only time you need a virtual address masked but unshifted is for
debugging.

~Andrew

Re: [PATCH v1 1/3] xen/riscv: introduce setup_initial_pages

[Julien Grall]

Hi Oleksii,

On 24/02/2023 15:06, Oleksii Kurochko wrote:
> Mostly the code for setup_initial_pages was taken from Bobby's
> repo except for the following changes:
> * Use only a minimal part of the code enough to enable MMU
> * rename {_}setup_initial_pagetables functions
> * add writable argument for _setup_initial_pagetables to have
>    an opportunity to make some sections read-only
> * update setup_initial_pagetables function to make some sections
>    read-only
> * change the order of _setup_inital_pagetables()
>    in setup_initial_pagetable():
>    * first it is called for text, init, rodata sections
>    * after call it for ranges [link_addr_start : link_addr_end] and
>      [load_addr_start : load_addr_end]
>    Before it was done first for the ranges and after for sections but
>    in that case read-only status will be equal to 'true' and
>    as sections' addresses  can/are inside the ranges the read-only status
>    won't be updated for them as it was set up before.
> 
> Origin: https://gitlab.com/xen-on-risc-v/xen/-/tree/riscv-rebase 4af165b468af
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
>   xen/arch/riscv/Makefile           |   1 +
>   xen/arch/riscv/include/asm/mm.h   |   9 ++
>   xen/arch/riscv/include/asm/page.h |  90 ++++++++++++
>   xen/arch/riscv/mm.c               | 223 ++++++++++++++++++++++++++++++
>   4 files changed, 323 insertions(+)
>   create mode 100644 xen/arch/riscv/include/asm/mm.h
>   create mode 100644 xen/arch/riscv/include/asm/page.h
>   create mode 100644 xen/arch/riscv/mm.c
> 
> diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
> index 443f6bf15f..956ceb02df 100644
> --- a/xen/arch/riscv/Makefile
> +++ b/xen/arch/riscv/Makefile
> @@ -1,5 +1,6 @@
>   obj-$(CONFIG_EARLY_PRINTK) += early_printk.o
>   obj-y += entry.o
> +obj-y += mm.o
>   obj-$(CONFIG_RISCV_64) += riscv64/
>   obj-y += sbi.o
>   obj-y += setup.o
> diff --git a/xen/arch/riscv/include/asm/mm.h b/xen/arch/riscv/include/asm/mm.h
> new file mode 100644
> index 0000000000..fc1866b1d8
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/mm.h
> @@ -0,0 +1,9 @@
> +#ifndef _ASM_RISCV_MM_H
> +#define _ASM_RISCV_MM_H
> +
> +void setup_initial_pagetables(unsigned long load_addr_start,
> +                              unsigned long load_addr_end,
> +                              unsigned long linker_addr_start,
> +                              unsigned long linker_addr_end);
> +
> +#endif /* _ASM_RISCV_MM_H */
> diff --git a/xen/arch/riscv/include/asm/page.h b/xen/arch/riscv/include/asm/page.h
> new file mode 100644
> index 0000000000..fabbe1305f
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/page.h
> @@ -0,0 +1,90 @@
> +#ifndef _ASM_RISCV_PAGE_H
> +#define _ASM_RISCV_PAGE_H
> +
> +#include <xen/const.h>
> +#include <xen/types.h>
> +
> +#define PAGE_ENTRIES            512

NIT: AFAIU, the number here is based on ...

> +#define VPN_BITS                (9)

... this. So I would suggest to define PAGE_ENTRIES using VPN_BITS.

> +#define VPN_MASK                ((unsigned long)((1 << VPN_BITS) - 1))
NIT: Use 1UL and you can avoid the cast.

> +
> +#ifdef CONFIG_RISCV_64
> +/* L3 index Bit[47:39] */
> +#define THIRD_SHIFT             (39)
> +#define THIRD_MASK              (VPN_MASK << THIRD_SHIFT)
> +/* L2 index Bit[38:30] */
> +#define SECOND_SHIFT            (30)
> +#define SECOND_MASK             (VPN_MASK << SECOND_SHIFT)
> +/* L1 index Bit[29:21] */
> +#define FIRST_SHIFT             (21)
> +#define FIRST_MASK              (VPN_MASK << FIRST_SHIFT)
> +/* L0 index Bit[20:12] */
> +#define ZEROETH_SHIFT           (12)
> +#define ZEROETH_MASK            (VPN_MASK << ZEROETH_SHIFT)

On Arm, we are trying to phase out ZEROETH_* and co because the name is 
too generic. Instead, we now introduce a generic macro that take a level 
and then compute the mask/shift (see XEN_PT_LEVEL_*).

You should be able to do in RISC-V and reduce the amount of defines 
introduced.

> +
> +#else // CONFIG_RISCV_32

Coding style: comments in Xen are using /* ... */

> +
> +/* L1 index Bit[31:22] */
> +#define FIRST_SHIFT             (22)
> +#define FIRST_MASK              (VPN_MASK << FIRST_SHIFT)
> +
> +/* L0 index Bit[21:12] */
> +#define ZEROETH_SHIFT           (12)
> +#define ZEROETH_MASK            (VPN_MASK << ZEROETH_SHIFT)
> +#endif
> +
> +#define THIRD_SIZE              (1 << THIRD_SHIFT)
> +#define THIRD_MAP_MASK          (~(THIRD_SIZE - 1))
> +#define SECOND_SIZE             (1 << SECOND_SHIFT)
> +#define SECOND_MAP_MASK         (~(SECOND_SIZE - 1))
> +#define FIRST_SIZE              (1 << FIRST_SHIFT)
> +#define FIRST_MAP_MASK          (~(FIRST_SIZE - 1))
> +#define ZEROETH_SIZE            (1 << ZEROETH_SHIFT)
> +#define ZEROETH_MAP_MASK        (~(ZEROETH_SIZE - 1))
> +
> +#define PTE_SHIFT               10
> +
> +#define PTE_VALID               BIT(0, UL)
> +#define PTE_READABLE            BIT(1, UL)
> +#define PTE_WRITABLE            BIT(2, UL)
> +#define PTE_EXECUTABLE          BIT(3, UL)
> +#define PTE_USER                BIT(4, UL)
> +#define PTE_GLOBAL              BIT(5, UL)
> +#define PTE_ACCESSED            BIT(6, UL)
> +#define PTE_DIRTY               BIT(7, UL)
> +#define PTE_RSW                 (BIT(8, UL) | BIT(9, UL))
> +
> +#define PTE_LEAF_DEFAULT        (PTE_VALID | PTE_READABLE | PTE_WRITABLE | PTE_EXECUTABLE)

We should avoid vulnerable default flags. So this should either be RW or RX.

> +#define PTE_TABLE               (PTE_VALID)
> +
> +/* Calculate the offsets into the pagetables for a given VA */
> +#define zeroeth_linear_offset(va)   ((va) >> ZEROETH_SHIFT)
> +#define first_linear_offset(va)     ((va) >> FIRST_SHIFT)
> +#define second_linear_offset(va)    ((va) >> SECOND_SHIFT)
> +#define third_linear_offset(va)     ((va) >> THIRD_SHIFT)
> +
> +#define pagetable_zeroeth_index(va) zeroeth_linear_offset((va) & ZEROETH_MASK)
> +#define pagetable_first_index(va)   first_linear_offset((va) & FIRST_MASK)
> +#define pagetable_second_index(va)  second_linear_offset((va) & SECOND_MASK)
> +#define pagetable_third_index(va)   third_linear_offset((va) & THIRD_MASK)
> +
> +/* Page Table entry */
> +typedef struct {
> +    uint64_t pte;
> +} pte_t;
> +
> +/* Shift the VPN[x] or PPN[x] fields of a virtual or physical address
> + * to become the shifted PPN[x] fields of a page table entry */
> +#define addr_to_ppn(x) (((x) >> PAGE_SHIFT) << PTE_SHIFT)
> +
> +static inline pte_t paddr_to_pte(unsigned long paddr)
> +{
> +    return (pte_t) { .pte = addr_to_ppn(paddr) };
> +}
> +
> +static inline bool pte_is_valid(pte_t *p)
> +{
> +    return p->pte & PTE_VALID;
> +}
> +
> +#endif /* _ASM_RISCV_PAGE_H */
> diff --git a/xen/arch/riscv/mm.c b/xen/arch/riscv/mm.c
> new file mode 100644
> index 0000000000..6e172376eb
> --- /dev/null
> +++ b/xen/arch/riscv/mm.c
> @@ -0,0 +1,223 @@
> +#include <xen/init.h>
> +#include <xen/lib.h>
> +
> +#include <asm/csr.h>
> +#include <asm/mm.h>
> +#include <asm/page.h>
> +
> +/*
> + * xen_second_pagetable is indexed with the VPN[2] page table entry field
> + * xen_first_pagetable is accessed from the VPN[1] page table entry field
> + * xen_zeroeth_pagetable is accessed from the VPN[0] page table entry field
> + */
> +pte_t xen_second_pagetable[PAGE_ENTRIES] __attribute__((__aligned__(PAGE_SIZE)));
> +static pte_t xen_first_pagetable[PAGE_ENTRIES]
> +    __attribute__((__aligned__(PAGE_SIZE)));
> +static pte_t xen_zeroeth_pagetable[PAGE_ENTRIES]
> +    __attribute__((__aligned__(PAGE_SIZE)));
> +
> +extern unsigned long _stext;
> +extern unsigned long _etext;
> +extern unsigned long __init_begin;
> +extern unsigned long __init_end;
> +extern unsigned long _srodata;
> +extern unsigned long _erodata;
> +
> +paddr_t phys_offset;

This is defined, set but not used.

> +
> +#define resolve_early_addr(x) \

This helper seems to behave the same wasy as linker_addr(). So any 
reason to not use it?

I will make this assumption this can be used and not comment on the 
implement of resolve_early_addr().

> +    ({                                                                          \
> +         unsigned long * __##x;                                                 \
> +        if ( load_addr_start <= x && x < load_addr_end )                        \
> +            __##x = (unsigned long *)x;                                         \
> +        else                                                                    \
> +            __##x = (unsigned long *)(x + load_addr_start - linker_addr_start); \
> +        __##x;                                                                  \
> +     })
> +
> +static void __init clear_pagetables(unsigned long load_addr_start,
> +                             unsigned long load_addr_end,
> +                             unsigned long linker_addr_start,
> +                             unsigned long linker_addr_end)
> +{
> +    unsigned long *p;
> +    unsigned long page;
> +    unsigned long i;
> +
> +    page = (unsigned long)&xen_second_pagetable[0];
> +
> +    p = resolve_early_addr(page);
> +    for ( i = 0; i < ARRAY_SIZE(xen_second_pagetable); i++ )

The entries in xen_second_pagetable are a pte_t (uint64_t). But ...

> +    {
> +        p[i] = 0ULL;

... the type here will be unsigned long. So you may not fully zero the 
page-table on 32-bit architecture. Therefore you want to define as pte_t.

That said, given the page table will be part of BSS, you should not need 
to zero again assuming you clear BSS before hand.

If you clear afterwards, then you *must* move them out of BSS.

The same applies for xen_{first, zeroeth}_pagetable below.

> +    }
> +
> +    page = (unsigned long)&xen_first_pagetable[0];
> +    p = resolve_early_addr(page);
> +    for ( i = 0; i < ARRAY_SIZE(xen_first_pagetable); i++ )
> +    {
> +        p[i] = 0ULL;
> +    }
> +
> +    page = (unsigned long)&xen_zeroeth_pagetable[0];
> +    p = resolve_early_addr(page);
> +    for ( i = 0; i < ARRAY_SIZE(xen_zeroeth_pagetable); i++ )
> +    {
> +        p[i] = 0ULL;
> +    }
> +}
> +
> +/*
> + * WARNING: load_addr() and linker_addr() are to be called only when the MMU is
> + * disabled and only when executed by the primary CPU.  They cannot refer to
> + * any global variable or functions.

I find interesting you are saying when _setup_initial_pagetables() is 
called from setup_initial_pagetables(). Would you be able to explain how 
this is different?

> + */
> +
> +/*
> + * Convert an addressed layed out at link time to the address where it was loaded

Typo: s/addressed/address/ ?

> + * by the bootloader.
> + */

Looking at the implementation, you seem to consider that any address not 
in the range [linker_addr_start, linker_addr_end[ will have a 1:1 mappings.

I am not sure this is what you want. So I would consider to throw an 
error if such address is passed.

> +#define load_addr(linker_address)                                              \
> +    ({                                                                         \
> +        unsigned long __linker_address = (unsigned long)(linker_address);      \
> +        if ( linker_addr_start <= __linker_address &&                          \
> +            __linker_address < linker_addr_end )                               \
> +        {                                                                      \
> +            __linker_address =                                                 \
> +                __linker_address - linker_addr_start + load_addr_start;        \
> +        }                                                                      \
> +        __linker_address;                                                      \
> +    })
> +
> +/* Convert boot-time Xen address from where it was loaded by the boot loader to the address it was layed out
> + * at link-time.
> + */

Coding style: The first line is too long and multi-line comments look like:

/*
  * Foo
  * Bar
  */

> +#define linker_addr(load_address)                                              \

Same remark as for load_addr() above.

> +    ({                                                                         \
> +        unsigned long __load_address = (unsigned long)(load_address);          \
> +        if ( load_addr_start <= __load_address &&                              \
> +            __load_address < load_addr_end )                                   \
> +        {                                                                      \
> +            __load_address =                                                   \
> +                __load_address - load_addr_start + linker_addr_start;          \
> +        }                                                                      \
> +        __load_address;                                                        \
> +    })
> +
> +static void __attribute__((section(".entry")))
> +_setup_initial_pagetables(pte_t *second, pte_t *first, pte_t *zeroeth,
Can this be named to setup_initial_mapping() so this is clearer and 
avoid the one '_' different with the function below.

> +                         unsigned long map_start,
> +                         unsigned long map_end,
> +                         unsigned long pa_start,
> +                         bool writable)

What about the executable bit?

> +{
> +    unsigned long page_addr;
> +    unsigned long index2;
> +    unsigned long index1;
> +    unsigned long index0;

index* could be defined in the loop below.

> +
> +    /* align start addresses */
> +    map_start &= ZEROETH_MAP_MASK;
> +    pa_start &= ZEROETH_MAP_MASK;

Hmmm... I would actually expect the address to be properly aligned and 
therefore not require an alignment here.

Otherwise, this raise the question of what happen if you have region 
using the same page?

> +
> +    page_addr = map_start;
> +    while ( page_addr < map_end )

Looking at the loop, it looks like you are assuming that the region will 
never cross a boundary of a page-table (either L0, L1, L2). I am not 
convinced you can make such assumption (see below).

But if you really want to make such assumption then you should add some 
guard (either BUILD_BUG_ON(), ASSERT(), proper check) in your code to 
avoid any surprise in the future.

> +    {
> +        index2 = pagetable_second_index(page_addr);
> +        index1 = pagetable_first_index(page_addr);
> +        index0 = pagetable_zeroeth_index(page_addr);
> +
> +        /* Setup level2 table */
> +        second[index2] = paddr_to_pte((unsigned long)first);
> +        second[index2].pte |= PTE_TABLE;
> +
> +        /* Setup level1 table */
> +        first[index1] = paddr_to_pte((unsigned long)zeroeth);
> +        first[index1].pte |= PTE_TABLE;
> +
> +        /* Setup level0 table */
> +        if ( !pte_is_valid(&zeroeth[index0]) )

Can you explain why you are checking !pte_is_valid() for the L0 entry 
but not the other?

> +        {
> +            /* Update level0 table */
> +            zeroeth[index0] = paddr_to_pte((page_addr - map_start) + pa_start);
> +            zeroeth[index0].pte |= PTE_LEAF_DEFAULT;
> +            zeroeth[index0].pte &= ~((!writable) ? PTE_WRITABLE : 0);

Looking at the default value, it would mean that a non-writable mapping 
is automatically executable. This seems wrong for the section is not 
meant to be executable (like rodata).

> +        }
> +
> +        /* Point to next page */
> +        page_addr += ZEROETH_SIZE;
> +    }
> +}
> +
> +/*
> + * setup_initial_pagetables:
> + *
> + * 1) Build the page tables for Xen that map the following:
> + *   1.1)  The physical location of Xen (where the bootloader loaded it)
> + *   1.2)  The link-time location of Xen (where the linker expected Xen's
> + *         addresses to be)
> + * 2) Load the page table into the SATP and enable the MMU
> + */
> +void __attribute__((section(".entry")))

I couldn't find a section ".entry" in the linker.

> +setup_initial_pagetables(unsigned long load_addr_start,
> +                         unsigned long load_addr_end,
> +                         unsigned long linker_addr_start,
> +                         unsigned long linker_addr_end)
> +{
> +    pte_t *second;
> +    pte_t *first;
> +    pte_t *zeroeth;
> +
> +    clear_pagetables(load_addr_start, load_addr_end,
> +                     linker_addr_start, linker_addr_end);
> +
> +    /* Get the addresses where the page tables were loaded */
> +    second  = (pte_t *)load_addr(&xen_second_pagetable);
> +    first   = (pte_t *)load_addr(&xen_first_pagetable);
> +    zeroeth = (pte_t *)load_addr(&xen_zeroeth_pagetable);

I would consider to embed the type cast in load_addr() so you are adding 
some type safety within your code.

> +
> +    /*
> +     * Create a mapping from Xen's link-time addresses to where they were actually loaded.

This is line is way long than 80 characters. Please make sure to wrap it 
80 characters.

> +     */
> +    _setup_initial_pagetables(second, first, zeroeth,
> +                              linker_addr(&_stext),
> +                              linker_addr(&_etext),
> +                              load_addr(&_stext),
> +                              false);
> +    _setup_initial_pagetables(second, first, zeroeth,
> +                              linker_addr(&__init_begin),
> +                              linker_addr(&__init_end),
> +                              load_addr(&__init_begin),
> +                              true);
> +    _setup_initial_pagetables(second, first, zeroeth,
> +                              linker_addr(&_srodata),
> +                              linker_addr(&_erodata),
> +                              load_addr(&_srodata),
> +                              false);
> +    _setup_initial_pagetables(second, first, zeroeth,
> +                              linker_addr_start,
> +                              linker_addr_end,
> +                              load_addr_start,
> +                              true);

Where do you guarantee that Xen will always fit in an L0 table and the 
start address is aligned to the size of an L0 table?

> +
> +    /*
> +     * Create a mapping of the load time address range to... the load time address range.

Same about the line length here.

> +     * This mapping is used at boot time only.
> +     */
> +    _setup_initial_pagetables(second, first, zeroeth,

This can only work if Xen is loaded at its linked address. So you need a 
separate set of L0, L1 tables for the identity mapping.

That said, this would not be sufficient because:
   1) Xen may not be loaded at a 2M boundary (you can control with 
U-boot, but not with EFI). So this may cross a boundary and therefore 
need multiple pages.
   2) The load region may overlap the link address

While I think it would be good to handle those cases from the start, I 
would understand why are not easy to solve. So I think the minimum is to 
throw some errors if you are in a case you can't support.

> +                              load_addr_start,
> +                              load_addr_end,
> +                              load_addr_start,
> +                              true); > +
> +    /* Ensure page table writes precede loading the SATP */
> +    asm volatile("sfence.vma");
> +
> +    /* Enable the MMU and load the new pagetable for Xen */
> +    csr_write(CSR_SATP,
> +              (load_addr(xen_second_pagetable) >> PAGE_SHIFT) | SATP_MODE_SV39 << SATP_MODE_SHIFT);

IHMO, it would make sense to introduce within the series the code to 
jump off the identity mapping and then remove it.

> +
> +    phys_offset = load_addr_start - linker_addr_start;
> +
> +    return;
> +}

Cheers,

-- 
Julien Grall

Re: [PATCH v1 1/3] xen/riscv: introduce setup_initial_pages

[Oleksii]

On Sat, 2023-02-25 at 17:53 +0000, Julien Grall wrote:
> Hi Oleksii,
> 
> On 24/02/2023 15:06, Oleksii Kurochko wrote:
> > Mostly the code for setup_initial_pages was taken from Bobby's
> > repo except for the following changes:
> > * Use only a minimal part of the code enough to enable MMU
> > * rename {_}setup_initial_pagetables functions
> > * add writable argument for _setup_initial_pagetables to have
> >    an opportunity to make some sections read-only
> > * update setup_initial_pagetables function to make some sections
> >    read-only
> > * change the order of _setup_inital_pagetables()
> >    in setup_initial_pagetable():
> >    * first it is called for text, init, rodata sections
> >    * after call it for ranges [link_addr_start : link_addr_end] and
> >      [load_addr_start : load_addr_end]
> >    Before it was done first for the ranges and after for sections
> > but
> >    in that case read-only status will be equal to 'true' and
> >    as sections' addresses  can/are inside the ranges the read-only
> > status
> >    won't be updated for them as it was set up before.
> > 
> > Origin:
> > https://gitlab.com/xen-on-risc-v/xen/-/tree/riscv-rebase 4af165b468
> > af
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> > ---
> >   xen/arch/riscv/Makefile           |   1 +
> >   xen/arch/riscv/include/asm/mm.h   |   9 ++
> >   xen/arch/riscv/include/asm/page.h |  90 ++++++++++++
> >   xen/arch/riscv/mm.c               | 223
> > ++++++++++++++++++++++++++++++
> >   4 files changed, 323 insertions(+)
> >   create mode 100644 xen/arch/riscv/include/asm/mm.h
> >   create mode 100644 xen/arch/riscv/include/asm/page.h
> >   create mode 100644 xen/arch/riscv/mm.c
> > 
> > diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
> > index 443f6bf15f..956ceb02df 100644
> > --- a/xen/arch/riscv/Makefile
> > +++ b/xen/arch/riscv/Makefile
> > @@ -1,5 +1,6 @@
> >   obj-$(CONFIG_EARLY_PRINTK) += early_printk.o
> >   obj-y += entry.o
> > +obj-y += mm.o
> >   obj-$(CONFIG_RISCV_64) += riscv64/
> >   obj-y += sbi.o
> >   obj-y += setup.o
> > diff --git a/xen/arch/riscv/include/asm/mm.h
> > b/xen/arch/riscv/include/asm/mm.h
> > new file mode 100644
> > index 0000000000..fc1866b1d8
> > --- /dev/null
> > +++ b/xen/arch/riscv/include/asm/mm.h
> > @@ -0,0 +1,9 @@
> > +#ifndef _ASM_RISCV_MM_H
> > +#define _ASM_RISCV_MM_H
> > +
> > +void setup_initial_pagetables(unsigned long load_addr_start,
> > +                              unsigned long load_addr_end,
> > +                              unsigned long linker_addr_start,
> > +                              unsigned long linker_addr_end);
> > +
> > +#endif /* _ASM_RISCV_MM_H */
> > diff --git a/xen/arch/riscv/include/asm/page.h
> > b/xen/arch/riscv/include/asm/page.h
> > new file mode 100644
> > index 0000000000..fabbe1305f
> > --- /dev/null
> > +++ b/xen/arch/riscv/include/asm/page.h
> > @@ -0,0 +1,90 @@
> > +#ifndef _ASM_RISCV_PAGE_H
> > +#define _ASM_RISCV_PAGE_H
> > +
> > +#include <xen/const.h>
> > +#include <xen/types.h>
> > +
> > +#define PAGE_ENTRIES            512
> 
> NIT: AFAIU, the number here is based on ...
> 
> > +#define VPN_BITS                (9)
> 
> ... this. So I would suggest to define PAGE_ENTRIES using VPN_BITS.
Sure. It should be defined using VPN_BITS. Thanks.
> 
> > +#define VPN_MASK                ((unsigned long)((1 << VPN_BITS) -
> > 1))
> NIT: Use 1UL and you can avoid the cast.
Thanks. I'll update that in the next version of patch series.
> 
> > +
> > +#ifdef CONFIG_RISCV_64
> > +/* L3 index Bit[47:39] */
> > +#define THIRD_SHIFT             (39)
> > +#define THIRD_MASK              (VPN_MASK << THIRD_SHIFT)
> > +/* L2 index Bit[38:30] */
> > +#define SECOND_SHIFT            (30)
> > +#define SECOND_MASK             (VPN_MASK << SECOND_SHIFT)
> > +/* L1 index Bit[29:21] */
> > +#define FIRST_SHIFT             (21)
> > +#define FIRST_MASK              (VPN_MASK << FIRST_SHIFT)
> > +/* L0 index Bit[20:12] */
> > +#define ZEROETH_SHIFT           (12)
> > +#define ZEROETH_MASK            (VPN_MASK << ZEROETH_SHIFT)
> 
> On Arm, we are trying to phase out ZEROETH_* and co because the name
> is 
> too generic. Instead, we now introduce a generic macro that take a
> level 
> and then compute the mask/shift (see XEN_PT_LEVEL_*).
> 
> You should be able to do in RISC-V and reduce the amount of defines 
> introduced.
Thanks. I'll look at XEN_PT_LEVEL_*. I'll re-read Andrew's comment but
as far as I understand after quick reading we can remove mostly that.
> 
> > +
> > +#else // CONFIG_RISCV_32
> 
> Coding style: comments in Xen are using /* ... */
> 
> > +
> > +/* L1 index Bit[31:22] */
> > +#define FIRST_SHIFT             (22)
> > +#define FIRST_MASK              (VPN_MASK << FIRST_SHIFT)
> > +
> > +/* L0 index Bit[21:12] */
> > +#define ZEROETH_SHIFT           (12)
> > +#define ZEROETH_MASK            (VPN_MASK << ZEROETH_SHIFT)
> > +#endif
> > +
> > +#define THIRD_SIZE              (1 << THIRD_SHIFT)
> > +#define THIRD_MAP_MASK          (~(THIRD_SIZE - 1))
> > +#define SECOND_SIZE             (1 << SECOND_SHIFT)
> > +#define SECOND_MAP_MASK         (~(SECOND_SIZE - 1))
> > +#define FIRST_SIZE              (1 << FIRST_SHIFT)
> > +#define FIRST_MAP_MASK          (~(FIRST_SIZE - 1))
> > +#define ZEROETH_SIZE            (1 << ZEROETH_SHIFT)
> > +#define ZEROETH_MAP_MASK        (~(ZEROETH_SIZE - 1))
> > +
> > +#define PTE_SHIFT               10
> > +
> > +#define PTE_VALID               BIT(0, UL)
> > +#define PTE_READABLE            BIT(1, UL)
> > +#define PTE_WRITABLE            BIT(2, UL)
> > +#define PTE_EXECUTABLE          BIT(3, UL)
> > +#define PTE_USER                BIT(4, UL)
> > +#define PTE_GLOBAL              BIT(5, UL)
> > +#define PTE_ACCESSED            BIT(6, UL)
> > +#define PTE_DIRTY               BIT(7, UL)
> > +#define PTE_RSW                 (BIT(8, UL) | BIT(9, UL))
> > +
> > +#define PTE_LEAF_DEFAULT        (PTE_VALID | PTE_READABLE |
> > PTE_WRITABLE | PTE_EXECUTABLE)
> 
> We should avoid vulnerable default flags. So this should either be RW
> or RX.
Thanks. I'll take it into account.
> 
> > +#define PTE_TABLE               (PTE_VALID)
> > +
> > +/* Calculate the offsets into the pagetables for a given VA */
> > +#define zeroeth_linear_offset(va)   ((va) >> ZEROETH_SHIFT)
> > +#define first_linear_offset(va)     ((va) >> FIRST_SHIFT)
> > +#define second_linear_offset(va)    ((va) >> SECOND_SHIFT)
> > +#define third_linear_offset(va)     ((va) >> THIRD_SHIFT)
> > +
> > +#define pagetable_zeroeth_index(va) zeroeth_linear_offset((va) &
> > ZEROETH_MASK)
> > +#define pagetable_first_index(va)   first_linear_offset((va) &
> > FIRST_MASK)
> > +#define pagetable_second_index(va)  second_linear_offset((va) &
> > SECOND_MASK)
> > +#define pagetable_third_index(va)   third_linear_offset((va) &
> > THIRD_MASK)
> > +
> > +/* Page Table entry */
> > +typedef struct {
> > +    uint64_t pte;
> > +} pte_t;
> > +
> > +/* Shift the VPN[x] or PPN[x] fields of a virtual or physical
> > address
> > + * to become the shifted PPN[x] fields of a page table entry */
> > +#define addr_to_ppn(x) (((x) >> PAGE_SHIFT) << PTE_SHIFT)
> > +
> > +static inline pte_t paddr_to_pte(unsigned long paddr)
> > +{
> > +    return (pte_t) { .pte = addr_to_ppn(paddr) };
> > +}
> > +
> > +static inline bool pte_is_valid(pte_t *p)
> > +{
> > +    return p->pte & PTE_VALID;
> > +}
> > +
> > +#endif /* _ASM_RISCV_PAGE_H */
> > diff --git a/xen/arch/riscv/mm.c b/xen/arch/riscv/mm.c
> > new file mode 100644
> > index 0000000000..6e172376eb
> > --- /dev/null
> > +++ b/xen/arch/riscv/mm.c
> > @@ -0,0 +1,223 @@
> > +#include <xen/init.h>
> > +#include <xen/lib.h>
> > +
> > +#include <asm/csr.h>
> > +#include <asm/mm.h>
> > +#include <asm/page.h>
> > +
> > +/*
> > + * xen_second_pagetable is indexed with the VPN[2] page table
> > entry field
> > + * xen_first_pagetable is accessed from the VPN[1] page table
> > entry field
> > + * xen_zeroeth_pagetable is accessed from the VPN[0] page table
> > entry field
> > + */
> > +pte_t xen_second_pagetable[PAGE_ENTRIES]
> > __attribute__((__aligned__(PAGE_SIZE)));
> > +static pte_t xen_first_pagetable[PAGE_ENTRIES]
> > +    __attribute__((__aligned__(PAGE_SIZE)));
> > +static pte_t xen_zeroeth_pagetable[PAGE_ENTRIES]
> > +    __attribute__((__aligned__(PAGE_SIZE)));
> > +
> > +extern unsigned long _stext;
> > +extern unsigned long _etext;
> > +extern unsigned long __init_begin;
> > +extern unsigned long __init_end;
> > +extern unsigned long _srodata;
> > +extern unsigned long _erodata;
> > +
> > +paddr_t phys_offset;
> 
> This is defined, set but not used.
> 
> > +
> > +#define resolve_early_addr(x) \
> 
> This helper seems to behave the same wasy as linker_addr(). So any 
> reason to not use it?
linker_addr() script can be used instead. It looks I missed something
before and it is spilled out into two equal macros.
> 
> I will make this assumption this can be used and not comment on the 
> implement of resolve_early_addr().
> 
> > +   
> > ({                                                                 
> >          \
> > +         unsigned long *
> > __##x;                                                 \
> > +        if ( load_addr_start <= x && x < load_addr_end
> > )                        \
> > +            __##x = (unsigned long
> > *)x;                                         \
> > +       
> > else                                                               
> >      \
> > +            __##x = (unsigned long *)(x + load_addr_start -
> > linker_addr_start); \
> > +       
> > __##x;                                                             
> >      \
> > +     })
> > +
> > +static void __init clear_pagetables(unsigned long load_addr_start,
> > +                             unsigned long load_addr_end,
> > +                             unsigned long linker_addr_start,
> > +                             unsigned long linker_addr_end)
> > +{
> > +    unsigned long *p;
> > +    unsigned long page;
> > +    unsigned long i;
> > +
> > +    page = (unsigned long)&xen_second_pagetable[0];
> > +
> > +    p = resolve_early_addr(page);
> > +    for ( i = 0; i < ARRAY_SIZE(xen_second_pagetable); i++ )
> 
> The entries in xen_second_pagetable are a pte_t (uint64_t). But ...
> 
> > +    {
> > +        p[i] = 0ULL;
> 
> ... the type here will be unsigned long. So you may not fully zero
> the 
> page-table on 32-bit architecture. Therefore you want to define as
> pte_t.
> 
> That said, given the page table will be part of BSS, you should not
> need 
> to zero again assuming you clear BSS before hand.
> 
> If you clear afterwards, then you *must* move them out of BSS.
> 
> The same applies for xen_{first, zeroeth}_pagetable below.
I didn't have initialized page tables so that is why I needed
clear_pagetables() but I think you are right and clear_pagetables can
be removed at all as page tables will be initialized during BSS
initialization.
> 
> > +    }
> > +
> > +    page = (unsigned long)&xen_first_pagetable[0];
> > +    p = resolve_early_addr(page);
> > +    for ( i = 0; i < ARRAY_SIZE(xen_first_pagetable); i++ )
> > +    {
> > +        p[i] = 0ULL;
> > +    }
> > +
> > +    page = (unsigned long)&xen_zeroeth_pagetable[0];
> > +    p = resolve_early_addr(page);
> > +    for ( i = 0; i < ARRAY_SIZE(xen_zeroeth_pagetable); i++ )
> > +    {
> > +        p[i] = 0ULL;
> > +    }
> > +}
> > +
> > +/*
> > + * WARNING: load_addr() and linker_addr() are to be called only
> > when the MMU is
> > + * disabled and only when executed by the primary CPU.  They
> > cannot refer to
> > + * any global variable or functions.
> 
> I find interesting you are saying when _setup_initial_pagetables() is
> called from setup_initial_pagetables(). Would you be able to explain
> how 
> this is different?
I am not sure that I understand your question correctly but
_setup_initial_pagetables() was introduced to map some addresses with
write/read flag. Probably I have to rename it to something that is more
clear.
> 
> > + */
> > +
> > +/*
> > + * Convert an addressed layed out at link time to the address
> > where it was loaded
> 
> Typo: s/addressed/address/ ?
Yes, it should be address. and 'layed out' should be changed to 'laid
out'...
> 
> > + * by the bootloader.
> > + */
> 
> Looking at the implementation, you seem to consider that any address
> not 
> in the range [linker_addr_start, linker_addr_end[ will have a 1:1
> mappings.
> 
> I am not sure this is what you want. So I would consider to throw an 
> error if such address is passed.
I thought that at this stage and if no relocation was done it is 1:1
except the case when load_addr_start != linker_addr_start.


> 
> > +#define
> > load_addr(linker_address)                                          
> >     \
> > +   
> > ({                                                                 
> >         \
> > +        unsigned long __linker_address = (unsigned
> > long)(linker_address);      \
> > +        if ( linker_addr_start <= __linker_address
> > &&                          \
> > +            __linker_address < linker_addr_end
> > )                               \
> > +       
> > {                                                                  
> >     \
> > +            __linker_address
> > =                                                 \
> > +                __linker_address - linker_addr_start +
> > load_addr_start;        \
> > +       
> > }                                                                  
> >     \
> > +       
> > __linker_address;                                                  
> >     \
> > +    })
> > +
> > +/* Convert boot-time Xen address from where it was loaded by the
> > boot loader to the address it was layed out
> > + * at link-time.
> > + */
> 
> Coding style: The first line is too long and multi-line comments look
> like:
> 
> /*
>   * Foo
>   * Bar
>   */
> 
> > +#define
> > linker_addr(load_address)                                          
> >     \
> 
> Same remark as for load_addr() above.
> 
> > +   
> > ({                                                                 
> >         \
> > +        unsigned long __load_address = (unsigned
> > long)(load_address);          \
> > +        if ( load_addr_start <= __load_address
> > &&                              \
> > +            __load_address < load_addr_end
> > )                                   \
> > +       
> > {                                                                  
> >     \
> > +            __load_address
> > =                                                   \
> > +                __load_address - load_addr_start +
> > linker_addr_start;          \
> > +       
> > }                                                                  
> >     \
> > +       
> > __load_address;                                                    
> >     \
> > +    })
> > +
> > +static void __attribute__((section(".entry")))
> > +_setup_initial_pagetables(pte_t *second, pte_t *first, pte_t
> > *zeroeth,
> Can this be named to setup_initial_mapping() so this is clearer and 
> avoid the one '_' different with the function below.
Sure. It will be better.
> 
> > +                         unsigned long map_start,
> > +                         unsigned long map_end,
> > +                         unsigned long pa_start,
> > +                         bool writable)
> 
> What about the executable bit?
It's always executable... But as you mentioned above PTE_LEAF_DEFAULT
should be either RX or RW.
I think it makes sense to add flags instead of writable.
> 
> > +{
> > +    unsigned long page_addr;
> > +    unsigned long index2;
> > +    unsigned long index1;
> > +    unsigned long index0;
> 
> index* could be defined in the loop below.
It could. But I am curious why it is better?
> 
> > +
> > +    /* align start addresses */
> > +    map_start &= ZEROETH_MAP_MASK;
> > +    pa_start &= ZEROETH_MAP_MASK;
> 
> Hmmm... I would actually expect the address to be properly aligned
> and 
> therefore not require an alignment here.
> 
> Otherwise, this raise the question of what happen if you have region 
> using the same page?
That map_start &=  ZEROETH_MAP_MASK is needed to page number of address
w/o page offset.

> 
> > +
> > +    page_addr = map_start;
> > +    while ( page_addr < map_end )
> 
> Looking at the loop, it looks like you are assuming that the region
> will 
> never cross a boundary of a page-table (either L0, L1, L2). I am not 
> convinced you can make such assumption (see below).
> 
> But if you really want to make such assumption then you should add
> some 
> guard (either BUILD_BUG_ON(), ASSERT(), proper check) in your code to
> avoid any surprise in the future.
I am not sure that I fully understand what is the problem here.
The address is aligned on (1<<12) boundary and each itearation is
mapped (1<<12) page so all looks fine or I misunderstood you.
> 
> > +    {
> > +        index2 = pagetable_second_index(page_addr);
> > +        index1 = pagetable_first_index(page_addr);
> > +        index0 = pagetable_zeroeth_index(page_addr);
> > +
> > +        /* Setup level2 table */
> > +        second[index2] = paddr_to_pte((unsigned long)first);
> > +        second[index2].pte |= PTE_TABLE;
> > +
> > +        /* Setup level1 table */
> > +        first[index1] = paddr_to_pte((unsigned long)zeroeth);
> > +        first[index1].pte |= PTE_TABLE;
> > +
> > +        /* Setup level0 table */
> > +        if ( !pte_is_valid(&zeroeth[index0]) )
> 
> Can you explain why you are checking !pte_is_valid() for the L0 entry
> but not the other?
My mistake it should be checked for each level.
> 
> > +        {
> > +            /* Update level0 table */
> > +            zeroeth[index0] = paddr_to_pte((page_addr - map_start)
> > + pa_start);
> > +            zeroeth[index0].pte |= PTE_LEAF_DEFAULT;
> > +            zeroeth[index0].pte &= ~((!writable) ? PTE_WRITABLE :
> > 0);
> 
> Looking at the default value, it would mean that a non-writable
> mapping 
> is automatically executable. This seems wrong for the section is not 
> meant to be executable (like rodata).
Yes, you are right. I'll reowrk setup_initial_mapping() to pass flags
instead of write/read - only flag.
> 
> > +        }
> > +
> > +        /* Point to next page */
> > +        page_addr += ZEROETH_SIZE;
> > +    }
> > +}
> > +
> > +/*
> > + * setup_initial_pagetables:
> > + *
> > + * 1) Build the page tables for Xen that map the following:
> > + *   1.1)  The physical location of Xen (where the bootloader
> > loaded it)
> > + *   1.2)  The link-time location of Xen (where the linker
> > expected Xen's
> > + *         addresses to be)
> > + * 2) Load the page table into the SATP and enable the MMU
> > + */
> > +void __attribute__((section(".entry")))
> 
> I couldn't find a section ".entry" in the linker.
> 
> > +setup_initial_pagetables(unsigned long load_addr_start,
> > +                         unsigned long load_addr_end,
> > +                         unsigned long linker_addr_start,
> > +                         unsigned long linker_addr_end)
> > +{
> > +    pte_t *second;
> > +    pte_t *first;
> > +    pte_t *zeroeth;
> > +
> > +    clear_pagetables(load_addr_start, load_addr_end,
> > +                     linker_addr_start, linker_addr_end);
> > +
> > +    /* Get the addresses where the page tables were loaded */
> > +    second  = (pte_t *)load_addr(&xen_second_pagetable);
> > +    first   = (pte_t *)load_addr(&xen_first_pagetable);
> > +    zeroeth = (pte_t *)load_addr(&xen_zeroeth_pagetable);
> 
> I would consider to embed the type cast in load_addr() so you are
> adding 
> some type safety within your code.
Definitely it will be better but setup_initial_mapping() uses 'unsigned
long' for passing address that should be mapped.
> 
> > +
> > +    /*
> > +     * Create a mapping from Xen's link-time addresses to where
> > they were actually loaded.
> 
> This is line is way long than 80 characters. Please make sure to wrap
> it 
> 80 characters.
> 
> > +     */
> > +    _setup_initial_pagetables(second, first, zeroeth,
> > +                              linker_addr(&_stext),
> > +                              linker_addr(&_etext),
> > +                              load_addr(&_stext),
> > +                              false);
> > +    _setup_initial_pagetables(second, first, zeroeth,
> > +                              linker_addr(&__init_begin),
> > +                              linker_addr(&__init_end),
> > +                              load_addr(&__init_begin),
> > +                              true);
> > +    _setup_initial_pagetables(second, first, zeroeth,
> > +                              linker_addr(&_srodata),
> > +                              linker_addr(&_erodata),
> > +                              load_addr(&_srodata),
> > +                              false);
> > +    _setup_initial_pagetables(second, first, zeroeth,
> > +                              linker_addr_start,
> > +                              linker_addr_end,
> > +                              load_addr_start,
> > +                              true);
> 
> Where do you guarantee that Xen will always fit in an L0 table and
> the 
> start address is aligned to the size of an L0 table?
I don't guarantee that it fit in an L0 table but the start address is
aligned to the size of the L0 table at the start.
> 
> > +
> > +    /*
> > +     * Create a mapping of the load time address range to... the
> > load time address range.
> 
> Same about the line length here.
> 
> > +     * This mapping is used at boot time only.
> > +     */
> > +    _setup_initial_pagetables(second, first, zeroeth,
> 
> This can only work if Xen is loaded at its linked address. So you
> need a 
> separate set of L0, L1 tables for the identity mapping.
> 
> That said, this would not be sufficient because:
>    1) Xen may not be loaded at a 2M boundary (you can control with 
> U-boot, but not with EFI). So this may cross a boundary and therefore
> need multiple pages.
>    2) The load region may overlap the link address
> 
> While I think it would be good to handle those cases from the start,
> I 
> would understand why are not easy to solve. So I think the minimum is
> to 
> throw some errors if you are in a case you can't support.
Do you mean to throw some error in load_addr()/linkder_addr()?
> 
> > +                              load_addr_start,
> > +                              load_addr_end,
> > +                              load_addr_start,
> > +                              true); > +
> > +    /* Ensure page table writes precede loading the SATP */
> > +    asm volatile("sfence.vma");
> > +
> > +    /* Enable the MMU and load the new pagetable for Xen */
> > +    csr_write(CSR_SATP,
> > +              (load_addr(xen_second_pagetable) >> PAGE_SHIFT) |
> > SATP_MODE_SV39 << SATP_MODE_SHIFT);
> 
> IHMO, it would make sense to introduce within the series the code to 
> jump off the identity mapping and then remove it.
Probably I have to spend more time to understand identity mapping but
it looks like current  one RISC-V port from Bobby's doesn't jump off
and remove the identity mapping.
> 
> > +
> > +    phys_offset = load_addr_start - linker_addr_start;
> > +
> > +    return;
> > +}
> 
> Cheers,
> 
~ Oleksii

Re: [PATCH v1 1/3] xen/riscv: introduce setup_initial_pages

[Julien Grall]

Hi Oleksii,

On 27/02/2023 16:52, Oleksii wrote:
> On Sat, 2023-02-25 at 17:53 +0000, Julien Grall wrote:
>>> +/*
>>> + * WARNING: load_addr() and linker_addr() are to be called only
>>> when the MMU is
>>> + * disabled and only when executed by the primary CPU.  They
>>> cannot refer to
>>> + * any global variable or functions.
>>
>> I find interesting you are saying when _setup_initial_pagetables() is
>> called from setup_initial_pagetables(). Would you be able to explain
>> how
>> this is different?
> I am not sure that I understand your question correctly but
> _setup_initial_pagetables() was introduced to map some addresses with
> write/read flag. Probably I have to rename it to something that is more
> clear.

So the comment suggests that you code cannot refer to global 
functions/variables when the MMU is off. So I have multiple questions:
   * Why only global? IOW, why static would be OK?
   * setup_initial_pagetables() has a call to 
_setup_initial_pagetables() (IOW referring to another function). Why is 
it fine?
   * You have code in the next patch referring to global variables 
(mainly _start and _end). How is this different?

>>
>>> + */
>>> +
>>> +/*
>>> + * Convert an addressed layed out at link time to the address
>>> where it was loaded
>>
>> Typo: s/addressed/address/ ?
> Yes, it should be address. and 'layed out' should be changed to 'laid
> out'...
>>
>>> + * by the bootloader.
>>> + */
>>
>> Looking at the implementation, you seem to consider that any address
>> not
>> in the range [linker_addr_start, linker_addr_end[ will have a 1:1
>> mappings.
>>
>> I am not sure this is what you want. So I would consider to throw an
>> error if such address is passed.
> I thought that at this stage and if no relocation was done it is 1:1
> except the case when load_addr_start != linker_addr_start.

The problem is what you try to map one to one may clash with the linked 
region for Xen. So it is not always possible to map the region 1:1.

Therefore, I don't see any use for the else part here.

> 
> 
>>
>>> +#define
>>> load_addr(linker_address)
>>>      \
>>> +
>>> ({
>>>          \
>>> +        unsigned long __linker_address = (unsigned
>>> long)(linker_address);      \
>>> +        if ( linker_addr_start <= __linker_address
>>> &&                          \
>>> +            __linker_address < linker_addr_end
>>> )                               \
>>> +
>>> {
>>>      \
>>> +            __linker_address
>>> =                                                 \
>>> +                __linker_address - linker_addr_start +
>>> load_addr_start;        \
>>> +
>>> }
>>>      \
>>> +
>>> __linker_address;
>>>      \
>>> +    })
>>> +
>>> +/* Convert boot-time Xen address from where it was loaded by the
>>> boot loader to the address it was layed out
>>> + * at link-time.
>>> + */
>>
>> Coding style: The first line is too long and multi-line comments look
>> like:
>>
>> /*
>>    * Foo
>>    * Bar
>>    */
>>
>>> +#define
>>> linker_addr(load_address)
>>>      \
>>
>> Same remark as for load_addr() above.
>>
>>> +
>>> ({
>>>          \
>>> +        unsigned long __load_address = (unsigned
>>> long)(load_address);          \
>>> +        if ( load_addr_start <= __load_address
>>> &&                              \
>>> +            __load_address < load_addr_end
>>> )                                   \
>>> +
>>> {
>>>      \
>>> +            __load_address
>>> =                                                   \
>>> +                __load_address - load_addr_start +
>>> linker_addr_start;          \
>>> +
>>> }
>>>      \
>>> +
>>> __load_address;
>>>      \
>>> +    })
>>> +
>>> +static void __attribute__((section(".entry")))
>>> +_setup_initial_pagetables(pte_t *second, pte_t *first, pte_t
>>> *zeroeth,
>> Can this be named to setup_initial_mapping() so this is clearer and
>> avoid the one '_' different with the function below.
> Sure. It will be better.
>>
>>> +                         unsigned long map_start,
>>> +                         unsigned long map_end,
>>> +                         unsigned long pa_start,
>>> +                         bool writable)
>>
>> What about the executable bit?
> It's always executable... But as you mentioned above PTE_LEAF_DEFAULT
> should be either RX or RW.
> I think it makes sense to add flags instead of writable.
>>
>>> +{
>>> +    unsigned long page_addr;
>>> +    unsigned long index2;
>>> +    unsigned long index1;
>>> +    unsigned long index0;
>>
>> index* could be defined in the loop below.
> It could. But I am curious why it is better?
>>
>>> +
>>> +    /* align start addresses */
>>> +    map_start &= ZEROETH_MAP_MASK;
>>> +    pa_start &= ZEROETH_MAP_MASK;
>>
>> Hmmm... I would actually expect the address to be properly aligned
>> and
>> therefore not require an alignment here.
>>
>> Otherwise, this raise the question of what happen if you have region
>> using the same page?
> That map_start &=  ZEROETH_MAP_MASK is needed to page number of address
> w/o page offset.

My point is why would the page offset be non-zero?

>>
>>> +
>>> +    page_addr = map_start;
>>> +    while ( page_addr < map_end )
>>
>> Looking at the loop, it looks like you are assuming that the region
>> will
>> never cross a boundary of a page-table (either L0, L1, L2). I am not
>> convinced you can make such assumption (see below).
>>
>> But if you really want to make such assumption then you should add
>> some
>> guard (either BUILD_BUG_ON(), ASSERT(), proper check) in your code to
>> avoid any surprise in the future.
> I am not sure that I fully understand what is the problem here.
> The address is aligned on (1<<12) boundary and each itearation is
> mapped (1<<12) page so all looks fine or I misunderstood you.

Let's take an example, imagine the region you want to map is 4MB. 
AFAICT, you are only passing one L0 page-table. So your code will end up 
to overwrite the previous entries in the zeroeth page-table and then add 
another link in the L1 page-table.

>>
>>> +    {
>>> +        index2 = pagetable_second_index(page_addr);
>>> +        index1 = pagetable_first_index(page_addr);
>>> +        index0 = pagetable_zeroeth_index(page_addr);
>>> +
>>> +        /* Setup level2 table */
>>> +        second[index2] = paddr_to_pte((unsigned long)first);
>>> +        second[index2].pte |= PTE_TABLE;
>>> +
>>> +        /* Setup level1 table */
>>> +        first[index1] = paddr_to_pte((unsigned long)zeroeth);
>>> +        first[index1].pte |= PTE_TABLE;
>>> +
>>> +        /* Setup level0 table */
>>> +        if ( !pte_is_valid(&zeroeth[index0]) )
>>
>> Can you explain why you are checking !pte_is_valid() for the L0 entry
>> but not the other?
> My mistake it should be checked for each level.

In which case, shouldn't you return an error if the entry is always valid?

>>
>>> +        {
>>> +            /* Update level0 table */
>>> +            zeroeth[index0] = paddr_to_pte((page_addr - map_start)
>>> + pa_start);
>>> +            zeroeth[index0].pte |= PTE_LEAF_DEFAULT;
>>> +            zeroeth[index0].pte &= ~((!writable) ? PTE_WRITABLE :
>>> 0);
>>
>> Looking at the default value, it would mean that a non-writable
>> mapping
>> is automatically executable. This seems wrong for the section is not
>> meant to be executable (like rodata).
> Yes, you are right. I'll reowrk setup_initial_mapping() to pass flags
> instead of write/read - only flag.
>>
>>> +        }
>>> +
>>> +        /* Point to next page */
>>> +        page_addr += ZEROETH_SIZE;
>>> +    }
>>> +}
>>> +
>>> +/*
>>> + * setup_initial_pagetables:
>>> + *
>>> + * 1) Build the page tables for Xen that map the following:
>>> + *   1.1)  The physical location of Xen (where the bootloader
>>> loaded it)
>>> + *   1.2)  The link-time location of Xen (where the linker
>>> expected Xen's
>>> + *         addresses to be)
>>> + * 2) Load the page table into the SATP and enable the MMU
>>> + */
>>> +void __attribute__((section(".entry")))
>>
>> I couldn't find a section ".entry" in the linker.
>>
>>> +setup_initial_pagetables(unsigned long load_addr_start,
>>> +                         unsigned long load_addr_end,
>>> +                         unsigned long linker_addr_start,
>>> +                         unsigned long linker_addr_end)
>>> +{
>>> +    pte_t *second;
>>> +    pte_t *first;
>>> +    pte_t *zeroeth;
>>> +
>>> +    clear_pagetables(load_addr_start, load_addr_end,
>>> +                     linker_addr_start, linker_addr_end);
>>> +
>>> +    /* Get the addresses where the page tables were loaded */
>>> +    second  = (pte_t *)load_addr(&xen_second_pagetable);
>>> +    first   = (pte_t *)load_addr(&xen_first_pagetable);
>>> +    zeroeth = (pte_t *)load_addr(&xen_zeroeth_pagetable);
>>
>> I would consider to embed the type cast in load_addr() so you are
>> adding
>> some type safety within your code.
> Definitely it will be better but setup_initial_mapping() uses 'unsigned
> long' for passing address that should be mapped.

One possibility would be to introduce a new wrapper for the typesafety. 
Anyway, it is not essential for now. Let's at least get the logic 
correct first :).

>>
>>> +
>>> +    /*
>>> +     * Create a mapping from Xen's link-time addresses to where
>>> they were actually loaded.
>>
>> This is line is way long than 80 characters. Please make sure to wrap
>> it
>> 80 characters.
>>
>>> +     */
>>> +    _setup_initial_pagetables(second, first, zeroeth,
>>> +                              linker_addr(&_stext),
>>> +                              linker_addr(&_etext),
>>> +                              load_addr(&_stext),
>>> +                              false);
>>> +    _setup_initial_pagetables(second, first, zeroeth,
>>> +                              linker_addr(&__init_begin),
>>> +                              linker_addr(&__init_end),
>>> +                              load_addr(&__init_begin),
>>> +                              true);
>>> +    _setup_initial_pagetables(second, first, zeroeth,
>>> +                              linker_addr(&_srodata),
>>> +                              linker_addr(&_erodata),
>>> +                              load_addr(&_srodata),
>>> +                              false);
>>> +    _setup_initial_pagetables(second, first, zeroeth,
>>> +                              linker_addr_start,
>>> +                              linker_addr_end,
>>> +                              load_addr_start,
>>> +                              true);
>>
>> Where do you guarantee that Xen will always fit in an L0 table and
>> the
>> start address is aligned to the size of an L0 table?
> I don't guarantee that it fit in an L0 table but the start address is
> aligned to the size of the L0 table at the start.
Then it should be fixed.

>>
>>> +
>>> +    /*
>>> +     * Create a mapping of the load time address range to... the
>>> load time address range.
>>
>> Same about the line length here.
>>
>>> +     * This mapping is used at boot time only.
>>> +     */
>>> +    _setup_initial_pagetables(second, first, zeroeth,
>>
>> This can only work if Xen is loaded at its linked address. So you
>> need a
>> separate set of L0, L1 tables for the identity mapping.
>>
>> That said, this would not be sufficient because:
>>     1) Xen may not be loaded at a 2M boundary (you can control with
>> U-boot, but not with EFI). So this may cross a boundary and therefore
>> need multiple pages.
>>     2) The load region may overlap the link address
>>
>> While I think it would be good to handle those cases from the start,
>> I
>> would understand why are not easy to solve. So I think the minimum is
>> to
>> throw some errors if you are in a case you can't support.
> Do you mean to throw some error in load_addr()/linkder_addr()?

In this case, I meant to check if load_addr != linker_addr, then throw 
an error.

Cheers,

-- 
Julien Grall

Re: [PATCH v1 1/3] xen/riscv: introduce setup_initial_pages

[Oleksii]

Hi Julien,

On Mon, 2023-02-27 at 17:36 +0000, Julien Grall wrote:
> Hi Oleksii,
> 
> On 27/02/2023 16:52, Oleksii wrote:
> > On Sat, 2023-02-25 at 17:53 +0000, Julien Grall wrote:
> > > > +/*
> > > > + * WARNING: load_addr() and linker_addr() are to be called
> > > > only
> > > > when the MMU is
> > > > + * disabled and only when executed by the primary CPU.  They
> > > > cannot refer to
> > > > + * any global variable or functions.
> > > 
> > > I find interesting you are saying when
> > > _setup_initial_pagetables() is
> > > called from setup_initial_pagetables(). Would you be able to
> > > explain
> > > how
> > > this is different?
> > I am not sure that I understand your question correctly but
> > _setup_initial_pagetables() was introduced to map some addresses
> > with
> > write/read flag. Probably I have to rename it to something that is
> > more
> > clear.
> 
> So the comment suggests that you code cannot refer to global 
> functions/variables when the MMU is off. So I have multiple
> questions:
>    * Why only global? IOW, why static would be OK?
>    * setup_initial_pagetables() has a call to 
> _setup_initial_pagetables() (IOW referring to another function). Why
> is 
> it fine?
>    * You have code in the next patch referring to global variables 
> (mainly _start and _end). How is this different?
> 
> > > 
> > > > + */
> > > > +
> > > > +/*
> > > > + * Convert an addressed layed out at link time to the address
> > > > where it was loaded
> > > 
> > > Typo: s/addressed/address/ ?
> > Yes, it should be address. and 'layed out' should be changed to
> > 'laid
> > out'...
> > > 
> > > > + * by the bootloader.
> > > > + */
> > > 
> > > Looking at the implementation, you seem to consider that any
> > > address
> > > not
> > > in the range [linker_addr_start, linker_addr_end[ will have a 1:1
> > > mappings.
> > > 
> > > I am not sure this is what you want. So I would consider to throw
> > > an
> > > error if such address is passed.
> > I thought that at this stage and if no relocation was done it is
> > 1:1
> > except the case when load_addr_start != linker_addr_start.
> 
> The problem is what you try to map one to one may clash with the
> linked 
> region for Xen. So it is not always possible to map the region 1:1.
> 
> Therefore, I don't see any use for the else part here.
Got it. Thanks.

I am curious than what is the correct approach in general to handle
this situation?
I mean that throw an error it is one option but if I would like to do
that w/o throwing an error. Should it done some relocation in that
case?

> 
> > 
> > 
> > > 
> > > > +#define
> > > > load_addr(linker_address)
> > > >      \
> > > > +
> > > > ({
> > > >          \
> > > > +        unsigned long __linker_address = (unsigned
> > > > long)(linker_address);      \
> > > > +        if ( linker_addr_start <= __linker_address
> > > > &&                          \
> > > > +            __linker_address < linker_addr_end
> > > > )                               \
> > > > +
> > > > {
> > > >      \
> > > > +            __linker_address
> > > > =                                                 \
> > > > +                __linker_address - linker_addr_start +
> > > > load_addr_start;        \
> > > > +
> > > > }
> > > >      \
> > > > +
> > > > __linker_address;
> > > >      \
> > > > +    })
> > > > +
> > > > +/* Convert boot-time Xen address from where it was loaded by
> > > > the
> > > > boot loader to the address it was layed out
> > > > + * at link-time.
> > > > + */
> > > 
> > > Coding style: The first line is too long and multi-line comments
> > > look
> > > like:
> > > 
> > > /*
> > >    * Foo
> > >    * Bar
> > >    */
> > > 
> > > > +#define
> > > > linker_addr(load_address)
> > > >      \
> > > 
> > > Same remark as for load_addr() above.
> > > 
> > > > +
> > > > ({
> > > >          \
> > > > +        unsigned long __load_address = (unsigned
> > > > long)(load_address);          \
> > > > +        if ( load_addr_start <= __load_address
> > > > &&                              \
> > > > +            __load_address < load_addr_end
> > > > )                                   \
> > > > +
> > > > {
> > > >      \
> > > > +            __load_address
> > > > =                                                   \
> > > > +                __load_address - load_addr_start +
> > > > linker_addr_start;          \
> > > > +
> > > > }
> > > >      \
> > > > +
> > > > __load_address;
> > > >      \
> > > > +    })
> > > > +
> > > > +static void __attribute__((section(".entry")))
> > > > +_setup_initial_pagetables(pte_t *second, pte_t *first, pte_t
> > > > *zeroeth,
> > > Can this be named to setup_initial_mapping() so this is clearer
> > > and
> > > avoid the one '_' different with the function below.
> > Sure. It will be better.
> > > 
> > > > +                         unsigned long map_start,
> > > > +                         unsigned long map_end,
> > > > +                         unsigned long pa_start,
> > > > +                         bool writable)
> > > 
> > > What about the executable bit?
> > It's always executable... But as you mentioned above
> > PTE_LEAF_DEFAULT
> > should be either RX or RW.
> > I think it makes sense to add flags instead of writable.
> > > 
> > > > +{
> > > > +    unsigned long page_addr;
> > > > +    unsigned long index2;
> > > > +    unsigned long index1;
> > > > +    unsigned long index0;
> > > 
> > > index* could be defined in the loop below.
> > It could. But I am curious why it is better?
> > > 
> > > > +
> > > > +    /* align start addresses */
> > > > +    map_start &= ZEROETH_MAP_MASK;
> > > > +    pa_start &= ZEROETH_MAP_MASK;
> > > 
> > > Hmmm... I would actually expect the address to be properly
> > > aligned
> > > and
> > > therefore not require an alignment here.
> > > 
> > > Otherwise, this raise the question of what happen if you have
> > > region
> > > using the same page?
> > That map_start &=  ZEROETH_MAP_MASK is needed to page number of
> > address
> > w/o page offset.
> 
> My point is why would the page offset be non-zero?
I checked a linker script and addresses that passed to
setup_initial_mapping() and they are really always aligned so there is
no any sense in additional alignment.

> 
> > > 
> > > > +
> > > > +    page_addr = map_start;
> > > > +    while ( page_addr < map_end )
> > > 
> > > Looking at the loop, it looks like you are assuming that the
> > > region
> > > will
> > > never cross a boundary of a page-table (either L0, L1, L2). I am
> > > not
> > > convinced you can make such assumption (see below).
> > > 
> > > But if you really want to make such assumption then you should
> > > add
> > > some
> > > guard (either BUILD_BUG_ON(), ASSERT(), proper check) in your
> > > code to
> > > avoid any surprise in the future.
> > I am not sure that I fully understand what is the problem here.
> > The address is aligned on (1<<12) boundary and each itearation is
> > mapped (1<<12) page so all looks fine or I misunderstood you.
> 
> Let's take an example, imagine the region you want to map is 4MB. 
> AFAICT, you are only passing one L0 page-table. So your code will end
> up 
> to overwrite the previous entries in the zeroeth page-table and then
> add 
> another link in the L1 page-table.
Got it. Then it looks that current approach isn't correct totally...

> 
> > > 
> > > > +    {
> > > > +        index2 = pagetable_second_index(page_addr);
> > > > +        index1 = pagetable_first_index(page_addr);
> > > > +        index0 = pagetable_zeroeth_index(page_addr);
> > > > +
> > > > +        /* Setup level2 table */
> > > > +        second[index2] = paddr_to_pte((unsigned long)first);
> > > > +        second[index2].pte |= PTE_TABLE;
> > > > +
> > > > +        /* Setup level1 table */
> > > > +        first[index1] = paddr_to_pte((unsigned long)zeroeth);
> > > > +        first[index1].pte |= PTE_TABLE;
> > > > +
> > > > +        /* Setup level0 table */
> > > > +        if ( !pte_is_valid(&zeroeth[index0]) )
> > > 
> > > Can you explain why you are checking !pte_is_valid() for the L0
> > > entry
> > > but not the other?
> > My mistake it should be checked for each level.
> 
> In which case, shouldn't you return an error if the entry is always
> valid?
> 
> > > 
> > > > +        {
> > > > +            /* Update level0 table */
> > > > +            zeroeth[index0] = paddr_to_pte((page_addr -
> > > > map_start)
> > > > + pa_start);
> > > > +            zeroeth[index0].pte |= PTE_LEAF_DEFAULT;
> > > > +            zeroeth[index0].pte &= ~((!writable) ?
> > > > PTE_WRITABLE :
> > > > 0);
> > > 
> > > Looking at the default value, it would mean that a non-writable
> > > mapping
> > > is automatically executable. This seems wrong for the section is
> > > not
> > > meant to be executable (like rodata).
> > Yes, you are right. I'll reowrk setup_initial_mapping() to pass
> > flags
> > instead of write/read - only flag.
> > > 
> > > > +        }
> > > > +
> > > > +        /* Point to next page */
> > > > +        page_addr += ZEROETH_SIZE;
> > > > +    }
> > > > +}
> > > > +
> > > > +/*
> > > > + * setup_initial_pagetables:
> > > > + *
> > > > + * 1) Build the page tables for Xen that map the following:
> > > > + *   1.1)  The physical location of Xen (where the bootloader
> > > > loaded it)
> > > > + *   1.2)  The link-time location of Xen (where the linker
> > > > expected Xen's
> > > > + *         addresses to be)
> > > > + * 2) Load the page table into the SATP and enable the MMU
> > > > + */
> > > > +void __attribute__((section(".entry")))
> > > 
> > > I couldn't find a section ".entry" in the linker.
> > > 
> > > > +setup_initial_pagetables(unsigned long load_addr_start,
> > > > +                         unsigned long load_addr_end,
> > > > +                         unsigned long linker_addr_start,
> > > > +                         unsigned long linker_addr_end)
> > > > +{
> > > > +    pte_t *second;
> > > > +    pte_t *first;
> > > > +    pte_t *zeroeth;
> > > > +
> > > > +    clear_pagetables(load_addr_start, load_addr_end,
> > > > +                     linker_addr_start, linker_addr_end);
> > > > +
> > > > +    /* Get the addresses where the page tables were loaded */
> > > > +    second  = (pte_t *)load_addr(&xen_second_pagetable);
> > > > +    first   = (pte_t *)load_addr(&xen_first_pagetable);
> > > > +    zeroeth = (pte_t *)load_addr(&xen_zeroeth_pagetable);
> > > 
> > > I would consider to embed the type cast in load_addr() so you are
> > > adding
> > > some type safety within your code.
> > Definitely it will be better but setup_initial_mapping() uses
> > 'unsigned
> > long' for passing address that should be mapped.
> 
> One possibility would be to introduce a new wrapper for the
> typesafety. 
> Anyway, it is not essential for now. Let's at least get the logic 
> correct first :).
Agree. the logic should be fixed first.
> 
> > > 
> > > > +
> > > > +    /*
> > > > +     * Create a mapping from Xen's link-time addresses to
> > > > where
> > > > they were actually loaded.
> > > 
> > > This is line is way long than 80 characters. Please make sure to
> > > wrap
> > > it
> > > 80 characters.
> > > 
> > > > +     */
> > > > +    _setup_initial_pagetables(second, first, zeroeth,
> > > > +                              linker_addr(&_stext),
> > > > +                              linker_addr(&_etext),
> > > > +                              load_addr(&_stext),
> > > > +                              false);
> > > > +    _setup_initial_pagetables(second, first, zeroeth,
> > > > +                              linker_addr(&__init_begin),
> > > > +                              linker_addr(&__init_end),
> > > > +                              load_addr(&__init_begin),
> > > > +                              true);
> > > > +    _setup_initial_pagetables(second, first, zeroeth,
> > > > +                              linker_addr(&_srodata),
> > > > +                              linker_addr(&_erodata),
> > > > +                              load_addr(&_srodata),
> > > > +                              false);
> > > > +    _setup_initial_pagetables(second, first, zeroeth,
> > > > +                              linker_addr_start,
> > > > +                              linker_addr_end,
> > > > +                              load_addr_start,
> > > > +                              true);
> > > 
> > > Where do you guarantee that Xen will always fit in an L0 table
> > > and
> > > the
> > > start address is aligned to the size of an L0 table?
> > I don't guarantee that it fit in an L0 table but the start address
> > is
> > aligned to the size of the L0 table at the start.
> Then it should be fixed.
> 
> > > 
> > > > +
> > > > +    /*
> > > > +     * Create a mapping of the load time address range to...
> > > > the
> > > > load time address range.
> > > 
> > > Same about the line length here.
> > > 
> > > > +     * This mapping is used at boot time only.
> > > > +     */
> > > > +    _setup_initial_pagetables(second, first, zeroeth,
> > > 
> > > This can only work if Xen is loaded at its linked address. So you
> > > need a
> > > separate set of L0, L1 tables for the identity mapping.
> > > 
> > > That said, this would not be sufficient because:
> > >     1) Xen may not be loaded at a 2M boundary (you can control
> > > with
> > > U-boot, but not with EFI). So this may cross a boundary and
> > > therefore
> > > need multiple pages.
> > >     2) The load region may overlap the link address
> > > 
> > > While I think it would be good to handle those cases from the
> > > start,
> > > I
> > > would understand why are not easy to solve. So I think the
> > > minimum is
> > > to
> > > throw some errors if you are in a case you can't support.
> > Do you mean to throw some error in load_addr()/linkder_addr()?
> 
> In this case, I meant to check if load_addr != linker_addr, then
> throw 
> an error.
I am not sure that it is needed now and it is easier to throw an error
but is option exist to handler situation when load_addr != linker_addr
except throwing an error? relocate?


~ Oleksii

Re: [PATCH v1 1/3] xen/riscv: introduce setup_initial_pages

[Oleksii]

> 
> > 
> > > > 
> > > > > +
> > > > > +    page_addr = map_start;
> > > > > +    while ( page_addr < map_end )
> > > > 
> > > > Looking at the loop, it looks like you are assuming that the
> > > > region
> > > > will
> > > > never cross a boundary of a page-table (either L0, L1, L2). I
> > > > am
> > > > not
> > > > convinced you can make such assumption (see below).
> > > > 
> > > > But if you really want to make such assumption then you should
> > > > add
> > > > some
> > > > guard (either BUILD_BUG_ON(), ASSERT(), proper check) in your
> > > > code to
> > > > avoid any surprise in the future.
> > > I am not sure that I fully understand what is the problem here.
> > > The address is aligned on (1<<12) boundary and each itearation is
> > > mapped (1<<12) page so all looks fine or I misunderstood you.
> > 
> > Let's take an example, imagine the region you want to map is 4MB. 
> > AFAICT, you are only passing one L0 page-table. So your code will
> > end
> > up 
> > to overwrite the previous entries in the zeroeth page-table and
> > then
> > add 
> > another link in the L1 page-table.
> Got it. Then it looks that current approach isn't correct totally...
Or as an option we can add to xen.lds.S something like:

  ASSERT(_end - _start <= MB(L0_ENTRIES*PAGE_SIZE), "Xen too large")

~ Oleksii

Re: [PATCH v1 1/3] xen/riscv: introduce setup_initial_pages

[Julien Grall]

On 05/03/2023 16:25, Oleksii wrote:
> Hi Julien,

Hi,

Sorry for the late answer. I was away for the past couple of weeks.

> On Mon, 2023-02-27 at 17:36 +0000, Julien Grall wrote:
>> Hi Oleksii,
>>
>> On 27/02/2023 16:52, Oleksii wrote:
>>> On Sat, 2023-02-25 at 17:53 +0000, Julien Grall wrote:
>>>>> +/*
>>>>> + * WARNING: load_addr() and linker_addr() are to be called
>>>>> only
>>>>> when the MMU is
>>>>> + * disabled and only when executed by the primary CPU.  They
>>>>> cannot refer to
>>>>> + * any global variable or functions.
>>>>
>>>> I find interesting you are saying when
>>>> _setup_initial_pagetables() is
>>>> called from setup_initial_pagetables(). Would you be able to
>>>> explain
>>>> how
>>>> this is different?
>>> I am not sure that I understand your question correctly but
>>> _setup_initial_pagetables() was introduced to map some addresses
>>> with
>>> write/read flag. Probably I have to rename it to something that is
>>> more
>>> clear.
>>
>> So the comment suggests that you code cannot refer to global
>> functions/variables when the MMU is off. So I have multiple
>> questions:
>>     * Why only global? IOW, why static would be OK?
>>     * setup_initial_pagetables() has a call to
>> _setup_initial_pagetables() (IOW referring to another function). Why
>> is
>> it fine?
>>     * You have code in the next patch referring to global variables
>> (mainly _start and _end). How is this different?
>>
>>>>
>>>>> + */
>>>>> +
>>>>> +/*
>>>>> + * Convert an addressed layed out at link time to the address
>>>>> where it was loaded
>>>>
>>>> Typo: s/addressed/address/ ?
>>> Yes, it should be address. and 'layed out' should be changed to
>>> 'laid
>>> out'...
>>>>
>>>>> + * by the bootloader.
>>>>> + */
>>>>
>>>> Looking at the implementation, you seem to consider that any
>>>> address
>>>> not
>>>> in the range [linker_addr_start, linker_addr_end[ will have a 1:1
>>>> mappings.
>>>>
>>>> I am not sure this is what you want. So I would consider to throw
>>>> an
>>>> error if such address is passed.
>>> I thought that at this stage and if no relocation was done it is
>>> 1:1
>>> except the case when load_addr_start != linker_addr_start.
>>
>> The problem is what you try to map one to one may clash with the
>> linked
>> region for Xen. So it is not always possible to map the region 1:1.
>>
>> Therefore, I don't see any use for the else part here.
> Got it. Thanks.
> 
> I am curious than what is the correct approach in general to handle
> this situation?
There are multiple approach to handle it and I don't know which one 
would be best :). Relocation is one...

> I mean that throw an error it is one option but if I would like to do
> that w/o throwing an error. Should it done some relocation in that
> case?
... solution. For Arm, I decided to avoid relocation it requires more 
work in assembly.

Let me describe what we did and you can decide what you want to do in 
RISC-V.

For Arm64, as we have plenty of virtual address space, I decided to 
reshuffle the layout so Xen is running a very high address (so it is 
unlikely to clash).

For Arm32, we have a smaller address space (4GB) so instead we are going 
through a temporary area to enable the MMU when the load and runtime 
region clash. The sequence is:

   1) Map Xen to a temporary area
   2) Enable the MMU and jump to the temporary area
   3) Map Xen to the runtime area
   4) Jump to the runtime area
   5) Remove the temporary area

[...]

>>>> Hmmm... I would actually expect the address to be properly
>>>> aligned
>>>> and
>>>> therefore not require an alignment here.
>>>>
>>>> Otherwise, this raise the question of what happen if you have
>>>> region
>>>> using the same page?
>>> That map_start &=  ZEROETH_MAP_MASK is needed to page number of
>>> address
>>> w/o page offset.
>>
>> My point is why would the page offset be non-zero?
> I checked a linker script and addresses that passed to
> setup_initial_mapping() and they are really always aligned so there is
> no any sense in additional alignment.

Ok. I would suggest to add some ASSERT()/BUG_ON() in order to confirm 
this is always the case.

[...]

>>>>
>>>>> +
>>>>> +    /*
>>>>> +     * Create a mapping of the load time address range to...
>>>>> the
>>>>> load time address range.
>>>>
>>>> Same about the line length here.
>>>>
>>>>> +     * This mapping is used at boot time only.
>>>>> +     */
>>>>> +    _setup_initial_pagetables(second, first, zeroeth,
>>>>
>>>> This can only work if Xen is loaded at its linked address. So you
>>>> need a
>>>> separate set of L0, L1 tables for the identity mapping.
>>>>
>>>> That said, this would not be sufficient because:
>>>>      1) Xen may not be loaded at a 2M boundary (you can control
>>>> with
>>>> U-boot, but not with EFI). So this may cross a boundary and
>>>> therefore
>>>> need multiple pages.
>>>>      2) The load region may overlap the link address
>>>>
>>>> While I think it would be good to handle those cases from the
>>>> start,
>>>> I
>>>> would understand why are not easy to solve. So I think the
>>>> minimum is
>>>> to
>>>> throw some errors if you are in a case you can't support.
>>> Do you mean to throw some error in load_addr()/linkder_addr()?
>>
>> In this case, I meant to check if load_addr != linker_addr, then
>> throw
>> an error.
> I am not sure that it is needed now and it is easier to throw an error
> but is option exist to handler situation when load_addr != linker_addr
> except throwing an error? relocate?

I believe I answered this above.

Cheers,

-- 
Julien Grall

Re: [PATCH v1 1/3] xen/riscv: introduce setup_initial_pages

[Oleksii]

Hi Julien,

On Tue, 2023-03-21 at 16:25 +0000, Julien Grall wrote:
> 
> 
> On 05/03/2023 16:25, Oleksii wrote:
> > Hi Julien,
> 
> Hi,
> 
> Sorry for the late answer. I was away for the past couple of weeks.
> 
> > On Mon, 2023-02-27 at 17:36 +0000, Julien Grall wrote:
> > > Hi Oleksii,
> > > 
> > > On 27/02/2023 16:52, Oleksii wrote:
> > > > On Sat, 2023-02-25 at 17:53 +0000, Julien Grall wrote:
> > > > > > +/*
> > > > > > + * WARNING: load_addr() and linker_addr() are to be called
> > > > > > only
> > > > > > when the MMU is
> > > > > > + * disabled and only when executed by the primary CPU. 
> > > > > > They
> > > > > > cannot refer to
> > > > > > + * any global variable or functions.
> > > > > 
> > > > > I find interesting you are saying when
> > > > > _setup_initial_pagetables() is
> > > > > called from setup_initial_pagetables(). Would you be able to
> > > > > explain
> > > > > how
> > > > > this is different?
> > > > I am not sure that I understand your question correctly but
> > > > _setup_initial_pagetables() was introduced to map some
> > > > addresses
> > > > with
> > > > write/read flag. Probably I have to rename it to something that
> > > > is
> > > > more
> > > > clear.
> > > 
> > > So the comment suggests that you code cannot refer to global
> > > functions/variables when the MMU is off. So I have multiple
> > > questions:
> > >     * Why only global? IOW, why static would be OK?
> > >     * setup_initial_pagetables() has a call to
> > > _setup_initial_pagetables() (IOW referring to another function).
> > > Why
> > > is
> > > it fine?
> > >     * You have code in the next patch referring to global
> > > variables
> > > (mainly _start and _end). How is this different?
> > > 
> > > > > 
> > > > > > + */
> > > > > > +
> > > > > > +/*
> > > > > > + * Convert an addressed layed out at link time to the
> > > > > > address
> > > > > > where it was loaded
> > > > > 
> > > > > Typo: s/addressed/address/ ?
> > > > Yes, it should be address. and 'layed out' should be changed to
> > > > 'laid
> > > > out'...
> > > > > 
> > > > > > + * by the bootloader.
> > > > > > + */
> > > > > 
> > > > > Looking at the implementation, you seem to consider that any
> > > > > address
> > > > > not
> > > > > in the range [linker_addr_start, linker_addr_end[ will have a
> > > > > 1:1
> > > > > mappings.
> > > > > 
> > > > > I am not sure this is what you want. So I would consider to
> > > > > throw
> > > > > an
> > > > > error if such address is passed.
> > > > I thought that at this stage and if no relocation was done it
> > > > is
> > > > 1:1
> > > > except the case when load_addr_start != linker_addr_start.
> > > 
> > > The problem is what you try to map one to one may clash with the
> > > linked
> > > region for Xen. So it is not always possible to map the region
> > > 1:1.
> > > 
> > > Therefore, I don't see any use for the else part here.
> > Got it. Thanks.
> > 
> > I am curious than what is the correct approach in general to handle
> > this situation?
> There are multiple approach to handle it and I don't know which one 
> would be best :). Relocation is one...
> 
> > I mean that throw an error it is one option but if I would like to
> > do
> > that w/o throwing an error. Should it done some relocation in that
> > case?
> ... solution. For Arm, I decided to avoid relocation it requires more
> work in assembly.
> 
> Let me describe what we did and you can decide what you want to do in
> RISC-V.
> 
> For Arm64, as we have plenty of virtual address space, I decided to 
> reshuffle the layout so Xen is running a very high address (so it is 
> unlikely to clash).
I thought about running Xen very high address.
Thanks. I think it is a nice option to do the same for RISC-V64.

> 
> For Arm32, we have a smaller address space (4GB) so instead we are
> going 
> through a temporary area to enable the MMU when the load and runtime 
> region clash. The sequence is:
> 
>    1) Map Xen to a temporary area
>    2) Enable the MMU and jump to the temporary area
>    3) Map Xen to the runtime area
>    4) Jump to the runtime area
>    5) Remove the temporary area
> 
It is the same for RV32. As we don't support RV32 I will use:
  #error "Add support of MMU for RV32"
> [...]
> 
> > > > > Hmmm... I would actually expect the address to be properly
> > > > > aligned
> > > > > and
> > > > > therefore not require an alignment here.
> > > > > 
> > > > > Otherwise, this raise the question of what happen if you have
> > > > > region
> > > > > using the same page?
> > > > That map_start &=  ZEROETH_MAP_MASK is needed to page number of
> > > > address
> > > > w/o page offset.
> > > 
> > > My point is why would the page offset be non-zero?
> > I checked a linker script and addresses that passed to
> > setup_initial_mapping() and they are really always aligned so there
> > is
> > no any sense in additional alignment.
> 
> Ok. I would suggest to add some ASSERT()/BUG_ON() in order to confirm
> this is always the case.
> 
> [...]
> 
> > > > > 
> > > > > > +
> > > > > > +    /*
> > > > > > +     * Create a mapping of the load time address range
> > > > > > to...
> > > > > > the
> > > > > > load time address range.
> > > > > 
> > > > > Same about the line length here.
> > > > > 
> > > > > > +     * This mapping is used at boot time only.
> > > > > > +     */
> > > > > > +    _setup_initial_pagetables(second, first, zeroeth,
> > > > > 
> > > > > This can only work if Xen is loaded at its linked address. So
> > > > > you
> > > > > need a
> > > > > separate set of L0, L1 tables for the identity mapping.
> > > > > 
> > > > > That said, this would not be sufficient because:
> > > > >      1) Xen may not be loaded at a 2M boundary (you can
> > > > > control
> > > > > with
> > > > > U-boot, but not with EFI). So this may cross a boundary and
> > > > > therefore
> > > > > need multiple pages.
> > > > >      2) The load region may overlap the link address
> > > > > 
> > > > > While I think it would be good to handle those cases from the
> > > > > start,
> > > > > I
> > > > > would understand why are not easy to solve. So I think the
> > > > > minimum is
> > > > > to
> > > > > throw some errors if you are in a case you can't support.
> > > > Do you mean to throw some error in load_addr()/linkder_addr()?
> > > 
> > > In this case, I meant to check if load_addr != linker_addr, then
> > > throw
> > > an error.
> > I am not sure that it is needed now and it is easier to throw an
> > error
> > but is option exist to handler situation when load_addr !=
> > linker_addr
> > except throwing an error? relocate?
> 
> I believe I answered this above.
Yeah, you answered my question completely. Thank you very much.

~ Oleksii

Re: [PATCH v1 1/3] xen/riscv: introduce setup_initial_pages

[Jan Beulich]

On 24.02.2023 16:06, Oleksii Kurochko wrote:
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/page.h
> @@ -0,0 +1,90 @@
> +#ifndef _ASM_RISCV_PAGE_H
> +#define _ASM_RISCV_PAGE_H
> +
> +#include <xen/const.h>
> +#include <xen/types.h>
> +
> +#define PAGE_ENTRIES            512
> +#define VPN_BITS                (9)
> +#define VPN_MASK                ((unsigned long)((1 << VPN_BITS) - 1))
> +
> +#ifdef CONFIG_RISCV_64
> +/* L3 index Bit[47:39] */
> +#define THIRD_SHIFT             (39)
> +#define THIRD_MASK              (VPN_MASK << THIRD_SHIFT)
> +/* L2 index Bit[38:30] */
> +#define SECOND_SHIFT            (30)
> +#define SECOND_MASK             (VPN_MASK << SECOND_SHIFT)
> +/* L1 index Bit[29:21] */
> +#define FIRST_SHIFT             (21)
> +#define FIRST_MASK              (VPN_MASK << FIRST_SHIFT)
> +/* L0 index Bit[20:12] */
> +#define ZEROETH_SHIFT           (12)
> +#define ZEROETH_MASK            (VPN_MASK << ZEROETH_SHIFT)
> +
> +#else // CONFIG_RISCV_32
> +
> +/* L1 index Bit[31:22] */
> +#define FIRST_SHIFT             (22)
> +#define FIRST_MASK              (VPN_MASK << FIRST_SHIFT)
> +
> +/* L0 index Bit[21:12] */
> +#define ZEROETH_SHIFT           (12)
> +#define ZEROETH_MASK            (VPN_MASK << ZEROETH_SHIFT)
> +#endif
> +
> +#define THIRD_SIZE              (1 << THIRD_SHIFT)
> +#define THIRD_MAP_MASK          (~(THIRD_SIZE - 1))
> +#define SECOND_SIZE             (1 << SECOND_SHIFT)
> +#define SECOND_MAP_MASK         (~(SECOND_SIZE - 1))
> +#define FIRST_SIZE              (1 << FIRST_SHIFT)
> +#define FIRST_MAP_MASK          (~(FIRST_SIZE - 1))
> +#define ZEROETH_SIZE            (1 << ZEROETH_SHIFT)
> +#define ZEROETH_MAP_MASK        (~(ZEROETH_SIZE - 1))
> +
> +#define PTE_SHIFT               10
> +
> +#define PTE_VALID               BIT(0, UL)
> +#define PTE_READABLE            BIT(1, UL)
> +#define PTE_WRITABLE            BIT(2, UL)
> +#define PTE_EXECUTABLE          BIT(3, UL)
> +#define PTE_USER                BIT(4, UL)
> +#define PTE_GLOBAL              BIT(5, UL)
> +#define PTE_ACCESSED            BIT(6, UL)
> +#define PTE_DIRTY               BIT(7, UL)
> +#define PTE_RSW                 (BIT(8, UL) | BIT(9, UL))
> +
> +#define PTE_LEAF_DEFAULT        (PTE_VALID | PTE_READABLE | PTE_WRITABLE | PTE_EXECUTABLE)
> +#define PTE_TABLE               (PTE_VALID)
> +
> +/* Calculate the offsets into the pagetables for a given VA */
> +#define zeroeth_linear_offset(va)   ((va) >> ZEROETH_SHIFT)
> +#define first_linear_offset(va)     ((va) >> FIRST_SHIFT)
> +#define second_linear_offset(va)    ((va) >> SECOND_SHIFT)
> +#define third_linear_offset(va)     ((va) >> THIRD_SHIFT)
> +
> +#define pagetable_zeroeth_index(va) zeroeth_linear_offset((va) & ZEROETH_MASK)
> +#define pagetable_first_index(va)   first_linear_offset((va) & FIRST_MASK)
> +#define pagetable_second_index(va)  second_linear_offset((va) & SECOND_MASK)
> +#define pagetable_third_index(va)   third_linear_offset((va) & THIRD_MASK)
> +
> +/* Page Table entry */
> +typedef struct {
> +    uint64_t pte;
> +} pte_t;
> +
> +/* Shift the VPN[x] or PPN[x] fields of a virtual or physical address
> + * to become the shifted PPN[x] fields of a page table entry */
> +#define addr_to_ppn(x) (((x) >> PAGE_SHIFT) << PTE_SHIFT)
> +
> +static inline pte_t paddr_to_pte(unsigned long paddr)
> +{
> +    return (pte_t) { .pte = addr_to_ppn(paddr) };
> +}
> +
> +static inline bool pte_is_valid(pte_t *p)

Btw - const whenever possible please, especially in such basic helpers.

> --- /dev/null
> +++ b/xen/arch/riscv/mm.c
> @@ -0,0 +1,223 @@
> +#include <xen/init.h>
> +#include <xen/lib.h>
> +
> +#include <asm/csr.h>
> +#include <asm/mm.h>
> +#include <asm/page.h>
> +
> +/*
> + * xen_second_pagetable is indexed with the VPN[2] page table entry field
> + * xen_first_pagetable is accessed from the VPN[1] page table entry field
> + * xen_zeroeth_pagetable is accessed from the VPN[0] page table entry field
> + */
> +pte_t xen_second_pagetable[PAGE_ENTRIES] __attribute__((__aligned__(PAGE_SIZE)));

static?

> +static pte_t xen_first_pagetable[PAGE_ENTRIES]
> +    __attribute__((__aligned__(PAGE_SIZE)));
> +static pte_t xen_zeroeth_pagetable[PAGE_ENTRIES]
> +    __attribute__((__aligned__(PAGE_SIZE)));

Please use __aligned() instead of open-coding it. You also may want to
specifiy the section here explicitly, as .bss.page_aligned (as we do
elsewhere).

> +extern unsigned long _stext;
> +extern unsigned long _etext;
> +extern unsigned long __init_begin;
> +extern unsigned long __init_end;
> +extern unsigned long _srodata;
> +extern unsigned long _erodata;

Please use kernel.h and drop then colliding declarations. For what's
left please use array types, as suggested elsewhere already.

> +paddr_t phys_offset;
> +
> +#define resolve_early_addr(x) \
> +    ({                                                                          \
> +         unsigned long * __##x;                                                 \
> +        if ( load_addr_start <= x && x < load_addr_end )                        \

Nit: Mismatched indentation.

> +            __##x = (unsigned long *)x;                                         \
> +        else                                                                    \
> +            __##x = (unsigned long *)(x + load_addr_start - linker_addr_start); \
> +        __##x;                                                                  \
> +     })
> +
> +static void __init clear_pagetables(unsigned long load_addr_start,
> +                             unsigned long load_addr_end,
> +                             unsigned long linker_addr_start,
> +                             unsigned long linker_addr_end)

Nit (style): Indentation.

> +{
> +    unsigned long *p;
> +    unsigned long page;
> +    unsigned long i;
> +
> +    page = (unsigned long)&xen_second_pagetable[0];
> +
> +    p = resolve_early_addr(page);
> +    for ( i = 0; i < ARRAY_SIZE(xen_second_pagetable); i++ )
> +    {
> +        p[i] = 0ULL;
> +    }

We typically omit braces around single-statement bodies. Here,
though: Why do you do this in the first place? These static arrays
all start out zero-initialized anyway (from when you clear .bss).
Plus even if they didn't - why not memset()?

> +    page = (unsigned long)&xen_first_pagetable[0];
> +    p = resolve_early_addr(page);
> +    for ( i = 0; i < ARRAY_SIZE(xen_first_pagetable); i++ )
> +    {
> +        p[i] = 0ULL;
> +    }
> +
> +    page = (unsigned long)&xen_zeroeth_pagetable[0];
> +    p = resolve_early_addr(page);
> +    for ( i = 0; i < ARRAY_SIZE(xen_zeroeth_pagetable); i++ )
> +    {
> +        p[i] = 0ULL;
> +    }
> +}
> +
> +/*
> + * WARNING: load_addr() and linker_addr() are to be called only when the MMU is
> + * disabled and only when executed by the primary CPU.  They cannot refer to
> + * any global variable or functions.
> + */
> +
> +/*
> + * Convert an addressed layed out at link time to the address where it was loaded
> + * by the bootloader.
> + */
> +#define load_addr(linker_address)                                              \
> +    ({                                                                         \
> +        unsigned long __linker_address = (unsigned long)(linker_address);      \
> +        if ( linker_addr_start <= __linker_address &&                          \
> +            __linker_address < linker_addr_end )                               \
> +        {                                                                      \
> +            __linker_address =                                                 \
> +                __linker_address - linker_addr_start + load_addr_start;        \
> +        }                                                                      \
> +        __linker_address;                                                      \
> +    })
> +
> +/* Convert boot-time Xen address from where it was loaded by the boot loader to the address it was layed out
> + * at link-time.
> + */
> +#define linker_addr(load_address)                                              \
> +    ({                                                                         \
> +        unsigned long __load_address = (unsigned long)(load_address);          \
> +        if ( load_addr_start <= __load_address &&                              \
> +            __load_address < load_addr_end )                                   \
> +        {                                                                      \
> +            __load_address =                                                   \
> +                __load_address - load_addr_start + linker_addr_start;          \
> +        }                                                                      \
> +        __load_address;                                                        \
> +    })
> +
> +static void __attribute__((section(".entry")))
> +_setup_initial_pagetables(pte_t *second, pte_t *first, pte_t *zeroeth,

Why the special section (also again further down)?

Jan

Re: [PATCH v1 1/3] xen/riscv: introduce setup_initial_pages

[Jan Beulich]

On 27.02.2023 16:12, Jan Beulich wrote:
> On 24.02.2023 16:06, Oleksii Kurochko wrote:
>> +static void __attribute__((section(".entry")))
>> +_setup_initial_pagetables(pte_t *second, pte_t *first, pte_t *zeroeth,
> 
> Why the special section (also again further down)?

Looking at patch 2 it occurred to me that you probably mean __init here.

Jan

Re: [PATCH v1 1/3] xen/riscv: introduce setup_initial_pages

[Oleksii]

On Mon, 2023-02-27 at 16:19 +0100, Jan Beulich wrote:
> On 27.02.2023 16:12, Jan Beulich wrote:
> > On 24.02.2023 16:06, Oleksii Kurochko wrote:
> > > +static void __attribute__((section(".entry")))
> > > +_setup_initial_pagetables(pte_t *second, pte_t *first, pte_t
> > > *zeroeth,
> > 
> > Why the special section (also again further down)?
> 
> Looking at patch 2 it occurred to me that you probably mean __init
> here.
Yes, you are right.
> 
> Jan

Re: [PATCH v1 1/3] xen/riscv: introduce setup_initial_pages

[Oleksii]

On Mon, 2023-02-27 at 16:12 +0100, Jan Beulich wrote:
> On 24.02.2023 16:06, Oleksii Kurochko wrote:
> > --- /dev/null
> > +++ b/xen/arch/riscv/include/asm/page.h
> > @@ -0,0 +1,90 @@
> > +#ifndef _ASM_RISCV_PAGE_H
> > +#define _ASM_RISCV_PAGE_H
> > +
> > +#include <xen/const.h>
> > +#include <xen/types.h>
> > +
> > +#define PAGE_ENTRIES            512
> > +#define VPN_BITS                (9)
> > +#define VPN_MASK                ((unsigned long)((1 << VPN_BITS) -
> > 1))
> > +
> > +#ifdef CONFIG_RISCV_64
> > +/* L3 index Bit[47:39] */
> > +#define THIRD_SHIFT             (39)
> > +#define THIRD_MASK              (VPN_MASK << THIRD_SHIFT)
> > +/* L2 index Bit[38:30] */
> > +#define SECOND_SHIFT            (30)
> > +#define SECOND_MASK             (VPN_MASK << SECOND_SHIFT)
> > +/* L1 index Bit[29:21] */
> > +#define FIRST_SHIFT             (21)
> > +#define FIRST_MASK              (VPN_MASK << FIRST_SHIFT)
> > +/* L0 index Bit[20:12] */
> > +#define ZEROETH_SHIFT           (12)
> > +#define ZEROETH_MASK            (VPN_MASK << ZEROETH_SHIFT)
> > +
> > +#else // CONFIG_RISCV_32
> > +
> > +/* L1 index Bit[31:22] */
> > +#define FIRST_SHIFT             (22)
> > +#define FIRST_MASK              (VPN_MASK << FIRST_SHIFT)
> > +
> > +/* L0 index Bit[21:12] */
> > +#define ZEROETH_SHIFT           (12)
> > +#define ZEROETH_MASK            (VPN_MASK << ZEROETH_SHIFT)
> > +#endif
> > +
> > +#define THIRD_SIZE              (1 << THIRD_SHIFT)
> > +#define THIRD_MAP_MASK          (~(THIRD_SIZE - 1))
> > +#define SECOND_SIZE             (1 << SECOND_SHIFT)
> > +#define SECOND_MAP_MASK         (~(SECOND_SIZE - 1))
> > +#define FIRST_SIZE              (1 << FIRST_SHIFT)
> > +#define FIRST_MAP_MASK          (~(FIRST_SIZE - 1))
> > +#define ZEROETH_SIZE            (1 << ZEROETH_SHIFT)
> > +#define ZEROETH_MAP_MASK        (~(ZEROETH_SIZE - 1))
> > +
> > +#define PTE_SHIFT               10
> > +
> > +#define PTE_VALID               BIT(0, UL)
> > +#define PTE_READABLE            BIT(1, UL)
> > +#define PTE_WRITABLE            BIT(2, UL)
> > +#define PTE_EXECUTABLE          BIT(3, UL)
> > +#define PTE_USER                BIT(4, UL)
> > +#define PTE_GLOBAL              BIT(5, UL)
> > +#define PTE_ACCESSED            BIT(6, UL)
> > +#define PTE_DIRTY               BIT(7, UL)
> > +#define PTE_RSW                 (BIT(8, UL) | BIT(9, UL))
> > +
> > +#define PTE_LEAF_DEFAULT        (PTE_VALID | PTE_READABLE |
> > PTE_WRITABLE | PTE_EXECUTABLE)
> > +#define PTE_TABLE               (PTE_VALID)
> > +
> > +/* Calculate the offsets into the pagetables for a given VA */
> > +#define zeroeth_linear_offset(va)   ((va) >> ZEROETH_SHIFT)
> > +#define first_linear_offset(va)     ((va) >> FIRST_SHIFT)
> > +#define second_linear_offset(va)    ((va) >> SECOND_SHIFT)
> > +#define third_linear_offset(va)     ((va) >> THIRD_SHIFT)
> > +
> > +#define pagetable_zeroeth_index(va) zeroeth_linear_offset((va) &
> > ZEROETH_MASK)
> > +#define pagetable_first_index(va)   first_linear_offset((va) &
> > FIRST_MASK)
> > +#define pagetable_second_index(va)  second_linear_offset((va) &
> > SECOND_MASK)
> > +#define pagetable_third_index(va)   third_linear_offset((va) &
> > THIRD_MASK)
> > +
> > +/* Page Table entry */
> > +typedef struct {
> > +    uint64_t pte;
> > +} pte_t;
> > +
> > +/* Shift the VPN[x] or PPN[x] fields of a virtual or physical
> > address
> > + * to become the shifted PPN[x] fields of a page table entry */
> > +#define addr_to_ppn(x) (((x) >> PAGE_SHIFT) << PTE_SHIFT)
> > +
> > +static inline pte_t paddr_to_pte(unsigned long paddr)
> > +{
> > +    return (pte_t) { .pte = addr_to_ppn(paddr) };
> > +}
> > +
> > +static inline bool pte_is_valid(pte_t *p)
> 
> Btw - const whenever possible please, especially in such basic
> helpers.
Sure. Thanks.
> 
> > --- /dev/null
> > +++ b/xen/arch/riscv/mm.c
> > @@ -0,0 +1,223 @@
> > +#include <xen/init.h>
> > +#include <xen/lib.h>
> > +
> > +#include <asm/csr.h>
> > +#include <asm/mm.h>
> > +#include <asm/page.h>
> > +
> > +/*
> > + * xen_second_pagetable is indexed with the VPN[2] page table
> > entry field
> > + * xen_first_pagetable is accessed from the VPN[1] page table
> > entry field
> > + * xen_zeroeth_pagetable is accessed from the VPN[0] page table
> > entry field
> > + */
> > +pte_t xen_second_pagetable[PAGE_ENTRIES]
> > __attribute__((__aligned__(PAGE_SIZE)));
> 
> static?
It should be static.
Thanks.
> 
> > +static pte_t xen_first_pagetable[PAGE_ENTRIES]
> > +    __attribute__((__aligned__(PAGE_SIZE)));
> > +static pte_t xen_zeroeth_pagetable[PAGE_ENTRIES]
> > +    __attribute__((__aligned__(PAGE_SIZE)));
> 
> Please use __aligned() instead of open-coding it. You also may want
> to
> specifiy the section here explicitly, as .bss.page_aligned (as we do
> elsewhere).
> 
> > +extern unsigned long _stext;
> > +extern unsigned long _etext;
> > +extern unsigned long __init_begin;
> > +extern unsigned long __init_end;
> > +extern unsigned long _srodata;
> > +extern unsigned long _erodata;
> 
> Please use kernel.h and drop then colliding declarations. For what's
> left please use array types, as suggested elsewhere already.
Thanks. I'll take it into account.
> 
> > +paddr_t phys_offset;
> > +
> > +#define resolve_early_addr(x) \
> > +   
> > ({                                                                 
> >          \
> > +         unsigned long *
> > __##x;                                                 \
> > +        if ( load_addr_start <= x && x < load_addr_end
> > )                        \
> 
> Nit: Mismatched indentation.
> 
> > +            __##x = (unsigned long
> > *)x;                                         \
> > +       
> > else                                                               
> >      \
> > +            __##x = (unsigned long *)(x + load_addr_start -
> > linker_addr_start); \
> > +       
> > __##x;                                                             
> >      \
> > +     })
> > +
> > +static void __init clear_pagetables(unsigned long load_addr_start,
> > +                             unsigned long load_addr_end,
> > +                             unsigned long linker_addr_start,
> > +                             unsigned long linker_addr_end)
> 
> Nit (style): Indentation.
> 
> > +{
> > +    unsigned long *p;
> > +    unsigned long page;
> > +    unsigned long i;
> > +
> > +    page = (unsigned long)&xen_second_pagetable[0];
> > +
> > +    p = resolve_early_addr(page);
> > +    for ( i = 0; i < ARRAY_SIZE(xen_second_pagetable); i++ )
> > +    {
> > +        p[i] = 0ULL;
> > +    }
> 
> We typically omit braces around single-statement bodies. Here,
> though: Why do you do this in the first place? These static arrays
> all start out zero-initialized anyway (from when you clear .bss).
> Plus even if they didn't - why not memset()?
clear_pagetables() will be deleted as you mentioned it will be zeroed
during initialization of .bss.
It wasn't done before because .bss wasn't initialized. Now the patches
are waiting to be merged.
> 
> > +    page = (unsigned long)&xen_first_pagetable[0];
> > +    p = resolve_early_addr(page);
> > +    for ( i = 0; i < ARRAY_SIZE(xen_first_pagetable); i++ )
> > +    {
> > +        p[i] = 0ULL;
> > +    }
> > +
> > +    page = (unsigned long)&xen_zeroeth_pagetable[0];
> > +    p = resolve_early_addr(page);
> > +    for ( i = 0; i < ARRAY_SIZE(xen_zeroeth_pagetable); i++ )
> > +    {
> > +        p[i] = 0ULL;
> > +    }
> > +}
> > +
> > +/*
> > + * WARNING: load_addr() and linker_addr() are to be called only
> > when the MMU is
> > + * disabled and only when executed by the primary CPU.  They
> > cannot refer to
> > + * any global variable or functions.
> > + */
> > +
> > +/*
> > + * Convert an addressed layed out at link time to the address
> > where it was loaded
> > + * by the bootloader.
> > + */
> > +#define
> > load_addr(linker_address)                                          
> >     \
> > +   
> > ({                                                                 
> >         \
> > +        unsigned long __linker_address = (unsigned
> > long)(linker_address);      \
> > +        if ( linker_addr_start <= __linker_address
> > &&                          \
> > +            __linker_address < linker_addr_end
> > )                               \
> > +       
> > {                                                                  
> >     \
> > +            __linker_address
> > =                                                 \
> > +                __linker_address - linker_addr_start +
> > load_addr_start;        \
> > +       
> > }                                                                  
> >     \
> > +       
> > __linker_address;                                                  
> >     \
> > +    })
> > +
> > +/* Convert boot-time Xen address from where it was loaded by the
> > boot loader to the address it was layed out
> > + * at link-time.
> > + */
> > +#define
> > linker_addr(load_address)                                          
> >     \
> > +   
> > ({                                                                 
> >         \
> > +        unsigned long __load_address = (unsigned
> > long)(load_address);          \
> > +        if ( load_addr_start <= __load_address
> > &&                              \
> > +            __load_address < load_addr_end
> > )                                   \
> > +       
> > {                                                                  
> >     \
> > +            __load_address
> > =                                                   \
> > +                __load_address - load_addr_start +
> > linker_addr_start;          \
> > +       
> > }                                                                  
> >     \
> > +       
> > __load_address;                                                    
> >     \
> > +    })
> > +
> > +static void __attribute__((section(".entry")))
> > +_setup_initial_pagetables(pte_t *second, pte_t *first, pte_t
> > *zeroeth,
> 
> Why the special section (also again further down)?
There is no specific reason in case of Xen. I'll remove that.

~ Oleksii

[PATCH v1 2/3] xen/riscv: setup initial pagetables

[Oleksii Kurochko]

Calculate load and linker linker image addresses and
setup initial pagetables.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/riscv/setup.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
index b7cd438a1d..f69bc278bb 100644
--- a/xen/arch/riscv/setup.c
+++ b/xen/arch/riscv/setup.c
@@ -1,9 +1,11 @@
 #include <xen/bug.h>
 #include <xen/compile.h>
 #include <xen/init.h>
+#include <xen/kernel.h>
 
 #include <asm/csr.h>
 #include <asm/early_printk.h>
+#include <asm/mm.h>
 #include <asm/traps.h>
 
 /* Xen stack for bringing up the first CPU. */
@@ -43,6 +45,11 @@ static void __init disable_fpu(void)
 
 void __init noreturn start_xen(void)
 {
+    unsigned long load_start    = (unsigned long)start;
+    unsigned long load_end      = load_start + (unsigned long)(_end - _start);
+    unsigned long linker_start  = (unsigned long)_start;
+    unsigned long linker_end    = (unsigned long)_end;
+
     /*
      * The following things are passed by bootloader:
      *   a0 -> hart_id
@@ -65,6 +72,10 @@ void __init noreturn start_xen(void)
 
     test_macros_from_bug_h();
 
+    setup_initial_pagetables(load_start, load_end, linker_start, linker_end);
+
+    early_printk("MMU has been enabled\n");
+
     for ( ;; )
         asm volatile ("wfi");
 
-- 
2.39.0

Re: [PATCH v1 2/3] xen/riscv: setup initial pagetables

[Julien Grall]

Hi,

On 24/02/2023 15:06, Oleksii Kurochko wrote:
> Calculate load and linker linker image addresses and
> setup initial pagetables.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
>   xen/arch/riscv/setup.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
> index b7cd438a1d..f69bc278bb 100644
> --- a/xen/arch/riscv/setup.c
> +++ b/xen/arch/riscv/setup.c
> @@ -1,9 +1,11 @@
>   #include <xen/bug.h>
>   #include <xen/compile.h>
>   #include <xen/init.h>
> +#include <xen/kernel.h>
>   
>   #include <asm/csr.h>
>   #include <asm/early_printk.h>
> +#include <asm/mm.h>
>   #include <asm/traps.h>
>   
>   /* Xen stack for bringing up the first CPU. */
> @@ -43,6 +45,11 @@ static void __init disable_fpu(void)
>   
>   void __init noreturn start_xen(void)
>   {
> +    unsigned long load_start    = (unsigned long)start;
> +    unsigned long load_end      = load_start + (unsigned long)(_end - _start);

I am a bit puzzled, on top of load_addr() and linker_addr(), you wrote 
it can't use global variable/function. But here... you are using them. 
So how is this different?

> +    unsigned long linker_start  = (unsigned long)_start;
> +    unsigned long linker_end    = (unsigned long)_end;

I am a bit confused with how you define the start/end for both the 
linker and load. In one you use _start and the other _end.

Both are fixed at compile time, so I assume the values will be a linked 
address rather than the load address. So how is this meant to how?

Furthermore, I would expect linker_start and load_start to point to the 
same symbol (the only different is one store the virtual address whereas 
the other the physical address). But here you are technically using two 
different symbol. Can you explain why?

> +
>       /*
>        * The following things are passed by bootloader:
>        *   a0 -> hart_id
> @@ -65,6 +72,10 @@ void __init noreturn start_xen(void)
>   
>       test_macros_from_bug_h();
>   
> +    setup_initial_pagetables(load_start, load_end, linker_start, linker_end);

Shouldn't this happen earlier in start_xen()?

Cheers,

-- 
Julien Grall

Re: [PATCH v1 2/3] xen/riscv: setup initial pagetables

[Jan Beulich]

On 25.02.2023 19:05, Julien Grall wrote:
> On 24/02/2023 15:06, Oleksii Kurochko wrote:
>> @@ -43,6 +45,11 @@ static void __init disable_fpu(void)
>>   
>>   void __init noreturn start_xen(void)
>>   {
>> +    unsigned long load_start    = (unsigned long)start;
>> +    unsigned long load_end      = load_start + (unsigned long)(_end - _start);
> 
> I am a bit puzzled, on top of load_addr() and linker_addr(), you wrote 
> it can't use global variable/function. But here... you are using them. 
> So how is this different?

I guess "use" means "access" (i.e. call a function or read/write a
variable). I suppose it does not mean "take the address of".

Jan

Re: [PATCH v1 2/3] xen/riscv: setup initial pagetables

[Julien Grall]

On 27/02/2023 15:17, Jan Beulich wrote:
> On 25.02.2023 19:05, Julien Grall wrote:
>> On 24/02/2023 15:06, Oleksii Kurochko wrote:
>>> @@ -43,6 +45,11 @@ static void __init disable_fpu(void)
>>>    
>>>    void __init noreturn start_xen(void)
>>>    {
>>> +    unsigned long load_start    = (unsigned long)start;
>>> +    unsigned long load_end      = load_start + (unsigned long)(_end - _start);
>>
>> I am a bit puzzled, on top of load_addr() and linker_addr(), you wrote
>> it can't use global variable/function. But here... you are using them.
>> So how is this different?
> 
> I guess "use" means "access" (i.e. call a function or read/write a
> variable). I suppose it does not mean "take the address of".

If so, then I don't understand why we need to pass linker_start, 
linker_end in parameters for setup_initial_pages().

Cheers,

-- 
Julien Grall

Re: [PATCH v1 2/3] xen/riscv: setup initial pagetables

[Oleksii]

On Sat, 2023-02-25 at 18:05 +0000, Julien Grall wrote:
> Hi,
> 
> On 24/02/2023 15:06, Oleksii Kurochko wrote:
> > Calculate load and linker linker image addresses and
> > setup initial pagetables.
> > 
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> > ---
> >   xen/arch/riscv/setup.c | 11 +++++++++++
> >   1 file changed, 11 insertions(+)
> > 
> > diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
> > index b7cd438a1d..f69bc278bb 100644
> > --- a/xen/arch/riscv/setup.c
> > +++ b/xen/arch/riscv/setup.c
> > @@ -1,9 +1,11 @@
> >   #include <xen/bug.h>
> >   #include <xen/compile.h>
> >   #include <xen/init.h>
> > +#include <xen/kernel.h>
> >   
> >   #include <asm/csr.h>
> >   #include <asm/early_printk.h>
> > +#include <asm/mm.h>
> >   #include <asm/traps.h>
> >   
> >   /* Xen stack for bringing up the first CPU. */
> > @@ -43,6 +45,11 @@ static void __init disable_fpu(void)
> >   
> >   void __init noreturn start_xen(void)
> >   {
> > +    unsigned long load_start    = (unsigned long)start;
> > +    unsigned long load_end      = load_start + (unsigned
> > long)(_end - _start);
> 
> I am a bit puzzled, on top of load_addr() and linker_addr(), you
> wrote 
> it can't use global variable/function. But here... you are using
> them. 
> So how is this different?
I don't use load_addr() and linker_addr() macros here.
> 
> > +    unsigned long linker_start  = (unsigned long)_start;
> > +    unsigned long linker_end    = (unsigned long)_end;
> 
> I am a bit confused with how you define the start/end for both the 
> linker and load. In one you use _start and the other _end.
> 
> Both are fixed at compile time, so I assume the values will be a
> linked 
> address rather than the load address. So how is this meant to how?
> 
_start, _end - it is label from linker script so I use them to define
linker_start and linker_end addresses.

load_start is defined as an address of start() function from head.S and
load_end is the load_start + the size  (_end - _start)

> Furthermore, I would expect linker_start and load_start to point to
> the 
> same symbol (the only different is one store the virtual address
> whereas 
> the other the physical address). But here you are technically using
> two 
> different symbol. Can you explain why?
It is used to make identity mapping for the range [load_addr, load_end]
and [linker_addr, linker_end]. It was done so because in Bobby's
patches in the linker script XEN_VIRT_START is defined as
_AT(vaddr_t,0x00200000) but bootloader loads Xen at 0x80200000 and so
in this case loadr_addr != linker_addr.
But I have changed XEN_VIRT_START to 0x8020...00 so they are equal now.
> 
> > +
> >       /*
> >        * The following things are passed by bootloader:
> >        *   a0 -> hart_id
> > @@ -65,6 +72,10 @@ void __init noreturn start_xen(void)
> >   
> >       test_macros_from_bug_h();
> >   
> > +    setup_initial_pagetables(load_start, load_end, linker_start,
> > linker_end);
> 
> Shouldn't this happen earlier in start_xen()?
It can. If to be honest I don't know if it should. I added at the end
only because it was the last thing I worked on...
> 
> Cheers,
> 
~ Oleksii

Re: [PATCH v1 2/3] xen/riscv: setup initial pagetables

[Julien Grall]

Hi,

On 27/02/2023 17:17, Oleksii wrote:
> On Sat, 2023-02-25 at 18:05 +0000, Julien Grall wrote:
>> Hi,
>>
>> On 24/02/2023 15:06, Oleksii Kurochko wrote:
>>> Calculate load and linker linker image addresses and
>>> setup initial pagetables.
>>>
>>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>>> ---
>>>    xen/arch/riscv/setup.c | 11 +++++++++++
>>>    1 file changed, 11 insertions(+)
>>>
>>> diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
>>> index b7cd438a1d..f69bc278bb 100644
>>> --- a/xen/arch/riscv/setup.c
>>> +++ b/xen/arch/riscv/setup.c
>>> @@ -1,9 +1,11 @@
>>>    #include <xen/bug.h>
>>>    #include <xen/compile.h>
>>>    #include <xen/init.h>
>>> +#include <xen/kernel.h>
>>>    
>>>    #include <asm/csr.h>
>>>    #include <asm/early_printk.h>
>>> +#include <asm/mm.h>
>>>    #include <asm/traps.h>
>>>    
>>>    /* Xen stack for bringing up the first CPU. */
>>> @@ -43,6 +45,11 @@ static void __init disable_fpu(void)
>>>    
>>>    void __init noreturn start_xen(void)
>>>    {
>>> +    unsigned long load_start    = (unsigned long)start;
>>> +    unsigned long load_end      = load_start + (unsigned
>>> long)(_end - _start);
>>
>> I am a bit puzzled, on top of load_addr() and linker_addr(), you
>> wrote
>> it can't use global variable/function. But here... you are using
>> them.
>> So how is this different?
> I don't use load_addr() and linker_addr() macros here.

I understand that. But my comment was related to:

/*
  * WARNING: load_addr() and linker_addr() are to be called only when 
the MMU is
  * disabled and only when executed by the primary CPU.  They cannot 
refer to
  * any global variable or functions.
  */

_start and _end are global variables. So why can you use them here but 
not there?

If you could use them in load_addr() then you could simplify a lot your 
logic.

>>
>>> +    unsigned long linker_start  = (unsigned long)_start;
>>> +    unsigned long linker_end    = (unsigned long)_end;
>>
>> I am a bit confused with how you define the start/end for both the
>> linker and load. In one you use _start and the other _end.
>>
>> Both are fixed at compile time, so I assume the values will be a
>> linked
>> address rather than the load address. So how is this meant to how?
>>
> _start, _end - it is label from linker script so I use them to define
> linker_start and linker_end addresses.
> 
> load_start is defined as an address of start() function from head.S and
> load_end is the load_start + the size  (_end - _start)

I think you misunderstood my comment. I understand what the variables 
are for. But I don't understand the computation because Xen could be 
loaded at a different address than the runtime address.

> 
>> Furthermore, I would expect linker_start and load_start to point to
>> the
>> same symbol (the only different is one store the virtual address
>> whereas
>> the other the physical address). But here you are technically using
>> two
>> different symbol. Can you explain why?
> It is used to make identity mapping for the range [load_addr, load_end]
> and [linker_addr, linker_end]. It was done so because in Bobby's
> patches in the linker script XEN_VIRT_START is defined as
> _AT(vaddr_t,0x00200000) but bootloader loads Xen at 0x80200000 and so
> in this case loadr_addr != linker_addr.
> But I have changed XEN_VIRT_START to 0x8020...00 so they are equal now.

So this will be broken as soon as this code will be tested on an 
hardware where there is no RAM at 0x8020...00. I would strongly 
recommend for you to test your code with XEN_VIRT_START != load address.

>>
>>> +
>>>        /*
>>>         * The following things are passed by bootloader:
>>>         *   a0 -> hart_id
>>> @@ -65,6 +72,10 @@ void __init noreturn start_xen(void)
>>>    
>>>        test_macros_from_bug_h();
>>>    
>>> +    setup_initial_pagetables(load_start, load_end, linker_start,
>>> linker_end);
>>
>> Shouldn't this happen earlier in start_xen()?
> It can. If to be honest I don't know if it should. I added at the end
> only because it was the last thing I worked on...

I think we should enable the MMU and switch to the runtime mapping as 
early as possible.

Cheers,

-- 
Julien Grall

Re: [PATCH v1 2/3] xen/riscv: setup initial pagetables

[Oleksii]

On Mon, 2023-02-27 at 17:45 +0000, Julien Grall wrote:
> Hi,
> 
> On 27/02/2023 17:17, Oleksii wrote:
> > On Sat, 2023-02-25 at 18:05 +0000, Julien Grall wrote:
> > > Hi,
> > > 
> > > On 24/02/2023 15:06, Oleksii Kurochko wrote:
> > > > Calculate load and linker linker image addresses and
> > > > setup initial pagetables.
> > > > 
> > > > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> > > > ---
> > > >    xen/arch/riscv/setup.c | 11 +++++++++++
> > > >    1 file changed, 11 insertions(+)
> > > > 
> > > > diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
> > > > index b7cd438a1d..f69bc278bb 100644
> > > > --- a/xen/arch/riscv/setup.c
> > > > +++ b/xen/arch/riscv/setup.c
> > > > @@ -1,9 +1,11 @@
> > > >    #include <xen/bug.h>
> > > >    #include <xen/compile.h>
> > > >    #include <xen/init.h>
> > > > +#include <xen/kernel.h>
> > > >    
> > > >    #include <asm/csr.h>
> > > >    #include <asm/early_printk.h>
> > > > +#include <asm/mm.h>
> > > >    #include <asm/traps.h>
> > > >    
> > > >    /* Xen stack for bringing up the first CPU. */
> > > > @@ -43,6 +45,11 @@ static void __init disable_fpu(void)
> > > >    
> > > >    void __init noreturn start_xen(void)
> > > >    {
> > > > +    unsigned long load_start    = (unsigned long)start;
> > > > +    unsigned long load_end      = load_start + (unsigned
> > > > long)(_end - _start);
> > > 
> > > I am a bit puzzled, on top of load_addr() and linker_addr(), you
> > > wrote
> > > it can't use global variable/function. But here... you are using
> > > them.
> > > So how is this different?
> > I don't use load_addr() and linker_addr() macros here.
> 
> I understand that. But my comment was related to:
> 
> /*
>   * WARNING: load_addr() and linker_addr() are to be called only when
> the MMU is
>   * disabled and only when executed by the primary CPU.  They cannot 
> refer to
>   * any global variable or functions.
>   */
> 
> _start and _end are global variables. So why can you use them here
> but 
> not there?
> 
> If you could use them in load_addr() then you could simplify a lot
> your 
> logic.
> 
I experimented with it and it seems to me that the macros can be used
with functions&global variables so the comment is incorrect.
> > > 
> > > > +    unsigned long linker_start  = (unsigned long)_start;
> > > > +    unsigned long linker_end    = (unsigned long)_end;
> > > 
> > > I am a bit confused with how you define the start/end for both
> > > the
> > > linker and load. In one you use _start and the other _end.
> > > 
> > > Both are fixed at compile time, so I assume the values will be a
> > > linked
> > > address rather than the load address. So how is this meant to
> > > how?
> > > 
> > _start, _end - it is label from linker script so I use them to
> > define
> > linker_start and linker_end addresses.
> > 
> > load_start is defined as an address of start() function from head.S
> > and
> > load_end is the load_start + the size  (_end - _start)
> 
> I think you misunderstood my comment. I understand what the variables
> are for. But I don't understand the computation because Xen could be 
> loaded at a different address than the runtime address.
> 
What do you mean here by the runtime address? Do you mean an address
where boot loader put Xen? Or an address after relocation, or something
else?

Actually after my latest experiments it looks that we don't need to
calculate that things at all because for RISC-V it is  used everywhere
PC-relative access.
Thereby it doesn't matter what is an address where Xen was loaded and
linker address.
Right now I found only to cases which aren't PC-relative.
Please look at the patch below:
diff --git a/xen/arch/riscv/include/asm/config.h
b/xen/arch/riscv/include/asm/config.h
index 763a922a04..e1ba613d81 100644
--- a/xen/arch/riscv/include/asm/config.h
+++ b/xen/arch/riscv/include/asm/config.h
@@ -39,7 +39,7 @@
   name:
 #endif
 
-#define XEN_VIRT_START  _AT(UL, 0x80200000)
+#define XEN_VIRT_START  _AT(UL, 0x00200000)
 
 #define SMP_CACHE_BYTES (1 << 6)
 
diff --git a/xen/arch/riscv/riscv64/head.S
b/xen/arch/riscv/riscv64/head.S
index ffd95f9f89..87842632e9 100644
--- a/xen/arch/riscv/riscv64/head.S
+++ b/xen/arch/riscv/riscv64/head.S
@@ -1,5 +1,7 @@
 #include <asm/riscv_encoding.h>
 
+        .option nopic
+
         .section .text.header, "ax", %progbits
 
 ENTRY(start)
diff --git a/xen/arch/riscv/traps.c b/xen/arch/riscv/traps.c
index d87a9cfd2c..cd0acdee51 100644
--- a/xen/arch/riscv/traps.c
+++ b/xen/arch/riscv/traps.c
@@ -123,8 +123,14 @@ int do_bug_frame(const struct cpu_user_regs *regs,
vaddr_t pc)
     const char *filename, *predicate;
     int lineno;
 
-    static const struct bug_frame* bug_frames[] = {
-        &__start_bug_frames[0],
+    /*
+     * force fill bug_frames array using auipc/addi instructions to
+     * make addresses in bug_frames PC-relative.
+    */
+    const struct bug_frame * force = (const struct bug_frame *)
&__start_bug_frames[0];
+
+    const struct bug_frame* bug_frames[] = {
+        force,
         &__stop_bug_frames_0[0],
         &__stop_bug_frames_1[0],
         &__stop_bug_frames_2[0],

The changes related to <asm/config.h> are  only to make linker_addr !=
load_address. So:
1. The first issue with cpu0_boot_stack in the head.S file. When we do:
      la      sp, cpu0_boot_stack
   Pseudo-instruction la will be transformed to auipc/addi OR
auipc/l{w|d}.
   It depends on an option: nopic, pic. [1]
   
   So the solution can be the following:
   * As it is done in the patch: add to the start of head.S ".option  
nopic"
   * Change la to lla thereby it will be always generated "auipc/addi"
to get an address of variable.

2. The second issue is with the code in do_bug_frame() with bug_frames
array:
   const struct bug_frame* bug_frames[] = {
        &__start_bug_frames[0],
        &__stop_bug_frames_0[0],
        &__stop_bug_frames_1[0],
        &__stop_bug_frames_2[0],
        &__stop_bug_frames_3[0],
    };
  In this case &{start,stop}bug_frames{,{0-3}} will be changed to     
linker address. In case of when load_addr is 0x80200000 and linker_addr
is 0x00200000 then &{start,stop}bug_frames{,{0-3}} will be equal to
0x00200000 + X.
   
    To force using addresses related to load_addr  in bug_frames, it is
necessary to declare a variable with getting an address of
&__{start,stop}bug_frames{,{0-3}} thereby it will generate the code:
        2002c2:       00001797                auipc   a5,0x1
	2002c6:       d3e78793                addi    a5,a5,-706 #
201000 <__start_bug_frames>
	2002ca:       faf43c23                sd      a5,-72(s0)
	2002ce:       00001797                auipc   a5,0x1
	2002d2:       d3a78793                addi    a5,a5,-710 #
201008 <__stop_bug_frames_

> > 
> > > Furthermore, I would expect linker_start and load_start to point
> > > to
> > > the
> > > same symbol (the only different is one store the virtual address
> > > whereas
> > > the other the physical address). But here you are technically
> > > using
> > > two
> > > different symbol. Can you explain why?
> > It is used to make identity mapping for the range [load_addr,
> > load_end]
> > and [linker_addr, linker_end]. It was done so because in Bobby's
> > patches in the linker script XEN_VIRT_START is defined as
> > _AT(vaddr_t,0x00200000) but bootloader loads Xen at 0x80200000 and
> > so
> > in this case loadr_addr != linker_addr.
> > But I have changed XEN_VIRT_START to 0x8020...00 so they are equal
> > now.
> 
> So this will be broken as soon as this code will be tested on an 
> hardware where there is no RAM at 0x8020...00. I would strongly 
> recommend for you to test your code with XEN_VIRT_START != load
> address.
I've added the check to the new version of the patch but please the
comments above.
> 
> > > 
> > > > +
> > > >        /*
> > > >         * The following things are passed by bootloader:
> > > >         *   a0 -> hart_id
> > > > @@ -65,6 +72,10 @@ void __init noreturn start_xen(void)
> > > >    
> > > >        test_macros_from_bug_h();
> > > >    
> > > > +    setup_initial_pagetables(load_start, load_end,
> > > > linker_start,
> > > > linker_end);
> > > 
> > > Shouldn't this happen earlier in start_xen()?
> > It can. If to be honest I don't know if it should. I added at the
> > end
> > only because it was the last thing I worked on...
> 
> I think we should enable the MMU and switch to the runtime mapping as
> early as possible.
I realized that during the mentioned above experiments.

~ Oleksii

Re: [PATCH v1 2/3] xen/riscv: setup initial pagetables

[Jan Beulich]

On 08.03.2023 15:54, Oleksii wrote:
> Actually after my latest experiments it looks that we don't need to
> calculate that things at all because for RISC-V it is  used everywhere
> PC-relative access.
> Thereby it doesn't matter what is an address where Xen was loaded and
> linker address.
> Right now I found only to cases which aren't PC-relative.
> Please look at the patch below:
> diff --git a/xen/arch/riscv/include/asm/config.h
> b/xen/arch/riscv/include/asm/config.h
> index 763a922a04..e1ba613d81 100644
> --- a/xen/arch/riscv/include/asm/config.h
> +++ b/xen/arch/riscv/include/asm/config.h
> @@ -39,7 +39,7 @@
>    name:
>  #endif
>  
> -#define XEN_VIRT_START  _AT(UL, 0x80200000)
> +#define XEN_VIRT_START  _AT(UL, 0x00200000)

I think this wants to remain the address where Xen actually runs, and
where Xen is linked to. This ...

> --- a/xen/arch/riscv/traps.c
> +++ b/xen/arch/riscv/traps.c
> @@ -123,8 +123,14 @@ int do_bug_frame(const struct cpu_user_regs *regs,
> vaddr_t pc)
>      const char *filename, *predicate;
>      int lineno;
>  
> -    static const struct bug_frame* bug_frames[] = {
> -        &__start_bug_frames[0],
> +    /*
> +     * force fill bug_frames array using auipc/addi instructions to
> +     * make addresses in bug_frames PC-relative.
> +    */
> +    const struct bug_frame * force = (const struct bug_frame *)
> &__start_bug_frames[0];
> +
> +    const struct bug_frame* bug_frames[] = {
> +        force,
>          &__stop_bug_frames_0[0],
>          &__stop_bug_frames_1[0],
>          &__stop_bug_frames_2[0],

... array would better be static anyway, and ...

> The changes related to <asm/config.h> are  only to make linker_addr !=
> load_address. So:
> 1. The first issue with cpu0_boot_stack in the head.S file. When we do:
>       la      sp, cpu0_boot_stack
>    Pseudo-instruction la will be transformed to auipc/addi OR
> auipc/l{w|d}.
>    It depends on an option: nopic, pic. [1]
>    
>    So the solution can be the following:
>    * As it is done in the patch: add to the start of head.S ".option  
> nopic"
>    * Change la to lla thereby it will be always generated "auipc/addi"
> to get an address of variable.
> 
> 2. The second issue is with the code in do_bug_frame() with bug_frames
> array:
>    const struct bug_frame* bug_frames[] = {
>         &__start_bug_frames[0],
>         &__stop_bug_frames_0[0],
>         &__stop_bug_frames_1[0],
>         &__stop_bug_frames_2[0],
>         &__stop_bug_frames_3[0],
>     };
>   In this case &{start,stop}bug_frames{,{0-3}} will be changed to     
> linker address. In case of when load_addr is 0x80200000 and linker_addr
> is 0x00200000 then &{start,stop}bug_frames{,{0-3}} will be equal to
> 0x00200000 + X.

... this "solution" to a problem you introduce by wrongly modifying
the linked address would then need applying to any other similar code
pattern found in Xen. Which is (I hope obviously) not a viable route.
Instead code running before address translation is enable needs to be
extra careful in what code and data items it accesses, and how.

Jan

>     To force using addresses related to load_addr  in bug_frames, it is
> necessary to declare a variable with getting an address of
> &__{start,stop}bug_frames{,{0-3}} thereby it will generate the code:
>         2002c2:       00001797                auipc   a5,0x1
> 	2002c6:       d3e78793                addi    a5,a5,-706 #
> 201000 <__start_bug_frames>
> 	2002ca:       faf43c23                sd      a5,-72(s0)
> 	2002ce:       00001797                auipc   a5,0x1
> 	2002d2:       d3a78793                addi    a5,a5,-710 #
> 201008 <__stop_bug_frames_

Re: [PATCH v1 2/3] xen/riscv: setup initial pagetables

[Oleksii]

On Wed, 2023-03-08 at 16:17 +0100, Jan Beulich wrote:
> On 08.03.2023 15:54, Oleksii wrote:
> > Actually after my latest experiments it looks that we don't need to
> > calculate that things at all because for RISC-V it is  used
> > everywhere
> > PC-relative access.
> > Thereby it doesn't matter what is an address where Xen was loaded
> > and
> > linker address.
> > Right now I found only to cases which aren't PC-relative.
> > Please look at the patch below:
> > diff --git a/xen/arch/riscv/include/asm/config.h
> > b/xen/arch/riscv/include/asm/config.h
> > index 763a922a04..e1ba613d81 100644
> > --- a/xen/arch/riscv/include/asm/config.h
> > +++ b/xen/arch/riscv/include/asm/config.h
> > @@ -39,7 +39,7 @@
> >    name:
> >  #endif
> >  
> > -#define XEN_VIRT_START  _AT(UL, 0x80200000)
> > +#define XEN_VIRT_START  _AT(UL, 0x00200000)
> 
> I think this wants to remain the address where Xen actually runs, and
> where Xen is linked to. This ...
> 
> > --- a/xen/arch/riscv/traps.c
> > +++ b/xen/arch/riscv/traps.c
> > @@ -123,8 +123,14 @@ int do_bug_frame(const struct cpu_user_regs
> > *regs,
> > vaddr_t pc)
> >      const char *filename, *predicate;
> >      int lineno;
> >  
> > -    static const struct bug_frame* bug_frames[] = {
> > -        &__start_bug_frames[0],
> > +    /*
> > +     * force fill bug_frames array using auipc/addi instructions
> > to
> > +     * make addresses in bug_frames PC-relative.
> > +    */
> > +    const struct bug_frame * force = (const struct bug_frame *)
> > &__start_bug_frames[0];
> > +
> > +    const struct bug_frame* bug_frames[] = {
> > +        force,
> >          &__stop_bug_frames_0[0],
> >          &__stop_bug_frames_1[0],
> >          &__stop_bug_frames_2[0],
> 
> ... array would better be static anyway, and ...
> 
> > The changes related to <asm/config.h> are  only to make linker_addr
> > !=
> > load_address. So:
> > 1. The first issue with cpu0_boot_stack in the head.S file. When we
> > do:
> >       la      sp, cpu0_boot_stack
> >    Pseudo-instruction la will be transformed to auipc/addi OR
> > auipc/l{w|d}.
> >    It depends on an option: nopic, pic. [1]
> >    
> >    So the solution can be the following:
> >    * As it is done in the patch: add to the start of head.S
> > ".option  
> > nopic"
> >    * Change la to lla thereby it will be always generated
> > "auipc/addi"
> > to get an address of variable.
> > 
> > 2. The second issue is with the code in do_bug_frame() with
> > bug_frames
> > array:
> >    const struct bug_frame* bug_frames[] = {
> >         &__start_bug_frames[0],
> >         &__stop_bug_frames_0[0],
> >         &__stop_bug_frames_1[0],
> >         &__stop_bug_frames_2[0],
> >         &__stop_bug_frames_3[0],
> >     };
> >   In this case &{start,stop}bug_frames{,{0-3}} will be changed
> > to     
> > linker address. In case of when load_addr is 0x80200000 and
> > linker_addr
> > is 0x00200000 then &{start,stop}bug_frames{,{0-3}} will be equal to
> > 0x00200000 + X.
> 
> ... this "solution" to a problem you introduce by wrongly modifying
> the linked address would then need applying to any other similar code
> pattern found in Xen. Which is (I hope obviously) not a viable route.
> Instead code running before address translation is enable needs to be
> extra careful in what code and data items it accesses, and how.
> 
I modified the linked address only for the experiment ( when load_addr
!= linker_addr to emulate situation Julien told me about), so it's not
something I planned to send as a part of the final patch, and I
probably forgot to mention that in my previous mail.

It is only one place where we have to do a kind of 'force' and is
needed to make the current state of RISC-V Xen work in case we don't
have MMU enabled yet and linker_addr != load_addr. All other cases
where it is used something from the range (linker_start, linker_end)
will be managed by MMU.

If we can't use mentioned above solution, we still need to handle the
situation when linker_addr != load_addr and MMU isn't enabled yet.
Other options to do that:
1. add phys_offset ( | load_addr - linker_addr | ) everywhere where
bug_frames array is used: bug_frames[id] + phys_offset
2. Check somewhere at the start if linker_addr != load_addr, then throw
an error and panic().

Other options might exist. So I would appreciate it if you could
suggest me some.

Could you let me know if any options are suitable for handling a case
when linker_addr?
> >     To force using addresses related to load_addr  in bug_frames,
> > it is
> > necessary to declare a variable with getting an address of
> > &__{start,stop}bug_frames{,{0-3}} thereby it will generate the
> > code:
> >         2002c2:       00001797                auipc   a5,0x1
> >         2002c6:       d3e78793                addi    a5,a5,-706 #
> > 201000 <__start_bug_frames>
> >         2002ca:       faf43c23                sd      a5,-72(s0)
> >         2002ce:       00001797                auipc   a5,0x1
> >         2002d2:       d3a78793                addi    a5,a5,-710 #
> > 201008 <__stop_bug_frames_
> 
~ Oleksii

Re: [PATCH v1 2/3] xen/riscv: setup initial pagetables

[Jan Beulich]

On 08.03.2023 17:16, Oleksii wrote:
> On Wed, 2023-03-08 at 16:17 +0100, Jan Beulich wrote:
>> On 08.03.2023 15:54, Oleksii wrote:
>>> Actually after my latest experiments it looks that we don't need to
>>> calculate that things at all because for RISC-V it is  used
>>> everywhere
>>> PC-relative access.
>>> Thereby it doesn't matter what is an address where Xen was loaded
>>> and
>>> linker address.
>>> Right now I found only to cases which aren't PC-relative.
>>> Please look at the patch below:
>>> diff --git a/xen/arch/riscv/include/asm/config.h
>>> b/xen/arch/riscv/include/asm/config.h
>>> index 763a922a04..e1ba613d81 100644
>>> --- a/xen/arch/riscv/include/asm/config.h
>>> +++ b/xen/arch/riscv/include/asm/config.h
>>> @@ -39,7 +39,7 @@
>>>    name:
>>>  #endif
>>>  
>>> -#define XEN_VIRT_START  _AT(UL, 0x80200000)
>>> +#define XEN_VIRT_START  _AT(UL, 0x00200000)
>>
>> I think this wants to remain the address where Xen actually runs, and
>> where Xen is linked to. This ...
>>
>>> --- a/xen/arch/riscv/traps.c
>>> +++ b/xen/arch/riscv/traps.c
>>> @@ -123,8 +123,14 @@ int do_bug_frame(const struct cpu_user_regs
>>> *regs,
>>> vaddr_t pc)
>>>      const char *filename, *predicate;
>>>      int lineno;
>>>  
>>> -    static const struct bug_frame* bug_frames[] = {
>>> -        &__start_bug_frames[0],
>>> +    /*
>>> +     * force fill bug_frames array using auipc/addi instructions
>>> to
>>> +     * make addresses in bug_frames PC-relative.
>>> +    */
>>> +    const struct bug_frame * force = (const struct bug_frame *)
>>> &__start_bug_frames[0];
>>> +
>>> +    const struct bug_frame* bug_frames[] = {
>>> +        force,
>>>          &__stop_bug_frames_0[0],
>>>          &__stop_bug_frames_1[0],
>>>          &__stop_bug_frames_2[0],
>>
>> ... array would better be static anyway, and ...
>>
>>> The changes related to <asm/config.h> are  only to make linker_addr
>>> !=
>>> load_address. So:
>>> 1. The first issue with cpu0_boot_stack in the head.S file. When we
>>> do:
>>>       la      sp, cpu0_boot_stack
>>>    Pseudo-instruction la will be transformed to auipc/addi OR
>>> auipc/l{w|d}.
>>>    It depends on an option: nopic, pic. [1]
>>>    
>>>    So the solution can be the following:
>>>    * As it is done in the patch: add to the start of head.S
>>> ".option  
>>> nopic"
>>>    * Change la to lla thereby it will be always generated
>>> "auipc/addi"
>>> to get an address of variable.
>>>
>>> 2. The second issue is with the code in do_bug_frame() with
>>> bug_frames
>>> array:
>>>    const struct bug_frame* bug_frames[] = {
>>>         &__start_bug_frames[0],
>>>         &__stop_bug_frames_0[0],
>>>         &__stop_bug_frames_1[0],
>>>         &__stop_bug_frames_2[0],
>>>         &__stop_bug_frames_3[0],
>>>     };
>>>   In this case &{start,stop}bug_frames{,{0-3}} will be changed
>>> to     
>>> linker address. In case of when load_addr is 0x80200000 and
>>> linker_addr
>>> is 0x00200000 then &{start,stop}bug_frames{,{0-3}} will be equal to
>>> 0x00200000 + X.
>>
>> ... this "solution" to a problem you introduce by wrongly modifying
>> the linked address would then need applying to any other similar code
>> pattern found in Xen. Which is (I hope obviously) not a viable route.
>> Instead code running before address translation is enable needs to be
>> extra careful in what code and data items it accesses, and how.
>>
> I modified the linked address only for the experiment ( when load_addr
> != linker_addr to emulate situation Julien told me about), so it's not
> something I planned to send as a part of the final patch, and I
> probably forgot to mention that in my previous mail.
> 
> It is only one place where we have to do a kind of 'force' and is
> needed to make the current state of RISC-V Xen work in case we don't
> have MMU enabled yet and linker_addr != load_addr. All other cases
> where it is used something from the range (linker_start, linker_end)
> will be managed by MMU.
> 
> If we can't use mentioned above solution, we still need to handle the
> situation when linker_addr != load_addr and MMU isn't enabled yet.
> Other options to do that:
> 1. add phys_offset ( | load_addr - linker_addr | ) everywhere where
> bug_frames array is used: bug_frames[id] + phys_offset

Well, that again special cases a certain data structure. As said before,
you need to be very careful with any C code involved before translation
is enabled. Unless you want to retain relocations (so you can "move"
from load-time to link time addresses alongside enabling translation,
like we do on x86 in xen.efi), you want to constrain code paths as much
as possible. One approach is to move enabling of translation to early
assembly code (like we do on x86 for xen.gz). The other is to amend
involved code paths with something like what you say above.

> 2. Check somewhere at the start if linker_addr != load_addr, then throw
> an error and panic().

That's not really an option if the boot loader isn't required to place
the image at its linked address (which would be odd if translation
isn't expected to be enabled yet at that point). Plus no matter what
linked address you choose, I guess there may be systems where that
address range simply isn't (fully) populated with RAM.

> Other options might exist. So I would appreciate it if you could
> suggest me some.
> 
> Could you let me know if any options are suitable for handling a case
> when linker_addr?

Main question is how tied you are to doing this in C. x86 and both
Arm flavors do it in assembly, with - as said - the exception of
x86's xen.efi where instead we retain (or generate) and process base
relocations (see efi_arch_relocate_image(), called by
efi_arch_post_exit_boot() immediately before switching to the "real"
page tables).

Jan

Re: [PATCH v1 2/3] xen/riscv: setup initial pagetables

[Oleksii]

On Thu, 2023-03-09 at 10:46 +0100, Jan Beulich wrote:
> On 08.03.2023 17:16, Oleksii wrote:
> > On Wed, 2023-03-08 at 16:17 +0100, Jan Beulich wrote:
> > > On 08.03.2023 15:54, Oleksii wrote:
> > > > Actually after my latest experiments it looks that we don't
> > > > need to
> > > > calculate that things at all because for RISC-V it is  used
> > > > everywhere
> > > > PC-relative access.
> > > > Thereby it doesn't matter what is an address where Xen was
> > > > loaded
> > > > and
> > > > linker address.
> > > > Right now I found only to cases which aren't PC-relative.
> > > > Please look at the patch below:
> > > > diff --git a/xen/arch/riscv/include/asm/config.h
> > > > b/xen/arch/riscv/include/asm/config.h
> > > > index 763a922a04..e1ba613d81 100644
> > > > --- a/xen/arch/riscv/include/asm/config.h
> > > > +++ b/xen/arch/riscv/include/asm/config.h
> > > > @@ -39,7 +39,7 @@
> > > >    name:
> > > >  #endif
> > > >  
> > > > -#define XEN_VIRT_START  _AT(UL, 0x80200000)
> > > > +#define XEN_VIRT_START  _AT(UL, 0x00200000)
> > > 
> > > I think this wants to remain the address where Xen actually runs,
> > > and
> > > where Xen is linked to. This ...
> > > 
> > > > --- a/xen/arch/riscv/traps.c
> > > > +++ b/xen/arch/riscv/traps.c
> > > > @@ -123,8 +123,14 @@ int do_bug_frame(const struct
> > > > cpu_user_regs
> > > > *regs,
> > > > vaddr_t pc)
> > > >      const char *filename, *predicate;
> > > >      int lineno;
> > > >  
> > > > -    static const struct bug_frame* bug_frames[] = {
> > > > -        &__start_bug_frames[0],
> > > > +    /*
> > > > +     * force fill bug_frames array using auipc/addi
> > > > instructions
> > > > to
> > > > +     * make addresses in bug_frames PC-relative.
> > > > +    */
> > > > +    const struct bug_frame * force = (const struct bug_frame
> > > > *)
> > > > &__start_bug_frames[0];
> > > > +
> > > > +    const struct bug_frame* bug_frames[] = {
> > > > +        force,
> > > >          &__stop_bug_frames_0[0],
> > > >          &__stop_bug_frames_1[0],
> > > >          &__stop_bug_frames_2[0],
> > > 
> > > ... array would better be static anyway, and ...
> > > 
> > > > The changes related to <asm/config.h> are  only to make
> > > > linker_addr
> > > > !=
> > > > load_address. So:
> > > > 1. The first issue with cpu0_boot_stack in the head.S file.
> > > > When we
> > > > do:
> > > >       la      sp, cpu0_boot_stack
> > > >    Pseudo-instruction la will be transformed to auipc/addi OR
> > > > auipc/l{w|d}.
> > > >    It depends on an option: nopic, pic. [1]
> > > >    
> > > >    So the solution can be the following:
> > > >    * As it is done in the patch: add to the start of head.S
> > > > ".option  
> > > > nopic"
> > > >    * Change la to lla thereby it will be always generated
> > > > "auipc/addi"
> > > > to get an address of variable.
> > > > 
> > > > 2. The second issue is with the code in do_bug_frame() with
> > > > bug_frames
> > > > array:
> > > >    const struct bug_frame* bug_frames[] = {
> > > >         &__start_bug_frames[0],
> > > >         &__stop_bug_frames_0[0],
> > > >         &__stop_bug_frames_1[0],
> > > >         &__stop_bug_frames_2[0],
> > > >         &__stop_bug_frames_3[0],
> > > >     };
> > > >   In this case &{start,stop}bug_frames{,{0-3}} will be changed
> > > > to     
> > > > linker address. In case of when load_addr is 0x80200000 and
> > > > linker_addr
> > > > is 0x00200000 then &{start,stop}bug_frames{,{0-3}} will be
> > > > equal to
> > > > 0x00200000 + X.
> > > 
> > > ... this "solution" to a problem you introduce by wrongly
> > > modifying
> > > the linked address would then need applying to any other similar
> > > code
> > > pattern found in Xen. Which is (I hope obviously) not a viable
> > > route.
> > > Instead code running before address translation is enable needs
> > > to be
> > > extra careful in what code and data items it accesses, and how.
> > > 
> > I modified the linked address only for the experiment ( when
> > load_addr
> > != linker_addr to emulate situation Julien told me about), so it's
> > not
> > something I planned to send as a part of the final patch, and I
> > probably forgot to mention that in my previous mail.
> > 
> > It is only one place where we have to do a kind of 'force' and is
> > needed to make the current state of RISC-V Xen work in case we
> > don't
> > have MMU enabled yet and linker_addr != load_addr. All other cases
> > where it is used something from the range (linker_start,
> > linker_end)
> > will be managed by MMU.
> > 
> > If we can't use mentioned above solution, we still need to handle
> > the
> > situation when linker_addr != load_addr and MMU isn't enabled yet.
> > Other options to do that:
> > 1. add phys_offset ( | load_addr - linker_addr | ) everywhere where
> > bug_frames array is used: bug_frames[id] + phys_offset
> 
> Well, that again special cases a certain data structure. As said
> before,
> you need to be very careful with any C code involved before
> translation
> is enabled. Unless you want to retain relocations (so you can "move"
> from load-time to link time addresses alongside enabling translation,
> like we do on x86 in xen.efi), you want to constrain code paths as
> much
> as possible. One approach is to move enabling of translation to early
> assembly code (like we do on x86 for xen.gz). The other is to amend
> involved code paths with something like what you say above.
> 
> > 2. Check somewhere at the start if linker_addr != load_addr, then
> > throw
> > an error and panic().
> 
> That's not really an option if the boot loader isn't required to
> place
> the image at its linked address (which would be odd if translation
> isn't expected to be enabled yet at that point). Plus no matter what
> linked address you choose, I guess there may be systems where that
> address range simply isn't (fully) populated with RAM.
> 
> > Other options might exist. So I would appreciate it if you could
> > suggest me some.
> > 
> > Could you let me know if any options are suitable for handling a
> > case
> > when linker_addr?
> 
> Main question is how tied you are to doing this in C. x86 and both
> Arm flavors do it in assembly, with - as said - the exception of
> x86's xen.efi where instead we retain (or generate) and process base
> relocations (see efi_arch_relocate_image(), called by
> efi_arch_post_exit_boot() immediately before switching to the "real"
> page tables).

Thanks for the clarification.

I will look at xen.efi & xen.gz, but I think I will follow scenario 1
or similar to it ( as I did in the path diff with the introduction of
force variable) for now, it is required only to make addresses relative
to phys_offset in one array.

After that, I will enable MMU, and it won't be necessary to add
phys_offset or make similar changes to the introduction of force
variable mentioned in the patch diff.

~ Oleksii

[PATCH v1 3/3] automation: update RISC-V smoke test

[Oleksii Kurochko]

The smoke test was updated to verify that MMU has been enabled.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 automation/scripts/qemu-smoke-riscv64.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/automation/scripts/qemu-smoke-riscv64.sh b/automation/scripts/qemu-smoke-riscv64.sh
index 02fc66be03..01cd08e407 100755
--- a/automation/scripts/qemu-smoke-riscv64.sh
+++ b/automation/scripts/qemu-smoke-riscv64.sh
@@ -16,5 +16,5 @@ qemu-system-riscv64 \
     |& tee smoke.serial
 
 set -e
-(grep -q "WARN is most likely working" smoke.serial) || exit 1
+(grep -q "MMU has been enabled" smoke.serial) || exit 1
 exit 0
-- 
2.39.0

Re: [PATCH v1 3/3] automation: update RISC-V smoke test

[Andrew Cooper]

On 24/02/2023 3:06 pm, Oleksii Kurochko wrote:
> The smoke test was updated to verify that MMU has been enabled.
>
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
>  automation/scripts/qemu-smoke-riscv64.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/automation/scripts/qemu-smoke-riscv64.sh b/automation/scripts/qemu-smoke-riscv64.sh
> index 02fc66be03..01cd08e407 100755
> --- a/automation/scripts/qemu-smoke-riscv64.sh
> +++ b/automation/scripts/qemu-smoke-riscv64.sh
> @@ -16,5 +16,5 @@ qemu-system-riscv64 \
>      |& tee smoke.serial
>  
>  set -e
> -(grep -q "WARN is most likely working" smoke.serial) || exit 1
> +(grep -q "MMU has been enabled" smoke.serial) || exit 1
>  exit 0

There's a more simple way than this.  I'll do a patch.

~Andrew

Re: [PATCH v1 3/3] automation: update RISC-V smoke test

[Oleksii]

On Fri, 2023-02-24 at 15:27 +0000, Andrew Cooper wrote:
> On 24/02/2023 3:06 pm, Oleksii Kurochko wrote:
> > The smoke test was updated to verify that MMU has been enabled.
> > 
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> > ---
> >  automation/scripts/qemu-smoke-riscv64.sh | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/automation/scripts/qemu-smoke-riscv64.sh
> > b/automation/scripts/qemu-smoke-riscv64.sh
> > index 02fc66be03..01cd08e407 100755
> > --- a/automation/scripts/qemu-smoke-riscv64.sh
> > +++ b/automation/scripts/qemu-smoke-riscv64.sh
> > @@ -16,5 +16,5 @@ qemu-system-riscv64 \
> >      |& tee smoke.serial
> >  
> >  set -e
> > -(grep -q "WARN is most likely working" smoke.serial) || exit 1
> > +(grep -q "MMU has been enabled" smoke.serial) || exit 1
> >  exit 0
> 
> There's a more simple way than this.  I'll do a patch.
Definitely your way is more simple. Thanks.
> 
> ~Andrew
~ Oleksii

Re: [PATCH v1 0/3] enable MMU for RISC-V

[Oleksii]

On Fri, 2023-02-24 at 17:06 +0200, Oleksii Kurochko wrote:
> The patch series introduces the following things:
> 1. Functionality to build the page tables for Xen that map the
>    following:
>      * The physical location of Xen (where the bootloader loaded it)
>      * The link-time location of Xen (where the linker expected Xen's
>        addresses to be.
> 2. Load the built page table into the SATP
> 3. Enables MMU.
> 4. Updates smoke test to grep message which should be printed after
>    MMU is enabled.
> 
> Oleksii Kurochko (3):
>   xen/riscv: introduce setup_initial_pages
>   xen/riscv: setup initial pagetables
>   automation: update RISC-V smoke test
> 
>  automation/scripts/qemu-smoke-riscv64.sh |   2 +-
>  xen/arch/riscv/Makefile                  |   1 +
>  xen/arch/riscv/include/asm/mm.h          |   9 +
>  xen/arch/riscv/include/asm/page.h        |  90 +++++++++
>  xen/arch/riscv/mm.c                      | 223
> +++++++++++++++++++++++
>  xen/arch/riscv/setup.c                   |  11 ++
>  6 files changed, 335 insertions(+), 1 deletion(-)
>  create mode 100644 xen/arch/riscv/include/asm/mm.h
>  create mode 100644 xen/arch/riscv/include/asm/page.h
>  create mode 100644 xen/arch/riscv/mm.c
> 

I forgot to mention in cover letter that the patch series is based on
top of 'introduce generic implementation of macros from bug.h' and
'RISCV basic exception handling implementation' patch series.