Re: [PATCH v4 18/18] xen/riscv: introduce metadata table to store P2M type

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.10.2025 18:00, Oleksii Kurochko wrote:
> On 9/23/25 12:41 AM, Jan Beulich wrote:
>> On 17.09.2025 23:55, Oleksii Kurochko wrote:
>>
>>> +/*
>>> + * `pte` – PTE entry for which the type `t` will be stored.
>>> + *
>>> + * If `t` is `p2m_ext_storage`, both `ctx` and `p2m` must be provided;
>>> + * otherwise, they may be NULL.
>>> + */
>>> +static void p2m_set_type(pte_t *pte, const p2m_type_t t,
>>> +                         struct p2m_pte_ctx *ctx,
>>> +                         struct p2m_domain *p2m)
>>>   {
>>> -    int rc = 0;
>>> +    /*
>>> +    * For the root page table (16 KB in size), we need to select the 
>>> correct
>>> +    * metadata table, since allocations are 4 KB each. In total, there are
>>> +    * 4 tables of 4 KB each.
>>> +    * For none-root page table index of ->pt_page[] will be always 0 as
>>> +    * index won't be higher then 511. ASSERT() below verifies that.
>>> +    */
>>> +    struct page_info **md_pg =
>>> +        &ctx->pt_page[ctx->index / PAGETABLE_ENTRIES].v.md.metadata;
>>> +    pte_t *metadata = NULL;
>>> +
>>> +     /* Be sure that an index correspondent to page level is passed. */
>>> +    ASSERT(ctx->index <= P2M_PAGETABLE_ENTRIES(ctx->level));
>> Doesn't this need to be < ?
> 
> Yeah, it should be <.
> 
>>
>>> +    if ( !*md_pg && (t >= p2m_first_external) )
>>> +    {
>>> +        /*
>>> +         * Ensure that when `t` is stored outside the PTE bits
>>> +         * (i.e. `t == p2m_ext_storage` or higher),
>>> +         * both `ctx` and `p2m` are provided.
>>> +         */
>>> +        ASSERT(p2m && ctx);
>> Imo this would want to be checked whenever t > p2m_first_external, no
>> matter whether a metadata page was already allocated.
> 
> I think that|ctx| should be checked before this|if| condition, since it is
> used to obtain the proper metadata page.
> 
> The check for|p2m| can remain inside the|if| condition, as it is essentially
> only needed for allocating a metadata page.

That is, you want to allow callers to pass in NULL for the "p2m" parameter?
Isn't this going to be risky?

>>> -    if ( t > p2m_first_external )
>>> -        panic("unimplemeted\n");
>>> -    else
>>> +        if ( ctx->level <= P2M_SUPPORTED_LEVEL_MAPPING )
>>> +        {
>>> +            struct domain *d = p2m->domain;
>>> +
>>> +            *md_pg = p2m_alloc_table(p2m);
>>> +            if ( !*md_pg )
>>> +            {
>>> +                printk("%s: can't allocate extra memory for dom%d\n",
>>> +                        __func__, d->domain_id);
>>> +                domain_crash(d);
>>> +            }
>>> +        }
>>> +        else
>>> +            /*
>>> +             * It is not legal to set a type for an entry which shouldn't
>>> +             * be mapped.
>>> +             */
>>> +            ASSERT_UNREACHABLE();
>> Something not being legal doesn't mean it can't happen. Imo in this case
>> BUG_ON() (in place of the if() above) would be better.
>>
>>> +    }
>>> +
>>> +    if ( *md_pg )
>>> +        metadata = __map_domain_page(*md_pg);

Not this conditional assignment for ...

>>> +    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;
>>> +    }
>>> +    else
>>> +    {
>>> +        pte->pte |= MASK_INSR(p2m_ext_storage, P2M_TYPE_PTE_BITS_MASK);
>>> +
>>> +        metadata[ctx->index].pte = t;
>> Afaict metadata can still be NULL when you get here.
> 
> It shouldn't be, because when this line is executed, the metadata page already
> exists or was allocated at the start of p2m_set_type().

... this reply of yours. And the condition there can be false, in case you
took the domain_crash() path.

>>> @@ -812,13 +955,21 @@ static int p2m_set_entry(struct p2m_domain *p2m,
>>>       {
>>>           /* We need to split the original page. */
>>>           pte_t split_pte = *entry;
>>> +        struct page_info *tbl_pg = virt_to_page(table);
>> This isn't valid on a pointer obtained from map_domain_page().
> 
> Oh, sure — virt_to_page() and page_to_virt() should be used only for Xen
> heap addresses.
> 
> By the way, do we have any documentation, comments, or notes describing
> what should be allocated and from where?
> 
> Since map_domain_page() returns an address from the direct map region,
> should we instead use maddr_to_page(virt_to_maddr(table))?

How would that be any better? Even if right now you only build RISC-V code
with map_domain_page() having a trivial expansion, you will want to avoid
any assumptions along those lines. Or else you could avoid the use of that
abstraction altogether. It exists so when you need to support memory
amounts beyond what the directmap can cover, you can provide a suitable
implementation of the function and be done. You (or whoever else) in
particular shouldn't be required to go audit all the places where
map_domain_page() (and the pointers it returns) is (are) used.

Jan