[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 11/17] xen/riscv: implement p2m_set_entry() and __p2m_set_entry()
- To: Jan Beulich <jbeulich@xxxxxxxx>
- From: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
- Date: Fri, 4 Jul 2025 17:01:53 +0200
- Cc: Alistair Francis <alistair.francis@xxxxxxx>, Bob Eshleman <bobbyeshleman@xxxxxxxxx>, Connor Davis <connojdavis@xxxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Anthony PERARD <anthony.perard@xxxxxxxxxx>, Michal Orzel <michal.orzel@xxxxxxx>, Julien Grall <julien@xxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
- Delivery-date: Fri, 04 Jul 2025 15:02:08 +0000
- List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
On 7/1/25 3:49 PM, Jan Beulich wrote:
On 10.06.2025 15:05, Oleksii Kurochko wrote:
This patch introduces p2m_set_entry() and its core helper __p2m_set_entry() for
RISC-V, based loosely on the Arm implementation, with several RISC-V-specific
modifications.
Key differences include:
- TLB Flushing: RISC-V allows caching of invalid PTEs and does not require
break-before-make (BBM). As a result, the flushing logic is simplified.
TLB invalidation can be deferred until p2m_write_unlock() is called.
Consequently, the p2m->need_flush flag is always considered true and is
removed.
- Page Table Traversal: The order of walking the page tables differs from Arm,
and this implementation reflects that reversed traversal.
- Macro Adjustments: The macros P2M_ROOT_LEVEL, P2M_ROOT_ORDER, and
P2M_ROOT_PAGES are updated to align with the new RISC-V implementation.
The main functionality is in __p2m_set_entry(), which handles mappings aligned
to page table block entries (e.g., 1GB, 2MB, or 4KB with 4KB granularity).
p2m_set_entry() breaks a region down into block-aligned mappings and calls
__p2m_set_entry() accordingly.
Stub implementations (to be completed later) include:
- p2m_free_entry()
What would a function of this name do?
Recursively visiting all leaf PTE's for sub-tree behind an entry, then calls
put_page() (which will free if there is no any reference to this page),
freeing intermediate page table (after all entries were freed) by removing
it from d->arch.paging.freelist, and removes correspondent page of intermediate page
table from p2m->pages list.
You can clear entries, but you can't
free them, can you?
Is is a question regarding terminology? I can't free entry itself, but a page table or
a page (if it is a leaf entry) on which it points could free.
--- a/xen/arch/riscv/include/asm/p2m.h
+++ b/xen/arch/riscv/include/asm/p2m.h
@@ -9,8 +9,13 @@
#include <xen/rwlock.h>
#include <xen/types.h>
+#include <asm/page.h>
#include <asm/page-bits.h>
+#define P2M_ROOT_LEVEL HYP_PT_ROOT_LEVEL
+#define P2M_ROOT_ORDER XEN_PT_LEVEL_ORDER(P2M_ROOT_LEVEL)
This is confusing, as in patch 6 we see that p2m root table order is 2.
Something needs doing about the naming, so the two sets of things can't
be confused.
Agree, confusing enough.
I will define P2M_ROOT_ORDER as get_order_from_bytes(GUEST_ROOT_PAGE_TABLE_SIZE)
(or declare a new variable to store this value).
Actually, the way it's currently defined was only needed for p2m_get_root_pointer()
to find the root page table by GFN, but XEN_PT_LEVEL_ORDER(P2M_ROOT_LEVEL)
is used explicitly there, so I just missed doing a proper cleanup.
--- a/xen/arch/riscv/p2m.c
+++ b/xen/arch/riscv/p2m.c
@@ -231,6 +231,8 @@ int p2m_init(struct domain *d)
INIT_PAGE_LIST_HEAD(&p2m->pages);
p2m->vmid = INVALID_VMID;
+ p2m->max_mapped_gfn = _gfn(0);
+ p2m->lowest_mapped_gfn = _gfn(ULONG_MAX);
p2m->default_access = p2m_access_rwx;
@@ -325,6 +327,214 @@ int p2m_set_allocation(struct domain *d, unsigned long pages, bool *preempted)
return 0;
}
+/*
+ * Find and map the root page table. The caller is responsible for
+ * unmapping the table.
+ *
+ * The function will return NULL if the offset of the root table is
+ * invalid.
Don't you mean "offset into ..."?
If you won't suggested that, I will think that the meaning of "of" and "into" is pretty close.
But it seems like semantically "into" is more accurate and better conveys the intent of the code.
+ */
+static pte_t *p2m_get_root_pointer(struct p2m_domain *p2m, gfn_t gfn)
+{
+ unsigned long root_table_indx;
+
+ root_table_indx = gfn_x(gfn) >> XEN_PT_LEVEL_ORDER(P2M_ROOT_LEVEL);
+ if ( root_table_indx >= P2M_ROOT_PAGES )
+ return NULL;
+
+ return __map_domain_page(p2m->root + root_table_indx);
+}
+
+static inline bool p2me_is_valid(struct p2m_domain *p2m, pte_t pte)
The rule of thumb is to have inline functions only in header files, leaving
decisions to the compiler elsewhere.
I am not sure what you mean in the second part (after coma) of your sentence.
+{
+ panic("%s: isn't implemented for now\n", __func__);
+
+ return false;
+}
For this function in particular, though: Besides the "p2me" in the name
being somewhat odd (supposedly page table entries here are simply pte_t),
how is this going to be different from pte_is_valid()?
pte_is_valid() is checking a real bit of PTE, but p2me_is_valid() is checking
what is a type stored in the radix tree (p2m->p2m_types):
/*
* In the case of the P2M, the valid bit is used for other purpose. Use
* the type to check whether an entry is valid.
*/
static inline bool p2me_is_valid(struct p2m_domain *p2m, pte_t pte)
{
return p2m_type_radix_get(p2m, pte) != p2m_invalid;
}
It is done to track which page was modified by a guest.
+static inline void p2m_write_pte(pte_t *p, pte_t pte, bool clean_pte)
+{
+ write_pte(p, pte);
+ if ( clean_pte )
+ clean_dcache_va_range(p, sizeof(*p));
+}
+
+static inline void p2m_remove_pte(pte_t *p, bool clean_pte)
+{
+ pte_t pte;
+
+ memset(&pte, 0x00, sizeof(pte));
+ p2m_write_pte(p, pte, clean_pte);
+}
May I suggest "clear" instead of "remove" and plain 0 instead of 0x00
(or simply give the variable a trivial initializer)?
Sure, I will rename and use plain 0.
As to the earlier function that I commented on: Seeing the names here,
wouldn't p2m_pte_is_valid() be a more consistent name there?
Then all p2me_*() should be updated to p2m_pte_*().
But initial logic was that p2me = p2m entry = p2m page table entry.
Probably we can just return back to the prefix p2m_ as based on arguments
it is clear that it is a function for working with P2M's PTE.
+static pte_t p2m_entry_from_mfn(struct p2m_domain *p2m, mfn_t mfn,
+ p2m_type_t t, p2m_access_t a)
+{
+ panic("%s: hasn't been implemented yet\n", __func__);
+
+ return (pte_t) { .pte = 0 };
+}
And then perhaps p2m_pte_from_mfn() here?
+#define GUEST_TABLE_MAP_NONE 0
+#define GUEST_TABLE_MAP_NOMEM 1
+#define GUEST_TABLE_SUPER_PAGE 2
+#define GUEST_TABLE_NORMAL 3
Is GUEST_ a good prefix? The guest doesn't control these tables, and the
word could also mean the guest's own page tables.
Then P2M_ prefix should be better.
+/*
+ * Take the currently mapped table, find the corresponding GFN entry,
That's not what you mean though, is it? It's more like "the entry
corresponding to the GFN" (implying "at the given level").
It will be more clear, I'll update the comment.
+ * and map the next table, if available. The previous table will be
+ * unmapped if the next level was mapped (e.g GUEST_TABLE_NORMAL
+ * returned).
+ *
+ * `alloc_tbl` parameter indicates whether intermediate tables should
+ * be allocated when not present.
+ *
+ * Return values:
+ * GUEST_TABLE_MAP_NONE: a table allocation isn't permitted.
+ * GUEST_TABLE_MAP_NOMEM: allocating a new page failed.
+ * GUEST_TABLE_SUPER_PAGE: next level or leaf mapped normally.
+ * GUEST_TABLE_NORMAL: The next entry points to a superpage.
+ */
+static int p2m_next_level(struct p2m_domain *p2m, bool alloc_tbl,
+ unsigned int level, pte_t **table,
+ unsigned int offset)
+{
+ panic("%s: hasn't been implemented yet\n", __func__);
+
+ return GUEST_TABLE_MAP_NONE;
+}
+
+/* Free pte sub-tree behind an entry */
+static void p2m_free_entry(struct p2m_domain *p2m,
+ pte_t entry, unsigned int level)
+{
+ panic("%s: hasn't been implemented yet\n", __func__);
+}
+
+/*
+ * Insert an entry in the p2m. This should be called with a mapping
+ * equal to a page/superpage.
+ */
+static int __p2m_set_entry(struct p2m_domain *p2m,
No double leading underscores, please. A single one is fine and will do.
+ gfn_t sgfn,
+ unsigned int page_order,
+ mfn_t smfn,
What are the "s" in "sgfn" and "smfn" indicating? Possibly "start", except
that you don't process multiple GFNs here (unlike in the caller).
Yes, it stands for "start". I agree that is not so necessary for __p2m_set_entry()
to use "s" prefix. I'll rename them for __p2m_set_entry().
+ p2m_type_t t,
+ p2m_access_t a)
+{
+ unsigned int level;
+ unsigned int target = page_order / PAGETABLE_ORDER;
+ pte_t *entry, *table, orig_pte;
+ int rc;
+ /* A mapping is removed if the MFN is invalid. */
+ bool removing_mapping = mfn_eq(smfn, INVALID_MFN);
+ DECLARE_OFFSETS(offsets, gfn_to_gaddr(sgfn));
+
+ ASSERT(p2m_is_write_locked(p2m));
+
+ /*
+ * Check if the level target is valid: we only support
+ * 4K - 2M - 1G mapping.
+ */
+ ASSERT(target <= 2);
No provisions towards the division that produced the value having left
a remainder?
The way the order is initialized will always result in division without
a remainder.
If it makes sense, the ASSERT() could be updated to ensure that the order
is always a multiple of PAGETABLE_ORDER:
ASSERT((target <= 2) && !IS_ALIGNED(page_order, PAGETABLE_ORDER));
+ table = p2m_get_root_pointer(p2m, sgfn);
+ if ( !table )
+ return -EINVAL;
+
+ for ( level = P2M_ROOT_LEVEL; level > target; level-- )
+ {
+ /*
+ * Don't try to allocate intermediate page table if the mapping
+ * is about to be removed.
+ */
+ rc = p2m_next_level(p2m, !removing_mapping,
+ level, &table, offsets[level]);
+ if ( (rc == GUEST_TABLE_MAP_NONE) || (rc == GUEST_TABLE_MAP_NOMEM) )
+ {
+ /*
+ * We are here because p2m_next_level has failed to map
+ * the intermediate page table (e.g the table does not exist
+ * and they p2m tree is read-only). It is a valid case
+ * when removing a mapping as it may not exist in the
+ * page table. In this case, just ignore it.
+ */
+ rc = removing_mapping ? 0 : -ENOENT;
Shouldn't GUEST_TABLE_MAP_NOMEM be transformed to -ENOMEM?
Maybe, but I think that it is not really necessary to be so precise here. -ENOENT
could cover both GUEST_TABLE_MAP_NONE and GUEST_TABLE_MAP_NOMEM.
Anyway. for consistency I will change this code to:
rc = (rc == P2M_TABLE_MAP_NONE) ? -ENOENT : -ENOMEM;
/*
* We are here because p2m_next_level has failed to map
* the intermediate page table (e.g the table does not exist
* and they p2m tree is read-only). It is a valid case
* when removing a mapping as it may not exist in the
* page table. In this case, just ignore it.
*/
rc = removing_mapping ? 0 : rc;
goto out;
@@ -332,7 +542,55 @@ static int p2m_set_entry(struct p2m_domain *p2m,
p2m_type_t t,
p2m_access_t a)
{
- return -EOPNOTSUPP;
+ int rc = 0;
+
+ /*
+ * Any reference taken by the P2M mappings (e.g. foreign mapping) will
+ * be dropped in relinquish_p2m_mapping(). As the P2M will still
+ * be accessible after, we need to prevent mapping to be added when the
+ * domain is dying.
+ */
+ if ( unlikely(p2m->domain->is_dying) )
+ return -ENOMEM;
Why ENOMEM?
I expect that when a domain is dying, it means there’s no point in using its
memory—either because it's no longer available or it has already been freed.
Basically, no memory.
+ while ( nr )
Why's there a loop here? The function name uses singular, i.e. means to
create exactly one entry.
I will rename the function to p2m_set_entries().
+ {
+ unsigned long mask;
+ unsigned long order = 0;
unsigned int?
+ /* 1gb, 2mb, 4k mappings are supported */
+ unsigned int i = ( P2M_ROOT_LEVEL > 2 ) ? 2 : P2M_ROOT_LEVEL;
Not (style): Excess blanks. Yet then aren't you open-coding min() here
anyway?
Yes, it is open-coded version of min(). I will use min() instead.
Plus isn't P2M_ROOT_LEVEL always >= 2?
For Sv32, P2M_ROOT_LEVEL is 1; for other modes it is really always >= 2.
+ /*
+ * Don't take into account the MFN when removing mapping (i.e
+ * MFN_INVALID) to calculate the correct target order.
+ *
+ * XXX: Support superpage mappings if nr is not aligned to a
+ * superpage size.
+ */
Does this really need leaving as a to-do?
I think so, yes. It won’t break the current workflow if nr isn’t aligned,
a smaller order will simply be chosen.
+ mask = !mfn_eq(smfn, INVALID_MFN) ? mfn_x(smfn) : 0;
+ mask |= gfn_x(sgfn) | nr;
+
+ for ( ; i != 0; i-- )
+ {
+ if ( !(mask & (BIT(XEN_PT_LEVEL_ORDER(i), UL) - 1)) )
+ {
+ order = XEN_PT_LEVEL_ORDER(i);
+ break;
Nit: Style.
+ }
+ }
+
+ rc = __p2m_set_entry(p2m, sgfn, order, smfn, t, a);
+ if ( rc )
+ break;
+
+ sgfn = gfn_add(sgfn, (1 << order));
+ if ( !mfn_eq(smfn, INVALID_MFN) )
+ smfn = mfn_add(smfn, (1 << order));
+
+ nr -= (1 << order);
Throughout maybe better be safe right away and use 1UL?
+ }
+
+ return rc;
}
How's the caller going to know how much of the range was successfully
mapped?
There is no such option. Do other arches do that? I mean returns somehow
the number of successfully mapped (sgfn,smfn).
That part may need undoing (if not here, then in the caller),
or a caller may want to retry.
So the caller in the case if rc != 0, can just undoing the full range
(by using the same sgfn, nr, smfn).
Or, as an option, just go for range (sgfn, nr), get each entry and if it
was mapped then just clear entry; otherwise just stop.
Yes, it isn't optimal, but should work.
Thanks.
~ Oleksii
|
