Re: [PATCH v2 13/17] xen/riscv: Implement p2m_entry_from_mfn() and support PBMT configuration

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 7/1/25 5:08 PM, Jan Beulich wrote:

> On 10.06.2025 15:05, Oleksii Kurochko wrote:
>
> > --- a/xen/arch/riscv/p2m.c
> > +++ b/xen/arch/riscv/p2m.c
> > @@ -345,6 +345,26 @@ 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_type_radix_set(struct p2m_domain *p2m, pte_t pte, p2m_type_t t)
> >       
> See comments on the earlier patch regarding naming.
>
> > +{
> > +    int rc;
> > +    gfn_t gfn = mfn_to_gfn(p2m->domain, mfn_from_pte(pte));
> >       
> How does this work, when you record GFNs only for Xenheap pages?
>     
I think I don't understand what is an issue. Could you please provide
some extra details?

> I don't
> think you can get around having the caller pass in the GFN. At which point
> the PTE probably doesn't need passing.
>     
It’s an option. PTE argument, I think, we still need because as we discussed
earlier, partly some P2M types will be stored in PTE bits.

I’m also wondering whether the MFN could be used to identify the P2M PTE’s type,
or if, in general, it isn’t unique (since different GFNs can map to the same MFN),
meaning it can't reliably be used to determine the PTE’s type. Right?

> > +    rc = radix_tree_insert(&p2m->p2m_type, gfn_x(gfn),
> > +                           radix_tree_int_to_ptr(t));
> > +    if ( rc == -EEXIST )
> > +    {
> > +        /* If a setting already exists, change it to the new one */
> > +        radix_tree_replace_slot(
> > +            radix_tree_lookup_slot(
> > +                &p2m->p2m_type, gfn_x(gfn)),
> > +            radix_tree_int_to_ptr(t));
> > +        rc = 0;
> > +    }
> > +
> > +    return rc;
> > +}
> > +
> >  static p2m_type_t p2m_type_radix_get(struct p2m_domain *p2m, pte_t pte)
> >  {
> >      void *ptr;
> > @@ -389,12 +409,87 @@ static inline void p2m_remove_pte(pte_t *p, bool clean_pte)
> >      p2m_write_pte(p, pte, clean_pte);
> >  }
> >  
> > -static pte_t p2m_entry_from_mfn(struct p2m_domain *p2m, mfn_t mfn,
> > -                                p2m_type_t t, p2m_access_t a)
> > +static void p2m_set_permission(pte_t *e, p2m_type_t t, p2m_access_t a)
> >  {
> > -    panic("%s: hasn't been implemented yet\n", __func__);
> > +    /* First apply type permissions */
> > +    switch ( t )
> > +    {
> > +    case p2m_ram_rw:
> > +        e->pte |= PTE_ACCESS_MASK;
> > +        break;
> > +
> > +    case p2m_mmio_direct_dev:
> > +        e->pte |= (PTE_READABLE | PTE_WRITABLE);
> > +        e->pte &= ~PTE_EXECUTABLE;
> >       
> What's wrong with code living in MMIO, e.g. in the ROM of a PCI device?
> Such code would want to be executable.
>     
I think you are right and nothing wrong with code living in MMIO.

According to the spec:
  I/O regions can specify which combinations of read, write, or execute accesses
  to which data widths are supported.
> > +        break;
> > +
> > +    case p2m_invalid:
> > +        e->pte &= ~PTE_ACCESS_MASK;
> > +        break;
> > +
> > +    default:
> > +        BUG();
> > +        break;
> > +    }
> >       
> I think you ought to handle all types that are defined right away. I also
> don't think you should BUG() in the default case (also in the other switch()
> below). ASSERT_UNEACHABLE() may be fine, along with clearing all permissions
> in the entry for release builds.
>
> > +    /* Then restrict with access permissions */
> > +    switch ( a )
> > +    {
> > +    case p2m_access_rwx:
> > +        break;
> > +    case p2m_access_wx:
> > +        e->pte &= ~PTE_READABLE;
> > +        break;
> > +    case p2m_access_rw:
> > +        e->pte &= ~PTE_EXECUTABLE;
> > +        break;
> > +    case p2m_access_w:
> > +        e->pte &= ~(PTE_READABLE | PTE_EXECUTABLE);
> > +        e->pte &= ~PTE_EXECUTABLE;
> > +        break;
> > +    case p2m_access_rx:
> > +    case p2m_access_rx2rw:
> > +        e->pte &= ~PTE_WRITABLE;
> > +        break;
> > +    case p2m_access_x:
> > +        e->pte &= ~(PTE_READABLE | PTE_WRITABLE);
> > +        break;
> > +    case p2m_access_r:
> > +        e->pte &= ~(PTE_WRITABLE | PTE_EXECUTABLE);
> > +        break;
> > +    case p2m_access_n:
> > +    case p2m_access_n2rwx:
> > +        e->pte &= ~PTE_ACCESS_MASK;
> > +        break;
> > +    default:
> > +        BUG();
> > +        break;
> > +    }
> >       
> Nit: Blank lines between non-fall-through case blocks, please.
>
> > +static pte_t p2m_entry_from_mfn(struct p2m_domain *p2m, mfn_t mfn, p2m_type_t t, p2m_access_t a)
> > +{
> > +    pte_t e = (pte_t) { 1 };
> >       
> What's the 1 doing here?
>     
Set valid bit of PTE to 1.

> > +    switch ( t )
> > +    {
> > +    case p2m_mmio_direct_dev:
> > +        e.pte |= PTE_PBMT_IO;
> > +        break;
> > +
> > +    default:
> > +        break;
> > +    }
> > +
> > +    p2m_set_permission(&e, t, a);
> > +
> > +    ASSERT(!(mfn_to_maddr(mfn) & ~PADDR_MASK));
> > +
> > +    pte_set_mfn(&e, mfn);
> >       
> Based on how things work on x86 (and how I would have expected them to also
> work on Arm), may I suggest that you set MFN ahead of permissions, so that
> the permissions setting function can use the MFN for e.g. a lookup in
> mmio_ro_ranges.
>     
Sure, just a note that on Arm, the MFN is set last.

> > +    BUG_ON(p2m_type_radix_set(p2m, e, t));
> >       
> I'm not convinced of this error handling here either. Radix tree insertion
> _can_ fail, e.g. when there's no memory left. This must not bring down Xen,
> or we'll have an XSA right away. You could zap the PTE, or if need be you
> could crash the offending domain.
>     
IIUC what is "zap the PTE", then I will do in this way:
    if ( p2m_set_type(p2m, e, t) )
        e.pte = 0;

But then it will lead to an MMU failure—how is that expected to be handled?
There’s no guarantee that, at the moment of handling this exception, enough
memory will be available to set a type for the PTE and also there is not really
clear how to detect in exception handler that it is needed just to re-try to
set a type. Or should we just call domain_crash()?
In that case, it seems more reasonable to call domain_crash() immediately in
p2m_pte_from_mfn().
> In this context (not sure if I asked before): With this use of a radix tree,
> how do you intend to bound the amount of memory that a domain can use, by
> making Xen insert very many entries?
>     
I didn’t think about that. I assumed it would be enough to set the amount of
memory a guest domain can use by specifying xen,domain-p2m-mem-mb in the DTS,
or using some predefined value if xen,domain-p2m-mem-mb isn’t explicitly set.
Also, it seems this would just lead to the issue you mentioned earlier: when
the memory runs out, domain_crash() will be called or PTE will be zapped.
~ Oleksii