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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

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 page before 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.
But just to be safe when such case will occur I am okay with putting
BUILD_BUG_ON(p2m_invalid) before p2m_alloc_page() in p2m_set_type() function.

> > > > +    }
> > > > +    else
> > > > +    {
> > > > +        pte->pte |= MASK_INSR(p2m_ext_storage, P2M_TYPE_PTE_BITS_MASK);
> > > > +
> > > > +        metadata[ctx->index].pte = t;
> > > >           
> > > If you set t to p2m_ext_storage here, the pte->pte updating could be moved ...
> > >         
> > 't' shouldn't be passed as 'p2m_ext_storage'.
> >       
> Of course not. I said "set", not "pass". I suggested to set t to p2m_ext_storage
> right after the assignment above. I notice though that I missed ...
>     
Now, I see then ...
> > For example, in this case we will have that in metadata page we will have type
> > equal to p2m_ext_storage and then in pte->pte will have the type set to
> > p2m_ext_storage, and the we end that we don't have a real type stored somewhere.
> > Even more, metadata.pte shouldn't be used to store p2m_ext_storage, only
> > p2m_invalid and types mentioned in enum p2m_t after p2m_ext_storage.
> >
> > > > +    }
> > > >           
> > > ... here, covering both cases. Overally this may then be easier as
> > >
> > >      if ( t >= p2m_first_external )
> > >          metadata[ctx->index].pte = t;
> > >         
> >       
> ... the respective line (and the figure braces which are the needed) here:
>
>         t = p2m_ext_storage;
>     
...  (what suggested above) will work.

> > >      else if ( metadata )
> > >          metadata[ctx->index].pte = p2m_invalid;
> > >
> > >      pte->pte |= MASK_INSR(t, P2M_TYPE_PTE_BITS_MASK);
> > >
> > > Then raising the question whether it couldn't still be the real type that's
> > > stored in metadata[] even for t < p2m_first_external. That woiuld further
> > > reduce conditionals.
> > >         
> > It would be nice, but I think that at the moment we can’t do that. As I explained
> > above, 't' should not normally be passed as p2m_ext_storage.
> >       
> Of course not, but how's that relevant to storing the _real_ type in the
> metadata page even when it's one which can also can be stored in the PTE?
> As said, for a frequently used path it may help to reduce the number of
> conditionals here.
>     
IIUC, you are asking whether, if pte->pte stores a type < p2m_ext_storage,
it would still be possible for metadata[].pte to contain any real type?
If yes, then the answer is that it could be done, because in the p2m_get_type()
function the value stored in pte->pte is checked first. If it isn't p2m_ext_storage,
then metadata[].pte will not be checked at all. So technically, it could contain
whatever we want in case when pte.pte's type != p2m_ext_storage.
But will it really reduce an amount of conditions? It seems like we still need one
condition to check of metadata is mapped and one condition to set 't' to p2m_ext_storage:
  if ( metadata )
       metadata[ctx->index].pte = t;
  
  if ( t >= p2m_first_external )
     t = p2m_ext_storage;

  pte->pte |= MASK_INSR(t, P2M_TYPE_PTE_BITS_MASK);

We can do:
  if ( metadata )
  {
       metadata[ctx->index].pte = t;
       
       if ( t >= p2m_first_external )
            t = p2m_ext_storage;
  }

  pte->pte |= MASK_INSR(t, P2M_TYPE_PTE_BITS_MASK);
It will reduce an amount of conditions if metadata wasn't used/allocated, but I think you
have a different idea, don't you?


> > > > +static void p2m_free_page(struct p2m_domain *p2m, struct page_info *pg);
> > > > +
> > > > +/*
> > > > + * Free page table's page and metadata page linked to page table's page.
> > > > + */
> > > > +static void p2m_free_table(struct p2m_domain *p2m, struct page_info *tbl_pg)
> > > > +{
> > > > +    if ( tbl_pg->v.md.pg )
> > > > +        p2m_free_page(p2m, tbl_pg->v.md.pg);
> > > >           
> > > To play safe, maybe better also clear tbl_pg->v.md.pg?
> > >         
> > I thought it would be enough to clear it during allocation in p2m_alloc_page(),
> > since I'm not sure it is critical if md.pg data were somehow leaked and read.
> > But to be safer, we can add this here:
> >    clear_and_clean_page(tbl_pg->v.md.pg, p2m->clean_dcache);
> >       
> I didn't say clear what tbl_pg->v.md.pg points to, though. I suggested to clear
> the struct field itself.
>     
Won't be enough just tbl_pg->v.md.pg = NULL; ?

Thanks!

~ Oleksii