Re: [PATCH v6 18/19] xen/riscv: add support of page lookup by GFN

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.12.2025 16:23, Oleksii Kurochko wrote:
> 
> On 12/9/25 4:49 PM, Jan Beulich wrote:
>>>>> +static mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
>>>>> +                           p2m_type_t *t,
>>>>> +                           unsigned int *page_order)
>>>>> +{
>>>>> +    unsigned int level = 0;
>>>>> +    pte_t entry, *table;
>>>>> +    int rc;
>>>>> +    mfn_t mfn = INVALID_MFN;
>>>>> +    P2M_BUILD_LEVEL_OFFSETS(p2m, offsets, gfn_to_gaddr(gfn));
>>>>> +
>>>>> +    ASSERT(p2m_is_locked(p2m));
>>>>> +
>>>>> +    if ( t )
>>>>> +        *t = p2m_invalid;
>>>> The sole caller passes non-NULL right now. Are you having patches pending
>>>> where NULL would be passed? Else, this being a static helper, I'd suggest
>>>> to drop the check here (and the other one further down).
>>> I don’t have any such call in pending patches. I saw that Arm has a case
>>> where it is called with t = NULL 
>>> (https://elixir.bootlin.com/xen/v4.21.0/source/xen/arch/arm/mem_access.c#L64),
>>> so I decided to keep the check.
>>>
>>> What you wrote makes sense to me, and given that the mem_access code is
>>> Arm-specific, RISC-V will probably never have the same situation.
>>> However, it still seems reasonable to keep this check for flexibility,
>>> so that we don’t risk a NULL-pointer dereference in the future or end up
>>> needing to reintroduce the check (or providing an unused variable for a 
>>> type)
>>> later. Does that make sense?
>> To a degree. The other perspective is that the check is dead code right now,
>> and dead code is often disliked (e.g. by Misra). Introducing the check when
>> it becomes necessary is pretty simple.
> 
> Similar check might be needed for p2m_get_page_from_gfn(), because in the 
> pending
> patches I have a call where t = NULL:

My initial reaction would be "add the checking in that patch then".

> unsigned long copy_to_guest_phys(struct domain *d, paddr_t gpa, void 
> *buf, unsigned int len) { - return -EINVAL; + /* XXX needs to handle 
> faults */ + paddr_t addr = gpa; + unsigned offset = PAGE_OFFSET(addr); + 
> + BUILD_BUG_ON((sizeof(addr)) < sizeof(vaddr_t)); + 
> BUILD_BUG_ON((sizeof(addr)) < sizeof(paddr_t)); + + printk(XENLOG_INFO 
> "copying d%d %#02lx-%#02lx to %#02lx-%#02lx\n", + d->domain_id, 
> (unsigned long)buf, (unsigned long)buf+len, addr, + addr+len); + + while 
> ( len ) + { + void *p; + unsigned size = min(len, (unsigned)PAGE_SIZE - 
> offset); + struct page_info *page; + + page = 
> p2m_get_page_from_gfn(p2m_get_hostp2m(d) , gaddr_to_gfn(addr), NULL); + 
> if ( page == NULL ) + return len; It now seems that I don’t actually 
> need p2m_get_page_from_gfn(), as it is no longer used. I could drop it 
> for now and reintroduce it later when it is truly needed by 
> copy_to_guest_phys() or get_page_from_gfn(). Is it acceptable to keep 
> p2m_get_page_from_gfn() as it is now, even without any current callers? 
> Would it be considered dead code?

Sorry, as you may see your response was effectively unreadable. Looks
like all newlines were zapped for whatever reason, and then new were
ones inserted just to wrap the resulting long line.

Jan