[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 14/17] xen/riscv: implement p2m_next_level()
- To: Jan Beulich <jbeulich@xxxxxxxx>
- From: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
- Date: Thu, 17 Jul 2025 11:42:10 +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: Thu, 17 Jul 2025 09:42:19 +0000
- List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
On 7/16/25 6:12 PM, Jan Beulich wrote:
On 16.07.2025 17:53, Oleksii Kurochko wrote:
On 7/16/25 1:43 PM, Jan Beulich wrote:
On 16.07.2025 13:32, Oleksii Kurochko wrote:
On 7/2/25 10:35 AM, Jan Beulich wrote:
On 10.06.2025 15:05, Oleksii Kurochko wrote:
--- a/xen/arch/riscv/p2m.c
+++ b/xen/arch/riscv/p2m.c
@@ -387,6 +387,17 @@ static inline bool p2me_is_valid(struct p2m_domain *p2m, pte_t pte)
return p2m_type_radix_get(p2m, pte) != p2m_invalid;
}
+/*
+ * pte_is_* helpers are checking the valid bit set in the
+ * PTE but we have to check p2m_type instead (look at the comment above
+ * p2me_is_valid())
+ * Provide our own overlay to check the valid bit.
+ */
+static inline bool p2me_is_mapping(struct p2m_domain *p2m, pte_t pte)
+{
+ return p2me_is_valid(p2m, pte) && (pte.pte & PTE_ACCESS_MASK);
+}
Same question as on the earlier patch - does P2M type apply to intermediate
page tables at all? (Conceptually it shouldn't.)
It doesn't matter whether it is an intermediate page table or a leaf PTE pointing
to a page — PTE should be valid. Considering that in the current implementation
it’s possible for PTE.v = 0 but P2M.v = 1, it is better to check P2M.v instead
of PTE.v.
I'm confused by this reply. If you want to name 2nd level page table entries
P2M - fine (but unhelpful). But then for any memory access there's only one
of the two involved: A PTE (Xen accesses) or a P2M (guest accesses). Hence
how can there be "PTE.v = 0 but P2M.v = 1"?
I think I understand your confusion, let me try to rephrase.
The reason for having both|p2m_is_valid()| and|pte_is_valid()| is that I want to
have the ability to use the P2M PTE valid bit to track which pages were accessed
by a vCPU, so that cleaning and invalidating RAM associated with the guest vCPU
won't be too expensive, for example.
I don't know what you're talking about here.
https://gitlab.com/xen-project/xen/-/blob/staging/xen/arch/arm/mmu/p2m.c#L1649
In this case, the P2M PTE valid bit will be set to 0, but the P2M PTE type bits
will be set to something other than|p2m_invalid| (even for a table entries),
so when an MMU fault occurs, we can properly resolve it.
So, if the P2M PTE type (what|p2m_is_valid()| checks) is set to|p2m_invalid|, it
means that the valid bit (what|pte_is_valid()| checks) should be set to 0, so
the P2M PTE is genuinely invalid.
It could also be the case that the P2M PTE type isn't|p2m_invalid (and P2M PTE valid will be intentionally set to 0 to have
ability to track which pages were accessed for the reason I wrote above)|, and when MMU fault occurs we could
properly handle it and set to 1 P2M PTE valid bit to 1...
An intermediate page table entry is something Xen controls entirely. Hence
it has no (guest induced) type.
... And actually it is a reason why it is needed to set a type even for an
intermediate page table entry.
I hope now it is a lit bit clearer what and why was done.
Sadly not. I still don't see what use the P2M type in of an intermediate page
table is going to be. It surely can't reliably describe all of the entries that
page table holds. Intermediate page tables and leaf pages are just too different
to share a concept like this, I think. That said, I'll be happy to be shown code
demonstrating the contrary.
Then it is needed to introduce new p2m_type_t - p2m_table and use it.
Would it be better?
I still need some type to have ability to distinguish if p2m is valid or not from
p2m management and hardware point of view.
If there is no need for such distinguish why all archs introduce p2m_invalid?
Isn't enough just to use P2M PTE valid bit?
+static struct page_info *p2m_alloc_page(struct domain *d)
+{
+ struct page_info *pg;
+
+ /*
+ * For hardware domain, there should be no limit in the number of pages that
+ * can be allocated, so that the kernel may take advantage of the extended
+ * regions. Hence, allocate p2m pages for hardware domains from heap.
+ */
+ if ( is_hardware_domain(d) )
+ {
+ pg = alloc_domheap_page(d, MEMF_no_owner);
+ if ( pg == NULL )
+ printk(XENLOG_G_ERR "Failed to allocate P2M pages for hwdom.\n");
+ }
The comment looks to have been taken verbatim from Arm. Whatever "extended
regions" are, does the same concept even exist on RISC-V?
Initially, I missed that it’s used only for Arm. Since it was mentioned in
|doc/misc/xen-command-line.pandoc|, I assumed it applied to all architectures.
But now I see that it’s Arm-specific:: ### ext_regions (Arm)
Also, special casing Dom0 like this has benefits, but also comes with a
pitfall: If the system's out of memory, allocations will fail. A pre-
populated pool would avoid that (until exhausted, of course). If special-
casing of Dom0 is needed, I wonder whether ...
+ else
+ {
+ spin_lock(&d->arch.paging.lock);
+ pg = page_list_remove_head(&d->arch.paging.p2m_freelist);
+ spin_unlock(&d->arch.paging.lock);
+ }
... going this path but with a Dom0-only fallback to general allocation
wouldn't be the better route.
IIUC, then it should be something like:
static struct page_info *p2m_alloc_page(struct domain *d)
{
struct page_info *pg;
spin_lock(&d->arch.paging.lock);
pg = page_list_remove_head(&d->arch.paging.p2m_freelist);
Note this: Here you _remove_ from freelist, because you want to actually
use the page. Then clearly ...
spin_unlock(&d->arch.paging.lock);
if ( !pg && is_hardware_domain(d) )
{
/* Need to allocate more memory from domheap */
pg = alloc_domheap_page(d, MEMF_no_owner);
if ( pg == NULL )
{
printk(XENLOG_ERR "Failed to allocate pages.\n");
return pg;
}
ACCESS_ONCE(d->arch.paging.total_pages)++;
page_list_add_tail(pg, &d->arch.paging.freelist);
}
return pg;
}
And basically use|d->arch.paging.freelist| for both dom0less and dom0 domains,
with the only difference being that in the case of Dom0,|d->arch.paging.freelist |could be extended.
Do I understand your idea correctly?
Broadly yes, but not in the details. For example, I don't think such a
page allocated from the general heap would want appending to freelist.
Commentary and alike also would want tidying.
Could you please explain why it wouldn't want appending to freelist?
... adding to freelist here is wrong: You want to use this separately
allocated page, too. Else once it is freed it'll be added to freelist
a 2nd time, leading to a corrupt list.
Got it, I understand why it shouldn’t be added to the freelist.
Incrementing total_pages still makes sense, right?
And of course going forward, for split hardware and control domains the
latter may want similar treatment.
Could you please clarify what is the difference between hardware and control
domains?
I thought that it is the same or is it for the case when we have
dom0 (control domain) which runs domD (hardware domain) and guest domain?
That's the common case, yes, but conceptually the two can be separate.
And if you've followed recent discussions on the list you would also
have noticed that work is being done in that direction. (But this was
really a forward-looking comment; I didn't mean to make you cover that
case right away. Just wanted you to be aware.)
Thanks.
~ Oleksii
|
