On 13.05.2024 15:40, Elias El Yandouzi wrote:
> @@ -77,18 +80,24 @@ void *map_domain_page(mfn_t mfn)
> struct vcpu_maphash_entry *hashent;
>
> #ifdef NDEBUG
> - if ( mfn_x(mfn) <= PFN_DOWN(__pa(HYPERVISOR_VIRT_END - 1)) )
> + if ( arch_mfns_in_directmap(mfn_x(mfn), 1) )
> return mfn_to_virt(mfn_x(mfn));
> #endif
>
> v = mapcache_current_vcpu();
> - if ( !v )
> - return mfn_to_virt(mfn_x(mfn));
> + if ( !v || !v->domain->arch.mapcache.inuse )
> + {
> + if ( arch_mfns_in_directmap(mfn_x(mfn), 1) )
> + return mfn_to_virt(mfn_x(mfn));
> + else
> + {
> + BUG_ON(system_state >= SYS_STATE_smp_boot);
> + return pmap_map(mfn);
> + }
> + }
>
> dcache = &v->domain->arch.mapcache;
> vcache = &v->arch.mapcache;
> - if ( !dcache->inuse )
> - return mfn_to_virt(mfn_x(mfn));
Is this case (the logic for which you move up) actually possible? I.e.
can we observe a domain here which hasn't made it through
mapcache_domain_init() (where ->inuse is set)?
> @@ -184,6 +193,12 @@ void unmap_domain_page(const void *ptr)
> if ( !va || va >= DIRECTMAP_VIRT_START )
> return;
>
> + if ( va >= FIXADDR_START && va < FIXADDR_TOP )
> + {
> + pmap_unmap((void *)ptr);
Misra is going to object to this casting away of const. It's rather
pmap_unmap() which wants changing, to accept a pointer-to-const.
> @@ -335,6 +350,23 @@ mfn_t domain_page_map_to_mfn(const void *ptr)
> if ( va >= DIRECTMAP_VIRT_START )
> return _mfn(virt_to_mfn(ptr));
>
> + /*
> + * The fixmap is stealing the top-end of the VMAP. So the check for
> + * the PMAP *must* happen first.
Not really. You could also ...
> + * Also, the fixmap translate a slot to an address backwards. The
> + * logic will rely on it to avoid any complexity. So check at
> + * compile time this will always hold.
> + */
> + BUILD_BUG_ON(fix_to_virt(FIX_PMAP_BEGIN) < fix_to_virt(FIX_PMAP_END));
> +
> + if ( ((unsigned long)fix_to_virt(FIX_PMAP_END) <= va) &&
> + ((va & PAGE_MASK) <= (unsigned long)fix_to_virt(FIX_PMAP_BEGIN)) )
> + {
> + BUG_ON(system_state >= SYS_STATE_smp_boot);
> + return l1e_get_mfn(l1_fixmap[l1_table_offset(va)]);
> + }
> +
> if ( va >= VMAP_VIRT_START && va < VMAP_VIRT_END )
... put it into the body of this if() then. Which might be preferable to
keep the non-global-mapping case straight / quick.
Jan
> return vmap_to_mfn(va);
>
