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

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2239 bytes --]


On 8/11/25 3:25 PM, Jan Beulich wrote:
>> + * The page_order will correspond to the order of the mapping in the page
>> + * table (i.e it could be a superpage).
>> + *
>> + * If the entry is not present, INVALID_MFN will be returned and the
>> + * page_order will be set according to the order of the invalid range.
>> + *
>> + * valid will contain the value of bit[0] (e.g valid bit) of the
>> + * entry.
>> + */
>> +static mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
>> +                           p2m_type_t *t,
>> +                           unsigned int *page_order,
>> +                           bool *valid)
>> +{
>> +    unsigned int level = 0;
>> +    pte_t entry, *table;
>> +    int rc;
>> +    mfn_t mfn = INVALID_MFN;
>> +    DECLARE_OFFSETS(offsets, gfn_to_gaddr(gfn));
>> +
>> +    ASSERT(p2m_is_locked(p2m));
>> +    BUILD_BUG_ON(XEN_PT_LEVEL_MAP_MASK(0) != PAGE_MASK);
> What function-wide property is this check about? Even when moved ...
>
>> +    if ( valid )
>> +        *valid = false;
>> +
>> +    /* XXX: Check if the mapping is lower than the mapped gfn */
> (Nested: What is this about?)
>
>> +    /* This gfn is higher than the highest the p2m map currently holds */
>> +    if ( gfn_x(gfn) > gfn_x(p2m->max_mapped_gfn) )
>> +    {
>> +        for ( level = P2M_ROOT_LEVEL; level; level-- )
>> +            if ( (gfn_x(gfn) & (XEN_PT_LEVEL_MASK(level) >> PAGE_SHIFT)) >
> ... into the more narrow scope where another XEN_PT_LEVEL_MASK() exists I
> can't really spot what the check is to guard against.

Missed to answer in my prev. reply to this and noticed that only during
start of reworking it.

I think it makes sense to update the comment above if condition, this is needed
to find the highest possible order by checking the base of the block mapping
is greater than the max mapped gfn as it is mentioned in the description of the
function, if the entry is not present, the function will return the order of
the invalid range.

I expect that probably it makes sense to do something similar for ->lowest_mapped_gfn
and it is a reason why /* XXX: ... */ comment exist.

~ Oleksii

>
>> +                 gfn_x(p2m->max_mapped_gfn) )
>> +                break;
>> +
>> +        goto out;
>> +    }

[-- Attachment #2: Type: text/html, Size: 3286 bytes --]