Re: [PATCH v7 2/5] xen/riscv: introduce setup_initial_pages

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Tue, 2023-05-16 at 18:02 +0200, Jan Beulich wrote:
> 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.
Sure. I'll fix that.

> 
> > +/* 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 ...
I'll change it to paddr_to_pfn. It sounds more clear than PFN_DOWN()
in this context.
Thanks for reminder.
> 
> > +}
> > +
> > +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).
pfn_to_paddr() would be better here so I'll update it.

> 
> > --- 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).
I will remove "__" to be consistent here and in future.
> 
> Also nit: Style (missing blanks inside the parentheses).
THanks. Missed that.
> 
> > +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 ||.
Sure. THanks.
> 
> > +    {
> > +        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).
I'll add missed space. Thanks.

~ Oleksii