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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.12.2025 16:41, Oleksii Kurochko wrote:
> 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.

That wasn't my point though. I don't follow the !is_lower case: Why would you
clear only the bits for the given level, not all further down as well? Or am
I reading P2M_LEVEL_MASK() incorrectly?

> 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.

If you set "mask" such that it has suitably many of its low bits set then you
should be able to simply do

      if ( is_lower )
          masked_gfn = gfn_x(gfn) | mask;
      else
          masked_gfn = gfn_x(gfn) & ~mask;

>>> +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?

To a degree. The other perspective is that the check is dead code right now,
and dead code is often disliked (e.g. by Misra). Introducing the check when
it becomes necessary is pretty simple.

Jan