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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 2/10/25 5:53 PM, Jan Beulich wrote:

> On 07.02.2025 14:13, Oleksii Kurochko wrote:
> > --- a/xen/arch/riscv/pt.c
> > +++ b/xen/arch/riscv/pt.c
> > @@ -238,11 +238,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;
> >       
> Considering the lack of an initializer here, ...
>
> > @@ -256,39 +255,45 @@ static int pt_update_entry(mfn_t root, vaddr_t virt,
> >      bool alloc_tbl = !mfn_eq(mfn, INVALID_MFN) || (flags & PTE_POPULATE);
> >      pte_t pte, *entry;
> >  
> > -    /* convenience aliases */
> > -    DECLARE_OFFSETS(offsets, virt);
> > -
> > -    table = map_table(root);
> > -    for ( ; level > target; level-- )
> > +    if ( *target == CONFIG_PAGING_LEVELS )
> > +        entry = _pt_walk(virt, target);
> > +    else
> >      {
> > -        rc = pt_next_level(alloc_tbl, &table, offsets[level]);
> > -        if ( rc == XEN_TABLE_MAP_NOMEM )
> > +        unsigned int level = HYP_PT_ROOT_LEVEL;
> > +        /* convenience aliases */
> > +        DECLARE_OFFSETS(offsets, virt);
> > +
> > +        table = map_table(root);
> > +        for ( ; level > *target; level-- )
> >          {
> > -            rc = -ENOMEM;
> > -            goto out;
> > +            rc = pt_next_level(alloc_tbl, &table, offsets[level]);
> > +            if ( rc == XEN_TABLE_MAP_NOMEM )
> > +            {
> > +                rc = -ENOMEM;
> > +                goto out;
> > +            }
> > +
> > +            if ( rc == XEN_TABLE_MAP_NONE )
> > +            {
> > +                rc = 0;
> > +                goto out;
> > +            }
> > +
> > +            if ( rc != XEN_TABLE_NORMAL )
> > +                break;
> >          }
> >  
> > -        if ( rc == XEN_TABLE_MAP_NONE )
> > +        if ( level != *target )
> >          {
> > -            rc = 0;
> > +            dprintk(XENLOG_ERR,
> > +                    "%s: Shattering superpage is not supported\n", __func__);
> > +            rc = -EOPNOTSUPP;
> >              goto out;
> >          }
> >  
> > -        if ( rc != XEN_TABLE_NORMAL )
> > -            break;
> > -    }
> > -
> > -    if ( level != target )
> > -    {
> > -        dprintk(XENLOG_ERR,
> > -                "%s: Shattering superpage is not supported\n", __func__);
> > -        rc = -EOPNOTSUPP;
> > -        goto out;
> > +        entry = table + offsets[level];
> >      }
> >  
> > -    entry = table + offsets[level];
> > -
> >      rc = -EINVAL;
> >      if ( !pt_check_entry(*entry, mfn, flags) )
> >          goto out;
> >       
> ... I'm surprised the compiler doesn't complain about use of a possibly
> uninitialized variable at
>
>  out:
>     unmap_table(table);
>
> For the new path you're adding the variable is uninitialized afaict.
> Which implies that you're again failing to unmap what _pt_walk() has
> handed you.
>     
Thanks, unmapping of table and entry (in the case of the new patch) should be
really updated. I'll take care of it in the next patch version.

~ Oleksii