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

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

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


On 10/9/25 2:10 PM, Jan Beulich wrote:
> 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.

I was confused by|arch_domain_shutdown()|, which is called somewhere inside
|domain_crash()|, since the function name suggests that some resource cleanup
might happen there. There’s also no comment explaining what
|arch_domain_shutdown()| is expected to do or not to do.

However, since it’s an architecture-specific function, we can control its
behavior for a given architecture.

So, if it doesn’t actually start tearing down the domain, I don’t see any
other negative effects.

Anyway, if|domain_crash()| is called, I’m not really sure we need to set
PTE type afterward. We could simply add a|return;| right after the
|domain_crash()| call and so we won't have NULL pointer deference.

Thanks.

~ Oleksii

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