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

[Jan Beulich <jbeulich@suse.com>]

On 09.10.2025 13:34, Oleksii Kurochko wrote:
> On 10/7/25 3:25 PM, Jan Beulich wrote:
>> 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:
>>>>> +    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.
> 
> Oh, right, for some reason, I thought we didn’t return from|domain_crash()|.
> I’m curious whether calling|domain_crash()| might break something, as some useful
> data could be freed and negatively affect the internals of|map_regions_p2mt()|.
> 
> It might make more sense to use|panic()| here instead.
> Do you have any thoughts or suggestions on this?

domain_crash() is generally preferable over crashing the system as a whole.
I don't follow what negative effects you're alluding to. Did you look at
what domain_crash() does? It doesn't start tearing down the domain, that'll
still need invoking from the toolstack. A crashed domain will stay around
with all its resources allocated.

Jan