Re: [PATCH for 4.20? v3 2/3] xen/riscv: update defintion of vmap_to_mfn()

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 2/7/25 2:30 PM, Jan Beulich wrote:

> On 07.02.2025 14:13, 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 mfn from pte_t.
> >
> > To avoid introduce a circular dependency between asm/mm.h and asm/page.h by
> > including each other, the macro _vmap_to_mfn() is introduced in asm/page.h,
> > as it uses struct pte_t and pte_is_mapping() from asm/page.h. _vmap_to_mfn()
> > macro is then reused in the definition of vmap_to_mfn() macro in asm/mm.h.
> >
> > Fixes: 7db8d2bd9b ("xen/riscv: add minimal stuff to mm.h to build full Xen")
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> > ---
> > Changes in v3:
> > - Move vmap_to_mfn_ to asm/page.h to deal with circular dependency.
> > - Convert vmap_to_mfn_() to macros.
> >       
> Why both?
>     
Just for consistency. vmap_to_mfn_() could be defined as static inline, I can
send the new patch version with such changes, probably, static inline would be
better in this case.

> > --- a/xen/arch/riscv/include/asm/page.h
> > +++ b/xen/arch/riscv/include/asm/page.h
> > @@ -210,6 +210,13 @@ static inline pte_t pte_from_mfn(mfn_t mfn, unsigned int flags)
> >  
> >  pte_t pt_walk(vaddr_t va, unsigned int *pte_level);
> >  
> > +#define _vmap_to_mfn(va)                \
> > +({                                      \
> > +    pte_t entry = pt_walk((va), NULL);  \
> >       
> If this is to remain a macro, va doesn't need parenthesizing (as the argument
> passed is just the identifier, not an _expression_.
>
> Be careful with the naming of macro local variables. Consider a use size (for
> whatever reason) having
>
>     unsigned long entry;
>     ...
>     mfn = vmap_to_mfn(entry);
>
> This is where appending an underscore comes into play.
>     
This could be another reason to use static inline _vmap_to_mfn(...) instead of a macro.

I think I will rewrite the _vmap_to_mfn() macro as a static inline function in the next
patch series version. I'll send it after receiving comments on the entire patch series.

Thanks.

~ Oleksii