Re: [PATCH v1 1/6] xen/riscv: implement get_page_from_gfn()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.02.2026 17:21, Oleksii Kurochko wrote:
> Provide a RISC-V implementation of get_page_from_gfn(), matching the
> semantics used by other architectures.
> 
> For translated guests, this is implemented as a wrapper around
> p2m_get_page_from_gfn(). For DOMID_XEN, which is not auto-translated,
> provide a 1:1 RAM/MMIO mapping and perform the required validation and
> reference counting.
> 
> The function is implemented out-of-line rather than as a static inline,
> to avoid header ordering issues where struct domain is incomplete when
> asm/p2m.h is included, leading to build failures:
>   In file included from ./arch/riscv/include/asm/domain.h:10,
>                    from ./include/xen/domain.h:16,
>                    from ./include/xen/sched.h:11,
>                    from ./include/xen/event.h:12,
>                    from common/cpu.c:3:
>   ./arch/riscv/include/asm/p2m.h: In function 'get_page_from_gfn':
>   ./arch/riscv/include/asm/p2m.h:50:33: error: invalid use of undefined type 
> 'struct domain'
>      50 | #define p2m_get_hostp2m(d) (&(d)->arch.p2m)
>         |                                 ^~
>   ./arch/riscv/include/asm/p2m.h:180:38: note: in expansion of macro 
> 'p2m_get_hostp2m'
>     180 |         return p2m_get_page_from_gfn(p2m_get_hostp2m(d), _gfn(gfn), 
> t);
>         |                                      ^~~~~~~~~~~~~~~
>   make[2]: *** [Rules.mk:253: common/cpu.o] Error 1
>   make[1]: *** [build.mk:72: common] Error 2
>   make: *** [Makefile:623: xen] Error 2

Surely this can be addressed, when x86 and Arm have the function as inline?

> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> ---
> Does it make sense to make this function almost fully generic?
> 
> It looks like most of the logic here is architecture-independent and identical
> across architectures, except for the following points:
> 
> 1. ```
>    if ( likely(d != dom_xen) )
>    ```
> 
>    This could be made generic by introducing paging_mode_translate() for ARM
>    and defining it as `(d != dom_xen)` there.
> 
> 2. ```
>    if ( t )
>        *t = likely(d != dom_io) ? p2m_ram_rw : p2m_mmio_direct_io;
>    ```
> 
>    Here, only `p2m_mmio_direct_io` appears to be architecture-specific. This
>    could be abstracted via a helper such as `dom_io_p2m_type()` and used here
>    instead.

With P2M stuff I'd be careful. Abstracting the two aspects above may make
future arch-specific changes there more difficult.

> --- a/xen/arch/riscv/p2m.c
> +++ b/xen/arch/riscv/p2m.c
> @@ -1557,3 +1557,31 @@ void p2m_handle_vmenter(void)
>          flush_tlb_guest_local();
>      }
>  }
> +
> +struct page_info *get_page_from_gfn(struct domain *d, unsigned long gfn,
> +                                    p2m_type_t *t, p2m_query_t q)
> +{
> +    struct page_info *page;
> +
> +    /*
> +     * Special case for DOMID_XEN as it is the only domain so far that is
> +     * not auto-translated.
> +     */

Once again something taken verbatim from Arm. Yes, dom_xen can in fact appear
here, but it's not a real domain, has no memory truly assigned to it, has no
GFN space, and hence calling it translated (or not) is simply wrong (at best:
misleading). IOW ...

> +    if ( likely(d != dom_xen) )
> +        return p2m_get_page_from_gfn(p2m_get_hostp2m(d), _gfn(gfn), t);
> +
> +    /* Non-translated guests see 1-1 RAM / MMIO mappings everywhere */

... this comment would also want re-wording.

> +    if ( t )
> +        *t = p2m_invalid;
> +
> +    page = mfn_to_page(_mfn(gfn));
> +
> +    if ( !mfn_valid(_mfn(gfn)) || !get_page(page, d) )
> +        return NULL;
> +
> +    if ( t )
> +        *t = likely(d != dom_io) ? p2m_ram_rw : p2m_mmio_direct_io;

If only dom_xen can make it here, why the check for dom_io?

Jan