On 12/9/25 12:38 PM, Jan Beulich wrote:
On 24.11.2025 13:33, Oleksii Kurochko wrote:
--- a/xen/arch/riscv/p2m.c
+++ b/xen/arch/riscv/p2m.c
@@ -1061,3 +1061,186 @@ int map_regions_p2mt(struct domain *d,
return rc;
}
+
+/*
+ * p2m_get_entry() should always return the correct order value, even if an
+ * entry is not present (i.e. the GFN is outside the range):
+ * [p2m->lowest_mapped_gfn, p2m->max_mapped_gfn] (1)
+ *
+ * This ensures that callers of p2m_get_entry() can determine what range of
+ * address space would be altered by a corresponding p2m_set_entry().
+ * Also, it would help to avoid costly page walks for GFNs outside range (1).
+ *
+ * Therefore, this function returns true for GFNs outside range (1), and in
+ * that case the corresponding level is returned via the level_out argument.
+ * Otherwise, it returns false and p2m_get_entry() performs a page walk to
+ * find the proper entry.
+ */
+static bool check_outside_boundary(const struct p2m_domain *p2m, gfn_t gfn,
+ gfn_t boundary, bool is_lower,
+ unsigned int *level_out)
+{
+ unsigned int level = P2M_ROOT_LEVEL(p2m);
+ bool ret = false;
+
+ ASSERT(p2m);
+
+ if ( is_lower ? gfn_x(gfn) < gfn_x(boundary)
+ : gfn_x(gfn) > gfn_x(boundary) )
+ {
+ unsigned long mask = 0;
+
+ for ( ; level; level-- )
+ {
+ unsigned long masked_gfn;
+
+ mask |= PFN_DOWN(P2M_LEVEL_MASK(p2m, level));
+ masked_gfn = gfn_x(gfn) & mask;
+ masked_gfn |= (is_lower * (BIT(P2M_LEVEL_ORDER(level), UL) - 1));
I fear I still don't fully understand this. I would have expected the same mask
to
be used for setting / clearing bits (once inverted, obviously). Why would you
clear
only some of the lower bits in one case but set all of them in the other?
Only when is_lower == true do we need to set the lower bits; in all other cases
this is not required, if i am not confusing something.
The idea is that if boundary = 0x1000 and gfn = 0x800, and is_lower == true,
then to return the correct level value we must set all lower bits of gfn to 1.
Otherwise, we would get level = root instead of level = 0 in this case.
I decided not to reuse mask to set the lower bits when is_lower == true, because
doing something like:
mask |= PFN_DOWN(P2M_LEVEL_MASK(p2m, level));
masked_gfn = gfn_x(gfn) & mask;
masked_gfn |= (is_lower * ~mask);
would allow ~mask to introduce 1s into the upper bits, which is not what we
want.
Overall, this alternative of clearing / setting of bits may also better (more
clearly / readably) be expressed using if/else or a conditional operator.
Sure, I will rework it then, unless I have missed something in what I wrote
above.
+ if ( is_lower ? masked_gfn < gfn_x(boundary)
+ : masked_gfn > gfn_x(boundary) )
+ break;
+ }
+
+ ret = true;
+ }
+
+ if ( level_out )
+ *level_out = level;
+
+ return ret;
+}
+
+/*
+ * Get the details of a given gfn.
+ *
+ * If the entry is present, the associated MFN will be returned and the
+ * p2m type of the mapping.
There may be a word order issue in this sentence, or there are words missing
at the end. It more likely being the former, isn't the order being returned
also worth mentioning, ...
+ * The page_order will correspond to the order of the mapping in the page
+ * table (i.e it could be a superpage).
... since this really is a separate piece of commentary?
I will reword it in the following way then:
* If the entry is present, the associated MFN, the p2m type of the mapping,
* and the page order of the mapping in the page table (i.e., it could be a
* superpage) will be returned.
+ * If the entry is not present, INVALID_MFN will be returned and the
+ * page_order will be set according to the order of the invalid range.
... and type will be "invalid".
And this one I'll reword in the following way:
* If the entry is not present, INVALID_MFN will be returned,
* the page_order will be set according to the order of the invalid
* range, and type will be p2m_invalid.
+ */
+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?
~ Oleksii
