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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 11/21/25 8:35 AM, Jan Beulich wrote:

> On 20.11.2025 17:52, Oleksii Kurochko wrote:
> > 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.
> >       
> No. All you know from the pre-condition is that p2m_set_entry() was called for the
> lowest and highest GFNs in that range.
>
>     
Oh, right. There could still be some GFNs inside the range for which p2m_set_entry() has
not yet been called. Then it probably makes sense to add a BUILD_BUG_ON here as well, before
"if ( type == p2m_ext_storage )":

    static p2m_type_t p2m_get_type(const pte_t pte, const struct p2m_pte_ctx *ctx)
    {
        p2m_type_t type = MASK_EXTR(pte.pte, P2M_TYPE_PTE_BITS_MASK);
    
        if ( type == p2m_ext_storage )
        {
            const pte_t *md = __map_domain_page(ctx->pt_page->v.md.pg);
    
            type = md[ctx->index].pte;
    
            unmap_domain_page(md);
        }
    
        return type;
    }

I would expect that if p2m_set_entry() has not been called for a GFN, then p2m_get_entry()
(p2m_get_type() will be called somewhere inside p2m_get_entry(), for example) should return
the p2m_invalid type. I think we want to have the same check (as the one before the call to
p2m_alloc_page()), placed before the condition:
  BUILD_BUG_ON(p2m_invalid);

~ Oleksii
> > 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
> >
> >       
>