[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v4 18/18] xen/riscv: introduce metadata table to store P2M type
- To: Jan Beulich <jbeulich@xxxxxxxx>
- From: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
- Date: Thu, 9 Oct 2025 13:34:16 +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, 09 Oct 2025 11:34:39 +0000
- List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
On 10/7/25 3:25 PM, Jan Beulich wrote:
On 01.10.2025 18:00, Oleksii Kurochko wrote:
On 9/23/25 12:41 AM, Jan Beulich wrote:
On 17.09.2025 23:55, Oleksii Kurochko wrote:
+/*
+ * `pte` – PTE entry for which the type `t` will be stored.
+ *
+ * If `t` is `p2m_ext_storage`, both `ctx` and `p2m` must be provided;
+ * otherwise, they may be NULL.
+ */
+static void p2m_set_type(pte_t *pte, const p2m_type_t t,
+ struct p2m_pte_ctx *ctx,
+ struct p2m_domain *p2m)
{
- int rc = 0;
+ /*
+ * For the root page table (16 KB in size), we need to select the correct
+ * metadata table, since allocations are 4 KB each. In total, there are
+ * 4 tables of 4 KB each.
+ * For none-root page table index of ->pt_page[] will be always 0 as
+ * index won't be higher then 511. ASSERT() below verifies that.
+ */
+ struct page_info **md_pg =
+ &ctx->pt_page[ctx->index / PAGETABLE_ENTRIES].v.md.metadata;
+ pte_t *metadata = NULL;
+
+ /* Be sure that an index correspondent to page level is passed. */
+ ASSERT(ctx->index <= P2M_PAGETABLE_ENTRIES(ctx->level));
Doesn't this need to be < ?
Yeah, it should be <.
+ if ( !*md_pg && (t >= p2m_first_external) )
+ {
+ /*
+ * Ensure that when `t` is stored outside the PTE bits
+ * (i.e. `t == p2m_ext_storage` or higher),
+ * both `ctx` and `p2m` are provided.
+ */
+ ASSERT(p2m && ctx);
Imo this would want to be checked whenever t > p2m_first_external, no
matter whether a metadata page was already allocated.
I think that|ctx| should be checked before this|if| condition, since it is
used to obtain the proper metadata page.
The check for|p2m| can remain inside the|if| condition, as it is essentially
only needed for allocating a metadata page.
That is, you want to allow callers to pass in NULL for the "p2m" parameter?
Isn't this going to be risky?
With the current implementation it is not risky and initially I thought that p2m
could be passed NULL for the types which are used for types stored within PTE
as for that type p2m argument isn't really needed.
But just to be sure that something won't be broker in future changes let move
ASSERT(p2m) at the top of the function.
- if ( t > p2m_first_external )
- panic("unimplemeted\n");
- else
+ if ( ctx->level <= P2M_SUPPORTED_LEVEL_MAPPING )
+ {
+ struct domain *d = p2m->domain;
+
+ *md_pg = p2m_alloc_table(p2m);
+ if ( !*md_pg )
+ {
+ printk("%s: can't allocate extra memory for dom%d\n",
+ __func__, d->domain_id);
+ domain_crash(d);
+ }
+ }
+ else
+ /*
+ * It is not legal to set a type for an entry which shouldn't
+ * be mapped.
+ */
+ ASSERT_UNREACHABLE();
Something not being legal doesn't mean it can't happen. Imo in this case
BUG_ON() (in place of the if() above) would be better.
+ }
+
+ if ( *md_pg )
+ metadata = __map_domain_page(*md_pg);
Not this conditional assignment for ...
+ if ( t < p2m_first_external )
+ {
pte->pte |= MASK_INSR(t, P2M_TYPE_PTE_BITS_MASK);
- return rc;
+ if ( metadata )
+ metadata[ctx->index].pte = p2m_invalid;
+ }
+ else
+ {
+ pte->pte |= MASK_INSR(p2m_ext_storage, P2M_TYPE_PTE_BITS_MASK);
+
+ metadata[ctx->index].pte = t;
Afaict metadata can still be NULL when you get here.
It shouldn't be, because when this line is executed, the metadata page already
exists or was allocated at the start of p2m_set_type().
... this reply of yours. And the condition there can be false, in case you
took the domain_crash() path.
Oh, right, for some reason, I thought we didn’t return from domain_crash().
I’m curious whether calling domain_crash() might break something, as some useful
data could be freed and negatively affect the internals of map_regions_p2mt().
It might make more sense to use panic() here instead.
Do you have any thoughts or suggestions on this?
@@ -812,13 +955,21 @@ static int p2m_set_entry(struct p2m_domain *p2m,
{
/* We need to split the original page. */
pte_t split_pte = *entry;
+ struct page_info *tbl_pg = virt_to_page(table);
This isn't valid on a pointer obtained from map_domain_page().
Oh, sure — virt_to_page() and page_to_virt() should be used only for Xen
heap addresses.
By the way, do we have any documentation, comments, or notes describing
what should be allocated and from where?
Since map_domain_page() returns an address from the direct map region,
should we instead use maddr_to_page(virt_to_maddr(table))?
How would that be any better? Even if right now you only build RISC-V code
with map_domain_page() having a trivial expansion, you will want to avoid
any assumptions along those lines. Or else you could avoid the use of that
abstraction altogether. It exists so when you need to support memory
amounts beyond what the directmap can cover, you can provide a suitable
implementation of the function and be done. You (or whoever else) in
particular shouldn't be required to go audit all the places where
map_domain_page() (and the pointers it returns) is (are) used.
Then domain_page_map_to_mfn() is the appropriate function to use, and to
get a page, mfn_to_page(domain_page_map_to_mfn(virt)) should be called.
Thanks.
~ Oleksii
|
