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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 31.07.2025 17:58, Oleksii Kurochko wrote:
> Introduce helper functions for safely querying the P2M (physical-to-machine)
> mapping:
>  - add p2m_read_lock(), p2m_read_unlock(), and p2m_is_locked() for managing
>    P2M lock state.
>  - Implement p2m_get_entry() to retrieve mapping details for a given GFN,
>    including MFN, page order, and validity.
>  - Add p2m_lookup() to encapsulate read-locked MFN retrieval.
>  - Introduce p2m_get_page_from_gfn() to convert a GFN into a page_info
>    pointer, acquiring a reference to the page if valid.
>  - Introduce get_page().
> 
> Implementations are based on Arm's functions with some minor modifications:
> - p2m_get_entry():
>   - Reverse traversal of page tables, as RISC-V uses the opposite level
>     numbering compared to Arm.
>   - Removed the return of p2m_access_t from p2m_get_entry() since
>     mem_access_settings is not introduced for RISC-V.
>   - Updated BUILD_BUG_ON() to check using the level 0 mask, which corresponds
>     to Arm's THIRD_MASK.
>   - Replaced open-coded bit shifts with the BIT() macro.
>   - Other minor changes, such as using RISC-V-specific functions to validate
>     P2M PTEs, and replacing Arm-specific GUEST_* macros with their RISC-V
>     equivalents.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> ---
> Changes in V3:
>  - Add is_p2m_foreign() macro and connected stuff.

What is this about?

> --- a/xen/arch/riscv/include/asm/p2m.h
> +++ b/xen/arch/riscv/include/asm/p2m.h
> @@ -202,6 +202,24 @@ static inline int p2m_is_write_locked(struct p2m_domain 
> *p2m)
>  
>  unsigned long construct_hgatp(struct p2m_domain *p2m, uint16_t vmid);
>  
> +static inline void p2m_read_lock(struct p2m_domain *p2m)
> +{
> +    read_lock(&p2m->lock);
> +}
> +
> +static inline void p2m_read_unlock(struct p2m_domain *p2m)
> +{
> +    read_unlock(&p2m->lock);
> +}
> +
> +static inline int p2m_is_locked(struct p2m_domain *p2m)

bool return type (also for p2m_is_write_locked() in patch 11)? Also perhaps
pointer-to-const parameter?

> --- a/xen/arch/riscv/p2m.c
> +++ b/xen/arch/riscv/p2m.c
> @@ -852,3 +852,139 @@ int map_regions_p2mt(struct domain *d,
>  {
>      return p2m_insert_mapping(p2m_get_hostp2m(d), gfn, nr, mfn, p2mt);
>  }
> +
> +/*
> + * Get the details of a given gfn.
> + *
> + * If the entry is present, the associated MFN will be returned type filled 
> up.

This sentence doesn't really parse, perhaps due to missing words.

> + * 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.

> +                 gfn_x(p2m->max_mapped_gfn) )
> +                break;
> +
> +        goto out;
> +    }
> +
> +    table = p2m_get_root_pointer(p2m, gfn);
> +
> +    /*
> +     * the table should always be non-NULL because the gfn is below
> +     * p2m->max_mapped_gfn and the root table pages are always present.
> +     */

Nit: Style.

> +    if ( !table )
> +    {
> +        ASSERT_UNREACHABLE();
> +        level = P2M_ROOT_LEVEL;
> +        goto out;
> +    }
> +
> +    for ( level = P2M_ROOT_LEVEL; level; level-- )
> +    {
> +        rc = p2m_next_level(p2m, true, level, &table, offsets[level]);

Why would you blindly allocate a page table (hierarchy) here? If anything,
this may need doing upon caller request (as it's only up the call chain
where the necessary knowledge exists). For example, ...

> +static mfn_t p2m_lookup(struct p2m_domain *p2m, gfn_t gfn, p2m_type_t *t)
> +{
> +    mfn_t mfn;
> +
> +    p2m_read_lock(p2m);
> +    mfn = p2m_get_entry(p2m, gfn, t, NULL, NULL);

... this (by its name) pretty likely won't want allocation, while ...

> +    p2m_read_unlock(p2m);
> +
> +    return mfn;
> +}
> +
> +struct page_info *p2m_get_page_from_gfn(struct p2m_domain *p2m, gfn_t gfn,
> +                                        p2m_type_t *t)
> +{

... this will. Yet then ...

> +    struct page_info *page;
> +    p2m_type_t p2mt = p2m_invalid;
> +    mfn_t mfn = p2m_lookup(p2m, gfn, t);

... you use the earlier one here.

> +    if ( !mfn_valid(mfn) )
> +        return NULL;
> +
> +    if ( t )
> +        p2mt = *t;
> +
> +    page = mfn_to_page(mfn);
> +
> +    /*
> +     * get_page won't work on foreign mapping because the page doesn't
> +     * belong to the current domain.
> +     */
> +    if ( p2m_is_foreign(p2mt) )
> +    {
> +        struct domain *fdom = page_get_owner_and_reference(page);
> +        ASSERT(fdom != NULL);
> +        ASSERT(fdom != p2m->domain);
> +        return page;

In a release build (with no assertions) this will be wrong if either of the
two condition would not be satisfied. See x86'es respective code.

Jan