On 11.05.2023 19:09, Oleksii Kurochko wrote:
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/page.h
> @@ -0,0 +1,58 @@
> +#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)) & VPN_MASK)
Nit: Please be consistent with parentheses. Here va doesn't need any,
but if you added / kept them, then lvl should also gain them.
> +/* Page Table entry */
> +typedef struct {
> +#ifdef CONFIG_RISCV_64
> + uint64_t pte;
> +#else
> + uint32_t pte;
> +#endif
> +} pte_t;
> +
> +static inline pte_t paddr_to_pte(paddr_t paddr,
> + unsigned int permissions)
> +{
> + return (pte_t) { .pte = (paddr >> PAGE_SHIFT) << PTE_PPN_SHIFT |
> permissions };
Please parenthesize the << against the |. I have also previously
recommended to avoid open-coding of things like PFN_DOWN() (or
paddr_to_pfn(), if you like that better) or ...
> +}
> +
> +static inline paddr_t pte_to_paddr(pte_t pte)
> +{
> + return ((paddr_t)pte.pte >> PTE_PPN_SHIFT) << PAGE_SHIFT;
... or pfn_to_paddr() (which here would avoid the misplaced cast).
> --- a/xen/arch/riscv/include/asm/processor.h
> +++ b/xen/arch/riscv/include/asm/processor.h
> @@ -69,6 +69,11 @@ static inline void die(void)
> wfi();
> }
>
> +static inline void sfence_vma(void)
> +{
> + __asm__ __volatile__ ("sfence.vma" ::: "memory");
> +}
Hmm, in switch_stack_and_jump() you use "asm volatile()" (no
underscores). This is another thing which would nice if it was
consistent (possibly among headers as one group, and .c files as
another - there may be reasons why one wants the underscore
variants in headers, but the "easier" ones in .c files).
Also nit: Style (missing blanks inside the parentheses).
> +static void __init setup_initial_mapping(struct mmu_desc *mmu_desc,
> + unsigned long map_start,
> + unsigned long map_end,
> + unsigned long pa_start)
> +{
> + unsigned int index;
> + pte_t *pgtbl;
> + unsigned long page_addr;
> +
> + if ( (unsigned long)_start % XEN_PT_LEVEL_SIZE(0) )
> + {
> + early_printk("(XEN) Xen should be loaded at 4k boundary\n");
> + die();
> + }
> +
> + if ( map_start & ~XEN_PT_LEVEL_MAP_MASK(0) ||
> + pa_start & ~XEN_PT_LEVEL_MAP_MASK(0) )
Nit: Please parenthesize the two & against the ||.
> + {
> + early_printk("(XEN) map and pa start addresses should be aligned\n");
> + /* panic(), BUG() or ASSERT() aren't ready now. */
> + die();
> + }
> +
> + for ( page_addr = map_start;
> + page_addr < map_end;
> + page_addr += XEN_PT_LEVEL_SIZE(0) )
> + {
> + pgtbl = mmu_desc->pgtbl_base;
> +
> + switch ( mmu_desc->num_levels )
> + {
> + case 4: /* Level 3 */
> + HANDLE_PGTBL(3);
> + case 3: /* Level 2 */
> + HANDLE_PGTBL(2);
> + case 2: /* Level 1 */
> + HANDLE_PGTBL(1);
> + case 1: /* Level 0 */
> + {
> + unsigned long paddr = (page_addr - map_start) + pa_start;
> + unsigned int permissions = PTE_LEAF_DEFAULT;
> + 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_rodata(LINK_TO_LOAD(page_addr)) )
> + permissions = PTE_READABLE | PTE_VALID;
> +
> + pte_to_be_written = paddr_to_pte(paddr, permissions);
> +
> + if ( !pte_is_valid(pgtbl[index]) )
> + pgtbl[index] = pte_to_be_written;
> + else
> + {
> + if ( (pgtbl[index].pte ^ pte_to_be_written.pte) &
> + ~(PTE_DIRTY | PTE_ACCESSED) )
Nit: Style (indentation).
Jan
