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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 12/9/25 12:38 PM, Jan Beulich wrote:
> 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?
Only when is_lower == true do we need to set the lower bits; in all other cases
this is not required, if i am not confusing something.

The idea is that if boundary = 0x1000 and gfn = 0x800, and is_lower == true,
then to return the correct level value we must set all lower bits of gfn to 1.
Otherwise, we would get level = root instead of level = 0 in this case.

I decided not to reuse mask to set the lower bits when is_lower == true, because
doing something like:

    mask |= PFN_DOWN(P2M_LEVEL_MASK(p2m, level));
    masked_gfn = gfn_x(gfn) & mask;
    masked_gfn |= (is_lower * ~mask);

would allow ~mask to introduce 1s into the upper bits, which is not what we 
want.

> Overall, this alternative of clearing / setting of bits may also better (more
> clearly / readably) be expressed using if/else or a conditional operator.
Sure, I will rework it then, unless I have missed something in what I wrote 
above.

> > +            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?
I will reword it in the following way then:
 * If the entry is present, the associated MFN, the p2m type of the mapping,
 * and the page order of the mapping in the page table (i.e., it could be a
 * superpage) will be returned.

> > + * 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".
And this one I'll reword in the following way:

 * If the entry is not present, INVALID_MFN will be returned,
 * the page_order will be set according to the order of the invalid
 * range, and type will be p2m_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).
I don’t have any such call in pending patches. I saw that Arm has a case
where it is called with t = NULL 
(https://elixir.bootlin.com/xen/v4.21.0/source/xen/arch/arm/mem_access.c#L64),
so I decided to keep the check.

What you wrote makes sense to me, and given that the mem_access code is
Arm-specific, RISC-V will probably never have the same situation.
However, it still seems reasonable to keep this check for flexibility,
so that we don’t risk a NULL-pointer dereference in the future or end up
needing to reintroduce the check (or providing an unused variable for a type)
later. Does that make sense?

~ Oleksii