On 27.01.2025 18:41, Oleksii Kurochko wrote: > > On 1/27/25 6:22 PM, Oleksii Kurochko wrote: >> >> >> On 1/27/25 1:57 PM, Jan Beulich wrote: >>> On 27.01.2025 13:29, Oleksii Kurochko wrote: >>>> On 1/27/25 11:06 AM, Jan Beulich wrote: >>>>> 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. >>>> In the long run, this function's prototype looks like|paddr_t >>>> pt_walk(vaddr_t root, vaddr_t va, bool is_xen)| [1]. However, I'm not sure >>>> if it will stay that way, >>>> as I think|is_xen| could be skipped, since using|map_table()| should be >>>> sufficient (as it now considers|system_state|) and I'm not really sure if >>>> I need root argument >>>> as initial goal was to use this function for debug only purposes and I've >>>> never used it for guest page table (stage-1) walking. >>>> Anyway, yes, it is still returning a physical address, and that seems >>>> enough to me. >>>> >>>> Could you share your thoughts on what I should take into account for >>>> returning value, probably, I am missing something really useful? >>> Often you care about the permissions as well. Sometimes it may even be >>> relevant >>> to know the (super-)page size of the mapping. >> Perhaps it would be better to change the prototype to: >> bool pt_walk(vaddr_t va, mfn_t *ret_pa); >> or even >> void pt_walk(vaddr_t va, mfn_t *ret_pa); >> In this case,|ret_pa = INVALID_MFN| could serve as a signal >> that|pt_walk()| failed. >> If there's a need to return permissions or (super-)page size in the future, >> another argument could be added. >> What do you think? Would this approach be better? > > We have to return mfn_t or paddr_t as pt_walk() is used invmap_to_mfn().
That use doesn't really limit what the function needs to return. It merely affects how simple (or complicated) the invocation there would be. Jan
