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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

> @@ -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)?

Jan