Re: [PATCH v8 2/3] xen/riscv: introduce metadata table to store P2M type

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.12.2025 16:47, Oleksii Kurochko wrote:
> 
> On 12/29/25 4:13 PM, Jan Beulich wrote:
>> On 22.12.2025 17:37, Oleksii Kurochko wrote:
>>> @@ -370,24 +396,96 @@ static struct page_info *p2m_alloc_page(struct 
>>> p2m_domain *p2m)
>>>       return pg;
>>>   }
>>>   
>>> -static int p2m_set_type(pte_t *pte, p2m_type_t t)
>>> +/*
>>> + * `pte` – PTE entry for which the type `t` will be stored.
>>> + *
>>> + * If `t` >= p2m_first_external, a valid `ctx` must be provided.
>>> + */
>>> +static void p2m_set_type(pte_t *pte, p2m_type_t t,
>>> +                         const struct p2m_pte_ctx *ctx)
>>>   {
>>> -    int rc = 0;
>>> +    struct page_info **md_pg;
>>> +    struct md_t *metadata = NULL;
>>>   
>>> -    if ( t > p2m_first_external )
>>> -        panic("unimplemeted\n");
>>> -    else
>>> -        pte->pte |= MASK_INSR(t, P2M_TYPE_PTE_BITS_MASK);
>>> +    /*
>>> +     * It is sufficient to compare ctx->index with PAGETABLE_ENTRIES 
>>> because,
>>> +     * even for the p2m root page table (which is a 16 KB page allocated as
>>> +     * four 4 KB pages), calc_offset() guarantees that the page-table index
>>> +     * will always fall within the range [0, 511].
>>> +     */
>>> +    ASSERT(ctx && ctx->index < PAGETABLE_ENTRIES);
>>>   
>>> -    return rc;
>>> +    /*
>>> +     * At the moment, p2m_get_root_pointer() returns one of four possible 
>>> p2m
>>> +     * root pages, so there is no need to search for the correct ->pt_page
>>> +     * here.
>>> +     * Non-root page tables are 4 KB pages, so simply using ->pt_page is
>>> +     * sufficient.
>>> +     */
>>> +    md_pg = &ctx->pt_page->v.md.pg;
>>> +
>>> +    if ( !*md_pg && (t >= p2m_first_external) )
>>> +    {
>>> +        /*
>>> +         * Since p2m_alloc_page() initializes an allocated page with
>>> +         * zeros, p2m_invalid is expected to have the value 0 as well.
>>> +         */
>>> +        BUILD_BUG_ON(p2m_invalid);
>>> +
>>> +        ASSERT(ctx->p2m);
>> I think I previously asked for this to be moved out of the if(). Else you
>> may not notice a caller side issue until a point where a metadata page
>> actually needs allocating. (This could simply be folded into the earlier
>> ASSERT().)
> 
> I think that I understand your intention and okay to fold ASSERT(ctx->p2m)
> into the earlier ASSERT().
> Just want to note that generally if the metadata page has been already
> allocated and then p2m_set_type() will be called with ctx->p2m == NULL then
> nothing serious will happen as basically ctx->p2m is needed in this function
> only for allocation of metadata page.

Correct, but how would any particular caller know? Yes, there may be special
cases where a caller does know, but imo the code is going to be more robust
if the check is always being made (forcing all callers to set ->p2m).

Jan