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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

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