Re: [for 4.22 v5 17/18] xen/riscv: add support of page lookup by GFN

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.10.2025 17:58, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/p2m.c
> +++ b/xen/arch/riscv/p2m.c
> @@ -1049,3 +1049,178 @@ 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)

There's one closing parenthesis too many here (likely the one before the colon).

> + * 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 cost page walks for GFNs outside range (1).

DYM "costly"?

> + * 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(gfn_t gfn, gfn_t boundary, bool is_lower,
> +                                   unsigned int *level_out)
> +{
> +    unsigned int level;
> +
> +    if ( is_lower ? gfn_x(gfn) < gfn_x(boundary)
> +                  : gfn_x(gfn) > gfn_x(boundary) )
> +    {
> +        unsigned long mask = 0;
> +
> +        for ( level = P2M_ROOT_LEVEL; level; level-- )
> +        {
> +            unsigned long masked_gfn;
> +
> +            mask |= PFN_DOWN(P2M_LEVEL_MASK(level));
> +            masked_gfn = gfn_x(gfn) & mask;
> +
> +            if ( is_lower ? masked_gfn < gfn_x(boundary)
> +                          : masked_gfn > gfn_x(boundary) )
> +            {
> +                *level_out = level;

For this to be correct in the is_lower case, don't you need to fill the
bottom bits of masked_gfn with all 1s, rather than with all 0s? Otherwise
the tail of the range may be above boundary.

> +struct page_info *p2m_get_page_from_gfn(struct p2m_domain *p2m, gfn_t gfn,
> +                                        p2m_type_t *t)
> +{
> +    struct page_info *page;
> +    p2m_type_t p2mt = p2m_invalid;
> +    mfn_t mfn;
> +
> +    p2m_read_lock(p2m);
> +    mfn = p2m_get_entry(p2m, gfn, t, NULL);
> +
> +    if ( !mfn_valid(mfn) )
> +    {
> +        p2m_read_unlock(p2m);
> +        return NULL;
> +    }
> +
> +    if ( t )
> +        p2mt = *t;

Doesn't it need to be the other way around? The way you have it, when a caller
passes NULL for t, p2m_get_entry() won't give you a type, and you'll do all
further work with p2m_invalid.

Also, might this better move ahead of the earlier if()? Callers might be able
to do still something based on the type, when they get back NULL as function
return value. (Practically this might only become of interest once you add
something like PoD, paging, or sharing.)

Jan