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

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

On 3/26/26 2:50 PM, Jan Beulich wrote:
> 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.

I will move it after if-() statement.

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

I will rephrase that to:
  "DOM_XEN has no stage-2 translation at all, so the gfn argument is 
treated directly as an mfn"

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

IIUC, then Arm having this special case for DOMID_XEN as it is used to 
share pages beloging to the hypervisor, for example, trace buffers and 
considering that trace buffers are part of common code it will be also 
true for RISC-V.

~ Oleksii