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.kuroc...@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
