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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

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

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

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

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

Jan