Re: [PATCH v6 18/19] xen/riscv: add support of page lookup by GFN

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.11.2025 13:33, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/p2m.c
> +++ b/xen/arch/riscv/p2m.c
> @@ -1061,3 +1061,186 @@ int map_regions_p2mt(struct domain *d,
>  
>      return rc;
>  }
> +
> +/*
> + * p2m_get_entry() should always return the correct order value, even if an
> + * entry is not present (i.e. the GFN is outside the range):
> + *   [p2m->lowest_mapped_gfn, p2m->max_mapped_gfn]    (1)
> + *
> + * This ensures that callers of p2m_get_entry() can determine what range of
> + * address space would be altered by a corresponding p2m_set_entry().
> + * Also, it would help to avoid costly page walks for GFNs outside range (1).
> + *
> + * Therefore, this function returns true for GFNs outside range (1), and in
> + * that case the corresponding level is returned via the level_out argument.
> + * Otherwise, it returns false and p2m_get_entry() performs a page walk to
> + * find the proper entry.
> + */
> +static bool check_outside_boundary(const struct p2m_domain *p2m, gfn_t gfn,
> +                                   gfn_t boundary, bool is_lower,
> +                                   unsigned int *level_out)
> +{
> +    unsigned int level = P2M_ROOT_LEVEL(p2m);
> +    bool ret = false;
> +
> +    ASSERT(p2m);
> +
> +    if ( is_lower ? gfn_x(gfn) < gfn_x(boundary)
> +                  : gfn_x(gfn) > gfn_x(boundary) )
> +    {
> +        unsigned long mask = 0;
> +
> +        for ( ; level; level-- )
> +        {
> +            unsigned long masked_gfn;
> +
> +            mask |= PFN_DOWN(P2M_LEVEL_MASK(p2m, level));
> +            masked_gfn = gfn_x(gfn) & mask;
> +            masked_gfn |= (is_lower * (BIT(P2M_LEVEL_ORDER(level), UL) - 1));

I fear I still don't fully understand this. I would have expected the same mask 
to
be used for setting / clearing bits (once inverted, obviously). Why would you 
clear
only some of the lower bits in one case but set all of them in the other?

Overall, this alternative of clearing / setting of bits may also better (more
clearly / readably) be expressed using if/else or a conditional operator.

> +            if ( is_lower ? masked_gfn < gfn_x(boundary)
> +                          : masked_gfn > gfn_x(boundary) )
> +                break;
> +        }
> +
> +        ret = true;
> +    }
> +
> +    if ( level_out )
> +        *level_out = level;
> +
> +    return ret;
> +}
> +
> +/*
> + * Get the details of a given gfn.
> + *
> + * If the entry is present, the associated MFN will be returned and the
> + * p2m type of the mapping.

There may be a word order issue in this sentence, or there are words missing
at the end. It more likely being the former, isn't the order being returned
also worth mentioning, ...

> + * The page_order will correspond to the order of the mapping in the page
> + * table (i.e it could be a superpage).

... since this really is a separate piece of commentary?

> + * If the entry is not present, INVALID_MFN will be returned and the
> + * page_order will be set according to the order of the invalid range.

... and type will be "invalid".

> + */
> +static mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
> +                           p2m_type_t *t,
> +                           unsigned int *page_order)
> +{
> +    unsigned int level = 0;
> +    pte_t entry, *table;
> +    int rc;
> +    mfn_t mfn = INVALID_MFN;
> +    P2M_BUILD_LEVEL_OFFSETS(p2m, offsets, gfn_to_gaddr(gfn));
> +
> +    ASSERT(p2m_is_locked(p2m));
> +
> +    if ( t )
> +        *t = p2m_invalid;

The sole caller passes non-NULL right now. Are you having patches pending
where NULL would be passed? Else, this being a static helper, I'd suggest
to drop the check here (and the other one further down).

Jan