Re: [PATCH v4 12/18] xen/riscv: Implement p2m_pte_from_mfn() and support PBMT configuration

[Jan Beulich <jbeulich@suse.com>]

On 17.09.2025 23:55, Oleksii Kurochko wrote:
> @@ -318,11 +331,87 @@ 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;
> +
> +    e->pte |= PTE_USER;
> +
> +    /*
> +     * Two schemes to manage the A and D bits are defined:
> +     *   • The Svade extension: when a virtual page is accessed and the A bit
> +     *     is clear, or is written and the D bit is clear, a page-fault
> +     *     exception is raised.
> +     *   • When the Svade extension is not implemented, the following scheme
> +     *     applies.
> +     *     When a virtual page is accessed and the A bit is clear, the PTE is
> +     *     updated to set the A bit. When the virtual page is written and the
> +     *     D bit is clear, the PTE is updated to set the D bit. When G-stage
> +     *     address translation is in use and is not Bare, the G-stage virtual
> +     *     pages may be accessed or written by implicit accesses to VS-level
> +     *     memory management data structures, such as page tables.
> +     * Thereby to avoid a page-fault in case of Svade is available, it is
> +     * necesssary to set A and D bits.
> +     */
> +    if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) )
> +        e->pte |= PTE_ACCESSED | PTE_DIRTY;

All of this depending on menvcfg.ADUE anyway, is this really needed? Isn't
machine mode software responsible for dealing with this kind of page faults
(just like the hypervisor is reponsible for dealing with ones resulting
from henvcfg.ADUE being clear)?

> +    switch ( t )
> +    {
> +    case p2m_map_foreign_rw:
> +    case p2m_mmio_direct_io:
> +        e->pte |= PTE_READABLE | PTE_WRITABLE;
> +        break;
> +
> +    case p2m_ram_rw:
> +        e->pte |= PTE_ACCESS_MASK;
> +        break;
> +
> +    case p2m_invalid:
> +        e->pte &= ~PTE_VALID;
> +        break;
> +
> +    case p2m_map_foreign_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 };
> +
> +    switch ( t )
> +    {
> +    case p2m_mmio_direct_io:
> +        e.pte |= PTE_PBMT_IO;
> +        break;

Shouldn't this be limited to the !is_table case (just like you have it ...

> +    default:
> +        break;
> +    }
> +
> +    pte_set_mfn(&e, mfn);
> +
> +    ASSERT(!(mfn_to_maddr(mfn) & ~PADDR_MASK) || mfn_eq(mfn, INVALID_MFN));
> +
> +    if ( !is_table )
> +    {
> +        p2m_set_permission(&e, t);

... here? Or else at least ASSERT(!is_table) up there? Personally I think
all of this !is_table stuff would best be done here.

Jan