Re: [PATCH v2 for 4.20? 1/3] xen/riscv: implement software page table walking

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 2/4/25 2:50 PM, Jan Beulich wrote:

> On 03.02.2025 14:12, Oleksii Kurochko wrote:
> > --- a/xen/arch/riscv/include/asm/mm.h
> > +++ b/xen/arch/riscv/include/asm/mm.h
> > @@ -156,6 +156,10 @@ static inline struct page_info *virt_to_page(const void *v)
> >      return frametable_virt_start + PFN_DOWN(va - directmap_virt_start);
> >  }
> >  
> > +#include <asm/page.h>
> >       
> asm/page.h already includes asm/mm.h, so you're introducing a circular
> dependency here (much of which the patch description is about, so it's
> unclear why you didn't solve this another way). Afaict ...
>
> > +pte_t * pt_walk(vaddr_t va, unsigned int *pte_level);
> >       
> ... it's pte_t that presents a problem here. Why not simply put the
> declaration in asm/page.h (and drop all the secondary changes that
> don't really belong in this patch anyway)?
>     
In the patch 2 it is used for implementing vmap_to_mfn():
  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);
  }

  #define vmap_to_mfn(va)     vmap_to_mfn_((vaddr_t)va)mfn_from_pte
what leads to including of <asm/page.h> in <asm/mm.h>.

As an option, if to move the following to <asm/page.h>:
  #define vmap_to_mfn(va)     vmap_to_mfn_((vaddr_t)va)
  #define vmap_to_page(va)    mfn_to_page(vmap_to_mfn(va))
the circular dependency could be dropped.

> Also nit: Excess blank after the first *.
>
> > --- a/xen/arch/riscv/pt.c
> > +++ b/xen/arch/riscv/pt.c
> > @@ -274,6 +274,61 @@ static int pt_update_entry(mfn_t root, vaddr_t virt,
> >      return rc;
> >  }
> >  
> > +/*
> > + * pt_walk() performs software page table walking and returns the pte_t of
> > + * a leaf node or the leaf-most not-present pte_t if no leaf node is found
> > + * for further analysis.
> > + * Additionally, pt_walk() returns the level of the found pte.
> > + */
> > +pte_t * pt_walk(vaddr_t va, unsigned int *pte_level)
> >       
> See nit above.
>
> > +{
> > +    const mfn_t root = get_root_page();
> > +    /*
> > +     * In pt_walk() only XEN_TABLE_MAP_NONE and XEN_TABLE_SUPER_PAGE are
> > +     * handled (as they are only possible for page table walking), so
> > +     * initialize `ret` with "impossible" XEN_TABLE_MAP_NOMEM.
> > +     */
> > +    int ret = XEN_TABLE_MAP_NOMEM;
> > +    unsigned int level = HYP_PT_ROOT_LEVEL;
> >       
> With this initialization and ...
>
> > +    pte_t *table;
> > +
> > +    DECLARE_OFFSETS(offsets, va);
> > +
> > +    table = map_table(root);
> > +
> > +    /*
> > +     * Find `table` of an entry which corresponds to `va` by iterating for each
> > +     * page level and checking if the entry points to a next page table or
> > +     * to a page.
> > +     *
> > +     * Two cases are possible:
> > +     * - ret == XEN_TABLE_SUPER_PAGE means that the entry was find;
> >       
> (nit: found)
>
> > +     *   (Despite the name) XEN_TABLE_SUPER_PAGE also covers 4K mappings. If
> > +     *   pt_next_level() is called for page table level 0, it results in the
> > +     *   entry being a pointer to a leaf node, thereby returning
> > +     *   XEN_TABLE_SUPER_PAGE, despite of the fact this leaf covers 4k mapping.
> > +     * - ret == XEN_TABLE_MAP_NONE means that requested `va` wasn't actually
> > +     *   mapped.
> > +     */
> > +    while ( (ret != XEN_TABLE_MAP_NONE) && (ret != XEN_TABLE_SUPER_PAGE) )
> > +    {
> > +        /*
> > +         * This case shouldn't really occur as it will mean that for table
> > +         * level 0 a pointer to next page table has been written, but at
> > +         * level 0 it could be only a pointer to 4k page.
> > +         */
> > +        ASSERT(level <= HYP_PT_ROOT_LEVEL);
> > +
> > +        ret = pt_next_level(false, &table, offsets[level]);
> > +        level--;
> >       
> ... this being the only updating of the variable, what are the assertion and
> accompanying comment about? What you're rather at risk of is for level to
> wrap around through 0. In fact I think it does, ...
>
> > +    }
> > +
> > +    if ( pte_level )
> > +        *pte_level = level + 1;
> > +
> > +    return table + offsets[level + 1];
> > +}
> >       
> ... which you account for by adding 1 in both of the places here. Don't you
> think that it would be better to avoid such, e.g.:
>
>     for ( level = HYP_PT_ROOT_LEVEL; ; --level )
>     {
>         int ret = pt_next_level(false, &table, offsets[level]);
>
>         if ( ret == XEN_TABLE_MAP_NONE || ret == XEN_TABLE_SUPER_PAGE )
>             break;
>         ASSERT(level);
>     } 
>
> or
>
>     for ( level = CONFIG_PAGING_LEVELS; level--; )
>     {
>         int ret = pt_next_level(false, &table, offsets[level]);
>
>         if ( ret == XEN_TABLE_MAP_NONE || ret == XEN_TABLE_SUPER_PAGE )
>             break;
>     } 
>
> This then also avoids the oddity about ret's initializer.
>     
We can do in the way you suggested, it is more clear. I will go with the second option.

Thanks.

~ Oleksii