On 12/10/25 12:44 PM, Jan Beulich wrote:
On 10.12.2025 12:36, Oleksii Kurochko wrote:
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.
But then you accumulate only for the ANDing, whereas you calculate the same mask
from scratch for ORing. That's inefficient and confusing imo.
Agree that it isn't really efficient and it is confusing, so lets stick to
suggested
way below.
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)
Yes.
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));
Invalid GFNs you want to make sure you reject earlier, if they can make it
into here. Only if elsewhere you have code to reject them, you can add such
an assertion here. (Recall: You may not assert on guest controlled input.)
Then I will reject invalid gfns at the start of p2m_get_entry() instead of
ASSERT().
Thanks.
~ Oleksii
