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:
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?
~ Oleksii
