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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 2/19/25 12:14 PM, Jan Beulich wrote:

> On 12.02.2025 17:50, Oleksii Kurochko wrote:
> > --- a/xen/arch/riscv/pt.c
> > +++ b/xen/arch/riscv/pt.c
> > @@ -185,6 +185,68 @@ static int pt_next_level(bool alloc_tbl, pte_t **table, unsigned int offset)
> >      return XEN_TABLE_NORMAL;
> >  }
> >  
> > +/*
> > + * _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 by using
> > + * `pte_level` argument.
> > + * `pte_level` is optional, set `pte_level`=NULL if a caller doesn't need
> > + * the level of the found pte.
> >       
> How about this, reducing redundancy a little?
>
>  * _pt_walk() can optionally return the level of the found pte. Pass NULL
>  * for `pte_level` if this information isn't needed.
>
> > +pte_t pt_walk(vaddr_t va, unsigned int *pte_level)
> > +{
> > +    pte_t *entry = _pt_walk(va, pte_level);
> > +    pte_t pte = *entry;
> > +
> > +    unmap_table(entry);
> > +
> > +    return pte;
> > +}
> >       
> "entry" especially in this context is ambiguous. I would expect a variable of
> this name to be of type pte_t, not pte_t *. How about "ptep"?
>     
Agree with both your suggestions, it would be better to use `ptep instead of `entry`
and rephrase the comment.

> Preferably with these adjustments, which I'd be fine making while committing,
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>
> Considering the 4.20? tag you'll need to decide whether you still want this
> in before the release.
>     
Considering that it is still needed a new version for patch3 of this patch series and
that the mentioned issues aren't affected no one, lets consider the full patch series for
4.21.

Thanks.

~ Oleksii