Re: [PATCH v1 1/3] xen/riscv: implement software page table walking

[Jan Beulich <jbeulich@suse.com>]

On 20.01.2025 17:54, Oleksii Kurochko wrote:
> RISC-V doesn't have hardware feature to ask MMU to translate
> virtual address to physical address ( like Arm has, for example ),
> so software page table walking in implemented.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
>  xen/arch/riscv/include/asm/mm.h |  2 ++
>  xen/arch/riscv/pt.c             | 56 +++++++++++++++++++++++++++++++++
>  2 files changed, 58 insertions(+)
> 
> diff --git a/xen/arch/riscv/include/asm/mm.h b/xen/arch/riscv/include/asm/mm.h
> index 292aa48fc1..d46018c132 100644
> --- a/xen/arch/riscv/include/asm/mm.h
> +++ b/xen/arch/riscv/include/asm/mm.h
> @@ -15,6 +15,8 @@
>  
>  extern vaddr_t directmap_virt_start;
>  
> +paddr_t pt_walk(vaddr_t va);

In the longer run, is returning just the PA really going to be sufficient?
If not, perhaps say a word on the limitation in the description.

> --- a/xen/arch/riscv/pt.c
> +++ b/xen/arch/riscv/pt.c
> @@ -274,6 +274,62 @@ static int pt_update_entry(mfn_t root, vaddr_t virt,
>      return rc;
>  }
>  
> +paddr_t pt_walk(vaddr_t va)
> +{
> +    const mfn_t root = get_root_page();
> +    /*
> +     * In pt_walk() only XEN_TALE_MAP_NONE and XEN_TABLE_SUPER_PAGE are

Nit: s/TALE/TABLE/ ?

> +     * handled ( as they are only possible for page table walking ), so

Nit: Blanks again inside parentheses in a comment.

> +     * initialize `ret` with "impossible" XEN_TABLE_MAP_NOMEM.
> +    */
> +    int ret = XEN_TABLE_MAP_NOMEM;
> +    unsigned int level = HYP_PT_ROOT_LEVEL;
> +    paddr_t pa = 0;

Seeing 0 as initializer here, just to double check: You do prevent MFN 0
to be handed to the page allocator, and you also don't use it "manually"
anywhere?

> +    pte_t *table;
> +
> +    DECLARE_OFFSETS(offsets, va);
> +
> +    table = map_table(root);
> +
> +    /*
> +     * Find `pa` of an entry which corresponds to `va` by iterating for each
> +     * page level and checking if the entry points to a next page table or
> +     * to a page.
> +     *
> +     * Two cases are possible:
> +     * - ret == XEN_TABLE_SUPER_PAGE means that the entry was find;
> +     *   (Despite of the name) XEN_TABLE_SUPER_PAGE covers 4k mapping too.
> +     * - ret == XEN_TABLE_MAP_NONE means that requested `va` wasn't actually
> +     *   mapped.
> +     */
> +    while ( (ret != XEN_TABLE_MAP_NONE) && (ret != XEN_TABLE_SUPER_PAGE) )
> +    {
> +        /*
> +         * This case shouldn't really occur as it will mean that for table
> +         * level 0 a pointer to next page table has been written, but at
> +         * level 0 it could be only a pointer to 4k page.
> +         */
> +        ASSERT(level <= HYP_PT_ROOT_LEVEL);
> +
> +        ret = pt_next_level(false, &table, offsets[level]);
> +        level--;
> +    }
> +
> +    if ( ret == XEN_TABLE_MAP_NONE )
> +        dprintk(XENLOG_WARNING, "Is va(%#lx) really mapped?\n", va);

Even if it's a dprintk(), I'd recommend against adding such.

> +    else if ( ret == XEN_TABLE_SUPER_PAGE )
> +        pa = pte_to_paddr(*(table + offsets[level + 1]));

Missing "else if ( ret == XEN_TABLE_NORMAL )" (or maybe simply "else")?

> +    /*
> +     * There is no need for unmap_table() after each pt_next_level() call as
> +     * pt_next_level() will do unmap_table() for the previous table before
> +     * returning next level table.
> +     */
> +    unmap_table(table);

I don't think the comment is needed. You map once before the loop, so it's
natural that you unmap once after.

> +    return pa;

Don't you want to OR in the low 12 bits of VA here (unless "pa" is still 0)?

Jan