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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.02.2025 14:13, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/pt.c
> +++ b/xen/arch/riscv/pt.c
> @@ -185,6 +185,57 @@ static int pt_next_level(bool alloc_tbl, pte_t **table, 
> unsigned int offset)
>      return XEN_TABLE_NORMAL;
>  }
>  
> +/*
> + * _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.

That's optional, which I think wants expressing here.

> + */
> +static pte_t *_pt_walk(vaddr_t va, unsigned int *pte_level)
> +{
> +    const mfn_t root = get_root_page();
> +    unsigned int level;
> +    pte_t *table;
> +
> +    DECLARE_OFFSETS(offsets, va);
> +
> +    table = map_table(root);

This mapping operation doesn't look to have a counterpart. Aiui ...

> +    /*
> +     * 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 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.
> +     */
> +    for ( level = HYP_PT_ROOT_LEVEL; ; --level )
> +    {
> +        int ret = pt_next_level(false, &table, offsets[level]);

... the mapping may be replaced here, but a new mapping will then still
be held by this function and ...

> +        if ( ret == XEN_TABLE_MAP_NONE || ret == XEN_TABLE_SUPER_PAGE )
> +            break;
> +
> +        ASSERT(level);
> +    }
> +
> +    if ( pte_level )
> +        *pte_level = level;
> +
> +    return table + offsets[level];
> +}

... the final one then be transferred to the caller.

> +pte_t pt_walk(vaddr_t va, unsigned int *pte_level)
> +{
> +    return *_pt_walk(va, pte_level);
> +}

Hence aiui there needs to be an unmap operation here.

Jan