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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 1/27/25 11:24 AM, Jan Beulich wrote:

> 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 agree that it is wasteful but, at that moment, it seemed to me the faster way to resolve that.

>  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.
>     
I think I got your idea. I will try to implement that.

Thanks!

~ Oleksii