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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 1/29/25 3:01 PM, Jan Beulich wrote:

> On 29.01.2025 14:12, Oleksii Kurochko wrote:
> > On 1/28/25 9:14 AM, Jan Beulich wrote:
> > > On 27.01.2025 18:22, 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?
> > > >
> > > > I am also considering returning a structure containing the|mfn| (or|paddr_t|) and adding other properties (such as permissions or
> > > > page size) as needed in the future. Both solutions seem more or less equivalent.
> > > >           
> > > Imo the most natural thing for a page walking function would be to return the
> > > leaf PTE (or the leaf-most not-present [or otherwise "no-access"] one). That
> > > would provide (almost) all possible information to the caller. "Almost"
> > > because depending on how page walk works, permissions may combine across page
> > > table levels. Yet then (see also the "no-access" above) this would also
> > > require further input, to specify the context for which the translation is
> > > being seeked. For example, the intention to write may want to yield no valid
> > > PTE when there are present ones down to the leaf, but effective permissions
> > > say "read-only".
> > >         
> > Perhaps returning the leaf PTE could be a really good option.
> >
> > I'm not entirely sure I understand what you mean by "leaf-most not-present". Could you please try to explain this moment one more time?
> > My expectation was that the function should return an existing leaf PTE (from which "access" rights could be determined)
> > or|NULL| to indicate that no leaf PTE was found.
> >       
> "no leaf PTE" may be for a variety of reasons. Hence why I think returning
> the PTE at which the walk stopped (leaf or leaf-most not-present) is likely
> best. Such a not-present PTE may, after all, still contain valuable
> information; it's not like it has to be all zero.
>     
Thanks, it is clearer now.

It will complicate a little bit vmap_to_mfn() (as we should to check that pt_walk()
returns a leaf; otherwise something wrong happens), but I think it is not really
critical as you mentioned before, and for convenience it would be better to implement
it as a static inline function:

  static inline mfn_t vmap_to_mfn(vaddr_t va)
  {
      pte_t *entry = pt_walk(va, NULL);

      BUG_ON(!pte_is_mapping(*entry));

      return mfn_from_pte(*entry);
  }
> > Another thing I'm curious about is whether this would be sufficient for determining the level.
> > It seems clear that, given a PTE and a virtual address, we could compute:
> > |mask = VA | paddr_from_pte(pte)|
> >       
> What would this value represent? No, from holding a PTE in your hands you
> can't determine the level it came from. So yes, ...
>
> > Then, iterating through each level, we could apply and understand on which one level it was mapped:
> > |mask & (BIT(XEN_PT_LEVEL_ORDER(i), UL) - 1)|.
> >
> > If I haven't overlooked any other way to calculate the page table level, would it be better to simply add another argument
> > to|pt_walk()| to return the level.
> >       
> ... for callers who care doing this might then be necessary (this would be
> a pointer parameter, and since I expect many callers wouldn't care about
> the level, it likely wants to be permissible to pass in NULL).
>
> Question then is whether it's better to hand back the level or the page
> order of the mapping. On x86 we return the latter from P2M lookups, for
> example.
>     
Actually, I think for proper calculation of order in pt_update().

Thanks.

~ Oleksii