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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

>>> +    }
>>> +    else
>>> +    {
>>> +        pte->pte |= MASK_INSR(p2m_ext_storage, P2M_TYPE_PTE_BITS_MASK);
>>> +
>>> +        metadata[ctx->index].pte = t;
>> If you set t to p2m_ext_storage here, the pte->pte updating could be moved 
>> ...
> 
> 't' shouldn't be passed as 'p2m_ext_storage'.

Of course not. I said "set", not "pass". I suggested to set t to p2m_ext_storage
right after the assignment above. I notice though that I missed ...

> For example, in this case we will have that in metadata page we will have type
> equal to p2m_ext_storage and then in pte->pte will have the type set to
> p2m_ext_storage, and the we end that we don't have a real type stored 
> somewhere.
> Even more, metadata.pte shouldn't be used to store p2m_ext_storage, only
> p2m_invalid and types mentioned in enum p2m_t after p2m_ext_storage.
> 
>>
>>> +    }
>> ... here, covering both cases. Overally this may then be easier as
>>
>>      if ( t >= p2m_first_external )
>>          metadata[ctx->index].pte = t;

... the respective line (and the figure braces which are the needed) here:

        t = p2m_ext_storage;

>>      else if ( metadata )
>>          metadata[ctx->index].pte = p2m_invalid;
>>
>>      pte->pte |= MASK_INSR(t, P2M_TYPE_PTE_BITS_MASK);
>>
>> Then raising the question whether it couldn't still be the real type that's
>> stored in metadata[] even for t < p2m_first_external. That woiuld further
>> reduce conditionals.
> 
> It would be nice, but I think that at the moment we can’t do that. As I 
> explained
> above, 't' should not normally be passed as p2m_ext_storage.

Of course not, but how's that relevant to storing the _real_ type in the
metadata page even when it's one which can also can be stored in the PTE?
As said, for a frequently used path it may help to reduce the number of
conditionals here.

>>> +static void p2m_free_page(struct p2m_domain *p2m, struct page_info *pg);
>>> +
>>> +/*
>>> + * Free page table's page and metadata page linked to page table's page.
>>> + */
>>> +static void p2m_free_table(struct p2m_domain *p2m, struct page_info 
>>> *tbl_pg)
>>> +{
>>> +    if ( tbl_pg->v.md.pg )
>>> +        p2m_free_page(p2m, tbl_pg->v.md.pg);
>> To play safe, maybe better also clear tbl_pg->v.md.pg?
> 
> I thought it would be enough to clear it during allocation in 
> p2m_alloc_page(),
> since I'm not sure it is critical if md.pg data were somehow leaked and read.
> But to be safer, we can add this here:
>    clear_and_clean_page(tbl_pg->v.md.pg, p2m->clean_dcache);

I didn't say clear what tbl_pg->v.md.pg points to, though. I suggested to clear
the struct field itself.

>>> @@ -749,6 +849,10 @@ static bool p2m_split_superpage(struct p2m_domain 
>>> *p2m, pte_t *entry,
>>>       unsigned int next_level = level - 1;
>>>       unsigned int level_order = P2M_LEVEL_ORDER(next_level);
>>>   +    struct p2m_pte_ctx p2m_pte_ctx;
>> I think this would better be one variable instance per scope where it's 
>> needed,
>> and then using an initzializer. Or else ...
>>
>>> +    /* Init with p2m_invalid just to make compiler happy. */
>>> +    p2m_type_t old_type = p2m_invalid;
>>> +
>>>       /*
>>>        * This should only be called with target != level and the entry is
>>>        * a superpage.
>>> @@ -770,6 +874,19 @@ static bool p2m_split_superpage(struct p2m_domain 
>>> *p2m, pte_t *entry,
>>>         table = __map_domain_page(page);
>>>   +    if ( MASK_EXTR(entry->pte, P2M_TYPE_PTE_BITS_MASK) == 
>>> p2m_ext_storage )
>>> +    {
>>> +        p2m_pte_ctx.pt_page = tbl_pg;
>>> +        p2m_pte_ctx.index = offsets[level];
>>> +        /*
>>> +         * It doesn't really matter what is a value for a level as
>>> +         * p2m_get_type() doesn't need it, so it is initialized just in 
>>> case.
>>> +         */
>>> +        p2m_pte_ctx.level = level;
>>> +
>>> +        old_type = p2m_get_type(*entry, &p2m_pte_ctx);
>>> +    }
>>> +
>>>       for ( i = 0; i < P2M_PAGETABLE_ENTRIES(next_level); i++ )
>>>       {
>>>           pte_t *new_entry = table + i;
>>> @@ -781,6 +898,15 @@ static bool p2m_split_superpage(struct p2m_domain 
>>> *p2m, pte_t *entry,
>>>           pte = *entry;
>>>           pte_set_mfn(&pte, mfn_add(mfn, i << level_order));
>>>   +        if ( MASK_EXTR(pte.pte, P2M_TYPE_PTE_BITS_MASK) == 
>>> p2m_ext_storage )
>>> +        {
>>> +            p2m_pte_ctx.pt_page = page;
>>> +            p2m_pte_ctx.index = i;
>>> +            p2m_pte_ctx.level = next_level;
>> ... why are the loop-invariat fields not filled ahead of the loop here?
> 
> Actually, they could be filled before the loop. If I move the initialization 
> of
> p2m_pte_ctx.pt_page and p2m_pte_ctx.level ahead of the loop, does it still 
> make
> sense to have a separate variable inside
> "if (MASK_EXTR(entry->pte, P2M_TYPE_PTE_BITS_MASK) == p2m_ext_storage)"?

No, it's one of the two - scope limited variable within the loop, or wider-scope
variable with loop-invariant fields set ahead of the loop.

Jan