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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.02.2025 14:12, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/include/asm/page.h
> +++ b/xen/arch/riscv/include/asm/page.h
> @@ -55,6 +55,22 @@
>  #define PTE_SMALL       BIT(10, UL)
>  #define PTE_POPULATE    BIT(11, 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()).
> + */
> +
> +#define PTE_LEAF_SEARCH BIT(12, UL)

Is it intended for callers outside of pt.c to make use of this? If not,
it better wouldn't be globally exposed.

Furthermore, this isn't a property of the PTE(s) to be created, so is
likely wrong to mix with PTE_* flags. (PTE_POPULATE is on the edge of
also falling in this category, btw.) Perhaps ...

> --- a/xen/arch/riscv/pt.c
> +++ b/xen/arch/riscv/pt.c
> @@ -187,11 +187,10 @@ static int pt_next_level(bool alloc_tbl, pte_t **table, 
> unsigned int offset)
>  
>  /* 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,

... you instead want to have callers of this function preset *target
to a special value (e.g. UINT_MAX or CONFIG_PAGING_LEVELS) indicating
the level is wanted as an output.

> @@ -205,39 +204,48 @@ 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 ( flags & PTE_LEAF_SEARCH )
>      {
> -        rc = pt_next_level(alloc_tbl, &table, offsets[level]);
> -        if ( rc == XEN_TABLE_MAP_NOMEM )
> +        entry = pt_walk(virt, target);
> +        BUG_ON(!pte_is_mapping(*entry));

Is this really necessarily a bug? Can't one want to determine how deep
the (populated) page tables are for a given VA?

Hmm, here I can see why you have pt_walk() return a pointer. As per the
comment on the earlier patch, I don't think this is a good idea. You
may want to have

static pte_t *_pt_walk(...)
{
    ...
}

pte_t pt_walk(...)
{
    return *_pt_walk(...);
}

> @@ -345,9 +353,6 @@ 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.
> -     *
>       * `vfn` and `mfn` must be both superpage aligned.
>       * They are or-ed together and then checked against the size of
>       * each level.

You drop part of the comment without altering the code being commented.
What's the deal?

> @@ -415,19 +420,33 @@ static int pt_update(vaddr_t virt, mfn_t mfn,
>  
>      spin_lock(&pt_lock);
>  
> -    while ( left )
> +    /* look at the comment above the definition of PTE_LEAF_SEARCH */
> +    if ( mfn_eq(mfn, INVALID_MFN) && !(flags & PTE_POPULATE) )
>      {
> -        unsigned int order, level;
> +        flags |= PTE_LEAF_SEARCH;
> +    }

For readability I think it would be better if the figure braces were
dropped.

> -        level = pt_mapping_level(vfn, mfn, left, flags);
> -        order = XEN_PT_LEVEL_ORDER(level);
> +    while ( left )
> +    {
> +        unsigned int order = 0, level;
>  
> -        ASSERT(left >= BIT(order, UL));
> +        if ( !(flags & PTE_LEAF_SEARCH) )
> +        {
> +            level = pt_mapping_level(vfn, mfn, left, flags);
> +            order = XEN_PT_LEVEL_ORDER(level);
> +            ASSERT(left >= BIT(order, UL));

Assignment to order and assertion are ...

> +        }
>  
> -        rc = pt_update_entry(root, vfn << PAGE_SHIFT, mfn, level, flags);
> +        rc = pt_update_entry(root, vfn << PAGE_SHIFT, mfn, &level, flags);
>          if ( rc )
>              break;
>  
> +        if ( flags & PTE_LEAF_SEARCH )
> +        {
> +            order = XEN_PT_LEVEL_ORDER(level);
> +            ASSERT(left >= BIT(order, UL));
> +        }

... repeated here, with neither left nor order being passed into
pt_update_entry(). Does this really need doing twice? (I have to
admit that I have trouble determining what the assertion is about.
For order alone it clearly could be done centrally after the call.)

Jan