Re: [PATCH v1 2/3] xen/riscv: update defintion of vmap_to_mfn()

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 1/27/25 11:09 AM, Jan Beulich wrote:

> On 20.01.2025 17:54, Oleksii Kurochko wrote:
> > vmap_to_mfn() uses virt_to_maddr(), which is designed to work with VA from
> > either the direct map region or Xen's linkage region (XEN_VIRT_START).
> > An assertion will occur if it is used with other regions, in particular for
> > the VMAP region.
> >
> > Since RISC-V lacks a hardware feature to request the MMU to translate a VA to
> > a PA (as Arm does, for example), software page table walking (pt_walk()) is
> > used for the VMAP region to obtain the PA.
> >
> > Fixes: 7db8d2bd9b ("xen/riscv: add minimal stuff to mm.h to build full Xen")
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> >       
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>
> > --- a/xen/arch/riscv/include/asm/mm.h
> > +++ b/xen/arch/riscv/include/asm/mm.h
> > @@ -25,7 +25,7 @@ paddr_t pt_walk(vaddr_t va);
> >  #define gaddr_to_gfn(ga)    _gfn(paddr_to_pfn(ga))
> >  #define mfn_to_maddr(mfn)   pfn_to_paddr(mfn_x(mfn))
> >  #define maddr_to_mfn(ma)    _mfn(paddr_to_pfn(ma))
> > -#define vmap_to_mfn(va)     maddr_to_mfn(virt_to_maddr((vaddr_t)(va)))
> > +#define vmap_to_mfn(va)     maddr_to_mfn(pt_walk((vaddr_t)(va)))
> >       
> With this being the first use of pt_walk(), I wonder whether the function might
> better return mfn_t (and simply ignore the low 12 bits of Vthe incoming VA; see
> my respective comment on the earlier patch). After all it is quite natural for
> a page table walk to return a page frame number, not a physical address.
>     
I think it would be really better to return mfn_t (now I understand your comment on the earlier
patch better, if only I had read this comment first...)

Thanks.

~ Oleksii