[PATCH v1 0/8] xen/riscv: introduce identity mapping

[PATCH v1 0/8] xen/riscv: introduce identity mapping

[Oleksii Kurochko]

The patch series introduces things necessary to implement identity mapping:
  1. Make identity mapping for _start and stack.
  2. Enable MMU.
  3. Jump to the virtual address world
  4. Remove identity mapping for _start and stack.

Also current patch series introduces the calculation of physical offset before
MMU is enabled as access to physical offset will be calculated wrong after
MMU will be enabled because access to phys_off variable is PC-relative and
in the case when linker address != load address, it will cause MMU fault.

One more thing that was done is:
  * Added SPDX tags.
  * Added __ASSEMBLY__ guards.
  * move extern of cpu0_boot_stack to a header.

The reason for this patch series can be found here:
https://lore.kernel.org/xen-devel/4e336121-fc0c-b007-bf7b-430352563d55@citrix.com/

Oleksii Kurochko (8):
  xen/riscv: make sure that identity mapping isn't bigger then page size
  xen/riscv: add .sbss section to .bss
  xen/riscv: introduce reset_stack() function
  xen/riscv: introduce function for physical offset calculation
  xen/riscv: introduce identity mapping
  xen/riscv: add SPDX tags
  xen/riscv: add __ASSEMBLY__ guards
  xen/riscv: move extern of cpu0_boot_stack to header

 xen/arch/riscv/include/asm/config.h       |   2 +
 xen/arch/riscv/include/asm/current.h      |   2 +
 xen/arch/riscv/include/asm/early_printk.h |   2 +
 xen/arch/riscv/include/asm/mm.h           |   9 +-
 xen/arch/riscv/include/asm/page-bits.h    |   2 +
 xen/arch/riscv/include/asm/page.h         |   6 ++
 xen/arch/riscv/include/asm/traps.h        |   2 +
 xen/arch/riscv/include/asm/types.h        |   2 +
 xen/arch/riscv/mm.c                       | 119 +++++++++++++++-------
 xen/arch/riscv/riscv64/head.S             |  40 +++++++-
 xen/arch/riscv/setup.c                    |  16 +--
 xen/arch/riscv/xen.lds.S                  |  11 +-
 12 files changed, 160 insertions(+), 53 deletions(-)

-- 
2.40.1

[PATCH v1 1/8] xen/riscv: make sure that identity mapping isn't bigger then page size

[Oleksii Kurochko]

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

diff --git a/xen/arch/riscv/xen.lds.S b/xen/arch/riscv/xen.lds.S
index 878130f313..74afbaab9b 100644
--- a/xen/arch/riscv/xen.lds.S
+++ b/xen/arch/riscv/xen.lds.S
@@ -20,6 +20,7 @@ SECTIONS
     . = XEN_VIRT_START;
     _start = .;
     .text : {
+        _idmap_start = .;
         _stext = .;            /* Text section */
         *(.text.header)
 
@@ -35,6 +36,7 @@ SECTIONS
         *(.gnu.warning)
         . = ALIGN(POINTER_ALIGN);
         _etext = .;             /* End of text section */
+        _idmap_end = .;
     } :text
 
     . = ALIGN(PAGE_SIZE);
@@ -174,3 +176,10 @@ ASSERT(!SIZEOF(.got),      ".got non-empty")
 ASSERT(!SIZEOF(.got.plt),  ".got.plt non-empty")
 
 ASSERT(_end - _start <= MB(2), "Xen too large for early-boot assumptions")
+
+/*
+ * We require that Xen is loaded at a page boundary, so this ensures that any
+ * code running on the identity map cannot cross a page boundary.
+ */
+ASSERT(IS_ALIGNED(_idmap_start, PAGE_SIZE), "_idmap_start should be page-aligned")
+ASSERT(_idmap_end - _idmap_start <= PAGE_SIZE, "Identity mapped code is larger than a page size")
-- 
2.40.1

[PATCH v1 2/8] xen/riscv: add .sbss section to .bss

[Oleksii Kurochko]

Sometimes variables are located in .sbss section but it won't
be mapped after MMU will be enabled.
To avoid MMU failures .sbss should be mapped

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/riscv/xen.lds.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/riscv/xen.lds.S b/xen/arch/riscv/xen.lds.S
index 74afbaab9b..9a2799bab5 100644
--- a/xen/arch/riscv/xen.lds.S
+++ b/xen/arch/riscv/xen.lds.S
@@ -151,7 +151,7 @@ SECTIONS
         *(.bss.percpu.read_mostly)
         . = ALIGN(SMP_CACHE_BYTES);
         __per_cpu_data_end = .;
-        *(.bss .bss.*)
+        *(.bss .bss.* .sbss)
         . = ALIGN(POINTER_ALIGN);
         __bss_end = .;
     } :text
-- 
2.40.1

[PATCH v1 3/8] xen/riscv: introduce reset_stack() function

[Oleksii Kurochko]

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/riscv/riscv64/head.S | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/xen/arch/riscv/riscv64/head.S b/xen/arch/riscv/riscv64/head.S
index 8887f0cbd4..6fb7dd80fd 100644
--- a/xen/arch/riscv/riscv64/head.S
+++ b/xen/arch/riscv/riscv64/head.S
@@ -27,8 +27,14 @@ ENTRY(start)
         add     t3, t3, __SIZEOF_POINTER__
         bltu    t3, t4, .L_clear_bss
 
+        jal     reset_stack
+
+        tail    start_xen
+
+ENTRY(reset_stack)
         la      sp, cpu0_boot_stack
         li      t0, STACK_SIZE
         add     sp, sp, t0
 
-        tail    start_xen
+        ret
+
-- 
2.40.1

[PATCH v1 4/8] xen/riscv: introduce function for physical offset calculation

[Oleksii Kurochko]

The function was introduced to not calculate and save physical
offset before MMU is enabled because access to start() is
PC-relative and in case of linker_addr != load_addr it will result
in incorrect value in phys_offset.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/riscv/include/asm/mm.h |  2 ++
 xen/arch/riscv/mm.c             | 18 +++++++++++++++---
 xen/arch/riscv/riscv64/head.S   |  2 ++
 3 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/xen/arch/riscv/include/asm/mm.h b/xen/arch/riscv/include/asm/mm.h
index 64293eacee..996041ce81 100644
--- a/xen/arch/riscv/include/asm/mm.h
+++ b/xen/arch/riscv/include/asm/mm.h
@@ -11,4 +11,6 @@ void setup_initial_pagetables(void);
 void enable_mmu(void);
 void cont_after_mmu_is_enabled(void);
 
+void calc_phys_offset(void);
+
 #endif /* _ASM_RISCV_MM_H */
diff --git a/xen/arch/riscv/mm.c b/xen/arch/riscv/mm.c
index 8ceed445cf..c092897f9a 100644
--- a/xen/arch/riscv/mm.c
+++ b/xen/arch/riscv/mm.c
@@ -19,9 +19,11 @@ struct mmu_desc {
 
 extern unsigned char cpu0_boot_stack[STACK_SIZE];
 
-#define PHYS_OFFSET ((unsigned long)_start - XEN_VIRT_START)
-#define LOAD_TO_LINK(addr) ((addr) - PHYS_OFFSET)
-#define LINK_TO_LOAD(addr) ((addr) + PHYS_OFFSET)
+static unsigned long phys_offset;
+
+#define LOAD_TO_LINK(addr) ((unsigned long)(addr) - phys_offset)
+#define LINK_TO_LOAD(addr) ((unsigned long)(addr) + phys_offset)
+
 
 /*
  * It is expected that Xen won't be more then 2 MB.
@@ -273,3 +275,13 @@ void __init noreturn noinline enable_mmu()
     switch_stack_and_jump((unsigned long)cpu0_boot_stack + STACK_SIZE,
                           cont_after_mmu_is_enabled);
 }
+
+/*
+ * calc_phys_offset() should be used before MMU is enabled because access to
+ * start() is PC-relative and in case when load_addr != linker_addr phys_offset
+ * will have an incorrect value
+ */
+void  calc_phys_offset(void)
+{
+    phys_offset = (unsigned long)start - XEN_VIRT_START;
+}
diff --git a/xen/arch/riscv/riscv64/head.S b/xen/arch/riscv/riscv64/head.S
index 6fb7dd80fd..69f3a24987 100644
--- a/xen/arch/riscv/riscv64/head.S
+++ b/xen/arch/riscv/riscv64/head.S
@@ -29,6 +29,8 @@ ENTRY(start)
 
         jal     reset_stack
 
+        jal     calc_phys_offset
+
         tail    start_xen
 
 ENTRY(reset_stack)
-- 
2.40.1

[PATCH v1 5/8] xen/riscv: introduce identity mapping

[Oleksii Kurochko]

The way how switch to virtual address was implemented in the
commit e66003e7be ("xen/riscv: introduce setup_initial_pages")
wasn't safe enough so identity mapping was introduced and
used.

Fixes: e66003e7be ("xen/riscv: introduce setup_initial_pages")
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/riscv/include/asm/mm.h |  3 +-
 xen/arch/riscv/mm.c             | 99 ++++++++++++++++++++++-----------
 xen/arch/riscv/riscv64/head.S   | 30 ++++++++++
 xen/arch/riscv/setup.c          | 14 +----
 4 files changed, 99 insertions(+), 47 deletions(-)

diff --git a/xen/arch/riscv/include/asm/mm.h b/xen/arch/riscv/include/asm/mm.h
index 996041ce81..500fdc9c5a 100644
--- a/xen/arch/riscv/include/asm/mm.h
+++ b/xen/arch/riscv/include/asm/mm.h
@@ -9,7 +9,8 @@
 void setup_initial_pagetables(void);
 
 void enable_mmu(void);
-void cont_after_mmu_is_enabled(void);
+
+void remove_identity_mapping(void);
 
 void calc_phys_offset(void);
 
diff --git a/xen/arch/riscv/mm.c b/xen/arch/riscv/mm.c
index c092897f9a..ab790f571d 100644
--- a/xen/arch/riscv/mm.c
+++ b/xen/arch/riscv/mm.c
@@ -24,6 +24,11 @@ static unsigned long phys_offset;
 #define LOAD_TO_LINK(addr) ((unsigned long)(addr) - phys_offset)
 #define LINK_TO_LOAD(addr) ((unsigned long)(addr) + phys_offset)
 
+/*
+ * Should be removed as soon as enough headers will be merged for inclusion of
+ * <xen/lib.h>.
+ */
+#define ARRAY_SIZE(arr)		(sizeof(arr) / sizeof((arr)[0]))
 
 /*
  * It is expected that Xen won't be more then 2 MB.
@@ -35,8 +40,10 @@ static unsigned long phys_offset;
  *
  * It might be needed one more page table in case when Xen load address
  * isn't 2 MB aligned.
+ *
+ * 3 additional page tables are needed for identity mapping.
  */
-#define PGTBL_INITIAL_COUNT ((CONFIG_PAGING_LEVELS - 1) + 1)
+#define PGTBL_INITIAL_COUNT ((CONFIG_PAGING_LEVELS - 1) + 1 + 3)
 
 pte_t __section(".bss.page_aligned") __aligned(PAGE_SIZE)
 stage1_pgtbl_root[PAGETABLE_ENTRIES];
@@ -75,6 +82,7 @@ static void __init setup_initial_mapping(struct mmu_desc *mmu_desc,
     unsigned int index;
     pte_t *pgtbl;
     unsigned long page_addr;
+    bool is_identity_mapping = (map_start == pa_start);
 
     if ( (unsigned long)_start % XEN_PT_LEVEL_SIZE(0) )
     {
@@ -108,16 +116,18 @@ static void __init setup_initial_mapping(struct mmu_desc *mmu_desc,
             {
                 unsigned long paddr = (page_addr - map_start) + pa_start;
                 unsigned int permissions = PTE_LEAF_DEFAULT;
+                unsigned long addr = (is_identity_mapping) ?
+                                     page_addr : LINK_TO_LOAD(page_addr);
                 pte_t pte_to_be_written;
 
                 index = pt_index(0, page_addr);
 
-                if ( is_kernel_text(LINK_TO_LOAD(page_addr)) ||
-                     is_kernel_inittext(LINK_TO_LOAD(page_addr)) )
-                    permissions =
-                        PTE_EXECUTABLE | PTE_READABLE | PTE_VALID;
+                if ( is_kernel_text(addr) ||
+                     is_kernel_inittext(addr) )
+                        permissions =
+                            PTE_EXECUTABLE | PTE_READABLE | PTE_VALID;
 
-                if ( is_kernel_rodata(LINK_TO_LOAD(page_addr)) )
+                if ( is_kernel_rodata(addr) )
                     permissions = PTE_READABLE | PTE_VALID;
 
                 pte_to_be_written = paddr_to_pte(paddr, permissions);
@@ -232,22 +242,27 @@ void __init setup_initial_pagetables(void)
                           linker_start,
                           linker_end,
                           load_start);
+
+    if ( linker_start == load_start )
+        return;
+
+    setup_initial_mapping(&mmu_desc,
+                          load_start,
+                          load_start + PAGE_SIZE,
+                          load_start);
+
+    setup_initial_mapping(&mmu_desc,
+                          (unsigned long)cpu0_boot_stack,
+                          (unsigned long)cpu0_boot_stack + PAGE_SIZE,
+                          (unsigned long)cpu0_boot_stack);
 }
 
-void __init noreturn noinline enable_mmu()
+/*
+ * enable_mmu() can't be __init because __init section isn't part of identity
+ * mapping so it will cause an issue after MMU will be enabled.
+ */
+void enable_mmu(void)
 {
-    /*
-     * Calculate a linker time address of the mmu_is_enabled
-     * label and update CSR_STVEC with it.
-     * MMU is configured in a way where linker addresses are mapped
-     * on load addresses so in a case when linker addresses are not equal
-     * to load addresses, after MMU is enabled, it will cause
-     * an exception and jump to linker time addresses.
-     * Otherwise if load addresses are equal to linker addresses the code
-     * after mmu_is_enabled label will be executed without exception.
-     */
-    csr_write(CSR_STVEC, LOAD_TO_LINK((unsigned long)&&mmu_is_enabled));
-
     /* Ensure page table writes precede loading the SATP */
     sfence_vma();
 
@@ -255,25 +270,41 @@ void __init noreturn noinline enable_mmu()
     csr_write(CSR_SATP,
               PFN_DOWN((unsigned long)stage1_pgtbl_root) |
               RV_STAGE1_MODE << SATP_MODE_SHIFT);
+}
+
+void __init remove_identity_mapping(void)
+{
+    int i, j;
+    pte_t *pgtbl;
+    unsigned int index, xen_index;
 
-    asm volatile ( ".p2align 2" );
- mmu_is_enabled:
     /*
-     * Stack should be re-inited as:
-     * 1. Right now an address of the stack is relative to load time
-     *    addresses what will cause an issue in case of load start address
-     *    isn't equal to linker start address.
-     * 2. Addresses in stack are all load time relative which can be an
-     *    issue in case when load start address isn't equal to linker
-     *    start address.
-     *
-     * We can't return to the caller because the stack was reseted
-     * and it may have stash some variable on the stack.
-     * Jump to a brand new function as the stack was reseted
+     * id_addrs should be in sync with id mapping in
+     * setup_initial_pagetables()
      */
+    unsigned long id_addrs[] =  {
+                                 LINK_TO_LOAD(_start),
+                                 LINK_TO_LOAD(cpu0_boot_stack),
+                                };
 
-    switch_stack_and_jump((unsigned long)cpu0_boot_stack + STACK_SIZE,
-                          cont_after_mmu_is_enabled);
+    pgtbl = stage1_pgtbl_root;
+
+    for ( j = 0; j < ARRAY_SIZE(id_addrs); j++ )
+    {
+        for ( pgtbl = stage1_pgtbl_root, i = CONFIG_PAGING_LEVELS - 1; i >= 0; i-- )
+        {
+            index = pt_index(i, id_addrs[j]);
+            xen_index = pt_index(i, XEN_VIRT_START);
+
+            if ( index != xen_index )
+            {
+                pgtbl[index].pte = 0;
+                break;
+            }
+
+            pgtbl = &pgtbl[index];
+        }
+    }
 }
 
 /*
diff --git a/xen/arch/riscv/riscv64/head.S b/xen/arch/riscv/riscv64/head.S
index 69f3a24987..582078798a 100644
--- a/xen/arch/riscv/riscv64/head.S
+++ b/xen/arch/riscv/riscv64/head.S
@@ -31,6 +31,36 @@ ENTRY(start)
 
         jal     calc_phys_offset
 
+        jal     setup_initial_pagetables
+
+        jal     enable_mmu
+
+        /*
+         * Calculate physical offset
+         *
+         * We can't re-use a value in phys_offset variable here as
+         * phys_offset is located in .bss and this section isn't
+         * 1:1 mapped and an access to it will cause MMU fault
+         */
+        li      t0, XEN_VIRT_START
+        la      t1, start
+        sub     t1, t1, t0
+
+        /* Calculate proper VA after jump from 1:1 mapping */
+        la      t0, .L_primary_switched
+        sub     t0, t0, t1
+
+        /* Jump from 1:1 mapping world */
+        jr      t0
+
+.L_primary_switched:
+        /*
+         * cpu0_boot_stack address is 1:1 mapping related so it should be
+         * recalculated after jump from 1:1 mapping world as 1:1 mapping
+         * will be removed soon in start_xen().
+         */
+        jal     reset_stack
+
         tail    start_xen
 
 ENTRY(reset_stack)
diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
index 845d18d86f..c4ef0b3165 100644
--- a/xen/arch/riscv/setup.c
+++ b/xen/arch/riscv/setup.c
@@ -11,20 +11,10 @@ unsigned char __initdata cpu0_boot_stack[STACK_SIZE]
 void __init noreturn start_xen(unsigned long bootcpu_id,
                                paddr_t dtb_addr)
 {
-    early_printk("Hello from C env\n");
-
-    setup_initial_pagetables();
-
-    enable_mmu();
-
-    for ( ;; )
-        asm volatile ("wfi");
+    remove_identity_mapping();
 
-    unreachable();
-}
+    early_printk("Hello from C env\n");
 
-void __init noreturn cont_after_mmu_is_enabled(void)
-{
     early_printk("All set up\n");
 
     for ( ;; )
-- 
2.40.1

[PATCH v1 6/8] xen/riscv: add SPDX tags

[Oleksii Kurochko]

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/riscv/include/asm/config.h       | 2 ++
 xen/arch/riscv/include/asm/current.h      | 2 ++
 xen/arch/riscv/include/asm/early_printk.h | 2 ++
 xen/arch/riscv/include/asm/mm.h           | 2 ++
 xen/arch/riscv/include/asm/page-bits.h    | 2 ++
 xen/arch/riscv/include/asm/page.h         | 2 ++
 xen/arch/riscv/include/asm/traps.h        | 2 ++
 xen/arch/riscv/include/asm/types.h        | 2 ++
 xen/arch/riscv/mm.c                       | 2 ++
 xen/arch/riscv/setup.c                    | 2 ++
 10 files changed, 20 insertions(+)

diff --git a/xen/arch/riscv/include/asm/config.h b/xen/arch/riscv/include/asm/config.h
index 38862df0b8..3ae35f57b3 100644
--- a/xen/arch/riscv/include/asm/config.h
+++ b/xen/arch/riscv/include/asm/config.h
@@ -1,3 +1,5 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
 #ifndef __RISCV_CONFIG_H__
 #define __RISCV_CONFIG_H__
 
diff --git a/xen/arch/riscv/include/asm/current.h b/xen/arch/riscv/include/asm/current.h
index d87e6717e0..b08f204df4 100644
--- a/xen/arch/riscv/include/asm/current.h
+++ b/xen/arch/riscv/include/asm/current.h
@@ -1,3 +1,5 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
 #ifndef __ASM_CURRENT_H
 #define __ASM_CURRENT_H
 
diff --git a/xen/arch/riscv/include/asm/early_printk.h b/xen/arch/riscv/include/asm/early_printk.h
index 05106e160d..c11693bbe1 100644
--- a/xen/arch/riscv/include/asm/early_printk.h
+++ b/xen/arch/riscv/include/asm/early_printk.h
@@ -1,3 +1,5 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
 #ifndef __EARLY_PRINTK_H__
 #define __EARLY_PRINTK_H__
 
diff --git a/xen/arch/riscv/include/asm/mm.h b/xen/arch/riscv/include/asm/mm.h
index 500fdc9c5a..3b04131628 100644
--- a/xen/arch/riscv/include/asm/mm.h
+++ b/xen/arch/riscv/include/asm/mm.h
@@ -1,3 +1,5 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
 #ifndef _ASM_RISCV_MM_H
 #define _ASM_RISCV_MM_H
 
diff --git a/xen/arch/riscv/include/asm/page-bits.h b/xen/arch/riscv/include/asm/page-bits.h
index 4a3e33589a..94190b3c61 100644
--- a/xen/arch/riscv/include/asm/page-bits.h
+++ b/xen/arch/riscv/include/asm/page-bits.h
@@ -1,3 +1,5 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
 #ifndef __RISCV_PAGE_BITS_H__
 #define __RISCV_PAGE_BITS_H__
 
diff --git a/xen/arch/riscv/include/asm/page.h b/xen/arch/riscv/include/asm/page.h
index a7e2eee964..8e8ec9ee36 100644
--- a/xen/arch/riscv/include/asm/page.h
+++ b/xen/arch/riscv/include/asm/page.h
@@ -1,3 +1,5 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
 #ifndef _ASM_RISCV_PAGE_H
 #define _ASM_RISCV_PAGE_H
 
diff --git a/xen/arch/riscv/include/asm/traps.h b/xen/arch/riscv/include/asm/traps.h
index f3fb6b25d1..2e896e0913 100644
--- a/xen/arch/riscv/include/asm/traps.h
+++ b/xen/arch/riscv/include/asm/traps.h
@@ -1,3 +1,5 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
 #ifndef __ASM_TRAPS_H__
 #define __ASM_TRAPS_H__
 
diff --git a/xen/arch/riscv/include/asm/types.h b/xen/arch/riscv/include/asm/types.h
index 0c0ce78c8f..7505663efe 100644
--- a/xen/arch/riscv/include/asm/types.h
+++ b/xen/arch/riscv/include/asm/types.h
@@ -1,3 +1,5 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
 #ifndef __RISCV_TYPES_H__
 #define __RISCV_TYPES_H__
 
diff --git a/xen/arch/riscv/mm.c b/xen/arch/riscv/mm.c
index ab790f571d..b50641a80e 100644
--- a/xen/arch/riscv/mm.c
+++ b/xen/arch/riscv/mm.c
@@ -1,3 +1,5 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
 #include <xen/compiler.h>
 #include <xen/init.h>
 #include <xen/kernel.h>
diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
index c4ef0b3165..befbd07fde 100644
--- a/xen/arch/riscv/setup.c
+++ b/xen/arch/riscv/setup.c
@@ -1,3 +1,5 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
 #include <xen/compile.h>
 #include <xen/init.h>
 
-- 
2.40.1

[PATCH v1 7/8] xen/riscv: add __ASSEMBLY__ guards

[Oleksii Kurochko]

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/riscv/include/asm/page.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/xen/arch/riscv/include/asm/page.h b/xen/arch/riscv/include/asm/page.h
index 8e8ec9ee36..1c6add70a5 100644
--- a/xen/arch/riscv/include/asm/page.h
+++ b/xen/arch/riscv/include/asm/page.h
@@ -3,6 +3,8 @@
 #ifndef _ASM_RISCV_PAGE_H
 #define _ASM_RISCV_PAGE_H
 
+#ifndef __ASSEMBLY__
+
 #include <xen/const.h>
 #include <xen/types.h>
 
@@ -60,4 +62,6 @@ static inline bool pte_is_valid(pte_t p)
     return p.pte & PTE_VALID;
 }
 
+#endif /* __ASSEMBLY__ */
+
 #endif /* _ASM_RISCV_PAGE_H */
-- 
2.40.1

[PATCH v1 8/8] xen/riscv: move extern of cpu0_boot_stack to header

[Oleksii Kurochko]

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/riscv/include/asm/mm.h | 2 ++
 xen/arch/riscv/mm.c             | 2 --
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/riscv/include/asm/mm.h b/xen/arch/riscv/include/asm/mm.h
index 3b04131628..fc3afa5521 100644
--- a/xen/arch/riscv/include/asm/mm.h
+++ b/xen/arch/riscv/include/asm/mm.h
@@ -8,6 +8,8 @@
 #define pfn_to_paddr(pfn) ((paddr_t)(pfn) << PAGE_SHIFT)
 #define paddr_to_pfn(pa)  ((unsigned long)((pa) >> PAGE_SHIFT))
 
+extern unsigned char cpu0_boot_stack[];
+
 void setup_initial_pagetables(void);
 
 void enable_mmu(void);
diff --git a/xen/arch/riscv/mm.c b/xen/arch/riscv/mm.c
index b50641a80e..56d3bbb422 100644
--- a/xen/arch/riscv/mm.c
+++ b/xen/arch/riscv/mm.c
@@ -19,8 +19,6 @@ struct mmu_desc {
     pte_t *pgtbl_base;
 };
 
-extern unsigned char cpu0_boot_stack[STACK_SIZE];
-
 static unsigned long phys_offset;
 
 #define LOAD_TO_LINK(addr) ((unsigned long)(addr) - phys_offset)
-- 
2.40.1

Re: [PATCH v1 0/8] xen/riscv: introduce identity mapping

[Jan Beulich]

On 06.06.2023 21:55, Oleksii Kurochko wrote:
> The patch series introduces things necessary to implement identity mapping:
>   1. Make identity mapping for _start and stack.
>   2. Enable MMU.
>   3. Jump to the virtual address world
>   4. Remove identity mapping for _start and stack.
> 
> Also current patch series introduces the calculation of physical offset before
> MMU is enabled as access to physical offset will be calculated wrong after
> MMU will be enabled because access to phys_off variable is PC-relative and
> in the case when linker address != load address, it will cause MMU fault.
> 
> One more thing that was done is:
>   * Added SPDX tags.
>   * Added __ASSEMBLY__ guards.

These are are, aiui, a response to a comment from Andrew. Hence I think
this wants expressing by a {Requested,Suggested,Reported}-by: tag in the
respective patch.

Jan

>   * move extern of cpu0_boot_stack to a header.
> 
> The reason for this patch series can be found here:
> https://lore.kernel.org/xen-devel/4e336121-fc0c-b007-bf7b-430352563d55@citrix.com/
> 
> Oleksii Kurochko (8):
>   xen/riscv: make sure that identity mapping isn't bigger then page size
>   xen/riscv: add .sbss section to .bss
>   xen/riscv: introduce reset_stack() function
>   xen/riscv: introduce function for physical offset calculation
>   xen/riscv: introduce identity mapping
>   xen/riscv: add SPDX tags
>   xen/riscv: add __ASSEMBLY__ guards
>   xen/riscv: move extern of cpu0_boot_stack to header
> 
>  xen/arch/riscv/include/asm/config.h       |   2 +
>  xen/arch/riscv/include/asm/current.h      |   2 +
>  xen/arch/riscv/include/asm/early_printk.h |   2 +
>  xen/arch/riscv/include/asm/mm.h           |   9 +-
>  xen/arch/riscv/include/asm/page-bits.h    |   2 +
>  xen/arch/riscv/include/asm/page.h         |   6 ++
>  xen/arch/riscv/include/asm/traps.h        |   2 +
>  xen/arch/riscv/include/asm/types.h        |   2 +
>  xen/arch/riscv/mm.c                       | 119 +++++++++++++++-------
>  xen/arch/riscv/riscv64/head.S             |  40 +++++++-
>  xen/arch/riscv/setup.c                    |  16 +--
>  xen/arch/riscv/xen.lds.S                  |  11 +-
>  12 files changed, 160 insertions(+), 53 deletions(-)
> 

Re: [PATCH v1 0/8] xen/riscv: introduce identity mapping

[Oleksii]

On Wed, 2023-06-07 at 09:49 +0200, Jan Beulich wrote:
> On 06.06.2023 21:55, Oleksii Kurochko wrote:
> > The patch series introduces things necessary to implement identity
> > mapping:
> >   1. Make identity mapping for _start and stack.
> >   2. Enable MMU.
> >   3. Jump to the virtual address world
> >   4. Remove identity mapping for _start and stack.
> > 
> > Also current patch series introduces the calculation of physical
> > offset before
> > MMU is enabled as access to physical offset will be calculated
> > wrong after
> > MMU will be enabled because access to phys_off variable is PC-
> > relative and
> > in the case when linker address != load address, it will cause MMU
> > fault.
> > 
> > One more thing that was done is:
> >   * Added SPDX tags.
> >   * Added __ASSEMBLY__ guards.
> 
> These are are, aiui, a response to a comment from Andrew. Hence I
> think
> this wants expressing by a {Requested,Suggested,Reported}-by: tag in
> the
> respective patch.
Thanks for a remark.

~ Oleksii

> >   * move extern of cpu0_boot_stack to a header.
> > 
> > The reason for this patch series can be found here:
> > https://lore.kernel.org/xen-devel/4e336121-fc0c-b007-bf7b-430352563d55@citrix.com/
> > 
> > Oleksii Kurochko (8):
> >   xen/riscv: make sure that identity mapping isn't bigger then page
> > size
> >   xen/riscv: add .sbss section to .bss
> >   xen/riscv: introduce reset_stack() function
> >   xen/riscv: introduce function for physical offset calculation
> >   xen/riscv: introduce identity mapping
> >   xen/riscv: add SPDX tags
> >   xen/riscv: add __ASSEMBLY__ guards
> >   xen/riscv: move extern of cpu0_boot_stack to header
> > 
> >  xen/arch/riscv/include/asm/config.h       |   2 +
> >  xen/arch/riscv/include/asm/current.h      |   2 +
> >  xen/arch/riscv/include/asm/early_printk.h |   2 +
> >  xen/arch/riscv/include/asm/mm.h           |   9 +-
> >  xen/arch/riscv/include/asm/page-bits.h    |   2 +
> >  xen/arch/riscv/include/asm/page.h         |   6 ++
> >  xen/arch/riscv/include/asm/traps.h        |   2 +
> >  xen/arch/riscv/include/asm/types.h        |   2 +
> >  xen/arch/riscv/mm.c                       | 119 +++++++++++++++---
> > ----
> >  xen/arch/riscv/riscv64/head.S             |  40 +++++++-
> >  xen/arch/riscv/setup.c                    |  16 +--
> >  xen/arch/riscv/xen.lds.S                  |  11 +-
> >  12 files changed, 160 insertions(+), 53 deletions(-)
> > 
> 

Re: [PATCH v1 6/8] xen/riscv: add SPDX tags

[Julien Grall]

Hi Oleksii,

On 06/06/2023 20:55, Oleksii Kurochko wrote:
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
>   xen/arch/riscv/include/asm/config.h       | 2 ++
>   xen/arch/riscv/include/asm/current.h      | 2 ++
>   xen/arch/riscv/include/asm/early_printk.h | 2 ++
>   xen/arch/riscv/include/asm/mm.h           | 2 ++
>   xen/arch/riscv/include/asm/page-bits.h    | 2 ++
>   xen/arch/riscv/include/asm/page.h         | 2 ++
>   xen/arch/riscv/include/asm/traps.h        | 2 ++
>   xen/arch/riscv/include/asm/types.h        | 2 ++
>   xen/arch/riscv/mm.c                       | 2 ++
>   xen/arch/riscv/setup.c                    | 2 ++
>   10 files changed, 20 insertions(+)
> 
> diff --git a/xen/arch/riscv/include/asm/config.h b/xen/arch/riscv/include/asm/config.h
> index 38862df0b8..3ae35f57b3 100644
> --- a/xen/arch/riscv/include/asm/config.h
> +++ b/xen/arch/riscv/include/asm/config.h
> @@ -1,3 +1,5 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */

The default license for Xen is GPL-2.0-only. So any particular reason to 
diverge?

Cheers,

-- 
Julien Grall

Re: [PATCH v1 6/8] xen/riscv: add SPDX tags

[Oleksii]

Hi Julien,

On Wed, 2023-06-07 at 11:20 +0100, Julien Grall wrote:
> Hi Oleksii,
> 
> On 06/06/2023 20:55, Oleksii Kurochko wrote:
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> > ---
> >   xen/arch/riscv/include/asm/config.h       | 2 ++
> >   xen/arch/riscv/include/asm/current.h      | 2 ++
> >   xen/arch/riscv/include/asm/early_printk.h | 2 ++
> >   xen/arch/riscv/include/asm/mm.h           | 2 ++
> >   xen/arch/riscv/include/asm/page-bits.h    | 2 ++
> >   xen/arch/riscv/include/asm/page.h         | 2 ++
> >   xen/arch/riscv/include/asm/traps.h        | 2 ++
> >   xen/arch/riscv/include/asm/types.h        | 2 ++
> >   xen/arch/riscv/mm.c                       | 2 ++
> >   xen/arch/riscv/setup.c                    | 2 ++
> >   10 files changed, 20 insertions(+)
> > 
> > diff --git a/xen/arch/riscv/include/asm/config.h
> > b/xen/arch/riscv/include/asm/config.h
> > index 38862df0b8..3ae35f57b3 100644
> > --- a/xen/arch/riscv/include/asm/config.h
> > +++ b/xen/arch/riscv/include/asm/config.h
> > @@ -1,3 +1,5 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> 
> The default license for Xen is GPL-2.0-only. So any particular reason
> to 
> diverge?
Thanks for a remark.

There is no any particular reason for that. I copied it from on of a
file from RISC-V arch directory.

I'll  change it to GPL-2.0-only in the next version of the patch
series.

~ Oleksii

Re: [PATCH v1 1/8] xen/riscv: make sure that identity mapping isn't bigger then page size

[Alistair Francis]

On Wed, Jun 7, 2023 at 5:55 AM Oleksii Kurochko
<oleksii.kurochko@gmail.com> wrote:
>
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

Acked-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  xen/arch/riscv/xen.lds.S | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/xen/arch/riscv/xen.lds.S b/xen/arch/riscv/xen.lds.S
> index 878130f313..74afbaab9b 100644
> --- a/xen/arch/riscv/xen.lds.S
> +++ b/xen/arch/riscv/xen.lds.S
> @@ -20,6 +20,7 @@ SECTIONS
>      . = XEN_VIRT_START;
>      _start = .;
>      .text : {
> +        _idmap_start = .;
>          _stext = .;            /* Text section */
>          *(.text.header)
>
> @@ -35,6 +36,7 @@ SECTIONS
>          *(.gnu.warning)
>          . = ALIGN(POINTER_ALIGN);
>          _etext = .;             /* End of text section */
> +        _idmap_end = .;
>      } :text
>
>      . = ALIGN(PAGE_SIZE);
> @@ -174,3 +176,10 @@ ASSERT(!SIZEOF(.got),      ".got non-empty")
>  ASSERT(!SIZEOF(.got.plt),  ".got.plt non-empty")
>
>  ASSERT(_end - _start <= MB(2), "Xen too large for early-boot assumptions")
> +
> +/*
> + * We require that Xen is loaded at a page boundary, so this ensures that any
> + * code running on the identity map cannot cross a page boundary.
> + */
> +ASSERT(IS_ALIGNED(_idmap_start, PAGE_SIZE), "_idmap_start should be page-aligned")
> +ASSERT(_idmap_end - _idmap_start <= PAGE_SIZE, "Identity mapped code is larger than a page size")
> --
> 2.40.1
>
>

Re: [PATCH v1 2/8] xen/riscv: add .sbss section to .bss

[Alistair Francis]

On Wed, Jun 7, 2023 at 5:55 AM Oleksii Kurochko
<oleksii.kurochko@gmail.com> wrote:
>
> Sometimes variables are located in .sbss section but it won't
> be mapped after MMU will be enabled.
> To avoid MMU failures .sbss should be mapped
>
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

Acked-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  xen/arch/riscv/xen.lds.S | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/arch/riscv/xen.lds.S b/xen/arch/riscv/xen.lds.S
> index 74afbaab9b..9a2799bab5 100644
> --- a/xen/arch/riscv/xen.lds.S
> +++ b/xen/arch/riscv/xen.lds.S
> @@ -151,7 +151,7 @@ SECTIONS
>          *(.bss.percpu.read_mostly)
>          . = ALIGN(SMP_CACHE_BYTES);
>          __per_cpu_data_end = .;
> -        *(.bss .bss.*)
> +        *(.bss .bss.* .sbss)
>          . = ALIGN(POINTER_ALIGN);
>          __bss_end = .;
>      } :text
> --
> 2.40.1
>
>

Re: [PATCH v1 3/8] xen/riscv: introduce reset_stack() function

[Alistair Francis]

On Wed, Jun 7, 2023 at 5:55 AM Oleksii Kurochko
<oleksii.kurochko@gmail.com> wrote:
>
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  xen/arch/riscv/riscv64/head.S | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/riscv/riscv64/head.S b/xen/arch/riscv/riscv64/head.S
> index 8887f0cbd4..6fb7dd80fd 100644
> --- a/xen/arch/riscv/riscv64/head.S
> +++ b/xen/arch/riscv/riscv64/head.S
> @@ -27,8 +27,14 @@ ENTRY(start)
>          add     t3, t3, __SIZEOF_POINTER__
>          bltu    t3, t4, .L_clear_bss
>
> +        jal     reset_stack
> +
> +        tail    start_xen
> +
> +ENTRY(reset_stack)
>          la      sp, cpu0_boot_stack
>          li      t0, STACK_SIZE
>          add     sp, sp, t0
>
> -        tail    start_xen
> +        ret
> +
> --
> 2.40.1
>
>

Re: [PATCH v1 2/8] xen/riscv: add .sbss section to .bss

[Jan Beulich]

On 06.06.2023 21:55, Oleksii Kurochko wrote:
> Sometimes variables are located in .sbss section but it won't
> be mapped after MMU will be enabled.
> To avoid MMU failures .sbss should be mapped
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
>  xen/arch/riscv/xen.lds.S | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/riscv/xen.lds.S b/xen/arch/riscv/xen.lds.S
> index 74afbaab9b..9a2799bab5 100644
> --- a/xen/arch/riscv/xen.lds.S
> +++ b/xen/arch/riscv/xen.lds.S
> @@ -151,7 +151,7 @@ SECTIONS
>          *(.bss.percpu.read_mostly)
>          . = ALIGN(SMP_CACHE_BYTES);
>          __per_cpu_data_end = .;
> -        *(.bss .bss.*)
> +        *(.bss .bss.* .sbss)
>          . = ALIGN(POINTER_ALIGN);
>          __bss_end = .;
>      } :text

Two remarks, despite Alistair's ack: Wouldn't it be better to add .sbss.*
right away as well? And strictly speaking wouldn't it be more logical to
have .sbss ahead of .bss?

Jan

Re: [PATCH v1 1/8] xen/riscv: make sure that identity mapping isn't bigger then page size

[Jan Beulich]

On 06.06.2023 21:55, Oleksii Kurochko wrote:
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

Such commits without description are worrying. This may be okay for
entirely trivial and obvious changes, but that's going to be the
exception.

> --- a/xen/arch/riscv/xen.lds.S
> +++ b/xen/arch/riscv/xen.lds.S
> @@ -20,6 +20,7 @@ SECTIONS
>      . = XEN_VIRT_START;
>      _start = .;
>      .text : {
> +        _idmap_start = .;
>          _stext = .;            /* Text section */
>          *(.text.header)
>  
> @@ -35,6 +36,7 @@ SECTIONS
>          *(.gnu.warning)
>          . = ALIGN(POINTER_ALIGN);
>          _etext = .;             /* End of text section */
> +        _idmap_end = .;
>      } :text

So this covers all of .text. Why is it expected that .text will be (and
remain) ...

> @@ -174,3 +176,10 @@ ASSERT(!SIZEOF(.got),      ".got non-empty")
>  ASSERT(!SIZEOF(.got.plt),  ".got.plt non-empty")
>  
>  ASSERT(_end - _start <= MB(2), "Xen too large for early-boot assumptions")
> +
> +/*
> + * We require that Xen is loaded at a page boundary, so this ensures that any
> + * code running on the identity map cannot cross a page boundary.
> + */
> +ASSERT(IS_ALIGNED(_idmap_start, PAGE_SIZE), "_idmap_start should be page-aligned")
> +ASSERT(_idmap_end - _idmap_start <= PAGE_SIZE, "Identity mapped code is larger than a page size")

... less than 4k in size? And why is only .text of interest, but not
other sections?

I find the other assertion a little puzzling too: Isn't that merely
checking that XEN_VIRT_START is page aligned?

Jan

Re: [PATCH v1 3/8] xen/riscv: introduce reset_stack() function

[Jan Beulich]

On 06.06.2023 21:55, Oleksii Kurochko wrote:
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

This wants addressing the "Why?" aspect in the description. Is the present
code wrong in some perhaps subtle way? Are you meaning to re-use the code?
If so, in which way (which is relevant to determine whether the new
function may actually continue to live in .text.header)?

Jan

> --- a/xen/arch/riscv/riscv64/head.S
> +++ b/xen/arch/riscv/riscv64/head.S
> @@ -27,8 +27,14 @@ ENTRY(start)
>          add     t3, t3, __SIZEOF_POINTER__
>          bltu    t3, t4, .L_clear_bss
>  
> +        jal     reset_stack
> +
> +        tail    start_xen
> +
> +ENTRY(reset_stack)
>          la      sp, cpu0_boot_stack
>          li      t0, STACK_SIZE
>          add     sp, sp, t0
>  
> -        tail    start_xen
> +        ret
> +

Re: [PATCH v1 4/8] xen/riscv: introduce function for physical offset calculation

[Jan Beulich]

On 06.06.2023 21:55, Oleksii Kurochko wrote:
> The function was introduced to not calculate and save physical
> offset before MMU is enabled because access to start() is
> PC-relative and in case of linker_addr != load_addr it will result
> in incorrect value in phys_offset.

"... to _not_ calculate ..."?

> --- a/xen/arch/riscv/mm.c
> +++ b/xen/arch/riscv/mm.c
> @@ -19,9 +19,11 @@ struct mmu_desc {
>  
>  extern unsigned char cpu0_boot_stack[STACK_SIZE];
>  
> -#define PHYS_OFFSET ((unsigned long)_start - XEN_VIRT_START)
> -#define LOAD_TO_LINK(addr) ((addr) - PHYS_OFFSET)
> -#define LINK_TO_LOAD(addr) ((addr) + PHYS_OFFSET)
> +static unsigned long phys_offset;

__ro_after_init?

> +#define LOAD_TO_LINK(addr) ((unsigned long)(addr) - phys_offset)
> +#define LINK_TO_LOAD(addr) ((unsigned long)(addr) + phys_offset)
> +
>  

Nit: No double blank lines please.

> @@ -273,3 +275,13 @@ void __init noreturn noinline enable_mmu()
>      switch_stack_and_jump((unsigned long)cpu0_boot_stack + STACK_SIZE,
>                            cont_after_mmu_is_enabled);
>  }
> +
> +/*
> + * calc_phys_offset() should be used before MMU is enabled because access to
> + * start() is PC-relative and in case when load_addr != linker_addr phys_offset
> + * will have an incorrect value
> + */
> +void  calc_phys_offset(void)

__init? And nit: No double blanks please.

Jan

Re: [PATCH v1 5/8] xen/riscv: introduce identity mapping

[Jan Beulich]

On 06.06.2023 21:55, Oleksii Kurochko wrote:
> The way how switch to virtual address was implemented in the
> commit e66003e7be ("xen/riscv: introduce setup_initial_pages")
> wasn't safe enough so identity mapping was introduced and
> used.

I don't think this is sufficient as a description. You want to make
clear what the "not safe enough" is, and you also want to go into
more detail as to the solution chosen. I'm particularly puzzled that
you map just two singular pages ...

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

What is this 3 coming from? It feels like the value should (again)
somehow depend on CONFIG_PAGING_LEVELS.

> @@ -108,16 +116,18 @@ static void __init setup_initial_mapping(struct mmu_desc *mmu_desc,
>              {
>                  unsigned long paddr = (page_addr - map_start) + pa_start;
>                  unsigned int permissions = PTE_LEAF_DEFAULT;
> +                unsigned long addr = (is_identity_mapping) ?

Nit: No need for parentheses here.

> +                                     page_addr : LINK_TO_LOAD(page_addr);

As a remark, while we want binary operators at the end of lines when
wrapping, we usually do things differently for the ternary operator:
Either

                unsigned long addr = is_identity_mapping
                                     ? page_addr : LINK_TO_LOAD(page_addr);

or

                unsigned long addr = is_identity_mapping
                                     ? page_addr
                                     : LINK_TO_LOAD(page_addr);

.

> @@ -232,22 +242,27 @@ void __init setup_initial_pagetables(void)
>                            linker_start,
>                            linker_end,
>                            load_start);
> +
> +    if ( linker_start == load_start )
> +        return;
> +
> +    setup_initial_mapping(&mmu_desc,
> +                          load_start,
> +                          load_start + PAGE_SIZE,
> +                          load_start);
> +
> +    setup_initial_mapping(&mmu_desc,
> +                          (unsigned long)cpu0_boot_stack,
> +                          (unsigned long)cpu0_boot_stack + PAGE_SIZE,

Shouldn't this be STACK_SIZE (and then also be prepared for
STACK_SIZE > PAGE_SIZE)?

> +                          (unsigned long)cpu0_boot_stack);
>  }
>  
> -void __init noreturn noinline enable_mmu()
> +/*
> + * enable_mmu() can't be __init because __init section isn't part of identity
> + * mapping so it will cause an issue after MMU will be enabled.
> + */

As hinted at above already - perhaps the identity mapping wants to be
larger, up to covering the entire Xen image? Since it's temporary
only anyway, you could even consider using a large page (and RWX
permission). You already require no overlap of link and load addresses,
so at least small page mappings ought to be possible for the entire
image.

> @@ -255,25 +270,41 @@ void __init noreturn noinline enable_mmu()
>      csr_write(CSR_SATP,
>                PFN_DOWN((unsigned long)stage1_pgtbl_root) |
>                RV_STAGE1_MODE << SATP_MODE_SHIFT);
> +}
> +
> +void __init remove_identity_mapping(void)
> +{
> +    int i, j;

Nit: unsigned int please.

> +    pte_t *pgtbl;
> +    unsigned int index, xen_index;

These would all probably better be declared in the narrowest possible
scope.

> -    asm volatile ( ".p2align 2" );
> - mmu_is_enabled:
>      /*
> -     * Stack should be re-inited as:
> -     * 1. Right now an address of the stack is relative to load time
> -     *    addresses what will cause an issue in case of load start address
> -     *    isn't equal to linker start address.
> -     * 2. Addresses in stack are all load time relative which can be an
> -     *    issue in case when load start address isn't equal to linker
> -     *    start address.
> -     *
> -     * We can't return to the caller because the stack was reseted
> -     * and it may have stash some variable on the stack.
> -     * Jump to a brand new function as the stack was reseted
> +     * id_addrs should be in sync with id mapping in
> +     * setup_initial_pagetables()

What is "id" meant to stand for here? Also if things need keeping in
sync, then a similar comment should exist on the other side.

>       */
> +    unsigned long id_addrs[] =  {
> +                                 LINK_TO_LOAD(_start),
> +                                 LINK_TO_LOAD(cpu0_boot_stack),
> +                                };
>  
> -    switch_stack_and_jump((unsigned long)cpu0_boot_stack + STACK_SIZE,
> -                          cont_after_mmu_is_enabled);
> +    pgtbl = stage1_pgtbl_root;
> +
> +    for ( j = 0; j < ARRAY_SIZE(id_addrs); j++ )
> +    {
> +        for ( pgtbl = stage1_pgtbl_root, i = CONFIG_PAGING_LEVELS - 1; i >= 0; i-- )
> +        {
> +            index = pt_index(i, id_addrs[j]);
> +            xen_index = pt_index(i, XEN_VIRT_START);
> +
> +            if ( index != xen_index )
> +            {
> +                pgtbl[index].pte = 0;
> +                break;
> +            }

Up to here I understand index specifies a particular PTE within pgtbl[].

> +            pgtbl = &pgtbl[index];

But then how can this be the continuation of the page table walk? Don't
you need to read the address out of the PTE?

> --- a/xen/arch/riscv/riscv64/head.S
> +++ b/xen/arch/riscv/riscv64/head.S
> @@ -31,6 +31,36 @@ ENTRY(start)
>  
>          jal     calc_phys_offset
>  
> +        jal     setup_initial_pagetables
> +
> +        jal     enable_mmu
> +
> +        /*
> +         * Calculate physical offset
> +         *
> +         * We can't re-use a value in phys_offset variable here as
> +         * phys_offset is located in .bss and this section isn't
> +         * 1:1 mapped and an access to it will cause MMU fault
> +         */
> +        li      t0, XEN_VIRT_START
> +        la      t1, start
> +        sub     t1, t1, t0

If I'm not mistaken this calculates start - XEN_VIRT_START, and ...

> +        /* Calculate proper VA after jump from 1:1 mapping */
> +        la      t0, .L_primary_switched
> +        sub     t0, t0, t1

... then this .L_primary_switched - (start - XEN_VIRT_START). Which can
also be expressed as (.L_primary_switched - start) + XEN_VIRT_START,
the first part of which gas ought to be able to resolve for you. But
upon experimenting a little it looks like it can't. (I had thought of
something along the lines of

        li      t0, .L_primary_switched - start
        li      t1, XEN_VIRT_START
        add     t0, t0, t1

or even

        li      t1, XEN_VIRT_START
        add     t0, t1, %pcrel_lo(.L_primary_switched - start)

.)

Jan

Re: [PATCH v1 5/8] xen/riscv: introduce identity mapping

[Jan Beulich]

On 12.06.2023 15:48, Jan Beulich wrote:
> On 06.06.2023 21:55, Oleksii Kurochko wrote:
>> -void __init noreturn noinline enable_mmu()
>> +/*
>> + * enable_mmu() can't be __init because __init section isn't part of identity
>> + * mapping so it will cause an issue after MMU will be enabled.
>> + */
> 
> As hinted at above already - perhaps the identity mapping wants to be
> larger, up to covering the entire Xen image? Since it's temporary
> only anyway, you could even consider using a large page (and RWX
> permission). You already require no overlap of link and load addresses,
> so at least small page mappings ought to be possible for the entire
> image.

To expand on that: Assume a future change on this path results in a call
to memcpy() or memset() being introduced by the compiler (and then let's
further assume this only occurs for a specific compiler version). Right
now such a case would be noticed simply because we don't build those
library functions yet. But it'll likely be a perplexing crash once a full
hypervisor can be built, the more that exception handlers also aren't
mapped.

>> - mmu_is_enabled:
>>      /*
>> -     * Stack should be re-inited as:
>> -     * 1. Right now an address of the stack is relative to load time
>> -     *    addresses what will cause an issue in case of load start address
>> -     *    isn't equal to linker start address.
>> -     * 2. Addresses in stack are all load time relative which can be an
>> -     *    issue in case when load start address isn't equal to linker
>> -     *    start address.
>> -     *
>> -     * We can't return to the caller because the stack was reseted
>> -     * and it may have stash some variable on the stack.
>> -     * Jump to a brand new function as the stack was reseted
>> +     * id_addrs should be in sync with id mapping in
>> +     * setup_initial_pagetables()
> 
> What is "id" meant to stand for here? Also if things need keeping in
> sync, then a similar comment should exist on the other side.

I guess it's meant to stand for "identity mapping", but the common use
of "id" makes we wonder if the variable wouldn't better be ident_addrs[].

Jan

Re: [PATCH v1 7/8] xen/riscv: add __ASSEMBLY__ guards

[Alistair Francis]

On Wed, Jun 7, 2023 at 5:55 AM Oleksii Kurochko
<oleksii.kurochko@gmail.com> wrote:
>
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

Acked-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  xen/arch/riscv/include/asm/page.h | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/xen/arch/riscv/include/asm/page.h b/xen/arch/riscv/include/asm/page.h
> index 8e8ec9ee36..1c6add70a5 100644
> --- a/xen/arch/riscv/include/asm/page.h
> +++ b/xen/arch/riscv/include/asm/page.h
> @@ -3,6 +3,8 @@
>  #ifndef _ASM_RISCV_PAGE_H
>  #define _ASM_RISCV_PAGE_H
>
> +#ifndef __ASSEMBLY__
> +
>  #include <xen/const.h>
>  #include <xen/types.h>
>
> @@ -60,4 +62,6 @@ static inline bool pte_is_valid(pte_t p)
>      return p.pte & PTE_VALID;
>  }
>
> +#endif /* __ASSEMBLY__ */
> +
>  #endif /* _ASM_RISCV_PAGE_H */
> --
> 2.40.1
>
>

Re: [PATCH v1 1/8] xen/riscv: make sure that identity mapping isn't bigger then page size

[Oleksii]

On Mon, 2023-06-12 at 09:09 +0200, Jan Beulich wrote:
> On 06.06.2023 21:55, Oleksii Kurochko wrote:
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> 
> Such commits without description are worrying. This may be okay for
> entirely trivial and obvious changes, but that's going to be the
> exception.
> 
> > --- a/xen/arch/riscv/xen.lds.S
> > +++ b/xen/arch/riscv/xen.lds.S
> > @@ -20,6 +20,7 @@ SECTIONS
> >      . = XEN_VIRT_START;
> >      _start = .;
> >      .text : {
> > +        _idmap_start = .;
> >          _stext = .;            /* Text section */
> >          *(.text.header)
> >  
> > @@ -35,6 +36,7 @@ SECTIONS
> >          *(.gnu.warning)
> >          . = ALIGN(POINTER_ALIGN);
> >          _etext = .;             /* End of text section */
> > +        _idmap_end = .;
> >      } :text
> 
> So this covers all of .text. Why is it expected that .text will be
> (and
> remain) ...
> 
> > @@ -174,3 +176,10 @@ ASSERT(!SIZEOF(.got),      ".got non-empty")
> >  ASSERT(!SIZEOF(.got.plt),  ".got.plt non-empty")
> >  
> >  ASSERT(_end - _start <= MB(2), "Xen too large for early-boot
> > assumptions")
> > +
> > +/*
> > + * We require that Xen is loaded at a page boundary, so this
> > ensures that any
> > + * code running on the identity map cannot cross a page boundary.
> > + */
> > +ASSERT(IS_ALIGNED(_idmap_start, PAGE_SIZE), "_idmap_start should
> > be page-aligned")
> > +ASSERT(_idmap_end - _idmap_start <= PAGE_SIZE, "Identity mapped
> > code is larger than a page size")
> 
> ... less than 4k in size? And why is only .text of interest, but not
> other sections?
An idea was to keep identity mapping as small as possible because
basically identity mapping is needed only for a few instructions.
(probably it will be better to create a separate section and put all
necessary functions there)

Another point was to map the necessary code for switching from 1:1
mapping in one cycle. ( we are using 4K as a page size )

But it looks like PAGE_SIZE isn't enough. I rebased all my patches that
are needed to run Dom0 and compiler complains that _idmap is bigger
than PAGE_SIZE so I probably have to reject this idea ( to map only
PAGE_SIZE ).

Actually not only .text section is needed but also stack should be 1:1
mapped. ( what is done in setup_initial_pagetables() )
> 
> I find the other assertion a little puzzling too: Isn't that merely
> checking that XEN_VIRT_START is page aligned?
Yeah, you are right.

~ Oleksii

Re: [PATCH v1 2/8] xen/riscv: add .sbss section to .bss

[Oleksii]

On Mon, 2023-06-12 at 09:04 +0200, Jan Beulich wrote:
> On 06.06.2023 21:55, Oleksii Kurochko wrote:
> > Sometimes variables are located in .sbss section but it won't
> > be mapped after MMU will be enabled.
> > To avoid MMU failures .sbss should be mapped
> > 
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> > ---
> >  xen/arch/riscv/xen.lds.S | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/xen/arch/riscv/xen.lds.S b/xen/arch/riscv/xen.lds.S
> > index 74afbaab9b..9a2799bab5 100644
> > --- a/xen/arch/riscv/xen.lds.S
> > +++ b/xen/arch/riscv/xen.lds.S
> > @@ -151,7 +151,7 @@ SECTIONS
> >          *(.bss.percpu.read_mostly)
> >          . = ALIGN(SMP_CACHE_BYTES);
> >          __per_cpu_data_end = .;
> > -        *(.bss .bss.*)
> > +        *(.bss .bss.* .sbss)
> >          . = ALIGN(POINTER_ALIGN);
> >          __bss_end = .;
> >      } :text
> 
> Two remarks, despite Alistair's ack: Wouldn't it be better to add
> .sbss.*
> right away as well? And strictly speaking wouldn't it be more logical
> to
> have .sbss ahead of .bss?
It makes sense. I'll take it into account in the following patch
series.
Thanks.
> 
> Jan

Re: [PATCH v1 3/8] xen/riscv: introduce reset_stack() function

[Oleksii]

On Mon, 2023-06-12 at 09:12 +0200, Jan Beulich wrote:
> On 06.06.2023 21:55, Oleksii Kurochko wrote:
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> 
> This wants addressing the "Why?" aspect in the description. Is the
> present
> code wrong in some perhaps subtle way? Are you meaning to re-use the
> code?
I am re-using it after switching from 1:1 world to reset a stack.

> If so, in which way (which is relevant to determine whether the new
> function may actually continue to live in .text.header)?
Actually there is no such requirement for reset_stack to live in
.text.header.

> > --- a/xen/arch/riscv/riscv64/head.S
> > +++ b/xen/arch/riscv/riscv64/head.S
> > @@ -27,8 +27,14 @@ ENTRY(start)
> >          add     t3, t3, __SIZEOF_POINTER__
> >          bltu    t3, t4, .L_clear_bss
> >  
> > +        jal     reset_stack
> > +
> > +        tail    start_xen
> > +
> > +ENTRY(reset_stack)
> >          la      sp, cpu0_boot_stack
> >          li      t0, STACK_SIZE
> >          add     sp, sp, t0
> >  
> > -        tail    start_xen
> > +        ret
> > +
> 

~ Oleksii

Re: [PATCH v1 4/8] xen/riscv: introduce function for physical offset calculation

[Oleksii]

On Mon, 2023-06-12 at 09:15 +0200, Jan Beulich wrote:
> On 06.06.2023 21:55, Oleksii Kurochko wrote:
> > The function was introduced to not calculate and save physical
> > offset before MMU is enabled because access to start() is
> > PC-relative and in case of linker_addr != load_addr it will result
> > in incorrect value in phys_offset.
> 
> "... to _not_ calculate ..."?
_not_ should be dropped. Thanks.

> 
> > --- a/xen/arch/riscv/mm.c
> > +++ b/xen/arch/riscv/mm.c
> > @@ -19,9 +19,11 @@ struct mmu_desc {
> >  
> >  extern unsigned char cpu0_boot_stack[STACK_SIZE];
> >  
> > -#define PHYS_OFFSET ((unsigned long)_start - XEN_VIRT_START)
> > -#define LOAD_TO_LINK(addr) ((addr) - PHYS_OFFSET)
> > -#define LINK_TO_LOAD(addr) ((addr) + PHYS_OFFSET)
> > +static unsigned long phys_offset;
> 
> __ro_after_init?
It makes sense to mark variable as __ro_after_init. I'll take into
account in new version of patch.

> 
> > +#define LOAD_TO_LINK(addr) ((unsigned long)(addr) - phys_offset)
> > +#define LINK_TO_LOAD(addr) ((unsigned long)(addr) + phys_offset)
> > +
> >  
> 
> Nit: No double blank lines please.
Sorry. Missed that.

> 
> > @@ -273,3 +275,13 @@ void __init noreturn noinline enable_mmu()
> >      switch_stack_and_jump((unsigned long)cpu0_boot_stack +
> > STACK_SIZE,
> >                            cont_after_mmu_is_enabled);
> >  }
> > +
> > +/*
> > + * calc_phys_offset() should be used before MMU is enabled because
> > access to
> > + * start() is PC-relative and in case when load_addr !=
> > linker_addr phys_offset
> > + * will have an incorrect value
> > + */
> > +void  calc_phys_offset(void)
> 
> __init? And nit: No double blanks please.
Thanks. It should be __init. I'll remove double blanks in new patch
version.

~ Oleksii

Re: [PATCH v1 5/8] xen/riscv: introduce identity mapping

[Oleksii]

On Mon, 2023-06-12 at 15:48 +0200, Jan Beulich wrote:
> On 06.06.2023 21:55, Oleksii Kurochko wrote:
> > The way how switch to virtual address was implemented in the
> > commit e66003e7be ("xen/riscv: introduce setup_initial_pages")
> > wasn't safe enough so identity mapping was introduced and
> > used.
> 
> I don't think this is sufficient as a description. You want to make
> clear what the "not safe enough" is, and you also want to go into
> more detail as to the solution chosen. I'm particularly puzzled that
> you map just two singular pages ...
I'll update a descrption.

I map two pages as it likely to be enough to switch from 1:1 mapping
world. My point is that we need 1:1 mapping only for few instructions
which are located in start() [ in .text.header section ]:

        li      t0, XEN_VIRT_START
        la      t1, start
        sub     t1, t1, t0

        /* Calculate proper VA after jump from 1:1 mapping */
        la      t0, .L_primary_switched
        sub     t0, t0, t1

        /* Jump from 1:1 mapping world */
        jr      t0

And it is needed to map 1:1 cpu0_boot_stack to not to fault when
executing epilog of enable_mmu() function as it accesses a value on the
stack:
    ffffffffc00001b0:       6422                    ld      s0,8(sp)
    ffffffffc00001b2:       0141                    addi    sp,sp,16
    ffffffffc00001b4:       8082                    ret

> 
> > @@ -35,8 +40,10 @@ static unsigned long phys_offset;
> >   *
> >   * It might be needed one more page table in case when Xen load
> > address
> >   * isn't 2 MB aligned.
> > + *
> > + * 3 additional page tables are needed for identity mapping.
> >   */
> > -#define PGTBL_INITIAL_COUNT ((CONFIG_PAGING_LEVELS - 1) + 1)
> > +#define PGTBL_INITIAL_COUNT ((CONFIG_PAGING_LEVELS - 1) + 1 + 3)
> 
> What is this 3 coming from? It feels like the value should (again)
> somehow depend on CONFIG_PAGING_LEVELS.
3 is too much in the current case. It should be 2 more.

The linker address is 0xFFFFFFFC00000000 thereby mapping the linker to
load addresses
we need 3 pages ( with the condition that the size of Xen won't be
larger than 2 MB ) and 1 page if the case when Xen load address isn't
2MV aligned.

We need 2 more page tables because Xen is loaded to address 0x80200000
by OpenSBI and the load address isn't equal to the linker address so we
need additional 2 pages as the L2 table we already allocated when
mapping the linker to load addresses.

And it looks like 2 is enough for Sv48, Sv57 as L4, L3 and L2
pagetables will be already allocated during mapping the linker to load
addresses.

And it only will be too much for Sv32 ( which has only L1, L0 in
general ) but I am not sure that anyone cares about Sv32.

> 
> > @@ -108,16 +116,18 @@ static void __init
> > setup_initial_mapping(struct mmu_desc *mmu_desc,
> >              {
> >                  unsigned long paddr = (page_addr - map_start) +
> > pa_start;
> >                  unsigned int permissions = PTE_LEAF_DEFAULT;
> > +                unsigned long addr = (is_identity_mapping) ?
> 
> Nit: No need for parentheses here.
Thanks.

> 
> > +                                     page_addr :
> > LINK_TO_LOAD(page_addr);
> 
> As a remark, while we want binary operators at the end of lines when
> wrapping, we usually do things differently for the ternary operator:
> Either
> 
>                 unsigned long addr = is_identity_mapping
>                                      ? page_addr :
> LINK_TO_LOAD(page_addr);
> 
> or
> 
>                 unsigned long addr = is_identity_mapping
>                                      ? page_addr
>                                      : LINK_TO_LOAD(page_addr);
It looks better. Thanks.
> 
> .
> 
> > @@ -232,22 +242,27 @@ void __init setup_initial_pagetables(void)
> >                            linker_start,
> >                            linker_end,
> >                            load_start);
> > +
> > +    if ( linker_start == load_start )
> > +        return;
> > +
> > +    setup_initial_mapping(&mmu_desc,
> > +                          load_start,
> > +                          load_start + PAGE_SIZE,
> > +                          load_start);
> > +
> > +    setup_initial_mapping(&mmu_desc,
> > +                          (unsigned long)cpu0_boot_stack,
> > +                          (unsigned long)cpu0_boot_stack +
> > PAGE_SIZE,
> 
> Shouldn't this be STACK_SIZE (and then also be prepared for
> STACK_SIZE > PAGE_SIZE)?
Yes, it will be better to use STACK_SIZE but for 1:1 mapping it will be
enough PAGE_SIZE as I mentioned above this only need for epilogue of
enable_mmu() function.
> 
> > +                          (unsigned long)cpu0_boot_stack);
> >  }
> >  
> > -void __init noreturn noinline enable_mmu()
> > +/*
> > + * enable_mmu() can't be __init because __init section isn't part
> > of identity
> > + * mapping so it will cause an issue after MMU will be enabled.
> > + */
> 
> As hinted at above already - perhaps the identity mapping wants to be
> larger, up to covering the entire Xen image? Since it's temporary
> only anyway, you could even consider using a large page (and RWX
> permission). You already require no overlap of link and load
> addresses,
> so at least small page mappings ought to be possible for the entire
> image.
It makes sense and started to thought about that after I applied the
patch for Dom0 running... I think we can map 1 GB page which will cover
the whole Xen image. Also in that case we haven't to allocate 2 more
page tables.

> 
> > @@ -255,25 +270,41 @@ void __init noreturn noinline enable_mmu()
> >      csr_write(CSR_SATP,
> >                PFN_DOWN((unsigned long)stage1_pgtbl_root) |
> >                RV_STAGE1_MODE << SATP_MODE_SHIFT);
> > +}
> > +
> > +void __init remove_identity_mapping(void)
> > +{
> > +    int i, j;
> 
> Nit: unsigned int please.
Thanks.

> 
> > +    pte_t *pgtbl;
> > +    unsigned int index, xen_index;
> 
> These would all probably better be declared in the narrowest possible
> scope.
Sure.

> 
> > -    asm volatile ( ".p2align 2" );
> > - mmu_is_enabled:
> >      /*
> > -     * Stack should be re-inited as:
> > -     * 1. Right now an address of the stack is relative to load
> > time
> > -     *    addresses what will cause an issue in case of load start
> > address
> > -     *    isn't equal to linker start address.
> > -     * 2. Addresses in stack are all load time relative which can
> > be an
> > -     *    issue in case when load start address isn't equal to
> > linker
> > -     *    start address.
> > -     *
> > -     * We can't return to the caller because the stack was reseted
> > -     * and it may have stash some variable on the stack.
> > -     * Jump to a brand new function as the stack was reseted
> > +     * id_addrs should be in sync with id mapping in
> > +     * setup_initial_pagetables()
> 
> What is "id" meant to stand for here? Also if things need keeping in
> sync, then a similar comment should exist on the other side.
"id" here mean identity.

> 
> >       */
> > +    unsigned long id_addrs[] =  {
> > +                                 LINK_TO_LOAD(_start),
> > +                                 LINK_TO_LOAD(cpu0_boot_stack),
> > +                                };
> >  
> > -    switch_stack_and_jump((unsigned long)cpu0_boot_stack +
> > STACK_SIZE,
> > -                          cont_after_mmu_is_enabled);
> > +    pgtbl = stage1_pgtbl_root;
> > +
> > +    for ( j = 0; j < ARRAY_SIZE(id_addrs); j++ )
> > +    {
> > +        for ( pgtbl = stage1_pgtbl_root, i = CONFIG_PAGING_LEVELS
> > - 1; i >= 0; i-- )
> > +        {
> > +            index = pt_index(i, id_addrs[j]);
> > +            xen_index = pt_index(i, XEN_VIRT_START);
> > +
> > +            if ( index != xen_index )
> > +            {
> > +                pgtbl[index].pte = 0;
> > +                break;
> > +            }
> 
> Up to here I understand index specifies a particular PTE within
> pgtbl[].
> 
> > +            pgtbl = &pgtbl[index];
> 
> But then how can this be the continuation of the page table walk?
> Don't
> you need to read the address out of the PTE?
You are right. it should be:
   pgtbl = (pte_t *)pte_to_paddr(pgtbl[index]); 

> 
> > --- a/xen/arch/riscv/riscv64/head.S
> > +++ b/xen/arch/riscv/riscv64/head.S
> > @@ -31,6 +31,36 @@ ENTRY(start)
> >  
> >          jal     calc_phys_offset
> >  
> > +        jal     setup_initial_pagetables
> > +
> > +        jal     enable_mmu
> > +
> > +        /*
> > +         * Calculate physical offset
> > +         *
> > +         * We can't re-use a value in phys_offset variable here as
> > +         * phys_offset is located in .bss and this section isn't
> > +         * 1:1 mapped and an access to it will cause MMU fault
> > +         */
> > +        li      t0, XEN_VIRT_START
> > +        la      t1, start
> > +        sub     t1, t1, t0
> 
> If I'm not mistaken this calculates start - XEN_VIRT_START, and ...
> 
> > +        /* Calculate proper VA after jump from 1:1 mapping */
> > +        la      t0, .L_primary_switched
> > +        sub     t0, t0, t1
> 
> ... then this .L_primary_switched - (start - XEN_VIRT_START). Which
> can
> also be expressed as (.L_primary_switched - start) + XEN_VIRT_START,
> the first part of which gas ought to be able to resolve for you. But
> upon experimenting a little it looks like it can't. (I had thought of
> something along the lines of
> 
>         li      t0, .L_primary_switched - start
>         li      t1, XEN_VIRT_START
>         add     t0, t0, t1
> 
> or even
> 
>         li      t1, XEN_VIRT_START
>         add     t0, t1, %pcrel_lo(.L_primary_switched - start)
> 
> .)
Calculation of ".L_primary_switched - start" might be an issue for gcc.
I tried to do something similar and recieved or "Error: can't resolve
.L_primary_switched - start" or "Error: illegal operands `li
t0,.L_primary_switched-start'"

~ Oleksii

Re: [PATCH v1 5/8] xen/riscv: introduce identity mapping

[Oleksii]

On Mon, 2023-06-12 at 16:24 +0200, Jan Beulich wrote:
> On 12.06.2023 15:48, Jan Beulich wrote:
> > On 06.06.2023 21:55, Oleksii Kurochko wrote:
> > > -void __init noreturn noinline enable_mmu()
> > > +/*
> > > + * enable_mmu() can't be __init because __init section isn't
> > > part of identity
> > > + * mapping so it will cause an issue after MMU will be enabled.
> > > + */
> > 
> > As hinted at above already - perhaps the identity mapping wants to
> > be
> > larger, up to covering the entire Xen image? Since it's temporary
> > only anyway, you could even consider using a large page (and RWX
> > permission). You already require no overlap of link and load
> > addresses,
> > so at least small page mappings ought to be possible for the entire
> > image.
> 
> To expand on that: Assume a future change on this path results in a
> call
> to memcpy() or memset() being introduced by the compiler (and then
> let's
> further assume this only occurs for a specific compiler version).
> Right
> now such a case would be noticed simply because we don't build those
> library functions yet. But it'll likely be a perplexing crash once a
> full
> hypervisor can be built, the more that exception handlers also aren't
> mapped.
It makes sense but for some reason it doesn't crash ( I have a bunch of
patches to run dom0 ) but as I mentioned in the previous e-mail I agree
that probably it would be better to map the whole image using 1 Gb page
table for example.


> 
> > > - mmu_is_enabled:
> > >      /*
> > > -     * Stack should be re-inited as:
> > > -     * 1. Right now an address of the stack is relative to load
> > > time
> > > -     *    addresses what will cause an issue in case of load
> > > start address
> > > -     *    isn't equal to linker start address.
> > > -     * 2. Addresses in stack are all load time relative which
> > > can be an
> > > -     *    issue in case when load start address isn't equal to
> > > linker
> > > -     *    start address.
> > > -     *
> > > -     * We can't return to the caller because the stack was
> > > reseted
> > > -     * and it may have stash some variable on the stack.
> > > -     * Jump to a brand new function as the stack was reseted
> > > +     * id_addrs should be in sync with id mapping in
> > > +     * setup_initial_pagetables()
> > 
> > What is "id" meant to stand for here? Also if things need keeping
> > in
> > sync, then a similar comment should exist on the other side.
> 
> I guess it's meant to stand for "identity mapping", but the common
> use
> of "id" makes we wonder if the variable wouldn't better be
> ident_addrs[].
It would be better. but probably we will remove that variable if we
agree to map the whole Xen instead of parts. So I'll wait for your
response before starting to work on new patch series.

Thanks a lot.

~ Oleksii

Re: [PATCH v1 5/8] xen/riscv: introduce identity mapping

[Jan Beulich]

On 14.06.2023 11:47, Oleksii wrote:
> On Mon, 2023-06-12 at 15:48 +0200, Jan Beulich wrote:
>> On 06.06.2023 21:55, Oleksii Kurochko wrote:
>>> The way how switch to virtual address was implemented in the
>>> commit e66003e7be ("xen/riscv: introduce setup_initial_pages")
>>> wasn't safe enough so identity mapping was introduced and
>>> used.
>>
>> I don't think this is sufficient as a description. You want to make
>> clear what the "not safe enough" is, and you also want to go into
>> more detail as to the solution chosen. I'm particularly puzzled that
>> you map just two singular pages ...
> I'll update a descrption.
> 
> I map two pages as it likely to be enough to switch from 1:1 mapping

I dislike your use of "likely" here: Unless this code is meant to be
redone anyway, it should just work. Everywhere.

> world. My point is that we need 1:1 mapping only for few instructions
> which are located in start() [ in .text.header section ]:
> 
>         li      t0, XEN_VIRT_START
>         la      t1, start
>         sub     t1, t1, t0
> 
>         /* Calculate proper VA after jump from 1:1 mapping */
>         la      t0, .L_primary_switched
>         sub     t0, t0, t1
> 
>         /* Jump from 1:1 mapping world */
>         jr      t0
> 
> And it is needed to map 1:1 cpu0_boot_stack to not to fault when
> executing epilog of enable_mmu() function as it accesses a value on the
> stack:
>     ffffffffc00001b0:       6422                    ld      s0,8(sp)
>     ffffffffc00001b2:       0141                    addi    sp,sp,16
>     ffffffffc00001b4:       8082                    ret
> 
>>
>>> @@ -35,8 +40,10 @@ static unsigned long phys_offset;
>>>   *
>>>   * It might be needed one more page table in case when Xen load
>>> address
>>>   * isn't 2 MB aligned.
>>> + *
>>> + * 3 additional page tables are needed for identity mapping.
>>>   */
>>> -#define PGTBL_INITIAL_COUNT ((CONFIG_PAGING_LEVELS - 1) + 1)
>>> +#define PGTBL_INITIAL_COUNT ((CONFIG_PAGING_LEVELS - 1) + 1 + 3)
>>
>> What is this 3 coming from? It feels like the value should (again)
>> somehow depend on CONFIG_PAGING_LEVELS.
> 3 is too much in the current case. It should be 2 more.
> 
> The linker address is 0xFFFFFFFC00000000 thereby mapping the linker to
> load addresses
> we need 3 pages ( with the condition that the size of Xen won't be
> larger than 2 MB ) and 1 page if the case when Xen load address isn't
> 2MV aligned.
> 
> We need 2 more page tables because Xen is loaded to address 0x80200000
> by OpenSBI and the load address isn't equal to the linker address so we
> need additional 2 pages as the L2 table we already allocated when
> mapping the linker to load addresses.
> 
> And it looks like 2 is enough for Sv48, Sv57 as L4, L3 and L2
> pagetables will be already allocated during mapping the linker to load
> addresses.

I agree the root table will be there. But depending on load address, I
don't think you can rely on any other tables to be re-usable from the
Xen mappings you already have. So my gut feeling would be

#define PGTBL_INITIAL_COUNT ((CONFIG_PAGING_LEVELS - 1) * 2 + 1)

>>> @@ -232,22 +242,27 @@ void __init setup_initial_pagetables(void)
>>>                            linker_start,
>>>                            linker_end,
>>>                            load_start);
>>> +
>>> +    if ( linker_start == load_start )
>>> +        return;
>>> +
>>> +    setup_initial_mapping(&mmu_desc,
>>> +                          load_start,
>>> +                          load_start + PAGE_SIZE,
>>> +                          load_start);
>>> +
>>> +    setup_initial_mapping(&mmu_desc,
>>> +                          (unsigned long)cpu0_boot_stack,
>>> +                          (unsigned long)cpu0_boot_stack +
>>> PAGE_SIZE,
>>
>> Shouldn't this be STACK_SIZE (and then also be prepared for
>> STACK_SIZE > PAGE_SIZE)?
> Yes, it will be better to use STACK_SIZE but for 1:1 mapping it will be
> enough PAGE_SIZE as I mentioned above this only need for epilogue of
> enable_mmu() function.

But then it should still be the correct page of the stack that you
map (the top one aiui).

>>> -void __init noreturn noinline enable_mmu()
>>> +/*
>>> + * enable_mmu() can't be __init because __init section isn't part
>>> of identity
>>> + * mapping so it will cause an issue after MMU will be enabled.
>>> + */
>>
>> As hinted at above already - perhaps the identity mapping wants to be
>> larger, up to covering the entire Xen image? Since it's temporary
>> only anyway, you could even consider using a large page (and RWX
>> permission). You already require no overlap of link and load
>> addresses,
>> so at least small page mappings ought to be possible for the entire
>> image.
> It makes sense and started to thought about that after I applied the
> patch for Dom0 running... I think we can map 1 GB page which will cover
> the whole Xen image. Also in that case we haven't to allocate 2 more
> page tables.

But you then need to be careful about not mapping space accesses to
which may have side effects (i.e. non-RAM, which might be MMIO or
holes). Otherwise speculative execution might cause surprises,
unless such is excluded by other guarantees (of which I don't know).

>>> --- a/xen/arch/riscv/riscv64/head.S
>>> +++ b/xen/arch/riscv/riscv64/head.S
>>> @@ -31,6 +31,36 @@ ENTRY(start)
>>>  
>>>          jal     calc_phys_offset
>>>  
>>> +        jal     setup_initial_pagetables
>>> +
>>> +        jal     enable_mmu
>>> +
>>> +        /*
>>> +         * Calculate physical offset
>>> +         *
>>> +         * We can't re-use a value in phys_offset variable here as
>>> +         * phys_offset is located in .bss and this section isn't
>>> +         * 1:1 mapped and an access to it will cause MMU fault
>>> +         */
>>> +        li      t0, XEN_VIRT_START
>>> +        la      t1, start
>>> +        sub     t1, t1, t0
>>
>> If I'm not mistaken this calculates start - XEN_VIRT_START, and ...
>>
>>> +        /* Calculate proper VA after jump from 1:1 mapping */
>>> +        la      t0, .L_primary_switched
>>> +        sub     t0, t0, t1
>>
>> ... then this .L_primary_switched - (start - XEN_VIRT_START). Which
>> can
>> also be expressed as (.L_primary_switched - start) + XEN_VIRT_START,
>> the first part of which gas ought to be able to resolve for you. But
>> upon experimenting a little it looks like it can't. (I had thought of
>> something along the lines of
>>
>>         li      t0, .L_primary_switched - start
>>         li      t1, XEN_VIRT_START
>>         add     t0, t0, t1
>>
>> or even
>>
>>         li      t1, XEN_VIRT_START
>>         add     t0, t1, %pcrel_lo(.L_primary_switched - start)
>>
>> .)
> Calculation of ".L_primary_switched - start" might be an issue for gcc.
> I tried to do something similar and recieved or "Error: can't resolve
> .L_primary_switched - start" or "Error: illegal operands `li
> t0,.L_primary_switched-start'"

Matches the results of my experiments. If I can find time, I may want
to look into why exactly gas is rejecting such.

Jan

Re: [PATCH v1 5/8] xen/riscv: introduce identity mapping

[Oleksii]

> > +}
> > +
> > +void __init remove_identity_mapping(void)
> > +{
> > +    int i, j;
> 
> Nit: unsigned int please.
> 
> 
It should be int in the current case because of the 'for' exit
condition:
      for ( pgtbl = stage1_pgtbl_root, i = CONFIG_PAGING_LEVELS - 1; i
>= 0; i-- )

Should exit condition be re-writen?

~ Oleksii

Re: [PATCH v1 5/8] xen/riscv: introduce identity mapping

[Jan Beulich]

On 14.06.2023 13:06, Oleksii wrote:
>>> +}
>>> +
>>> +void __init remove_identity_mapping(void)
>>> +{
>>> +    int i, j;
>>
>> Nit: unsigned int please.
>>
>>
> It should be int in the current case because of the 'for' exit
> condition:
>       for ( pgtbl = stage1_pgtbl_root, i = CONFIG_PAGING_LEVELS - 1; i
>> = 0; i-- )
> 
> Should exit condition be re-writen?

Since it easily can be, I think that would be preferable. But in a case
like this, if you think it's better the way you have it, so be it (until
someone comes along and changes it).

Jan

Re: [PATCH v1 3/8] xen/riscv: introduce reset_stack() function

[Oleksii]

On Mon, 2023-06-12 at 09:12 +0200, Jan Beulich wrote:
> On 06.06.2023 21:55, Oleksii Kurochko wrote:
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> 
> This wants addressing the "Why?" aspect in the description. Is the
> present
> code wrong in some perhaps subtle way? Are you meaning to re-use the
> code?
> If so, in which way (which is relevant to determine whether the new
> function may actually continue to live in .text.header)?
> 
As I mentioned in previous e-mail there is no such requirement for
reset_stack() function to live in .text.header.

I think such requirement exists only for _start() function as we would
like to see it at the start of image as a bootloader jumps to it and it
is expected to be the first instructions.

Even though I don't see any issue for reset_stack() to live in
.text.header section, should it be moved to .text section? If yes, I
would appreciate hearing why it would be better.

~ Oleksii

Re: [PATCH v1 3/8] xen/riscv: introduce reset_stack() function

[Jan Beulich]

On 14.06.2023 14:19, Oleksii wrote:
> On Mon, 2023-06-12 at 09:12 +0200, Jan Beulich wrote:
>> On 06.06.2023 21:55, Oleksii Kurochko wrote:
>>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>>
>> This wants addressing the "Why?" aspect in the description. Is the
>> present
>> code wrong in some perhaps subtle way? Are you meaning to re-use the
>> code?
>> If so, in which way (which is relevant to determine whether the new
>> function may actually continue to live in .text.header)?
>>
> As I mentioned in previous e-mail there is no such requirement for
> reset_stack() function to live in .text.header.
> 
> I think such requirement exists only for _start() function as we would
> like to see it at the start of image as a bootloader jumps to it and it
> is expected to be the first instructions.
> 
> Even though I don't see any issue for reset_stack() to live in
> .text.header section, should it be moved to .text section? If yes, I
> would appreciate hearing why it would be better.

Because of the simple principle of special sections better only holding
what really needs to be there.

Jan