Re: [PATCH 1/5] common/vmap: Fall back to simple allocator when !HAS_VMAP

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.11.2024 11:50, Luca Fancellu wrote:
> --- a/xen/common/vmap.c
> +++ b/xen/common/vmap.c
> @@ -426,3 +426,10 @@ void *_xvrealloc(void *va, size_t size, unsigned int 
> align)
>  
>      return ptr;
>  }
> +
> +void iounmap(void __iomem *va)
> +{
> +    unsigned long addr = (unsigned long)(void __force *)va;
> +
> +    vunmap((void *)(addr & PAGE_MASK));
> +}

Why is this being moved here, and converted from inline to out-of-line?
What the description says is insufficient imo, as even if you mean to
only support vmap_contig() and ioremap() on MPU systems, you'll still
need both vunmap() and iounmap().

Plus, if it really needs converting, I don't think it should live at the
very end of the file, past _xvmalloc() and friends. Better suitable places
may then be next to vunmap() itself, or between vfree() and xvfree().

> --- a/xen/include/xen/vmap.h
> +++ b/xen/include/xen/vmap.h
> @@ -5,7 +5,7 @@
>   * purpose area (VMAP_DEFAULT) and a livepatch-specific area (VMAP_XEN). The
>   * latter is used when loading livepatches and the former for everything 
> else.
>   */
> -#if !defined(__XEN_VMAP_H__) && defined(VMAP_VIRT_START)
> +#if !defined(__XEN_VMAP_H__)
>  #define __XEN_VMAP_H__

With this adjustment, where are the functions defined that you "unhide"
the declarations of, in the MPU case? As you say in the description,
vmap.c won't be built in that case.

Also both here and ...

> --- a/xen/include/xen/xvmalloc.h
> +++ b/xen/include/xen/xvmalloc.h
> @@ -40,20 +40,46 @@
>      ((typeof(ptr))_xvrealloc(ptr, offsetof(typeof(*(ptr)), field[nr]), \
>                               __alignof__(typeof(*(ptr)))))
>  
> +#if defined(CONFIG_HAS_VMAP)

... here: Please use the shorter #ifdef when possible.

Jan