Re: [for 4.22 v5 18/18] xen/riscv: introduce metadata table to store P2M type

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 5006 bytes --]


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

[-- Attachment #2: Type: text/html, Size: 7103 bytes --]