Re: [PATCH v2 30/40] xen/mpu: disable VMAP sub-system for MPU systems

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.01.2023 06:29, Penny Zheng wrote:
> VMAP in MMU system, is used to remap a range of normal memory
> or device memory to another virtual address with new attributes
> for specific purpose, like ALTERNATIVE feature. Since there is
> no virtual address translation support in MPU system, we can
> not support VMAP in MPU system.
> 
> So in this patch, we disable VMAP for MPU systems, and some
> features depending on VMAP also need to be disabled at the same
> time, Like ALTERNATIVE, CPU ERRATA.
> 
> Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> ---
>  xen/arch/arm/Kconfig                   |  3 +-
>  xen/arch/arm/Makefile                  |  2 +-
>  xen/arch/arm/include/asm/alternative.h | 15 +++++
>  xen/arch/arm/include/asm/cpuerrata.h   | 12 ++++
>  xen/arch/arm/setup.c                   |  7 +++
>  xen/arch/x86/Kconfig                   |  1 +
>  xen/common/Kconfig                     |  3 +
>  xen/common/Makefile                    |  2 +-
>  xen/include/xen/vmap.h                 | 81 ++++++++++++++++++++++++--
>  9 files changed, 119 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index c6b6b612d1..9230c8b885 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -11,12 +11,13 @@ config ARM_64
>  
>  config ARM
>       def_bool y
> -     select HAS_ALTERNATIVE
> +     select HAS_ALTERNATIVE if !ARM_V8R

Judging from the connection you make in the description, I think this
wants to be "if HAS_VMAP".

>       select HAS_DEVICE_TREE
>       select HAS_PASSTHROUGH
>       select HAS_PDX
>       select HAS_PMAP
>       select IOMMU_FORCE_PT_SHARE
> +     select HAS_VMAP if !ARM_V8R

I think entries here are intended to be sorted alphabetically.

> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -28,6 +28,7 @@ config X86
>       select HAS_UBSAN
>       select HAS_VPCI if HVM
>       select NEEDS_LIBELF
> +     select HAS_VMAP

Here they are certainly meant to be.

> --- a/xen/include/xen/vmap.h
> +++ b/xen/include/xen/vmap.h
> @@ -1,15 +1,17 @@
> -#if !defined(__XEN_VMAP_H__) && defined(VMAP_VIRT_START)
> +#if !defined(__XEN_VMAP_H__) && (defined(VMAP_VIRT_START) || 
> !defined(CONFIG_HAS_VMAP))
>  #define __XEN_VMAP_H__
>  
> -#include <xen/mm-frame.h>
> -#include <xen/page-size.h>
> -
>  enum vmap_region {
>      VMAP_DEFAULT,
>      VMAP_XEN,
>      VMAP_REGION_NR,
>  };
>  
> +#ifdef CONFIG_HAS_VMAP
> +
> +#include <xen/mm-frame.h>
> +#include <xen/page-size.h>
> +
>  void vm_init_type(enum vmap_region type, void *start, void *end);
>  
>  void *__vmap(const mfn_t *mfn, unsigned int granularity, unsigned int nr,
> @@ -38,4 +40,75 @@ static inline void vm_init(void)
>      vm_init_type(VMAP_DEFAULT, (void *)VMAP_VIRT_START, 
> arch_vmap_virt_end());
>  }
>  
> +#else /* !CONFIG_HAS_VMAP */
> +
> +static inline void vm_init_type(enum vmap_region type, void *start, void 
> *end)
> +{
> +    ASSERT_UNREACHABLE();
> +}

Do you really need this and all other inline stubs? Imo the goal ought
to be to have as few of them as possible: The one above won't be
referenced if you further make LIVEPATCH depend on HAS_VMAP (which I
think you need to do anyway), and the only other call to the function
is visible in context above (i.e. won't be used either when !HAS_VMAP).
In other cases merely having a declaration (but no definition) may be
sufficient, as the compiler may be able to eliminate calls.

Jan