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

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

On 2/17/26 10:10 AM, Jan Beulich wrote:
> On 17.02.2026 10:01, Oleksii Kurochko wrote:
>> On 2/16/26 1:38 PM, Jan Beulich wrote:
>>> On 12.02.2026 17:21, Oleksii Kurochko wrote:
>>>> --- 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.
>> As you mentioned in the another reply to this patch, I messed up x86 and Arm
>> implementation in a bad way, so here should be DOMID_XEN used instead of
>> "Non-translated".
>>
>> Based on your reply it seems like the first comment should be also rephrased
>> as you mentioned that DOMID_XEN can't be called also "not auto-translated".
>> I think it would be better to write the following:
>>    /*
>>     * Special case for DOMID_XEN as it is the only domain so far that has
>>     * no GFN space.
>>     */
> Simply say that dom_xen isn't a "normal" domain?
>
>>>> +    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?
>> Incorrectly copied from x86. It should be just:
>>    *t = p2m_ram_rw
>> here as in RISC-V for MMIO pages owner isn't set to dom_io (and the same is
>> true for Arm I think).
> May I suggest that right away you use the construct that I suggested Arm to
> switch to (you were Cc-ed on that patch, I think)? Despite the absence of
> p2m_ram_ro on RISC-V, that'll be usable, and it will allow keeping the code
> untouched when p2m_ram_ro is introduced (sooner or later you will need it,
> I expect).

Sure, but doesn't that patch is connected to another function (translate_get_page())
and just fixing the handling of what get_page_from_gfn() in *t?

For get_page_from_gfn() to not miss the case when new type is introduced it make
sense to do the following:
     if ( page->u.inuse.type_info & PGT_writable_page )
         *t = p2m_ram_rw;
     else
	BUG_ON("unimplemented");

~ Oleksii