Re: [PATCH v1 3/3] xen/riscv: update mfn calculation in pt_mapping_level()

[Jan Beulich <jbeulich@suse.com>]

On 20.01.2025 17:54, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/pt.c
> +++ b/xen/arch/riscv/pt.c
> @@ -346,9 +346,33 @@ static int pt_mapping_level(unsigned long vfn, mfn_t mfn, unsigned long nr,
>          return level;
>  
>      /*
> -     * Don't take into account the MFN when removing mapping (i.e
> -     * MFN_INVALID) to calculate the correct target order.
> +     * `mfn` should be set properly in cases when modifying/destroying a
> +     * mapping to ensure the correct page table `level` is received. In the
> +     * case of `mfn` == INVALID_MFN, the `mask` will take into account only
> +     * `vfn` and could accidentally return an incorrect level. For example,
> +     * if `vfn` is page table level 1 aligned, but it was mapped as page table
> +     * level 0, then without the check below it will return `level` = 1
> +     * because only `vfn`, which is page table level 1 aligned, is taken into
> +     * account when `mfn` == INVALID_MFN.
>       *
> +     * POPULATE shouldn't be considered as `va` hasn't been mapped yet.
> +     */
> +    if ( mfn_eq(mfn, INVALID_MFN) && !(flags & PTE_POPULATE) )
> +    {
> +        vaddr_t va = vfn << PAGE_SHIFT;
> +        paddr_t pa;
> +        unsigned long xen_virt_end = (XEN_VIRT_START + XEN_VIRT_SIZE - 1);
> +
> +        if ( ((va >= DIRECTMAP_VIRT_START) && (va <= DIRECTMAP_VIRT_END)) ||
> +             ((va >= XEN_VIRT_START) && (va <= xen_virt_end)) )
> +            pa = virt_to_maddr(va);
> +        else
> +            pa = pt_walk(va);
> +
> +        mfn = _mfn(paddr_to_pfn(pa));
> +    }

Doing a walk here and then another in pt_update() feels wasteful. I
wonder whether the overall approach to page table updating doesn't want
changing. It ought to be possible to tell an "update" operation to walk
down to wherever the leaf entry is, and update there.

Jan