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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.12.2025 12:36, Oleksii Kurochko wrote:
> On 12/9/25 4:49 PM, Jan Beulich wrote:
>> 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?
> 
> Maybe I am still misunderstanding your question, but let’s consider what 
> happens
> in the loop in the case of !is_lower.
> 
> P2M_LEVEL_MASK() returns the mask for a given level, so:
> 
>    P2M_LEVEL_MASK(2) = 0x1FFC0000000
>    P2M_LEVEL_MASK(1) = 0x0003FE00000
>    P2M_LEVEL_MASK(0) = 0x000001FF000  (not really used/checked, because if we 
> need
>                                        to calculate it, we already know we 
> are at
>                                        level 0)
> 
> Since we accumulate the mask across iterations, we get:
> 
>    level 2: mask = 0x1FFC0000000
>    level 1: mask = 0x1FFFFE00000
>    level 0: doesn’t matter for the same reason as above.
> 
> So, in the !is_lower case, it is clearing only the low bits for the current 
> level.
> On each iteration, we get only the portion of the GFN that corresponds to the
> current level, plus the portions from previous level(s) if the level is not 
> the root.

But then you accumulate only for the ANDing, whereas you calculate the same mask
from scratch for ORing. That's inefficient and confusing imo.

>>> 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;
> 
> So, if I understand correctly, your suggestion is to calculate the mask as 
> follows:
>    level 2: mask = 0x3fffffff
>    level 1: mask = 0x001fffff
> (i.e., mask = BIT(P2M_GFN_LEVEL_SHIFT(level), UL) - 1)

Yes.

> I agree that this works fully in the is_lower case, but it may cause issues
> in the !is_lower case. According to the spec, the (guest) physical address is
> 56 bits (and the corresponding GFN is 44 bits). My concern is that bits above
> bit 44 must be zero. However, ~mask would have all higher bits set to 1, so
> those (above bit 44) upper bits would not be cleared.
> 
> Perhaps this is not an issue at all, since a GFN larger than 44 bits should be
> considered invalid. In that case, it may be sufficient for 
> check_outside_boundary()
> to ensure something like:
>    ASSERT(gfn_x(gfn) < (BIT(PADDR_BITS - PAGE_SHIFT + 1, UL) - 1));

Invalid GFNs you want to make sure you reject earlier, if they can make it
into here. Only if elsewhere you have code to reject them, you can add such
an assertion here. (Recall: You may not assert on guest controlled input.)

Jan