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