On 12/9/25 4:49 PM, Jan Beulich wrote:
On 09.12.2025 16:41, Oleksii Kurochko wrote:
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.
That wasn't my point though. I don't follow the !is_lower case: Why would you
clear only the bits for the given level, not all further down as well? Or am
I reading P2M_LEVEL_MASK() incorrectly?
Maybe I am still misunderstanding your question, but let’s consider what happens
in the loop in the case of !is_lower.
P2M_LEVEL_MASK() returns the mask for a given level, so:
P2M_LEVEL_MASK(2) = 0x1FFC0000000
P2M_LEVEL_MASK(1) = 0x0003FE00000
P2M_LEVEL_MASK(0) = 0x000001FF000 (not really used/checked, because if we
need
to calculate it, we already know we are at
level 0)
Since we accumulate the mask across iterations, we get:
level 2: mask = 0x1FFC0000000
level 1: mask = 0x1FFFFE00000
level 0: doesn’t matter for the same reason as above.
So, in the !is_lower case, it is clearing only the low bits for the current
level.
On each iteration, we get only the portion of the GFN that corresponds to the
current level, plus the portions from previous level(s) if the level is not the
root.
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.
If you set "mask" such that it has suitably many of its low bits set then you
should be able to simply do
if ( is_lower )
masked_gfn = gfn_x(gfn) | mask;
else
masked_gfn = gfn_x(gfn) & ~mask;
So, if I understand correctly, your suggestion is to calculate the mask as
follows:
level 2: mask = 0x3fffffff
level 1: mask = 0x001fffff
(i.e., mask = BIT(P2M_GFN_LEVEL_SHIFT(level), UL) - 1)
I agree that this works fully in the is_lower case, but it may cause issues
in the !is_lower case. According to the spec, the (guest) physical address is
56 bits (and the corresponding GFN is 44 bits). My concern is that bits above
bit 44 must be zero. However, ~mask would have all higher bits set to 1, so
those (above bit 44) upper bits would not be cleared.
Perhaps this is not an issue at all, since a GFN larger than 44 bits should be
considered invalid. In that case, it may be sufficient for
check_outside_boundary()
to ensure something like:
ASSERT(gfn_x(gfn) < (BIT(PADDR_BITS - PAGE_SHIFT + 1, UL) - 1));
Does it make sense or I still continue to confuse something?
+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.
Then it makes sense to me to drop the check for now.
Thanks.
~ Oleksii
