[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v4 1/3] xen/riscv: introduce setup_initial_pages
Hi Jan, On Mon, 2023-04-17 at 13:50 +0200, Jan Beulich wrote: > On 07.04.2023 17:48, Oleksii Kurochko wrote: > > The idea was taken from xvisor but the following changes > > were done: > > * Use only a minimal part of the code enough to enable MMU > > * rename {_}setup_initial_pagetables functions > > * add an argument for setup_initial_mapping to have > > an opportunity to make set PTE flags. > > * update setup_initial_pagetables function to map sections > > with correct PTE flags. > > * Rewrite enable_mmu() to C. > > * map linker addresses range to load addresses range without > > 1:1 mapping. It will be 1:1 only in case when > > load_start_addr is equal to linker_start_addr. > > * add safety checks such as: > > * Xen size is less than page size > > * linker addresses range doesn't overlap load addresses > > range > > * Rework macros {THIRD,SECOND,FIRST,ZEROETH}_{SHIFT,MASK} > > * change PTE_LEAF_DEFAULT to RW instead of RWX. > > * Remove phys_offset as it is not used now > > * Remove alignment of {map, pa}_start &= XEN_PT_LEVEL_MAP_MASK(0); > > in setup_inital_mapping() as they should be already aligned. > > Make a check that {map_pa}_start are aligned. > > * Remove clear_pagetables() as initial pagetables will be > > zeroed during bss initialization > > * Remove __attribute__((section(".entry")) for > > setup_initial_pagetables() > > as there is no such section in xen.lds.S > > * Update the argument of pte_is_valid() to "const pte_t *p" > > * Add check that Xen's load address is aligned at 4k boundary > > * Refactor setup_initial_pagetables() so it is mapping linker > > address range to load address range. After setup needed > > permissions for specific section ( such as .text, .rodata, etc ) > > otherwise RW permission will be set by default. > > * Add function to check that requested SATP_MODE is supported > > > > Origin: git@xxxxxxxxxx:xvisor/xvisor.git 9be2fdd7 > > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx> > > --- > > Changes in V4: > > * use GB() macros instead of defining SZ_1G > > * hardcode XEN_VIRT_START and add comment (ADDRESS_SPACE_END + 1 > > - GB(1)) > > Perhaps in a separate patch, may I ask that you add - like x86 and > Arm > have it - a block comment to config.h laying out virtual address > space > use? Knowing what is planned to be put where (even if just vaguely, > i.e. > keeping open the option of changing the layout) is likely going to > help > with figuring whether this is a good placement. > > Such a comment could then also be accompanied by mentioning that > virtual address space really "wraps" at certain boundaries (due to > the > upper address bits simply being ignored). For an x86 person like me > this is certainly unexpected / unusual behavior. > Sure, it makes sense. I'll add that to new version of the patch series. > > * remove unnecessary 'asm' word at the end of #error > > * encapsulate pte_t definition in a struct > > * rename addr_to_ppn() to ppn_to_paddr(). > > * change type of paddr argument from const unsigned long to > > paddr_t > > * pte_to_paddr() update prototype. > > * calculate size of Xen binary based on an amount of page tables > > * use unsgined int instead of 'uint32_t' instead of uint32_t as > > their use isn't warranted. > > * remove extern of bss_{start,end} as they aren't used in mm.c > > anymore > > * fix code style > > * add argument for HANDLE_PGTBL macros instead of curr_lvl_num > > variable > > * make enable_mmu() as noinline to prevent under link-time > > optimization > > because of the nature of enable_mmu() > > * add function to check that SATP_MODE is supported. > > * update the commit message > > * update setup_initial_pagetables to set correct PTE flags in one > > pass > > instead of calling setup_pte_permissions after > > setup_initial_pagetables() > > as setup_initial_pagetables() isn't used to change permission > > flags. > > --- > > Changes in V3: > > - update definition of pte_t structure to have a proper size of > > pte_t > > in case of RV32. > > - update asm/mm.h with new functions and remove unnecessary > > 'extern'. > > - remove LEVEL_* macros as only XEN_PT_LEVEL_* are enough. > > - update paddr_to_pte() to receive permissions as an argument. > > - add check that map_start & pa_start is properly aligned. > > - move defines PAGETABLE_ORDER, PAGETABLE_ENTRIES, PTE_PPN_SHIFT > > to > > <asm/page-bits.h> > > - Rename PTE_SHIFT to PTE_PPN_SHIFT > > - refactor setup_initial_pagetables: map all LINK addresses to > > LOAD addresses > > and after setup PTEs permission for sections; update check that > > linker > > and load addresses don't overlap. > > - refactor setup_initial_mapping: allocate pagetable 'dynamically' > > if it is > > necessary. > > - rewrite enable_mmu in C; add the check that map_start and > > pa_start are > > aligned on 4k boundary. > > - update the comment for setup_initial_pagetable funcion > > - Add RV_STAGE1_MODE to support different MMU modes > > - set XEN_VIRT_START very high to not overlap with load address > > range > > - align bss section > > --- > > Changes in V2: > > * update the commit message: > > * Remove {ZEROETH,FIRST,...}_{SHIFT,MASK, SIZE,...} and > > introduce instead of them XEN_PT_LEVEL_*() and LEVEL_* > > * Rework pt_linear_offset() and pt_index based on > > XEN_PT_LEVEL_*() > > * Remove clear_pagetables() functions as pagetables were zeroed > > during > > .bss initialization > > * Rename _setup_initial_pagetables() to setup_initial_mapping() > > * Make PTE_DEFAULT equal to RX. > > * Update prototype of setup_initial_mapping(..., bool writable) -> > > setup_initial_mapping(..., UL flags) > > * Update calls of setup_initial_mapping according to new prototype > > * Remove unnecessary call of: > > _setup_initial_pagetables(..., load_addr_start, load_addr_end, > > load_addr_start, ...) > > * Define index* in the loop of setup_initial_mapping > > * Remove attribute "__attribute__((section(".entry")))" for > > setup_initial_pagetables() > > as we don't have such section > > * make arguments of paddr_to_pte() and pte_is_valid() as const. > > * make xen_second_pagetable static. > > * use <xen/kernel.h> instead of declaring extern unsigned long > > _stext, 0etext, _srodata, _erodata > > * update 'extern unsigned long __init_begin' to 'extern unsigned > > long __init_begin[]' > > * use aligned() instead of > > "__attribute__((__aligned__(PAGE_SIZE)))" > > * set __section(".bss.page_aligned") for page tables arrays > > * fix identatations > > * Change '__attribute__((section(".entry")))' to '__init' > > * Remove phys_offset as it isn't used now. > > * Remove alignment of {map, pa}_start &= > > XEN_PT_LEVEL_MAP_MASK(0); in > > setup_inital_mapping() as they should be already aligned. > > * Remove clear_pagetables() as initial pagetables will be > > zeroed during bss initialization > > * Remove __attribute__((section(".entry")) for > > setup_initial_pagetables() > > as there is no such section in xen.lds.S > > * Update the argument of pte_is_valid() to "const pte_t *p" > > --- > > > > xen/arch/riscv/Makefile | 1 + > > xen/arch/riscv/include/asm/config.h | 12 +- > > xen/arch/riscv/include/asm/mm.h | 9 + > > xen/arch/riscv/include/asm/page-bits.h | 10 + > > xen/arch/riscv/include/asm/page.h | 65 +++++ > > xen/arch/riscv/mm.c | 319 > > +++++++++++++++++++++++++ > > xen/arch/riscv/riscv64/head.S | 2 + > > xen/arch/riscv/setup.c | 11 + > > xen/arch/riscv/xen.lds.S | 4 + > > 9 files changed, 432 insertions(+), 1 deletion(-) > > create mode 100644 xen/arch/riscv/include/asm/mm.h > > create mode 100644 xen/arch/riscv/include/asm/page.h > > create mode 100644 xen/arch/riscv/mm.c > > > > diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile > > index 443f6bf15f..956ceb02df 100644 > > --- a/xen/arch/riscv/Makefile > > +++ b/xen/arch/riscv/Makefile > > @@ -1,5 +1,6 @@ > > obj-$(CONFIG_EARLY_PRINTK) += early_printk.o > > obj-y += entry.o > > +obj-y += mm.o > > obj-$(CONFIG_RISCV_64) += riscv64/ > > obj-y += sbi.o > > obj-y += setup.o > > diff --git a/xen/arch/riscv/include/asm/config.h > > b/xen/arch/riscv/include/asm/config.h > > index 763a922a04..0cf9673558 100644 > > --- a/xen/arch/riscv/include/asm/config.h > > +++ b/xen/arch/riscv/include/asm/config.h > > @@ -39,12 +39,22 @@ > > name: > > #endif > > > > -#define XEN_VIRT_START _AT(UL, 0x80200000) > > +#ifdef CONFIG_RISCV_64 > > +#define XEN_VIRT_START 0xFFFFFFFFC0000000 /* (_AC(-1, UL) + 1 - > > GB(1)) */ > > +#else > > +#error "RV32 isn't supported" > > +#endif > > > > #define SMP_CACHE_BYTES (1 << 6) > > > > #define STACK_SIZE PAGE_SIZE > > > > +#ifdef CONFIG_RISCV_64 > > +#define RV_STAGE1_MODE SATP_MODE_SV39 > > +#else > > +#define RV_STAGE1_MODE SATP_MODE_SV32 > > +#endif > > + > > #endif /* __RISCV_CONFIG_H__ */ > > /* > > * Local variables: > > diff --git a/xen/arch/riscv/include/asm/mm.h > > b/xen/arch/riscv/include/asm/mm.h > > new file mode 100644 > > index 0000000000..e16ce66fae > > --- /dev/null > > +++ b/xen/arch/riscv/include/asm/mm.h > > @@ -0,0 +1,9 @@ > > +#ifndef _ASM_RISCV_MM_H > > +#define _ASM_RISCV_MM_H > > + > > +void setup_initial_pagetables(void); > > + > > +void enable_mmu(void); > > +void cont_after_mmu_is_enabled(void); > > + > > +#endif /* _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 1801820294..0879a527f2 100644 > > --- a/xen/arch/riscv/include/asm/page-bits.h > > +++ b/xen/arch/riscv/include/asm/page-bits.h > > @@ -1,6 +1,16 @@ > > #ifndef __RISCV_PAGE_BITS_H__ > > #define __RISCV_PAGE_BITS_H__ > > > > +#ifdef CONFIG_RISCV_64 > > +#define PAGETABLE_ORDER (9) > > +#else /* CONFIG_RISCV_32 */ > > +#define PAGETABLE_ORDER (10) > > +#endif > > + > > +#define PAGETABLE_ENTRIES (1 << PAGETABLE_ORDER) > > + > > +#define PTE_PPN_SHIFT 10 > > + > > #define PAGE_SHIFT 12 /* 4 KiB Pages */ > > #define PADDR_BITS 56 /* 44-bit PPN */ > > > > diff --git a/xen/arch/riscv/include/asm/page.h > > b/xen/arch/riscv/include/asm/page.h > > new file mode 100644 > > index 0000000000..30406aa614 > > --- /dev/null > > +++ b/xen/arch/riscv/include/asm/page.h > > @@ -0,0 +1,65 @@ > > +#ifndef _ASM_RISCV_PAGE_H > > +#define _ASM_RISCV_PAGE_H > > + > > +#include <xen/const.h> > > +#include <xen/types.h> > > + > > +#define VPN_MASK ((unsigned > > long)(PAGETABLE_ENTRIES - 1)) > > + > > +#define XEN_PT_LEVEL_ORDER(lvl) ((lvl) * PAGETABLE_ORDER) > > +#define XEN_PT_LEVEL_SHIFT(lvl) (XEN_PT_LEVEL_ORDER(lvl) + > > PAGE_SHIFT) > > +#define XEN_PT_LEVEL_SIZE(lvl) (_AT(paddr_t, 1) << > > XEN_PT_LEVEL_SHIFT(lvl)) > > +#define XEN_PT_LEVEL_MAP_MASK(lvl) (~(XEN_PT_LEVEL_SIZE(lvl) - > > 1)) > > +#define XEN_PT_LEVEL_MASK(lvl) (VPN_MASK << > > XEN_PT_LEVEL_SHIFT(lvl)) > > + > > +#define PTE_VALID BIT(0, UL) > > +#define PTE_READABLE BIT(1, UL) > > +#define PTE_WRITABLE BIT(2, UL) > > +#define PTE_EXECUTABLE BIT(3, UL) > > +#define PTE_USER BIT(4, UL) > > +#define PTE_GLOBAL BIT(5, UL) > > +#define PTE_ACCESSED BIT(6, UL) > > +#define PTE_DIRTY BIT(7, UL) > > +#define PTE_RSW (BIT(8, UL) | BIT(9, UL)) > > + > > +#define PTE_LEAF_DEFAULT (PTE_VALID | PTE_READABLE | > > PTE_WRITABLE) > > +#define PTE_TABLE (PTE_VALID) > > + > > +/* Calculate the offsets into the pagetables for a given VA */ > > +#define pt_linear_offset(lvl, va) ((va) >> > > XEN_PT_LEVEL_SHIFT(lvl)) > > + > > +#define pt_index(lvl, va) pt_linear_offset(lvl, (va) & > > XEN_PT_LEVEL_MASK(lvl)) > > + > > +/* Page Table entry */ > > +typedef struct { > > +#ifdef CONFIG_RISCV_64 > > +uint64_t pte; > > +#else > > +uint32_t pte; > > +#endif > > +} pte_t; > > Please indent both field declarations accordingly. > > > +#define addr_to_pte(x) (((x) >> PTE_PPN_SHIFT) << PAGE_SHIFT) > > This still looks to be converting _to_ an address, not to PTE layout, > ... > > > +/* Shift the VPN[x] or PPN[x] fields of a virtual or physical > > address > > + * to become the shifted PPN[x] fields of a page table entry */ > > +#define ppn_to_paddr(x) (((x) >> PAGE_SHIFT) << PTE_PPN_SHIFT) > > ... while this converts an address (not a ppn) to PTE layout (not a > paddr). Getting the names of these helpers right is crucial for easy > following of any code using them. To be honest, I'll stop reviewing > here, because the names being wrong is just going to be too > confusing. You are right the names are confusing and should be renamed. Thanks. ~ Oleksii
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |