Re: [PATCH v2 01/11] xen/riscv: implement get_page_from_gfn()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.03.2026 17:29, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/p2m.c
> +++ b/xen/arch/riscv/p2m.c
> @@ -1534,3 +1534,32 @@ void p2m_handle_vmenter(void)
>       * won't be reused until need_flush is set to true.
>       */
>  }
> +
> +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;
> +    p2m_type_t p2mt;
> +
> +    /* Special case for DOMID_XEN as it isn't "normal" domain */
> +    if ( likely(d != dom_xen) )
> +        return p2m_get_page_from_gfn(p2m_get_hostp2m(d), _gfn(gfn), t);

Comments usually apply to immediately following code. When that's not
the case (as it is here), the comment either wants moving or wording
accordingly.

> +    if ( !t )
> +        t = &p2mt;
> +
> +    *t = p2m_invalid;
> +
> +    /* DOMID_XEN sees 1-1 RAM. The p2m_type is based on the type of the page 
> */

As before - I don't think implying any kind of translation (even 1:1) is
correct for system domains.

> +    page = mfn_to_page(_mfn(gfn));

This, strictly speaking, is UB until ...

> +    if ( !mfn_valid(_mfn(gfn)) || !get_page(page, d) )

... the mfn_valid() check succeeded. Yes, Arm code has it like this, but
I can only repeat that you want to carefully inspect any code you copy.

> +        return NULL;
> +
> +    if ( page->u.inuse.type_info & PGT_writable_page )
> +        *t = p2m_ram_rw;
> +    else
> +        BUG_ON("unimplemented. p2m_ram_ro hasn't been introduced yet");
> +
> +    return page;
> +}

Finally, what doesn't become clear at all is why dom_xen needs special
casing. ISTR that when looking at the Arm code in the context of reviewing
v1, I spotted why Arm has this special case. Maybe I'm misremembering, as
now I can't spot it again / anymore. Yet whatever the reason there may not
apply at all to RISC-V.

Jan