[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: Mon, 7 Jul 2025 13:46:40 +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: Mon, 07 Jul 2025 11:46:50 +0000
- List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
On 7/7/25 9:20 AM, Jan Beulich wrote:
On 04.07.2025 17:01, Oleksii Kurochko wrote:
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?
Yes. If one sees a call to a function, it should be possible to at least
roughly know what it does without needing to go look at the implementation.
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.
Then e.g. pte_free_subtree() or some such?
It sounds fine to me. I'll use suggested name.
Just want to notice that other arches also have the same function
for the same purpose with the same name.
Does it make sense then to change a name for all arches?
+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.
The compiler does its own inlining decisions quite fine when it can see all
call sites (as is the case for static functions). Hence in general you want
to omit "inline" there. Except of course in header files, where non-inline
static-s are a problem.
Thanks, now it is clear what you meant.
+{
+ 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.
But then (again) the name doesn't convey what the function does.
Then probably p2me_type_is_valid(struct p2m_domain *p2m, pte_t pte) would better.
Plus
can't a guest also arrange for an entry's type to move to p2m_invalid?
That's then still an entry that was modified by the guest.
I am not really sure that I fully understand the question.
Do you ask if a guest can do something which will lead to a call of p2m_set_entry()
with p2m_invalid argument?
If yes, then it seems like it will be done only in case of p2m_remove_mapping() what
will mean that alongside with p2m_invalid INVALID_MFN will be also passed, what means
this entry isn't expected to be used anymore.
Overall I think I'm lacking clarity what you mean to use this predicate
for.
By using of "p2me_" predicate I wanted to express that not PTE's valid bit will be
checked, but the type saved in radix tree will be used.
As suggested above probably it will be better drop "e" too and just use p2m_type_is_valid().
+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.
In the end it's up to you. Having thought about it some more, perhaps
p2me_*() is still quite helpful to separate from functions dealing with
P2Ms as a while, and to also avoid the verbosity of p2m_pte_*().
+{
+ 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));
Except that the ! looks wrong here.
Agree, it shouldn't be here. Thanks.
@@ -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.
That can end up odd for call sites. Please consider using e.g. EACCES.
+ 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().
Or maybe p2m_set_range()?
It is much better.
+ /*
+ * 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.
Well, my question was more like "Isn't it simple enough to cover the case
right away?"
+ 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).
On x86 we had to introduce some not very nice code to cover for the absence
of proper handling there. For a new port I think it wants at least seriously
considering not to repeat such a potentially unhelpful pattern.
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).
Can it? How would it know what the original state was?
You're right — blindly unmapping the range assumes that no entries were valid
beforehand and I missed that it could be that something valid was mapped before
p2m_set_entry(sgfn,...,smfn) was called.
But then I am not really understand why it won't be an issue if will know
how many GFNs were successfully mapped.
~ Oleksii
|
