On 11/18/25 7:58 AM, Jan Beulich wrote:
On 17.11.2025 20:51, Oleksii Kurochko wrote:
On 11/12/25 12:49 PM, Jan Beulich wrote:
On 20.10.2025 17:58, Oleksii Kurochko wrote:
+ if ( *md_pg )
+ metadata = __map_domain_page(*md_pg);
+
+ 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;
Shouldn't this be accompanied with a BUILD_BUG_ON(p2m_invalid), as otherwise
p2m_alloc_page()'s clearing of the page won't have the intended effect?
I think that, at least, at the moment we are always explicitly set p2m type and
do not rely on that by default 0==p2m_invalid.
You don't, and ...
Just to be safe, I will add after "if ( metadata )" suggested
BUILD_BUG_ON(p2m_invalid):
if ( metadata )
metadata[ctx->index].type = p2m_invalid;
/*
* metadata.type is expected to be p2m_invalid (0) after the page is
* allocated and zero-initialized in p2m_alloc_page().
*/
BUILD_BUG_ON(p2m_invalid);
...
... this leaves me with the impression that you didn't read my reply correctly.
p2m_alloc_page() clear the page, thus _implicitly_ setting all entries to
p2m_invalid. That's where the BUILD_BUG_ON() would want to go (the call site,
ftaod).
I think I still don’t fully understand what the issue would be if|p2m_invalid|
were
ever equal to 1 instead of 0 in the context of a metadata page.
Yes, if|p2m_invalid| were 1, there would be a problem if someone tried to read
this
metadata pagebefore it was assigned any type. They would find a value of 0,
which
corresponds to a valid type rather than to|p2m_invalid|, as one might expect.
However, I’m not sure I currently see a scenario in which the metadata page
would
be read before being initialized.
But just to be safe when such case will occur I am okay with putting
BUILD_BUG_ON(p2m_invalid) before p2m_alloc_page() in p2m_set_type() function.
+ }
+ else
+ {
+ pte->pte |= MASK_INSR(p2m_ext_storage, P2M_TYPE_PTE_BITS_MASK);
+
+ metadata[ctx->index].pte = t;
If you set t to p2m_ext_storage here, the pte->pte updating could be moved ...
't' shouldn't be passed as 'p2m_ext_storage'.
Of course not. I said "set", not "pass". I suggested to set t to p2m_ext_storage
right after the assignment above. I notice though that I missed ...
Now, I see then ...
For example, in this case we will have that in metadata page we will have type
equal to p2m_ext_storage and then in pte->pte will have the type set to
p2m_ext_storage, and the we end that we don't have a real type stored somewhere.
Even more, metadata.pte shouldn't be used to store p2m_ext_storage, only
p2m_invalid and types mentioned in enum p2m_t after p2m_ext_storage.
+ }
... here, covering both cases. Overally this may then be easier as
if ( t >= p2m_first_external )
metadata[ctx->index].pte = t;
... the respective line (and the figure braces which are the needed) here:
t = p2m_ext_storage;
... (what suggested above) will work.
else if ( metadata )
metadata[ctx->index].pte = p2m_invalid;
pte->pte |= MASK_INSR(t, P2M_TYPE_PTE_BITS_MASK);
Then raising the question whether it couldn't still be the real type that's
stored in metadata[] even for t < p2m_first_external. That woiuld further
reduce conditionals.
It would be nice, but I think that at the moment we can’t do that. As I
explained
above, 't' should not normally be passed as p2m_ext_storage.
Of course not, but how's that relevant to storing the _real_ type in the
metadata page even when it's one which can also can be stored in the PTE?
As said, for a frequently used path it may help to reduce the number of
conditionals here.
IIUC, you are asking whether, if|pte->pte| stores a type|< p2m_ext_storage|,
it would still be possible for|metadata[].pte| to contain anyreal type?
If yes, then the answer is that it could be done, because in the|p2m_get_type()
|function the value stored in|pte->pte| is checked first. If it
isn't|p2m_ext_storage|,
then|metadata[].pte| will not be checked at all. So technically, it could
contain
whatever we want in case when pte.pte's type != p2m_ext_storage.
But will it really reduce an amount of conditions? It seems like we still need
one
condition to check of metadata is mapped and one condition to set 't' to
p2m_ext_storage:
if ( metadata )
metadata[ctx->index].pte = t;
if ( t >= p2m_first_external )
t = p2m_ext_storage;
pte->pte |= MASK_INSR(t, P2M_TYPE_PTE_BITS_MASK);
We can do:
if ( metadata )
{
metadata[ctx->index].pte = t;
if ( t >= p2m_first_external )
t = p2m_ext_storage;
}
pte->pte |= MASK_INSR(t, P2M_TYPE_PTE_BITS_MASK);
It will reduce an amount of conditions if metadata wasn't used/allocated, but I
think you
have a different idea, don't you?
+static void p2m_free_page(struct p2m_domain *p2m, struct page_info *pg);
+
+/*
+ * Free page table's page and metadata page linked to page table's page.
+ */
+static void p2m_free_table(struct p2m_domain *p2m, struct page_info *tbl_pg)
+{
+ if ( tbl_pg->v.md.pg )
+ p2m_free_page(p2m, tbl_pg->v.md.pg);
To play safe, maybe better also clear tbl_pg->v.md.pg?
I thought it would be enough to clear it during allocation in p2m_alloc_page(),
since I'm not sure it is critical if md.pg data were somehow leaked and read.
But to be safer, we can add this here:
clear_and_clean_page(tbl_pg->v.md.pg, p2m->clean_dcache);
I didn't say clear what tbl_pg->v.md.pg points to, though. I suggested to clear
the struct field itself.
Won't be enough just tbl_pg->v.md.pg = NULL; ?
Thanks!
~ Oleksii
