On 11.08.2025 16:44, Oleksii Kurochko wrote: > On 8/11/25 1:36 PM, Jan Beulich wrote: >> On 31.07.2025 17:58, Oleksii Kurochko wrote: >>> +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? > > Then it should be enough instead of what we have now: > ASSERT(mfn_valid(mfn));
No, that would exclude MMIO living beyond max_page. >>> + 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? > > It isn't really matters now (so could be dropped), but in further patch this > part > of code will look like: > metadata[indx].pte = p2m_invalid; > > if ( t < p2m_ext_storage ) > p2m_set_type(&e, t, indx); > else > { > e.pte |= MASK_INSR(p2m_ext_storage, P2M_TYPE_PTE_BITS_MASK); > p2m_set_type(metadata, t, indx); > } > So my intention was to re-use p2m_set_type() without changing of a prototype. > So, > if a type is stored in PTE bits then we pass PTE directly, if not - then pass > metadata. Then at the very least p2m_set_type() may not be a good name; a function of this name imo should set the type, whatever it takes to do so. But I'm unconvinced of the model as a whole. Jan