Introduce a function to set up fixmap mappings and L0 page
table for fixmap.
Additionally, defines were introduced in riscv/config.h to
calculate the FIXMAP_BASE address.
This involved introducing BOOT_FDT_VIRT_{START, SIZE} and
XEN_SIZE, XEN_VIRT_END.
Also, the check of Xen size was updated in the riscv/lds.S
script to use XEN_SIZE instead of a hardcoded constant.
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V2:
- newly introduced patch
---
xen/arch/riscv/include/asm/config.h | 9 ++++++
xen/arch/riscv/include/asm/fixmap.h | 48 +++++++++++++++++++++++++++++
xen/arch/riscv/include/asm/mm.h | 2 ++
xen/arch/riscv/include/asm/page.h | 7 +++++
xen/arch/riscv/mm.c | 35 +++++++++++++++++++++
xen/arch/riscv/setup.c | 2 ++
xen/arch/riscv/xen.lds.S | 2 +-
7 files changed, 104 insertions(+), 1 deletion(-)
create mode 100644 xen/arch/riscv/include/asm/fixmap.h
diff --git a/xen/arch/riscv/include/asm/config.h b/xen/arch/riscv/include/asm/config.h
index 50583aafdc..3275477c17 100644
--- a/xen/arch/riscv/include/asm/config.h
+++ b/xen/arch/riscv/include/asm/config.h
@@ -74,11 +74,20 @@
#error "unsupported RV_STAGE1_MODE"
#endif
+#define XEN_SIZE MB(2)
+#define XEN_VIRT_END (XEN_VIRT_START + XEN_SIZE)
+
+#define BOOT_FDT_VIRT_START XEN_VIRT_END
+#define BOOT_FDT_VIRT_SIZE MB(4)
+
#define DIRECTMAP_SLOT_END 509
#define DIRECTMAP_SLOT_START 200
#define DIRECTMAP_VIRT_START SLOTN(DIRECTMAP_SLOT_START)
#define DIRECTMAP_SIZE (SLOTN(DIRECTMAP_SLOT_END) - SLOTN(DIRECTMAP_SLOT_START))
+#define FIXMAP_BASE (BOOT_FDT_VIRT_START + BOOT_FDT_VIRT_SIZE)
+#define FIXMAP_ADDR(n) (FIXMAP_BASE + (n) * PAGE_SIZE)
+
#define FRAMETABLE_SCALE_FACTOR (PAGE_SIZE/sizeof(struct page_info))
#define FRAMETABLE_SIZE_IN_SLOTS (((DIRECTMAP_SIZE / SLOTN(1)) / FRAMETABLE_SCALE_FACTOR) + 1)
diff --git a/xen/arch/riscv/include/asm/fixmap.h b/xen/arch/riscv/include/asm/fixmap.h
new file mode 100644
index 0000000000..fcfb82d69c
--- /dev/null
+++ b/xen/arch/riscv/include/asm/fixmap.h
@@ -0,0 +1,48 @@
+/*
+ * fixmap.h: compile-time virtual memory allocation
+ */
+#ifndef __ASM_FIXMAP_H
+#define __ASM_FIXMAP_H
+
+#include <xen/bug.h>
+#include <xen/page-size.h>
+#include <xen/pmap.h>
+
+#include <asm/page.h>
+
+/* Fixmap slots */
+#define FIX_PMAP_BEGIN (0) /* Start of PMAP */
+#define FIX_PMAP_END (FIX_PMAP_BEGIN + NUM_FIX_PMAP - 1) /* End of PMAP */
+#define FIX_MISC (FIX_PMAP_END + 1) /* Ephemeral mappings of hardware */
+
+#define FIX_LAST FIX_MISC
+
+#define FIXADDR_START FIXMAP_ADDR(0)
+#define FIXADDR_TOP FIXMAP_ADDR(FIX_LAST)
+
+#ifndef __ASSEMBLY__
+
+/*
+ * Direct access to xen_fixmap[] should only happen when {set,
+ * clear}_fixmap() is unusable (e.g. where we would end up to
+ * recursively call the helpers).
+ */
+extern pte_t xen_fixmap[];
+
+/* Map a page in a fixmap entry */
+extern void set_fixmap(unsigned int map, mfn_t mfn, unsigned int attributes);
+/* Remove a mapping from a fixmap entry */
+extern void clear_fixmap(unsigned int map);
+
+#define fix_to_virt(slot) ((void *)FIXMAP_ADDR(slot))
+
+static inline unsigned int virt_to_fix(vaddr_t vaddr)
+{
+ BUG_ON(vaddr >= FIXADDR_TOP || vaddr < FIXADDR_START);
+
+ return ((vaddr - FIXADDR_START) >> PAGE_SHIFT);
+}
+
+#endif /* __ASSEMBLY__ */
+
+#endif /* __ASM_FIXMAP_H */
diff --git a/xen/arch/riscv/include/asm/mm.h b/xen/arch/riscv/include/asm/mm.h
index 25af9e1aaa..a0bdc2bc3a 100644
--- a/xen/arch/riscv/include/asm/mm.h
+++ b/xen/arch/riscv/include/asm/mm.h
@@ -255,4 +255,6 @@ static inline unsigned int arch_get_dma_bitsize(void)
return 32; /* TODO */
}
+void setup_fixmap_mappings(void);
+
#endif /* _ASM_RISCV_MM_H */
diff --git a/xen/arch/riscv/include/asm/page.h b/xen/arch/riscv/include/asm/page.h
index c831e16417..cbbf3656d1 100644
--- a/xen/arch/riscv/include/asm/page.h
+++ b/xen/arch/riscv/include/asm/page.h
@@ -81,6 +81,13 @@ static inline void flush_page_to_ram(unsigned long mfn, bool sync_icache)
BUG_ON("unimplemented");
}
+/* Write a pagetable entry. */
+static inline void write_pte(pte_t *p, pte_t pte)
+{
+ *p = pte;
+ asm volatile ("sfence.vma");
+}
+
#endif /* __ASSEMBLY__ */
#endif /* _ASM_RISCV_PAGE_H */
diff --git a/xen/arch/riscv/mm.c b/xen/arch/riscv/mm.c
index 7d09e781bf..d69a174b5d 100644
--- a/xen/arch/riscv/mm.c
+++ b/xen/arch/riscv/mm.c
@@ -49,6 +49,9 @@ stage1_pgtbl_root[PAGETABLE_ENTRIES];
pte_t __section(".bss.page_aligned") __aligned(PAGE_SIZE)
stage1_pgtbl_nonroot[PGTBL_INITIAL_COUNT * PAGETABLE_ENTRIES];
+pte_t __section(".bss.page_aligned") __aligned(PAGE_SIZE)
+xen_fixmap[PAGETABLE_ENTRIES];
+
#define HANDLE_PGTBL(curr_lvl_num) \
index = pt_index(curr_lvl_num, page_addr); \
if ( pte_is_valid(pgtbl[index]) ) \
@@ -191,6 +194,38 @@ static bool __init check_pgtbl_mode_support(struct mmu_desc *mmu_desc,
return is_mode_supported;
}
+void __init setup_fixmap_mappings(void)
+{
+ pte_t *pte;
+ unsigned int i;
+
+ pte = &stage1_pgtbl_root[pt_index(HYP_PT_ROOT_LEVEL, FIXMAP_ADDR(0))];
+
+ for ( i = HYP_PT_ROOT_LEVEL - 1; i != 0; i-- )
+ {
+ BUG_ON(!pte_is_valid(*pte));
+
+ pte = (pte_t *)LOAD_TO_LINK(pte_to_paddr(*pte));
+ pte = &pte[pt_index(i, FIXMAP_ADDR(0))];
+ }
+
+ BUG_ON( pte_is_valid(*pte) );
+
+ if ( !pte_is_valid(*pte) )
+ {
+ pte_t tmp = paddr_to_pte(LINK_TO_LOAD((unsigned long)&xen_fixmap), PTE_TABLE);
+
+ write_pte(pte, tmp);
+
+ printk("(XEN) fixmap is mapped\n");
+ }
+
+ /*
+ * We only need the zeroeth table allocated, but not the PTEs set, because
+ * set_fixmap() will set them on the fly.
+ */
+}
+
/*
* setup_initial_pagetables:
*
diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
index 4defad68f4..13f0e8c77d 100644
--- a/xen/arch/riscv/setup.c
+++ b/xen/arch/riscv/setup.c
@@ -46,6 +46,8 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
test_macros_from_bug_h();
#endif
+ setup_fixmap_mappings();
+
printk("All set up\n");
for ( ;; )
diff --git a/xen/arch/riscv/xen.lds.S b/xen/arch/riscv/xen.lds.S
index 070b19d915..63b1dd7bb6 100644
--- a/xen/arch/riscv/xen.lds.S
+++ b/xen/arch/riscv/xen.lds.S
@@ -181,6 +181,6 @@ ASSERT(!SIZEOF(.got.plt), ".got.plt non-empty")
* Changing the size of Xen binary can require an update of
* PGTBL_INITIAL_COUNT.
*/
-ASSERT(_end - _start <= MB(2), "Xen too large for early-boot assumptions")
+ASSERT(_end - _start <= XEN_SIZE, "Xen too large for early-boot assumptions")
ASSERT(_ident_end - _ident_start <= IDENT_AREA_SIZE, "identity region is too big");
--
2.45.2
On 12.07.2024 18:22, Oleksii Kurochko wrote: > --- a/xen/arch/riscv/include/asm/config.h > +++ b/xen/arch/riscv/include/asm/config.h > @@ -74,11 +74,20 @@ > #error "unsupported RV_STAGE1_MODE" > #endif > > +#define XEN_SIZE MB(2) > +#define XEN_VIRT_END (XEN_VIRT_START + XEN_SIZE) > + > +#define BOOT_FDT_VIRT_START XEN_VIRT_END > +#define BOOT_FDT_VIRT_SIZE MB(4) > + > #define DIRECTMAP_SLOT_END 509 > #define DIRECTMAP_SLOT_START 200 > #define DIRECTMAP_VIRT_START SLOTN(DIRECTMAP_SLOT_START) > #define DIRECTMAP_SIZE (SLOTN(DIRECTMAP_SLOT_END) - SLOTN(DIRECTMAP_SLOT_START)) > > +#define FIXMAP_BASE (BOOT_FDT_VIRT_START + BOOT_FDT_VIRT_SIZE) > +#define FIXMAP_ADDR(n) (FIXMAP_BASE + (n) * PAGE_SIZE) Why exactly do you insert this here, and not adjacent to BOOT_FDT_VIRT_*, which it actually is adjacent with? Then ... > #define FRAMETABLE_SCALE_FACTOR (PAGE_SIZE/sizeof(struct page_info)) > #define FRAMETABLE_SIZE_IN_SLOTS (((DIRECTMAP_SIZE / SLOTN(1)) / FRAMETABLE_SCALE_FACTOR) + 1) ... this would also stay next to DIRECTMAP_*, which it uses. > --- a/xen/arch/riscv/include/asm/page.h > +++ b/xen/arch/riscv/include/asm/page.h > @@ -81,6 +81,13 @@ static inline void flush_page_to_ram(unsigned long mfn, bool sync_icache) > BUG_ON("unimplemented"); > } > > +/* Write a pagetable entry. */ > +static inline void write_pte(pte_t *p, pte_t pte) > +{ > + *p = pte; > + asm volatile ("sfence.vma"); When they don't have operands, asm()-s are volatile anyway (being explicit about this may still be desirable, yes). But: Can you get away without operands here? Don't you need a memory clobber for the fence to actually remain where it is needed? Also, nit (style): Blanks missing. > --- a/xen/arch/riscv/mm.c > +++ b/xen/arch/riscv/mm.c > @@ -49,6 +49,9 @@ stage1_pgtbl_root[PAGETABLE_ENTRIES]; > pte_t __section(".bss.page_aligned") __aligned(PAGE_SIZE) > stage1_pgtbl_nonroot[PGTBL_INITIAL_COUNT * PAGETABLE_ENTRIES]; > > +pte_t __section(".bss.page_aligned") __aligned(PAGE_SIZE) > +xen_fixmap[PAGETABLE_ENTRIES]; Any reason this cannot be static? Jan
On Mon, 2024-07-22 at 14:42 +0200, Jan Beulich wrote: > On 12.07.2024 18:22, Oleksii Kurochko wrote: > > --- a/xen/arch/riscv/include/asm/config.h > > +++ b/xen/arch/riscv/include/asm/config.h > > @@ -74,11 +74,20 @@ > > #error "unsupported RV_STAGE1_MODE" > > #endif > > > > +#define XEN_SIZE MB(2) > > +#define XEN_VIRT_END (XEN_VIRT_START + XEN_SIZE) > > + > > +#define BOOT_FDT_VIRT_START XEN_VIRT_END > > +#define BOOT_FDT_VIRT_SIZE MB(4) > > + > > #define DIRECTMAP_SLOT_END 509 > > #define DIRECTMAP_SLOT_START 200 > > #define DIRECTMAP_VIRT_START SLOTN(DIRECTMAP_SLOT_START) > > #define DIRECTMAP_SIZE (SLOTN(DIRECTMAP_SLOT_END) - > > SLOTN(DIRECTMAP_SLOT_START)) > > > > +#define FIXMAP_BASE (BOOT_FDT_VIRT_START + > > BOOT_FDT_VIRT_SIZE) > > +#define FIXMAP_ADDR(n) (FIXMAP_BASE + (n) * PAGE_SIZE) > > Why exactly do you insert this here, and not adjacent to > BOOT_FDT_VIRT_*, > which it actually is adjacent with? I tried to follow alphabetical order. > Then ... > > > #define FRAMETABLE_SCALE_FACTOR (PAGE_SIZE/sizeof(struct > > page_info)) > > #define FRAMETABLE_SIZE_IN_SLOTS (((DIRECTMAP_SIZE / SLOTN(1)) / > > FRAMETABLE_SCALE_FACTOR) + 1) > > ... this would also stay next to DIRECTMAP_*, which it uses. > > > --- a/xen/arch/riscv/include/asm/page.h > > +++ b/xen/arch/riscv/include/asm/page.h > > @@ -81,6 +81,13 @@ static inline void flush_page_to_ram(unsigned > > long mfn, bool sync_icache) > > BUG_ON("unimplemented"); > > } > > > > +/* Write a pagetable entry. */ > > +static inline void write_pte(pte_t *p, pte_t pte) > > +{ > > + *p = pte; > > + asm volatile ("sfence.vma"); > > When they don't have operands, asm()-s are volatile anyway (being > explicit > about this may still be desirable, yes). But: Can you get away > without > operands here? Don't you need a memory clobber for the fence to > actually > remain where it is needed? It should be "::memory" here. Thanks. > > Also, nit (style): Blanks missing. > > > --- a/xen/arch/riscv/mm.c > > +++ b/xen/arch/riscv/mm.c > > @@ -49,6 +49,9 @@ stage1_pgtbl_root[PAGETABLE_ENTRIES]; > > pte_t __section(".bss.page_aligned") __aligned(PAGE_SIZE) > > stage1_pgtbl_nonroot[PGTBL_INITIAL_COUNT * PAGETABLE_ENTRIES]; > > > > +pte_t __section(".bss.page_aligned") __aligned(PAGE_SIZE) > > +xen_fixmap[PAGETABLE_ENTRIES]; > > Any reason this cannot be static? It will be used by pmap: static inline void arch_pmap_map(unsigned int slot, mfn_t mfn) { pte_t *entry = &xen_fixmap[slot]; pte_t pte; ASSERT(!pte_is_valid(*entry)); pte = mfn_to_xen_entry(mfn, PAGE_HYPERVISOR_RW); pte.pte |= PTE_LEAF_DEFAULT; write_pte(entry, pte); } static inline void arch_pmap_unmap(unsigned int slot) { pte_t pte = {}; write_pte(&xen_fixmap[slot], pte); } ~ Oleksii
On 22.07.2024 16:36, Oleksii wrote: > On Mon, 2024-07-22 at 14:42 +0200, Jan Beulich wrote: >> On 12.07.2024 18:22, Oleksii Kurochko wrote: >>> --- a/xen/arch/riscv/include/asm/config.h >>> +++ b/xen/arch/riscv/include/asm/config.h >>> @@ -74,11 +74,20 @@ >>> #error "unsupported RV_STAGE1_MODE" >>> #endif >>> >>> +#define XEN_SIZE MB(2) >>> +#define XEN_VIRT_END (XEN_VIRT_START + XEN_SIZE) >>> + >>> +#define BOOT_FDT_VIRT_START XEN_VIRT_END >>> +#define BOOT_FDT_VIRT_SIZE MB(4) >>> + >>> #define DIRECTMAP_SLOT_END 509 >>> #define DIRECTMAP_SLOT_START 200 >>> #define DIRECTMAP_VIRT_START SLOTN(DIRECTMAP_SLOT_START) >>> #define DIRECTMAP_SIZE (SLOTN(DIRECTMAP_SLOT_END) - >>> SLOTN(DIRECTMAP_SLOT_START)) >>> >>> +#define FIXMAP_BASE (BOOT_FDT_VIRT_START + >>> BOOT_FDT_VIRT_SIZE) >>> +#define FIXMAP_ADDR(n) (FIXMAP_BASE + (n) * PAGE_SIZE) >> >> Why exactly do you insert this here, and not adjacent to >> BOOT_FDT_VIRT_*, >> which it actually is adjacent with? > I tried to follow alphabetical order. Oh, X before B (just making fun) ... Anyway, my take here is that sorting by address is going to be more helpful. >>> --- a/xen/arch/riscv/mm.c >>> +++ b/xen/arch/riscv/mm.c >>> @@ -49,6 +49,9 @@ stage1_pgtbl_root[PAGETABLE_ENTRIES]; >>> pte_t __section(".bss.page_aligned") __aligned(PAGE_SIZE) >>> stage1_pgtbl_nonroot[PGTBL_INITIAL_COUNT * PAGETABLE_ENTRIES]; >>> >>> +pte_t __section(".bss.page_aligned") __aligned(PAGE_SIZE) >>> +xen_fixmap[PAGETABLE_ENTRIES]; >> >> Any reason this cannot be static? > It will be used by pmap: > static inline void arch_pmap_map(unsigned int slot, mfn_t mfn) > { > pte_t *entry = &xen_fixmap[slot]; > pte_t pte; > > ASSERT(!pte_is_valid(*entry)); > > pte = mfn_to_xen_entry(mfn, PAGE_HYPERVISOR_RW); > pte.pte |= PTE_LEAF_DEFAULT; > write_pte(entry, pte); > } > > static inline void arch_pmap_unmap(unsigned int slot) > { > pte_t pte = {}; > > write_pte(&xen_fixmap[slot], pte); > } Yet as asked there - shouldn't that be set_fixmap() and clear_fixmap()? Jan
On Mon, 2024-07-22 at 17:25 +0200, Jan Beulich wrote: > On 22.07.2024 16:36, Oleksii wrote: > > On Mon, 2024-07-22 at 14:42 +0200, Jan Beulich wrote: > > > On 12.07.2024 18:22, Oleksii Kurochko wrote: > > > > --- a/xen/arch/riscv/include/asm/config.h > > > > +++ b/xen/arch/riscv/include/asm/config.h > > > > @@ -74,11 +74,20 @@ > > > > #error "unsupported RV_STAGE1_MODE" > > > > #endif > > > > > > > > +#define XEN_SIZE MB(2) > > > > +#define XEN_VIRT_END (XEN_VIRT_START + XEN_SIZE) > > > > + > > > > +#define BOOT_FDT_VIRT_START XEN_VIRT_END > > > > +#define BOOT_FDT_VIRT_SIZE MB(4) > > > > + > > > > #define DIRECTMAP_SLOT_END 509 > > > > #define DIRECTMAP_SLOT_START 200 > > > > #define DIRECTMAP_VIRT_START SLOTN(DIRECTMAP_SLOT_START) > > > > #define DIRECTMAP_SIZE (SLOTN(DIRECTMAP_SLOT_END) - > > > > SLOTN(DIRECTMAP_SLOT_START)) > > > > > > > > +#define FIXMAP_BASE (BOOT_FDT_VIRT_START + > > > > BOOT_FDT_VIRT_SIZE) > > > > +#define FIXMAP_ADDR(n) (FIXMAP_BASE + (n) * > > > > PAGE_SIZE) > > > > > > Why exactly do you insert this here, and not adjacent to > > > BOOT_FDT_VIRT_*, > > > which it actually is adjacent with? > > I tried to follow alphabetical order. > > Oh, X before B (just making fun) ... Anyway, my take here is that > sorting > by address is going to be more helpful. > > > > > --- a/xen/arch/riscv/mm.c > > > > +++ b/xen/arch/riscv/mm.c > > > > @@ -49,6 +49,9 @@ stage1_pgtbl_root[PAGETABLE_ENTRIES]; > > > > pte_t __section(".bss.page_aligned") __aligned(PAGE_SIZE) > > > > stage1_pgtbl_nonroot[PGTBL_INITIAL_COUNT * PAGETABLE_ENTRIES]; > > > > > > > > +pte_t __section(".bss.page_aligned") __aligned(PAGE_SIZE) > > > > +xen_fixmap[PAGETABLE_ENTRIES]; > > > > > > Any reason this cannot be static? > > It will be used by pmap: > > static inline void arch_pmap_map(unsigned int slot, mfn_t mfn) > > { > > pte_t *entry = &xen_fixmap[slot]; > > pte_t pte; > > > > ASSERT(!pte_is_valid(*entry)); > > > > pte = mfn_to_xen_entry(mfn, PAGE_HYPERVISOR_RW); > > pte.pte |= PTE_LEAF_DEFAULT; > > write_pte(entry, pte); > > } > > > > static inline void arch_pmap_unmap(unsigned int slot) > > { > > pte_t pte = {}; > > > > write_pte(&xen_fixmap[slot], pte); > > } > > Yet as asked there - shouldn't that be set_fixmap() and > clear_fixmap()? It should be, I'll rework that in the next patch version. ~ Oleksii
On Mon, 2024-07-22 at 19:04 +0200, oleksii.kurochko@gmail.com wrote: > On Mon, 2024-07-22 at 17:25 +0200, Jan Beulich wrote: > > On 22.07.2024 16:36, Oleksii wrote: > > > On Mon, 2024-07-22 at 14:42 +0200, Jan Beulich wrote: > > > > On 12.07.2024 18:22, Oleksii Kurochko wrote: > > > > > --- a/xen/arch/riscv/include/asm/config.h > > > > > +++ b/xen/arch/riscv/include/asm/config.h > > > > > @@ -74,11 +74,20 @@ > > > > > #error "unsupported RV_STAGE1_MODE" > > > > > #endif > > > > > > > > > > +#define XEN_SIZE MB(2) > > > > > +#define XEN_VIRT_END (XEN_VIRT_START + XEN_SIZE) > > > > > + > > > > > +#define BOOT_FDT_VIRT_START XEN_VIRT_END > > > > > +#define BOOT_FDT_VIRT_SIZE MB(4) > > > > > + > > > > > #define DIRECTMAP_SLOT_END 509 > > > > > #define DIRECTMAP_SLOT_START 200 > > > > > #define DIRECTMAP_VIRT_START SLOTN(DIRECTMAP_SLOT_START) > > > > > #define DIRECTMAP_SIZE (SLOTN(DIRECTMAP_SLOT_END) - > > > > > SLOTN(DIRECTMAP_SLOT_START)) > > > > > > > > > > +#define FIXMAP_BASE (BOOT_FDT_VIRT_START + > > > > > BOOT_FDT_VIRT_SIZE) > > > > > +#define FIXMAP_ADDR(n) (FIXMAP_BASE + (n) * > > > > > PAGE_SIZE) > > > > > > > > Why exactly do you insert this here, and not adjacent to > > > > BOOT_FDT_VIRT_*, > > > > which it actually is adjacent with? > > > I tried to follow alphabetical order. > > > > Oh, X before B (just making fun) ... Anyway, my take here is that > > sorting > > by address is going to be more helpful. > > > > > > > --- a/xen/arch/riscv/mm.c > > > > > +++ b/xen/arch/riscv/mm.c > > > > > @@ -49,6 +49,9 @@ stage1_pgtbl_root[PAGETABLE_ENTRIES]; > > > > > pte_t __section(".bss.page_aligned") __aligned(PAGE_SIZE) > > > > > stage1_pgtbl_nonroot[PGTBL_INITIAL_COUNT * > > > > > PAGETABLE_ENTRIES]; > > > > > > > > > > +pte_t __section(".bss.page_aligned") __aligned(PAGE_SIZE) > > > > > +xen_fixmap[PAGETABLE_ENTRIES]; > > > > > > > > Any reason this cannot be static? > > > It will be used by pmap: > > > static inline void arch_pmap_map(unsigned int slot, mfn_t mfn) > > > { > > > pte_t *entry = &xen_fixmap[slot]; > > > pte_t pte; > > > > > > ASSERT(!pte_is_valid(*entry)); > > > > > > pte = mfn_to_xen_entry(mfn, PAGE_HYPERVISOR_RW); > > > pte.pte |= PTE_LEAF_DEFAULT; > > > write_pte(entry, pte); > > > } > > > > > > static inline void arch_pmap_unmap(unsigned int slot) > > > { > > > pte_t pte = {}; > > > > > > write_pte(&xen_fixmap[slot], pte); > > > } > > > > Yet as asked there - shouldn't that be set_fixmap() and > > clear_fixmap()? > It should be, I'll rework that in the next patch version. It couldn't be set_fixmap() and clear_fixmap() as I am going to implement them using map_pages_to_xen() because: ... /* * We cannot use set_fixmap() here. We use PMAP when the domain map * page infrastructure is not yet initialized, so map_pages_to_xen() called * by set_fixmap() needs to map pages on demand, which then calls pmap() * again, resulting in a loop. Modify the PTEs directly instead. The same * is true for pmap_unmap(). */ arch_pmap_map(slot, mfn); ... ~ Oleksii
Hi Oleksii, On 12/07/2024 17:22, Oleksii Kurochko wrote: > Introduce a function to set up fixmap mappings and L0 page > table for fixmap. > > Additionally, defines were introduced in riscv/config.h to > calculate the FIXMAP_BASE address. > This involved introducing BOOT_FDT_VIRT_{START, SIZE} and > XEN_SIZE, XEN_VIRT_END. > > Also, the check of Xen size was updated in the riscv/lds.S > script to use XEN_SIZE instead of a hardcoded constant. > > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> > --- > Changes in V2: > - newly introduced patch > --- > xen/arch/riscv/include/asm/config.h | 9 ++++++ > xen/arch/riscv/include/asm/fixmap.h | 48 +++++++++++++++++++++++++++++ > xen/arch/riscv/include/asm/mm.h | 2 ++ > xen/arch/riscv/include/asm/page.h | 7 +++++ > xen/arch/riscv/mm.c | 35 +++++++++++++++++++++ > xen/arch/riscv/setup.c | 2 ++ > xen/arch/riscv/xen.lds.S | 2 +- > 7 files changed, 104 insertions(+), 1 deletion(-) > create mode 100644 xen/arch/riscv/include/asm/fixmap.h > > diff --git a/xen/arch/riscv/include/asm/config.h b/xen/arch/riscv/include/asm/config.h > index 50583aafdc..3275477c17 100644 > --- a/xen/arch/riscv/include/asm/config.h > +++ b/xen/arch/riscv/include/asm/config.h > @@ -74,11 +74,20 @@ > #error "unsupported RV_STAGE1_MODE" > #endif > > +#define XEN_SIZE MB(2) NIT: I would name it XEN_VIRT_SIZE to be consistent with the start/end. > +#define XEN_VIRT_END (XEN_VIRT_START + XEN_SIZE) Can we get away with not introducing *_END and just use START, SIZE? The reason I am asking is with "end" it is never clear whether it is inclusive or exclusive. For instance, here you use an exclusive range but ... > + > +#define BOOT_FDT_VIRT_START XEN_VIRT_END > +#define BOOT_FDT_VIRT_SIZE MB(4) > + > #define DIRECTMAP_SLOT_END 509 > #define DIRECTMAP_SLOT_START 200 > #define DIRECTMAP_VIRT_START SLOTN(DIRECTMAP_SLOT_START) > #define DIRECTMAP_SIZE (SLOTN(DIRECTMAP_SLOT_END) - SLOTN(DIRECTMAP_SLOT_START)) > > +#define FIXMAP_BASE (BOOT_FDT_VIRT_START + BOOT_FDT_VIRT_SIZE) > +#define FIXMAP_ADDR(n) (FIXMAP_BASE + (n) * PAGE_SIZE) > + > #define FRAMETABLE_SCALE_FACTOR (PAGE_SIZE/sizeof(struct page_info)) > #define FRAMETABLE_SIZE_IN_SLOTS (((DIRECTMAP_SIZE / SLOTN(1)) / FRAMETABLE_SCALE_FACTOR) + 1) > > diff --git a/xen/arch/riscv/include/asm/fixmap.h b/xen/arch/riscv/include/asm/fixmap.h > new file mode 100644 > index 0000000000..fcfb82d69c > --- /dev/null > +++ b/xen/arch/riscv/include/asm/fixmap.h > @@ -0,0 +1,48 @@ > +/* > + * fixmap.h: compile-time virtual memory allocation > + */ > +#ifndef __ASM_FIXMAP_H > +#define __ASM_FIXMAP_H > + > +#include <xen/bug.h> > +#include <xen/page-size.h> > +#include <xen/pmap.h> > + > +#include <asm/page.h> > + > +/* Fixmap slots */ > +#define FIX_PMAP_BEGIN (0) /* Start of PMAP */ > +#define FIX_PMAP_END (FIX_PMAP_BEGIN + NUM_FIX_PMAP - 1) /* End of PMAP */ ... here is seems to be inclusive. Furthermore if you had 32-bit address space, it is also quite easy to have to create a region right at the top of it. So when END is exclusive, it would become 0. So on Arm, we decided to start to get rid of "end". I would consider to do the same on RISC-V for new functions. > +#define FIX_MISC (FIX_PMAP_END + 1) /* Ephemeral mappings of hardware */ Are you going to use this fixmap? If not, then I would consider to remove it. > + > +#define FIX_LAST FIX_MISC > + > +#define FIXADDR_START FIXMAP_ADDR(0) > +#define FIXADDR_TOP FIXMAP_ADDR(FIX_LAST) > + > +#ifndef __ASSEMBLY__ > + > +/* > + * Direct access to xen_fixmap[] should only happen when {set, > + * clear}_fixmap() is unusable (e.g. where we would end up to > + * recursively call the helpers). > + */ > +extern pte_t xen_fixmap[]; > + > +/* Map a page in a fixmap entry */ > +extern void set_fixmap(unsigned int map, mfn_t mfn, unsigned int attributes); > +/* Remove a mapping from a fixmap entry */ > +extern void clear_fixmap(unsigned int map); Neither of the functions seem to be implemented in this patch. Can you clarify what's the plan for them? Also, I know that for x86/arm, we have some function prefixed with extern. But AFAIK, we are trying to get rid of them. In any case, I think for RISC-V we need some consistency. For instance, here you define with extern but... > + > +#define fix_to_virt(slot) ((void *)FIXMAP_ADDR(slot)) > + > +static inline unsigned int virt_to_fix(vaddr_t vaddr) > +{ > + BUG_ON(vaddr >= FIXADDR_TOP || vaddr < FIXADDR_START); > + > + return ((vaddr - FIXADDR_START) >> PAGE_SHIFT); > +} > + > +#endif /* __ASSEMBLY__ */ > + > +#endif /* __ASM_FIXMAP_H */ > diff --git a/xen/arch/riscv/include/asm/mm.h b/xen/arch/riscv/include/asm/mm.h > index 25af9e1aaa..a0bdc2bc3a 100644 > --- a/xen/arch/riscv/include/asm/mm.h > +++ b/xen/arch/riscv/include/asm/mm.h > @@ -255,4 +255,6 @@ static inline unsigned int arch_get_dma_bitsize(void) > return 32; /* TODO */ > } > > +void setup_fixmap_mappings(void); ... here it is without. > + > #endif /* _ASM_RISCV_MM_H */ > diff --git a/xen/arch/riscv/include/asm/page.h b/xen/arch/riscv/include/asm/page.h > index c831e16417..cbbf3656d1 100644 > --- a/xen/arch/riscv/include/asm/page.h > +++ b/xen/arch/riscv/include/asm/page.h > @@ -81,6 +81,13 @@ static inline void flush_page_to_ram(unsigned long mfn, bool sync_icache) > BUG_ON("unimplemented"); > } > > +/* Write a pagetable entry. */ > +static inline void write_pte(pte_t *p, pte_t pte) > +{ > + *p = pte; > + asm volatile ("sfence.vma"); > +} > + > #endif /* __ASSEMBLY__ */ > > #endif /* _ASM_RISCV_PAGE_H */ > diff --git a/xen/arch/riscv/mm.c b/xen/arch/riscv/mm.c > index 7d09e781bf..d69a174b5d 100644 > --- a/xen/arch/riscv/mm.c > +++ b/xen/arch/riscv/mm.c > @@ -49,6 +49,9 @@ stage1_pgtbl_root[PAGETABLE_ENTRIES]; > pte_t __section(".bss.page_aligned") __aligned(PAGE_SIZE) > stage1_pgtbl_nonroot[PGTBL_INITIAL_COUNT * PAGETABLE_ENTRIES]; > > +pte_t __section(".bss.page_aligned") __aligned(PAGE_SIZE) > +xen_fixmap[PAGETABLE_ENTRIES]; Can you add a BUILD_BUG_ON() to check that the number of entries in the fixmap will never be above PAGETABLE_ENTRIES? > + > #define HANDLE_PGTBL(curr_lvl_num) \ > index = pt_index(curr_lvl_num, page_addr); \ > if ( pte_is_valid(pgtbl[index]) ) \ > @@ -191,6 +194,38 @@ static bool __init check_pgtbl_mode_support(struct mmu_desc *mmu_desc, > return is_mode_supported; > } > > +void __init setup_fixmap_mappings(void) > +{ > + pte_t *pte; > + unsigned int i; > + > + pte = &stage1_pgtbl_root[pt_index(HYP_PT_ROOT_LEVEL, FIXMAP_ADDR(0))]; > + > + for ( i = HYP_PT_ROOT_LEVEL - 1; i != 0; i-- ) I am a little bit confused with the - 1. Is this because you only want to map at L1 (I am not sure if this is the correct naming for RISC-V)? In any case, I think it would be worth a comment. > + { > + BUG_ON(!pte_is_valid(*pte)); > + > + pte = (pte_t *)LOAD_TO_LINK(pte_to_paddr(*pte)); > + pte = &pte[pt_index(i, FIXMAP_ADDR(0))]; > + } > + > + BUG_ON( pte_is_valid(*pte) ); Coding style: BUG_ON(pte_is_valid(*pte)); > + > + if ( !pte_is_valid(*pte) ) I am a bit confused with this check. Above, Xen will crash if the PTE is valid. So why do we need a runtime check? > + { > + pte_t tmp = paddr_to_pte(LINK_TO_LOAD((unsigned long)&xen_fixmap), PTE_TABLE); > + > + write_pte(pte, tmp); > + > + printk("(XEN) fixmap is mapped\n"); > + } > + > + /* > + * We only need the zeroeth table allocated, but not the PTEs set, because > + * set_fixmap() will set them on the fly. This function doesn't seem to exists yet (?). > + */ > +} > + > /* > * setup_initial_pagetables: > * > diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c > index 4defad68f4..13f0e8c77d 100644 > --- a/xen/arch/riscv/setup.c > +++ b/xen/arch/riscv/setup.c > @@ -46,6 +46,8 @@ void __init noreturn start_xen(unsigned long bootcpu_id, > test_macros_from_bug_h(); > #endif > > + setup_fixmap_mappings(); > + > printk("All set up\n"); > > for ( ;; ) > diff --git a/xen/arch/riscv/xen.lds.S b/xen/arch/riscv/xen.lds.S > index 070b19d915..63b1dd7bb6 100644 > --- a/xen/arch/riscv/xen.lds.S > +++ b/xen/arch/riscv/xen.lds.S > @@ -181,6 +181,6 @@ ASSERT(!SIZEOF(.got.plt), ".got.plt non-empty") > * Changing the size of Xen binary can require an update of > * PGTBL_INITIAL_COUNT. > */ > -ASSERT(_end - _start <= MB(2), "Xen too large for early-boot assumptions") > +ASSERT(_end - _start <= XEN_SIZE, "Xen too large for early-boot assumptions") > > ASSERT(_ident_end - _ident_start <= IDENT_AREA_SIZE, "identity region is too big"); Cheers, -- Julien Grall
Hello Julien, On Sun, 2024-07-21 at 09:46 +0100, Julien Grall wrote: > > diff --git a/xen/arch/riscv/mm.c b/xen/arch/riscv/mm.c > > index 7d09e781bf..d69a174b5d 100644 > > --- a/xen/arch/riscv/mm.c > > +++ b/xen/arch/riscv/mm.c > > @@ -49,6 +49,9 @@ stage1_pgtbl_root[PAGETABLE_ENTRIES]; > > pte_t __section(".bss.page_aligned") __aligned(PAGE_SIZE) > > stage1_pgtbl_nonroot[PGTBL_INITIAL_COUNT * PAGETABLE_ENTRIES]; > > > > +pte_t __section(".bss.page_aligned") __aligned(PAGE_SIZE) > > +xen_fixmap[PAGETABLE_ENTRIES]; > > Can you add a BUILD_BUG_ON() to check that the number of entries in > the > fixmap will never be above PAGETABLE_ENTRIES? I just realized that we don't have the information about how many entries has been used. Am I confusing something? ~ Oleksii
On 23/07/2024 14:27, oleksii.kurochko@gmail.com wrote: > Hello Julien, Hi Oleksii, > On Sun, 2024-07-21 at 09:46 +0100, Julien Grall wrote: >>> diff --git a/xen/arch/riscv/mm.c b/xen/arch/riscv/mm.c >>> index 7d09e781bf..d69a174b5d 100644 >>> --- a/xen/arch/riscv/mm.c >>> +++ b/xen/arch/riscv/mm.c >>> @@ -49,6 +49,9 @@ stage1_pgtbl_root[PAGETABLE_ENTRIES]; >>> pte_t __section(".bss.page_aligned") __aligned(PAGE_SIZE) >>> stage1_pgtbl_nonroot[PGTBL_INITIAL_COUNT * PAGETABLE_ENTRIES]; >>> >>> +pte_t __section(".bss.page_aligned") __aligned(PAGE_SIZE) >>> +xen_fixmap[PAGETABLE_ENTRIES]; >> >> Can you add a BUILD_BUG_ON() to check that the number of entries in >> the >> fixmap will never be above PAGETABLE_ENTRIES? > I just realized that we don't have the information about how many > entries has been used. Am I confusing something? I think we do. It is FIX_LAST. Cheers, -- Julien Grall
On Tue, 2024-07-23 at 14:33 +0100, Julien Grall wrote: > On 23/07/2024 14:27, oleksii.kurochko@gmail.com wrote: > > Hello Julien, > > Hi Oleksii, > > > > On Sun, 2024-07-21 at 09:46 +0100, Julien Grall wrote: > > > > diff --git a/xen/arch/riscv/mm.c b/xen/arch/riscv/mm.c > > > > index 7d09e781bf..d69a174b5d 100644 > > > > --- a/xen/arch/riscv/mm.c > > > > +++ b/xen/arch/riscv/mm.c > > > > @@ -49,6 +49,9 @@ stage1_pgtbl_root[PAGETABLE_ENTRIES]; > > > > pte_t __section(".bss.page_aligned") __aligned(PAGE_SIZE) > > > > stage1_pgtbl_nonroot[PGTBL_INITIAL_COUNT * > > > > PAGETABLE_ENTRIES]; > > > > > > > > +pte_t __section(".bss.page_aligned") __aligned(PAGE_SIZE) > > > > +xen_fixmap[PAGETABLE_ENTRIES]; > > > > > > Can you add a BUILD_BUG_ON() to check that the number of entries > > > in > > > the > > > fixmap will never be above PAGETABLE_ENTRIES? > > I just realized that we don't have the information about how many > > entries has been used. Am I confusing something? > > I think we do. It is FIX_LAST. Sure. We have FIX_LAST. Thanks ~ Oleksii
Hi Julien, On Sun, 2024-07-21 at 09:46 +0100, Julien Grall wrote: > > +/* Fixmap slots */ > > +#define FIX_PMAP_BEGIN (0) /* Start of PMAP */ > > +#define FIX_PMAP_END (FIX_PMAP_BEGIN + NUM_FIX_PMAP - 1) /* End of > > PMAP */ > > ... here is seems to be inclusive. Furthermore if you had 32-bit > address > space, it is also quite easy to have to create a region right at the > top > of it. So when END is exclusive, it would become 0. > > So on Arm, we decided to start to get rid of "end". I would consider > to > do the same on RISC-V for new functions. I assume that you wrote here just as an example of confusion occurs because of using *_END but just to be clear I have to leave FIXMAP_MAP_END as-is because it is used now by common code. ~ Oleksii
On 23/07/2024 13:58, oleksii.kurochko@gmail.com wrote: > Hi Julien, Hi Oleksii, > On Sun, 2024-07-21 at 09:46 +0100, Julien Grall wrote: >>> +/* Fixmap slots */ >>> +#define FIX_PMAP_BEGIN (0) /* Start of PMAP */ >>> +#define FIX_PMAP_END (FIX_PMAP_BEGIN + NUM_FIX_PMAP - 1) /* End of >>> PMAP */ >> >> ... here is seems to be inclusive. Furthermore if you had 32-bit >> address >> space, it is also quite easy to have to create a region right at the >> top >> of it. So when END is exclusive, it would become 0. >> >> So on Arm, we decided to start to get rid of "end". I would consider >> to >> do the same on RISC-V for new functions. > I assume that you wrote here just as an example of confusion occurs > because of using *_END but just to be clear I have to leave > FIXMAP_MAP_END as-is because it is used now by common code. Indeed. FIXMAP_PMAP_END should stay for now. Cheers, -- Julien Grall
Hi Julien, On Sun, 2024-07-21 at 09:46 +0100, Julien Grall wrote: > Hi Oleksii, > > On 12/07/2024 17:22, Oleksii Kurochko wrote: > > Introduce a function to set up fixmap mappings and L0 page > > table for fixmap. > > > > Additionally, defines were introduced in riscv/config.h to > > calculate the FIXMAP_BASE address. > > This involved introducing BOOT_FDT_VIRT_{START, SIZE} and > > XEN_SIZE, XEN_VIRT_END. > > > > Also, the check of Xen size was updated in the riscv/lds.S > > script to use XEN_SIZE instead of a hardcoded constant. > > > > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> > > --- > > Changes in V2: > > - newly introduced patch > > --- > > xen/arch/riscv/include/asm/config.h | 9 ++++++ > > xen/arch/riscv/include/asm/fixmap.h | 48 > > +++++++++++++++++++++++++++++ > > xen/arch/riscv/include/asm/mm.h | 2 ++ > > xen/arch/riscv/include/asm/page.h | 7 +++++ > > xen/arch/riscv/mm.c | 35 +++++++++++++++++++++ > > xen/arch/riscv/setup.c | 2 ++ > > xen/arch/riscv/xen.lds.S | 2 +- > > 7 files changed, 104 insertions(+), 1 deletion(-) > > create mode 100644 xen/arch/riscv/include/asm/fixmap.h > > > > diff --git a/xen/arch/riscv/include/asm/config.h > > b/xen/arch/riscv/include/asm/config.h > > index 50583aafdc..3275477c17 100644 > > --- a/xen/arch/riscv/include/asm/config.h > > +++ b/xen/arch/riscv/include/asm/config.h > > @@ -74,11 +74,20 @@ > > #error "unsupported RV_STAGE1_MODE" > > #endif > > > > +#define XEN_SIZE MB(2) > > NIT: I would name it XEN_VIRT_SIZE to be consistent with the > start/end. > > > +#define XEN_VIRT_END (XEN_VIRT_START + XEN_SIZE) > Can we get away with not introducing *_END and just use START, SIZE? > The > reason I am asking is with "end" it is never clear whether it is > inclusive or exclusive. For instance, here you use an exclusive range > but ... > > > + > > +#define BOOT_FDT_VIRT_START XEN_VIRT_END > > +#define BOOT_FDT_VIRT_SIZE MB(4) > > + > > #define DIRECTMAP_SLOT_END 509 > > #define DIRECTMAP_SLOT_START 200 > > #define DIRECTMAP_VIRT_START SLOTN(DIRECTMAP_SLOT_START) > > #define DIRECTMAP_SIZE (SLOTN(DIRECTMAP_SLOT_END) - > > SLOTN(DIRECTMAP_SLOT_START)) > > > > +#define FIXMAP_BASE (BOOT_FDT_VIRT_START + > > BOOT_FDT_VIRT_SIZE) > > +#define FIXMAP_ADDR(n) (FIXMAP_BASE + (n) * PAGE_SIZE) > > + > > #define FRAMETABLE_SCALE_FACTOR (PAGE_SIZE/sizeof(struct > > page_info)) > > #define FRAMETABLE_SIZE_IN_SLOTS (((DIRECTMAP_SIZE / SLOTN(1)) / > > FRAMETABLE_SCALE_FACTOR) + 1) > > > > diff --git a/xen/arch/riscv/include/asm/fixmap.h > > b/xen/arch/riscv/include/asm/fixmap.h > > new file mode 100644 > > index 0000000000..fcfb82d69c > > --- /dev/null > > +++ b/xen/arch/riscv/include/asm/fixmap.h > > @@ -0,0 +1,48 @@ > > +/* > > + * fixmap.h: compile-time virtual memory allocation > > + */ > > +#ifndef __ASM_FIXMAP_H > > +#define __ASM_FIXMAP_H > > + > > +#include <xen/bug.h> > > +#include <xen/page-size.h> > > +#include <xen/pmap.h> > > + > > +#include <asm/page.h> > > + > > +/* Fixmap slots */ > > +#define FIX_PMAP_BEGIN (0) /* Start of PMAP */ > > +#define FIX_PMAP_END (FIX_PMAP_BEGIN + NUM_FIX_PMAP - 1) /* End of > > PMAP */ > > ... here is seems to be inclusive. Furthermore if you had 32-bit > address > space, it is also quite easy to have to create a region right at the > top > of it. So when END is exclusive, it would become 0. > > So on Arm, we decided to start to get rid of "end". I would consider > to > do the same on RISC-V for new functions. I will refactor the code and get rid of "end". > > > +#define FIX_MISC (FIX_PMAP_END + 1) /* Ephemeral mappings of > > hardware */ > > Are you going to use this fixmap? If not, then I would consider to > remove it. Yes, it used now in copy_from_paddr(): /** * copy_from_paddr - copy data from a physical address * @dst: destination virtual address * @paddr: source physical address * @len: length to copy */ void __init copy_from_paddr(void *dst, paddr_t paddr, unsigned long len) { void *src = (void *)FIXMAP_ADDR(FIXMAP_MISC); while (len) { unsigned long l, s; s = paddr & (PAGE_SIZE-1); l = min(PAGE_SIZE - s, len); set_fixmap(FIXMAP_MISC, maddr_to_mfn(paddr), PAGE_HYPERVISOR_WC); memcpy(dst, src + s, l); clear_fixmap(FIXMAP_MISC); paddr += l; dst += l; len -= l; } } > > > + > > +#define FIX_LAST FIX_MISC > > + > > +#define FIXADDR_START FIXMAP_ADDR(0) > > +#define FIXADDR_TOP FIXMAP_ADDR(FIX_LAST) > > + > > +#ifndef __ASSEMBLY__ > > + > > +/* > > + * Direct access to xen_fixmap[] should only happen when {set, > > + * clear}_fixmap() is unusable (e.g. where we would end up to > > + * recursively call the helpers). > > + */ > > +extern pte_t xen_fixmap[]; > > + > > +/* Map a page in a fixmap entry */ > > +extern void set_fixmap(unsigned int map, mfn_t mfn, unsigned int > > attributes); > > +/* Remove a mapping from a fixmap entry */ > > +extern void clear_fixmap(unsigned int map); > > Neither of the functions seem to be implemented in this patch. Can > you > clarify what's the plan for them? You are right, it could be dropped now. But in future this functions are used for copy_from_paddr(). Look at the code above. > > Also, I know that for x86/arm, we have some function prefixed with > extern. But AFAIK, we are trying to get rid of them. > > In any case, I think for RISC-V we need some consistency. For > instance, > here you define with extern but... > > > + > > +#define fix_to_virt(slot) ((void *)FIXMAP_ADDR(slot)) > > + > > +static inline unsigned int virt_to_fix(vaddr_t vaddr) > > +{ > > + BUG_ON(vaddr >= FIXADDR_TOP || vaddr < FIXADDR_START); > > + > > + return ((vaddr - FIXADDR_START) >> PAGE_SHIFT); > > +} > > + > > +#endif /* __ASSEMBLY__ */ > > + > > +#endif /* __ASM_FIXMAP_H */ > > diff --git a/xen/arch/riscv/include/asm/mm.h > > b/xen/arch/riscv/include/asm/mm.h > > index 25af9e1aaa..a0bdc2bc3a 100644 > > --- a/xen/arch/riscv/include/asm/mm.h > > +++ b/xen/arch/riscv/include/asm/mm.h > > @@ -255,4 +255,6 @@ static inline unsigned int > > arch_get_dma_bitsize(void) > > return 32; /* TODO */ > > } > > > > +void setup_fixmap_mappings(void); > > ... here it is without. > > > + > > #endif /* _ASM_RISCV_MM_H */ > > diff --git a/xen/arch/riscv/include/asm/page.h > > b/xen/arch/riscv/include/asm/page.h > > index c831e16417..cbbf3656d1 100644 > > --- a/xen/arch/riscv/include/asm/page.h > > +++ b/xen/arch/riscv/include/asm/page.h > > @@ -81,6 +81,13 @@ static inline void flush_page_to_ram(unsigned > > long mfn, bool sync_icache) > > BUG_ON("unimplemented"); > > } > > > > +/* Write a pagetable entry. */ > > +static inline void write_pte(pte_t *p, pte_t pte) > > +{ > > + *p = pte; > > + asm volatile ("sfence.vma"); > > +} > > + > > #endif /* __ASSEMBLY__ */ > > > > #endif /* _ASM_RISCV_PAGE_H */ > > diff --git a/xen/arch/riscv/mm.c b/xen/arch/riscv/mm.c > > index 7d09e781bf..d69a174b5d 100644 > > --- a/xen/arch/riscv/mm.c > > +++ b/xen/arch/riscv/mm.c > > @@ -49,6 +49,9 @@ stage1_pgtbl_root[PAGETABLE_ENTRIES]; > > pte_t __section(".bss.page_aligned") __aligned(PAGE_SIZE) > > stage1_pgtbl_nonroot[PGTBL_INITIAL_COUNT * PAGETABLE_ENTRIES]; > > > > +pte_t __section(".bss.page_aligned") __aligned(PAGE_SIZE) > > +xen_fixmap[PAGETABLE_ENTRIES]; > > Can you add a BUILD_BUG_ON() to check that the number of entries in > the > fixmap will never be above PAGETABLE_ENTRIES? Sure. What is the best place? Somewhere in setup_fixmap_mappings()? > > > + > > #define > > HANDLE_PGTBL(curr_lvl_num) > > \ > > index = pt_index(curr_lvl_num, > > page_addr); \ > > if ( pte_is_valid(pgtbl[index]) > > ) \ > > @@ -191,6 +194,38 @@ static bool __init > > check_pgtbl_mode_support(struct mmu_desc *mmu_desc, > > return is_mode_supported; > > } > > > > +void __init setup_fixmap_mappings(void) > > +{ > > + pte_t *pte; > > + unsigned int i; > > + > > + pte = &stage1_pgtbl_root[pt_index(HYP_PT_ROOT_LEVEL, > > FIXMAP_ADDR(0))]; > > + > > + for ( i = HYP_PT_ROOT_LEVEL - 1; i != 0; i-- ) > > I am a little bit confused with the - 1. Is this because you only > want > to map at L1 (I am not sure if this is the correct naming for RISC- > V)? Yes, the idea is that I want to stop in L1 ( 2Mb pages ) as this mapping is already exist and there will not be need to create a new table. ( what will fail because boot allocator isn't initialized yet and alloc_boot_pages() will start to alarm because of BUG_ON(!nr_bootmem_regions) ). RISC-V also uses word levels, but the order is an opposite to Arm. > > In any case, I think it would be worth a comment. Sure, I will add it. > > > + { > > + BUG_ON(!pte_is_valid(*pte)); > > + > > + pte = (pte_t *)LOAD_TO_LINK(pte_to_paddr(*pte)); > > + pte = &pte[pt_index(i, FIXMAP_ADDR(0))]; > > + } > > + > > + BUG_ON( pte_is_valid(*pte) ); > > Coding style: BUG_ON(pte_is_valid(*pte)); > > > + > > + if ( !pte_is_valid(*pte) ) > > I am a bit confused with this check. Above, Xen will crash if the PTE > is > valid. So why do we need a runtime check? You are right, there is no any sense. We should drop it. > > > + { > > + pte_t tmp = paddr_to_pte(LINK_TO_LOAD((unsigned > > long)&xen_fixmap), PTE_TABLE); > > + > > + write_pte(pte, tmp); > > + > > + printk("(XEN) fixmap is mapped\n"); > > + } > > + > > + /* > > + * We only need the zeroeth table allocated, but not the PTEs > > set, because > > + * set_fixmap() will set them on the fly. > > This function doesn't seem to exists yet (?). Not yet. It will be introduced later... ~ Oleksii > > > + */ > > +} > > + > > /* > > * setup_initial_pagetables: > > * > > diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c > > index 4defad68f4..13f0e8c77d 100644 > > --- a/xen/arch/riscv/setup.c > > +++ b/xen/arch/riscv/setup.c > > @@ -46,6 +46,8 @@ void __init noreturn start_xen(unsigned long > > bootcpu_id, > > test_macros_from_bug_h(); > > #endif > > > > + setup_fixmap_mappings(); > > + > > printk("All set up\n"); > > > > for ( ;; ) > > diff --git a/xen/arch/riscv/xen.lds.S b/xen/arch/riscv/xen.lds.S > > index 070b19d915..63b1dd7bb6 100644 > > --- a/xen/arch/riscv/xen.lds.S > > +++ b/xen/arch/riscv/xen.lds.S > > @@ -181,6 +181,6 @@ ASSERT(!SIZEOF(.got.plt), ".got.plt non- > > empty") > > * Changing the size of Xen binary can require an update of > > * PGTBL_INITIAL_COUNT. > > */ > > -ASSERT(_end - _start <= MB(2), "Xen too large for early-boot > > assumptions") > > +ASSERT(_end - _start <= XEN_SIZE, "Xen too large for early-boot > > assumptions") > > > > ASSERT(_ident_end - _ident_start <= IDENT_AREA_SIZE, "identity > > region is too big"); > > Cheers, >
On 22/07/2024 15:31, Oleksii wrote: > Hi Julien, Hi Oleksii, > On Sun, 2024-07-21 at 09:46 +0100, Julien Grall wrote: >> Hi Oleksii, >> >> On 12/07/2024 17:22, Oleksii Kurochko wrote: >>> Introduce a function to set up fixmap mappings and L0 page >>> table for fixmap. >>> >>> Additionally, defines were introduced in riscv/config.h to >>> calculate the FIXMAP_BASE address. >>> This involved introducing BOOT_FDT_VIRT_{START, SIZE} and >>> XEN_SIZE, XEN_VIRT_END. >>> >>> Also, the check of Xen size was updated in the riscv/lds.S >>> script to use XEN_SIZE instead of a hardcoded constant. >>> >>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> >>> --- >>> Changes in V2: >>> - newly introduced patch >>> --- >>> xen/arch/riscv/include/asm/config.h | 9 ++++++ >>> xen/arch/riscv/include/asm/fixmap.h | 48 >>> +++++++++++++++++++++++++++++ >>> xen/arch/riscv/include/asm/mm.h | 2 ++ >>> xen/arch/riscv/include/asm/page.h | 7 +++++ >>> xen/arch/riscv/mm.c | 35 +++++++++++++++++++++ >>> xen/arch/riscv/setup.c | 2 ++ >>> xen/arch/riscv/xen.lds.S | 2 +- >>> 7 files changed, 104 insertions(+), 1 deletion(-) >>> create mode 100644 xen/arch/riscv/include/asm/fixmap.h >>> >>> diff --git a/xen/arch/riscv/include/asm/config.h >>> b/xen/arch/riscv/include/asm/config.h >>> index 50583aafdc..3275477c17 100644 >>> --- a/xen/arch/riscv/include/asm/config.h >>> +++ b/xen/arch/riscv/include/asm/config.h >>> @@ -74,11 +74,20 @@ >>> #error "unsupported RV_STAGE1_MODE" >>> #endif >>> >>> +#define XEN_SIZE MB(2) >> >> NIT: I would name it XEN_VIRT_SIZE to be consistent with the >> start/end. >> >>> +#define XEN_VIRT_END (XEN_VIRT_START + XEN_SIZE) >> Can we get away with not introducing *_END and just use START, SIZE? >> The >> reason I am asking is with "end" it is never clear whether it is >> inclusive or exclusive. For instance, here you use an exclusive range >> but ... >> >>> + >>> +#define BOOT_FDT_VIRT_START XEN_VIRT_END >>> +#define BOOT_FDT_VIRT_SIZE MB(4) >>> + >>> #define DIRECTMAP_SLOT_END 509 >>> #define DIRECTMAP_SLOT_START 200 >>> #define DIRECTMAP_VIRT_START SLOTN(DIRECTMAP_SLOT_START) >>> #define DIRECTMAP_SIZE (SLOTN(DIRECTMAP_SLOT_END) - >>> SLOTN(DIRECTMAP_SLOT_START)) >>> >>> +#define FIXMAP_BASE (BOOT_FDT_VIRT_START + >>> BOOT_FDT_VIRT_SIZE) >>> +#define FIXMAP_ADDR(n) (FIXMAP_BASE + (n) * PAGE_SIZE) >>> + >>> #define FRAMETABLE_SCALE_FACTOR (PAGE_SIZE/sizeof(struct >>> page_info)) >>> #define FRAMETABLE_SIZE_IN_SLOTS (((DIRECTMAP_SIZE / SLOTN(1)) / >>> FRAMETABLE_SCALE_FACTOR) + 1) >>> >>> diff --git a/xen/arch/riscv/include/asm/fixmap.h >>> b/xen/arch/riscv/include/asm/fixmap.h >>> new file mode 100644 >>> index 0000000000..fcfb82d69c >>> --- /dev/null >>> +++ b/xen/arch/riscv/include/asm/fixmap.h >>> @@ -0,0 +1,48 @@ >>> +/* >>> + * fixmap.h: compile-time virtual memory allocation >>> + */ >>> +#ifndef __ASM_FIXMAP_H >>> +#define __ASM_FIXMAP_H >>> + >>> +#include <xen/bug.h> >>> +#include <xen/page-size.h> >>> +#include <xen/pmap.h> >>> + >>> +#include <asm/page.h> >>> + >>> +/* Fixmap slots */ >>> +#define FIX_PMAP_BEGIN (0) /* Start of PMAP */ >>> +#define FIX_PMAP_END (FIX_PMAP_BEGIN + NUM_FIX_PMAP - 1) /* End of >>> PMAP */ >> >> ... here is seems to be inclusive. Furthermore if you had 32-bit >> address >> space, it is also quite easy to have to create a region right at the >> top >> of it. So when END is exclusive, it would become 0. >> >> So on Arm, we decided to start to get rid of "end". I would consider >> to >> do the same on RISC-V for new functions. > I will refactor the code and get rid of "end". > >> >>> +#define FIX_MISC (FIX_PMAP_END + 1) /* Ephemeral mappings of >>> hardware */ >> >> Are you going to use this fixmap? If not, then I would consider to >> remove it. > Yes, it used now in copy_from_paddr(): > /** > * copy_from_paddr - copy data from a physical address > * @dst: destination virtual address > * @paddr: source physical address > * @len: length to copy > */ > void __init copy_from_paddr(void *dst, paddr_t paddr, unsigned long > len) > { > void *src = (void *)FIXMAP_ADDR(FIXMAP_MISC); > > while (len) { > unsigned long l, s; > > s = paddr & (PAGE_SIZE-1); > l = min(PAGE_SIZE - s, len); > > set_fixmap(FIXMAP_MISC, maddr_to_mfn(paddr), > PAGE_HYPERVISOR_WC); > memcpy(dst, src + s, l); > clear_fixmap(FIXMAP_MISC); > > paddr += l; > dst += l; > len -= l; > } > } > >> >>> + >>> +#define FIX_LAST FIX_MISC >>> + >>> +#define FIXADDR_START FIXMAP_ADDR(0) >>> +#define FIXADDR_TOP FIXMAP_ADDR(FIX_LAST) >>> + >>> +#ifndef __ASSEMBLY__ >>> + >>> +/* >>> + * Direct access to xen_fixmap[] should only happen when {set, >>> + * clear}_fixmap() is unusable (e.g. where we would end up to >>> + * recursively call the helpers). >>> + */ >>> +extern pte_t xen_fixmap[]; >>> + >>> +/* Map a page in a fixmap entry */ >>> +extern void set_fixmap(unsigned int map, mfn_t mfn, unsigned int >>> attributes); >>> +/* Remove a mapping from a fixmap entry */ >>> +extern void clear_fixmap(unsigned int map); >> >> Neither of the functions seem to be implemented in this patch. Can >> you >> clarify what's the plan for them? > You are right, it could be dropped now. But in future this functions > are used for copy_from_paddr(). Look at the code above. Right, to me it is just odd we are definition prototype for functions that don't yet exist. >> >> Also, I know that for x86/arm, we have some function prefixed with >> extern. But AFAIK, we are trying to get rid of them. >> >> In any case, I think for RISC-V we need some consistency. For >> instance, >> here you define with extern but... >> >>> + >>> +#define fix_to_virt(slot) ((void *)FIXMAP_ADDR(slot)) >>> + >>> +static inline unsigned int virt_to_fix(vaddr_t vaddr) >>> +{ >>> + BUG_ON(vaddr >= FIXADDR_TOP || vaddr < FIXADDR_START); >>> + >>> + return ((vaddr - FIXADDR_START) >> PAGE_SHIFT); >>> +} >>> + >>> +#endif /* __ASSEMBLY__ */ >>> + >>> +#endif /* __ASM_FIXMAP_H */ >>> diff --git a/xen/arch/riscv/include/asm/mm.h >>> b/xen/arch/riscv/include/asm/mm.h >>> index 25af9e1aaa..a0bdc2bc3a 100644 >>> --- a/xen/arch/riscv/include/asm/mm.h >>> +++ b/xen/arch/riscv/include/asm/mm.h >>> @@ -255,4 +255,6 @@ static inline unsigned int >>> arch_get_dma_bitsize(void) >>> return 32; /* TODO */ >>> } >>> >>> +void setup_fixmap_mappings(void); >> >> ... here it is without. >> >>> + >>> #endif /* _ASM_RISCV_MM_H */ >>> diff --git a/xen/arch/riscv/include/asm/page.h >>> b/xen/arch/riscv/include/asm/page.h >>> index c831e16417..cbbf3656d1 100644 >>> --- a/xen/arch/riscv/include/asm/page.h >>> +++ b/xen/arch/riscv/include/asm/page.h >>> @@ -81,6 +81,13 @@ static inline void flush_page_to_ram(unsigned >>> long mfn, bool sync_icache) >>> BUG_ON("unimplemented"); >>> } >>> >>> +/* Write a pagetable entry. */ >>> +static inline void write_pte(pte_t *p, pte_t pte) >>> +{ >>> + *p = pte; >>> + asm volatile ("sfence.vma"); >>> +} >>> + >>> #endif /* __ASSEMBLY__ */ >>> >>> #endif /* _ASM_RISCV_PAGE_H */ >>> diff --git a/xen/arch/riscv/mm.c b/xen/arch/riscv/mm.c >>> index 7d09e781bf..d69a174b5d 100644 >>> --- a/xen/arch/riscv/mm.c >>> +++ b/xen/arch/riscv/mm.c >>> @@ -49,6 +49,9 @@ stage1_pgtbl_root[PAGETABLE_ENTRIES]; >>> pte_t __section(".bss.page_aligned") __aligned(PAGE_SIZE) >>> stage1_pgtbl_nonroot[PGTBL_INITIAL_COUNT * PAGETABLE_ENTRIES]; >>> >>> +pte_t __section(".bss.page_aligned") __aligned(PAGE_SIZE) >>> +xen_fixmap[PAGETABLE_ENTRIES]; >> >> Can you add a BUILD_BUG_ON() to check that the number of entries in >> the >> fixmap will never be above PAGETABLE_ENTRIES? > Sure. What is the best place? Somewhere in setup_fixmap_mappings()? I think so. Cheers, -- Julien Grall
© 2016 - 2024 Red Hat, Inc.