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
