Re: [PATCH v8 1/3] xen/riscv: add support of page lookup by GFN

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.12.2025 16:25, Oleksii Kurochko wrote:
> 
> On 12/29/25 4:06 PM, Jan Beulich wrote:
>> On 22.12.2025 17:37, Oleksii Kurochko wrote:
>>> --- a/xen/arch/riscv/p2m.c
>>> +++ b/xen/arch/riscv/p2m.c
>>> @@ -1057,3 +1057,188 @@ 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) )
>>> +    {
>>> +        for ( ; level; level-- )
>>> +        {
>>> +            unsigned long mask = BIT(P2M_GFN_LEVEL_SHIFT(level), UL) - 1;
>>> +
>>> +            if ( is_lower ? (gfn_x(gfn) | mask) < gfn_x(boundary)
>>> +                          : (gfn_x(gfn) & ~mask) > gfn_x(boundary) )
>>> +                break;
>>> +        }
>>> +
>>> +        ret = true;
>> For this case ...
>>
>>> +    }
>>> +
>>> +    if ( level_out )
>>> +        *level_out = level;
>> ... this is correct, but of "ret" is still false it very likely isn't, and
>> arranging things this way may end up being confusing. Perhaps "level" should
>> be constrained to the if()'s scope? The caller cares about the value only
>> when the return value is true, after all.
> 
> We could simply move the "|if ( level_out )"| check inside the|if| block, but
> is this really a significant issue?

As I said - it is (or has the potential to be) confusing. No more, but also no
less.

> We still need to check the return value,
> and if it is false,|level_out| should just be ignored and there is not big
> difference then if level_out will contain what it contained before the call
> of check_outside_boundary() or it will be set to P2M_ROOT_LEVEL(p2m).
> 
> Alternatively, could we initialize|level| to a non-existent value in the
> "ret=false" case, for example|P2M_MAX_ROOT_LEVEL| + 1, and return that value
> via|level_out|?

Might be another option, yes. Depending on how the ultimate set of callers
are going to behave.

Jan