Re: [PATCH v3 14/20] xen/riscv: Implement p2m_pte_from_mfn() and support PBMT configuration

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 8/11/25 1:36 PM, Jan Beulich wrote:

> On 31.07.2025 17:58, Oleksii Kurochko wrote:
> > --- a/xen/arch/riscv/p2m.c
> > +++ b/xen/arch/riscv/p2m.c
> > @@ -1,3 +1,4 @@
> > +#include <xen/bug.h>
> >  #include <xen/domain_page.h>
> >  #include <xen/mm.h>
> >  #include <xen/rwlock.h>
> > @@ -197,6 +198,18 @@ static pte_t *p2m_get_root_pointer(struct p2m_domain *p2m, gfn_t gfn)
> >      return __map_domain_page(p2m->root + root_table_indx);
> >  }
> >  
> > +static int p2m_set_type(pte_t *pte, p2m_type_t t)
> > +{
> > +    int rc = 0;
> > +
> > +    if ( t > p2m_ext_storage )
> >       
> Seeing this separator enumerator in use, it becomes pretty clear that its name
> needs to change, so one doesn't need to go look at its definition to understand
> whether it's inclusive or exclusive. (This isn't helped by there presently being
> a spare entry, which, when made use of, might then cause problems with
> expressions like this one as well.)
>     
Then p2m_pte_type_count might be a better name, as it indicates how many types are
stored directly in the PTE bits.

> > @@ -222,11 +235,71 @@ static inline void p2m_clean_pte(pte_t *p, bool clean_pte)
> >      p2m_write_pte(p, pte, clean_pte);
> >  }
> >  
> > -static pte_t p2m_pte_from_mfn(mfn_t mfn, p2m_type_t t)
> > +static void p2m_set_permission(pte_t *e, p2m_type_t t)
> >  {
> > -    panic("%s: hasn't been implemented yet\n", __func__);
> > +    e->pte &= ~PTE_ACCESS_MASK;
> > +
> > +    switch ( t )
> > +    {
> > +    case p2m_grant_map_rw:
> > +    case p2m_ram_rw:
> > +        e->pte |= PTE_READABLE | PTE_WRITABLE;
> > +        break;
> >       
> While I agree for r/w grants, shouldn't r/w RAM also be executable?
>
> > +    case p2m_ext_storage:
> >       
> Why exactly would this placeholder ...
>
> > +    case p2m_mmio_direct_io:
> > +        e->pte |= PTE_ACCESS_MASK;
> > +        break;
> >       
> ... gain full access? It shouldn't make it here at all, should it?
>     
I missed to add break between them, but I don't remember why I
put it here.
It could be freely moved before "default".

And, yes, you are right it seems like is shouldn't be handled at all
in this function as this function isn't expected to be called with
this type as this type only is used to indicate that a real type is
stored somwehere.
> > +
> > +    case p2m_invalid:
> > +        e->pte &= ~(PTE_ACCESS_MASK | PTE_VALID);
> >       
> Redundantly masking off PTE_ACCESS_MASK? (Plus, for the entry to be
> invalid, turning off PTE_VALID alone ought to suffice anyway?)
>     
Agree, turning off PTE_VALID would be just enough.
> > +        break;
> > +
> > +    case p2m_grant_map_ro:
> > +        e->pte |= PTE_READABLE;
> > +        break;
> > +
> > +    default:
> > +        ASSERT_UNREACHABLE();
> > +        break;
> > +    }
> > +}
> > +
> > +static pte_t p2m_pte_from_mfn(mfn_t mfn, p2m_type_t t, bool is_table)
> > +{
> > +    pte_t e = (pte_t) { PTE_VALID };
> >       
> This and the rest of the function demand that mfn != INVALID_MFN, no matter
> whether ...
>
> > +    switch ( t )
> > +    {
> > +    case p2m_mmio_direct_io:
> > +        e.pte |= PTE_PBMT_IO;
> > +        break;
> > +
> > +    default:
> > +        break;
> > +    }
> > +
> > +    pte_set_mfn(&e, mfn);
> > +
> > +    ASSERT(!(mfn_to_maddr(mfn) & ~PADDR_MASK));
> >       
> ... PADDR_MASK is actually narrow enough to catch that case. Maybe best to
> add an explicit assertion to that effect?
>     
Then it should be enough instead of what we have now:
  ASSERT(mfn_valid(mfn));

> > +    if ( !is_table )
> > +    {
> > +        p2m_set_permission(&e, t);
> > +
> > +        if ( t < p2m_ext_storage )
> > +            p2m_set_type(&e, t);
> > +        else
> > +            panic("unimplemeted\n");
> >       
> The check is already done inside p2m_set_type() - why open-code it here?
>     
It isn't really matters now (so could be dropped), but in further patch this part
of code will look like:
        metadata[indx].pte = p2m_invalid;

        if ( t < p2m_ext_storage )
            p2m_set_type(&e, t, indx);
        else
        {
            e.pte |= MASK_INSR(p2m_ext_storage, P2M_TYPE_PTE_BITS_MASK);
            p2m_set_type(metadata, t, indx);
        }
So my intention was to re-use p2m_set_type() without changing of a prototype. So,
if a type is stored in PTE bits then we pass PTE directly, if not - then pass
metadata.

Thanks.
~ Oleksii