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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan