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.
Jan
> 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
>
