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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

>>> +    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.

>>> +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.

Jan