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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.kurochko@xxxxxxxxx>
>>>>>> ---
>>>>>>    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