Re: [PATCH for 4.20? v4 3/3] xen/riscv: update mfn calculation in pt_mapping_level()

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 2/19/25 12:28 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
> > @@ -249,12 +249,10 @@ pte_t pt_walk(vaddr_t va, unsigned int *pte_level)
> >  
> >  /* Update an entry at the level @target. */
> >  static int pt_update_entry(mfn_t root, vaddr_t virt,
> > -                           mfn_t mfn, unsigned int target,
> > +                           mfn_t mfn, unsigned int *target,
> >                             unsigned int flags)
> >  {
> >      int rc;
> > -    unsigned int level = HYP_PT_ROOT_LEVEL;
> > -    pte_t *table;
> >      /*
> >       * The intermediate page table shouldn't be allocated when MFN isn't
> >       * valid and we are not populating page table.
> > @@ -265,41 +263,48 @@ static int pt_update_entry(mfn_t root, vaddr_t virt,
> >       * combinations of (mfn, flags).
> >      */
> >      bool alloc_tbl = !mfn_eq(mfn, INVALID_MFN) || (flags & PTE_POPULATE);
> > -    pte_t pte, *entry;
> > -
> > -    /* convenience aliases */
> > -    DECLARE_OFFSETS(offsets, virt);
> > +    pte_t pte, *entry = NULL;
> >       
> With there also being "table" below, "entry" isn't quite as bad as in the
> other patch. Yet I'd still like to ask that you consider renaming.
>
> > -    table = map_table(root);
> > -    for ( ; level > target; level-- )
> > +    if ( *target == CONFIG_PAGING_LEVELS )
> > +        entry = _pt_walk(virt, target);
> >       
> Imo it's quite important for the comment ahead of the function to be updated
> to mention this special case.
>
> > +    else
> >      {
> > -        rc = pt_next_level(alloc_tbl, &table, offsets[level]);
> > -        if ( rc == XEN_TABLE_MAP_NOMEM )
> > +        pte_t *table;
> > +        unsigned int level = HYP_PT_ROOT_LEVEL;
> > +        /* convenience aliases */
> >       
> Nit: Style.
>     
From the 'Comments' section of CODING_STYLE, I see that the comment should start
with capital letter. Do you mean that?

> > @@ -331,7 +336,8 @@ static int pt_update_entry(mfn_t root, vaddr_t virt,
> >      rc = 0;
> >  
> >   out:
> > -    unmap_table(table);
> > +    if ( entry )
> > +        unmap_table(entry);
> >       
> Would it perhaps be worth for unmap_table() to gracefully handle being passed
> NULL, to avoid such conditionals (there may be more in the future)?
>     
Agree, it would be more safe to move this check inside unmap_table(). I will update
that.

Thanks.
~ Oleksii