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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.12.2025 18:25, Oleksii Kurochko wrote:
> On 12/18/25 1:55 PM, Jan Beulich wrote:
>> On 16.12.2025 17:55, Oleksii Kurochko wrote:
>>> +/*
>>> + * Get the details of a given gfn.
>>> + *
>>> + * 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, page_order 
>>> will
>>> + * be set according to the order of the invalid range, and the 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));
>>> +
>>> +    *t = p2m_invalid;
>>> +
>>> +    if ( gfn_x(gfn) > (BIT(PADDR_BITS - PAGE_SHIFT + 1, UL) - 1) )
>>> +        return mfn;
>> Since on all other error paths you set *page_order (as long as the pointer
>> is non-NULL), shouldn't you do so here as well (to the order corresponding
>> to the full [2nd-level] address space)?
> 
> It makes sense. I am thinking if something like would be fine:
>    @@ -1123,7 +1117,7 @@ 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;
>    +    unsigned int level = P2M_ROOT_LEVEL(p2m);
>         pte_t entry, *table;
>         int rc;
>         mfn_t mfn = INVALID_MFN;
>    @@ -1134,7 +1128,13 @@ static mfn_t p2m_get_entry(struct p2m_domain *p2m, 
> gfn_t gfn,
>         *t = p2m_invalid;
>     
>         if ( gfn_x(gfn) > (BIT(PADDR_BITS - PAGE_SHIFT + 1, UL) - 1) )
>    +    {
>    +        if ( page_order )
>    +            *page_order =
>    +                P2M_LEVEL_ORDER(level + 1) + P2M_ROOT_EXTRA_BITS(p2m, 
> level);
>    +
>             return mfn;
>    +    }
>     
>         if ( check_outside_boundary(p2m, gfn, p2m->lowest_mapped_gfn, true,
>                                     &level) )
>    @@ -1152,7 +1152,6 @@ static mfn_t p2m_get_entry(struct p2m_domain *p2m, 
> gfn_t gfn,
>         if ( !table )
>         {
>             ASSERT_UNREACHABLE();
>    -        level = P2M_ROOT_LEVEL(p2m);
>             goto out;
>         }
> 
> Or it isn't the best one option to define page_order using "non-existing" 
> level?

Well, ...

>> Furthermore, is PADDR_BITS really the right basis? Don't things rather depend
>> on the number of levels the 2nd-level page tables have for the given guest?
> 
> I think you are right, it depends on the number of levels the 2nd-level page 
> tables
> have for the given guest.

... the more suitable thing here is also going to be applicable to the order
that you pass back. I.e. whatever the bounds check is going to be based on
can also be used to produce the appropriate order.

Jan