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: g...@github.com:xvisor/xvisor.git 9be2fdd7
> > Signed-off-by: Oleksii Kurochko
> > ---
> > 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
> >
> > - 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 dif
Re: [PATCH v4 1/3] xen/riscv: introduce setup_initial_pages
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: g...@github.com:xvisor/xvisor.git 9be2fdd7
> Signed-off-by: Oleksii Kurochko
> ---
> 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.
> * 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
>
> - 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() fun
