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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 11/20/25 4:47 PM, Jan Beulich wrote:

> On 20.11.2025 16:38, Oleksii Kurochko wrote:
> > On 11/18/25 7:58 AM, Jan Beulich wrote:
> > > 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).
> > I think I still don’t fully understand what the issue would be if|p2m_invalid| were
> > ever equal to 1 instead of 0 in the context of a metadata page.
> >
> > Yes, if|p2m_invalid| were 1, there would be a problem if someone tried to read this
> > metadata pagebefore it was assigned any type. They would find a value of 0, which
> > corresponds to a valid type rather than to|p2m_invalid|, as one might expect.
> > However, I’m not sure I currently see a scenario in which the metadata page would
> > be read before being initialized.
> Are you sure walks can only happen for GFNs that were set up? What you need to
> do walks on is under guest control, after all.
>     
If a GFN lies within the range [p2m->lowest_mapped_gfn, p2m->max_mapped_gfn], then
p2m_set_entry() must already have been called for this GFN. This means that either
- a metadata page has been created and its entry filled with the appropriate type, or
- no metadata page was needed and the type was stored directly in pte->pte
For a GFN outside the range (p2m->lowest_mapped_gfn, p2m->max_mapped_gfn),
p2m_get_entry() will not even attempt a walk because of the boundary checks:
    static mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
                               p2m_type_t *t,
                               unsigned int *page_order)
    ...
        if ( check_outside_boundary(p2m, gfn, p2m->lowest_mapped_gfn, true,
                                    &level) )
            goto out;
    
        if ( check_outside_boundary(p2m, gfn, p2m->max_mapped_gfn, false, &level) )
            goto out;

If I am misunderstanding something and there are other cases where a walk can occur for
GFNs that were never set up, then such GFNs would have neither an allocated metadata
page nor a type stored in pte->pte, which looks like we are in trouble.

~ Oleksii