On 11/21/25 8:35 AM, Jan Beulich wrote:
On 20.11.2025 17:52, Oleksii Kurochko wrote:
On 11/20/25 4:47 PM, Jan Beulich wrote:
On 20.11.2025 16:38, Oleksii Kurochko wrote:
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.
Are you sure walks can only happen for GFNs that were set up? What you need to
do walks on is under guest control, after all.
If a GFN lies within the range[p2m->lowest_mapped_gfn, p2m->max_mapped_gfn],
then
|p2m_set_entry()| must already have been called for this GFN.
No. All you know from the pre-condition is that p2m_set_entry() was called for
the
lowest and highest GFNs in that range.
Oh, right. There could still be some GFNs inside the range for
which|p2m_set_entry()| has
not yet been called. Then it probably makes sense to add a|BUILD_BUG_ON| here
as well, before
"if ( type == p2m_ext_storage )":
static p2m_type_t p2m_get_type(const pte_t pte, const struct p2m_pte_ctx
*ctx)
{
p2m_type_t type = MASK_EXTR(pte.pte, P2M_TYPE_PTE_BITS_MASK);
if ( type == p2m_ext_storage )
{
const pte_t *md = __map_domain_page(ctx->pt_page->v.md.pg);
type = md[ctx->index].pte;
unmap_domain_page(md);
}
return type;
}
I would expect that if|p2m_set_entry()| has not been called for a GFN,
then|p2m_get_entry()|
(|p2m_get_type()| will be called somewhere inside|p2m_get_entry()|, for
example) should return
the|p2m_invalid| type. I think we want to have the same check (as the one
before the call to
|p2m_alloc_page()|), placed before the condition:
BUILD_BUG_ON(p2m_invalid);
~ Oleksii
This means that either
- a metadata page has been created and its entry filled with the appropriate
type, or
- no metadata page was needed and the type was stored directly in|pte->pte|
For a GFN outside the range(p2m->lowest_mapped_gfn, p2m->max_mapped_gfn),
|p2m_get_entry()| will not even attempt a walk because of the boundary checks:
static mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
p2m_type_t *t,
unsigned int *page_order)
...
if ( check_outside_boundary(p2m, gfn, p2m->lowest_mapped_gfn, true,
&level) )
goto out;
if ( check_outside_boundary(p2m, gfn, p2m->max_mapped_gfn, false, &level) )
goto out;
If I am misunderstanding something and there are other cases where a walk can
occur for
GFNs that were never set up, then such GFNs would have neither an allocated
metadata
page nor a type stored in|pte->pte|, which looks like we are in trouble.
~ Oleksii
