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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 2/25/25 8:59 AM, Jan Beulich wrote:

> On 20.02.2025 18:44, Oleksii Kurochko wrote:
> > When pt_update() is called with arguments (..., INVALID_MFN, ..., 0 or 1),
> > it indicates that a mapping is being destroyed/modifyed.
> >
> > In the case when modifying or destroying a mapping, it is necessary to
> > search until a leaf node is found, instead of searching for a page table
> > entry based on the precalculated `level` and `order`(look at pt_update()).
> > This is because when `mfn` == INVALID_MFN, the `mask` (in pt_mapping_level())
> > will take into account only `vfn`, which could accidentally return an
> > incorrect level, leading to the discovery of an incorrect page table entry.
> >
> > For example, if `vfn` is page table level 1 aligned, but it was mapped as
> > page table level 0, then pt_mapping_level() will return `level` = 1, since
> > only `vfn` (which is page table level 1 aligned) is taken into account when
> > `mfn` == INVALID_MFN (look at pt_mapping_level()).
> >
> > Fixes: c2f1ded524 ("xen/riscv: page table handling")
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> > ---
> > Changes in v5:
> > - Rename *entry to *ptep in pt_update_entry().
> > - Fix code style issue in the comment.
> > - Move NULL check of pointer to `table` inside unmap_table and then drop
> >   it in pt_update_entry().
> >       
> Imo this last aspect wants mentioning in the description.
>     
Agree, considering that it is a change in the code of previously introduced function,
it makes to mention that.
> > @@ -422,17 +439,40 @@ static int pt_update(vaddr_t virt, mfn_t mfn,
> >  
> >      while ( left )
> >      {
> > -        unsigned int order, level;
> > -
> > -        level = pt_mapping_level(vfn, mfn, left, flags);
> > -        order = XEN_PT_LEVEL_ORDER(level);
> > +        unsigned int order, level = CONFIG_PAGING_LEVELS;
> >  
> > -        ASSERT(left >= BIT(order, UL));
> > +        /*
> > +         * In the case when modifying or destroying a mapping, it is necessary
> > +         * to search until a leaf node is found, instead of searching for
> > +         * a page table entry based on the precalculated `level` and `order`
> > +         * (look at pt_update()).
> > +         * This is because when `mfn` == INVALID_MFN, the `mask`(in
> > +         * pt_mapping_level()) will take into account only `vfn`, which could
> > +         * accidentally return an incorrect level, leading to the discovery of
> > +         * an incorrect page table entry.
> > +         *
> > +         * For example, if `vfn` is page table level 1 aligned, but it was
> > +         * mapped as page table level 0, then pt_mapping_level() will return
> > +         * `level` = 1, since only `vfn` (which is page table level 1 aligned)
> > +         * is taken into account when `mfn` == INVALID_MFN
> > +         * (look at pt_mapping_level()).
> > +         *
> > +         * To force searching until a leaf node is found is necessary to have
> > +         * `level` == CONFIG_PAGING_LEVELS which is a default value for
> > +         * `level`.
> >       
> There looks to be an "it" missing before the 2nd "is". I'm also unconvinced the
> part starting with "which" is really necessary.
>     
Lets then just drop this part. I added only to explicitly show that it is the value
which is used to define `level`.
> Preferably with these small adjustments (I'd be happy to do so while committing)
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>     
Thanks!
~ Oleksii