|
[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 |