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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.06.2025 15:05, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/include/asm/page.h
> +++ b/xen/arch/riscv/include/asm/page.h
> @@ -76,6 +76,14 @@
>  #define PTE_SMALL       BIT(10, UL)
>  #define PTE_POPULATE    BIT(11, UL)
>  
> +enum pbmt_type_t {

Please can we stick to _t suffixes only being used on typedef-ed identifiers?

> +    pbmt_pma,
> +    pbmt_nc,
> +    pbmt_io,
> +    pbmt_rsvd,
> +    pbmt_max,

It's a 2-bit field in the PTE, isn't it? In which case the maximum valid value
to put there is 3. That's what an identifier named "max" should evaluate to.
The value 4 here would want to be named "count", "num", "nr", or alike.

> --- 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 don't
think you can get around having the caller pass in the GFN. At which point
the PTE probably doesn't need passing.

> +    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.

> +        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?

> +    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.

> +    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.

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?

Jan