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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.02.2026 13:38, Jan Beulich wrote:
> 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.

Actually it's a mix, up to ...

> 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);

... here it's Arm code, but what follows is x86 code. Why did you create such
a mix?

Jan

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