Re: [PATCH v2 for 4.20? 1/3] xen/riscv: implement software page table walking

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.02.2025 14:12, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/include/asm/mm.h
> +++ b/xen/arch/riscv/include/asm/mm.h
> @@ -156,6 +156,10 @@ static inline struct page_info *virt_to_page(const void 
> *v)
>      return frametable_virt_start + PFN_DOWN(va - directmap_virt_start);
>  }
>  
> +#include <asm/page.h>

asm/page.h already includes asm/mm.h, so you're introducing a circular
dependency here (much of which the patch description is about, so it's
unclear why you didn't solve this another way). Afaict ...

> +pte_t * pt_walk(vaddr_t va, unsigned int *pte_level);

... it's pte_t that presents a problem here. Why not simply put the
declaration in asm/page.h (and drop all the secondary changes that
don't really belong in this patch anyway)?

Also nit: Excess blank after the first *.

> --- a/xen/arch/riscv/pt.c
> +++ b/xen/arch/riscv/pt.c
> @@ -274,6 +274,61 @@ static int pt_update_entry(mfn_t root, vaddr_t virt,
>      return rc;
>  }
>  
> +/*
> + * pt_walk() performs software page table walking and returns the pte_t of
> + * a leaf node or the leaf-most not-present pte_t if no leaf node is found
> + * for further analysis.
> + * Additionally, pt_walk() returns the level of the found pte.
> + */
> +pte_t * pt_walk(vaddr_t va, unsigned int *pte_level)

See nit above.

> +{
> +    const mfn_t root = get_root_page();
> +    /*
> +     * In pt_walk() only XEN_TABLE_MAP_NONE and XEN_TABLE_SUPER_PAGE are
> +     * handled (as they are only possible for page table walking), so
> +     * initialize `ret` with "impossible" XEN_TABLE_MAP_NOMEM.
> +     */
> +    int ret = XEN_TABLE_MAP_NOMEM;
> +    unsigned int level = HYP_PT_ROOT_LEVEL;

With this initialization and ...

> +    pte_t *table;
> +
> +    DECLARE_OFFSETS(offsets, va);
> +
> +    table = map_table(root);
> +
> +    /*
> +     * Find `table` 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;

(nit: found)

> +     *   (Despite the name) XEN_TABLE_SUPER_PAGE also covers 4K mappings. If
> +     *   pt_next_level() is called for page table level 0, it results in the
> +     *   entry being a pointer to a leaf node, thereby returning
> +     *   XEN_TABLE_SUPER_PAGE, despite of the fact this leaf covers 4k 
> mapping.
> +     * - 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--;

... this being the only updating of the variable, what are the assertion and
accompanying comment about? What you're rather at risk of is for level to
wrap around through 0. In fact I think it does, ...

> +    }
> +
> +    if ( pte_level )
> +        *pte_level = level + 1;
> +
> +    return table + offsets[level + 1];
> +}

... which you account for by adding 1 in both of the places here. Don't you
think that it would be better to avoid such, e.g.:

    for ( level = HYP_PT_ROOT_LEVEL; ; --level )
    {
        int ret = pt_next_level(false, &table, offsets[level]);

        if ( ret == XEN_TABLE_MAP_NONE || ret == XEN_TABLE_SUPER_PAGE )
            break;
        ASSERT(level);
    } 

or

    for ( level = CONFIG_PAGING_LEVELS; level--; )
    {
        int ret = pt_next_level(false, &table, offsets[level]);

        if ( ret == XEN_TABLE_MAP_NONE || ret == XEN_TABLE_SUPER_PAGE )
            break;
    } 

This then also avoids the oddity about ret's initializer.

Jan