Re: [PATCH v6 2/4] xen/riscv: introduce setup_initial_pages

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Tue, 2023-05-09 at 16:38 +0200, Jan Beulich wrote:
> On 09.05.2023 14:59, Oleksii wrote:
> > On Mon, 2023-05-08 at 10:58 +0200, Jan Beulich wrote:
> > > On 03.05.2023 18:31, Oleksii Kurochko wrote:
> > > > --- /dev/null
> > > > +++ b/xen/arch/riscv/include/asm/page.h
> > > > @@ -0,0 +1,62 @@
> > > > +#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))
> > > 
> > > Maybe better
> > > 
> > > #define pt_index(lvl, va) (pt_linear_offset(lvl, va) & VPN_MASK)
> > > 
> > > as the involved constant will be easier to use for the compiler?
> > But VPN_MASK should be shifted by level shift value.
> 
> Why? pt_linear_offset() already does the necessary shifting.
I missed a way how you offered to define pt_index(). I thought you
offered to define it as "pt_linear_offset(lvl, va & VPN_MASK)" instead
of
"(pt_linear_offset(lvl, va) & VPN_MASK)".

So you are right we can re-write as you offered.

> 
> > > > +    csr_write(CSR_SATP, 0);
> > > > +
> > > > +    /* Clean MMU root page table */
> > > > +    stage1_pgtbl_root[index] = paddr_to_pte(0x0, 0x0);
> > > > +
> > > > +    asm volatile ( "sfence.vma" );
> > > 
> > > Doesn't this want to move between the SATP write and the clearing
> > > of
> > > the
> > > root table slot? Also here and elsewhere - don't these asm()s
> > > need
> > > memory
> > > clobbers? And anyway - could I talk you into introducing an
> > > inline
> > > wrapper
> > > (e.g. named sfence_vma()) so all uses end up consistent?
> > I think clearing of root page table should be done before
> > "sfence.vma"
> > because we have to first clear slot of MMU's root page table and
> > then
> > make updating root page table visible for all. ( by usage of sfence
> > instruction )
> 
> I disagree. The SATP write has removed the connection of the CPU
> to the page tables. That's the action you want to fence, not the
> altering of some (then) no longer referenced data structure.
>From that point of view you are right. Thanks for clarification.
> 
> > > > +void __init setup_initial_pagetables(void)
> > > > +{
> > > > +    struct mmu_desc mmu_desc = { 0, 0, NULL, NULL };
> > > > +
> > > > +    /*
> > > > +     * Access to _stard, _end is always PC-relative
> > > 
> > > Nit: Typo-ed symbol name. Also ...
> > > 
> > > > +     * thereby when access them we will get load adresses
> > > > +     * of start and end of Xen
> > > > +     * To get linker addresses LOAD_TO_LINK() is required
> > > > +     * to use
> > > > +     */
> > > 
> > > see the earlier line wrapping remark again. Finally in multi-
> > > sentence
> > > comments full stops are required.
> > Full stops mean '.' at the end of sentences?
> 
> Yes. Please see ./CODING_STYLE.
Thanks. I'll read it again.

~ Oleksii