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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.12.2025 14:00, Oleksii Kurochko wrote:
> 
> On 12/11/25 10:34 AM, Jan Beulich wrote:
>> 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.
> 
> Fully unreadable. I wrote there that in the copy_to_guest_phys() here
> (https://gitlab.com/xen-project/people/olkur/xen/-/blob/riscv-next-upstreaming/xen/arch/riscv/guestcopy.c?ref_type=heads#L31)
> there is a call of p2m_get_page_from_gfn() with t = NULL.
> 
> It now seems that I don’t actually need p2m_get_page_from_gfn(), as it
> is no longer used in this patch series. 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 (with adding a NULL check pointer for 't' argument),
> even without any current callers?
> Would it be considered dead code?

As said, as long as no Misra checks are run on the RISC-V part of the
tree, no dead code concerns really exist. As to the NULL check - if the
sole (future) caller passes NULL, then why have the parameter at all?

Jan