Re: [PATCH v3 15/52] xen: make VMAP only support in MMU system

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.06.2023 05:34, Penny Zheng wrote:
> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -27,6 +27,7 @@ config X86
>       select HAS_PDX
>       select HAS_SCHED_GRANULARITY
>       select HAS_UBSAN
> +     select HAS_VMAP

With this being unconditional, ...

> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1750,12 +1750,14 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>          end_boot_allocator();
>  
>      system_state = SYS_STATE_boot;
> +#ifdef CONFIG_HAS_VMAP
>      /*
>       * No calls involving ACPI code should go between the setting of
>       * SYS_STATE_boot and vm_init() (or else acpi_os_{,un}map_memory()
>       * will break).
>       */
>      vm_init();
> +#endif

... there's no need to make this code less readable by adding #ifdef.
Even for the Arm side I question the #ifdef-ary - it likely would be
better to have an empty stub in the header for that case.

> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -15,6 +15,7 @@ config CORE_PARKING
>  config GRANT_TABLE
>       bool "Grant table support" if EXPERT
>       default y
> +     depends on HAS_VMAP

Looking back at the commit which added use of vmap.h there, I can't
seem to be bale to spot why it did. Where's the dependency there?
And even if there is one, I think you don't want to unconditionally
turn off a core, guest-visible feature. Instead you would want to
identify a way to continue to support the feature in perhaps
slightly less efficient a way.

> --- a/xen/common/vmap.c
> +++ b/xen/common/vmap.c
> @@ -331,4 +331,11 @@ void vfree(void *va)
>      while ( (pg = page_list_remove_head(&pg_list)) != NULL )
>          free_domheap_page(pg);
>  }
> +
> +void iounmap(void __iomem *va)
> +{
> +    unsigned long addr = (unsigned long)(void __force *)va;
> +
> +    vunmap((void *)(addr & PAGE_MASK));
> +}

Why does this move here?

> --- a/xen/include/xen/vmap.h
> +++ b/xen/include/xen/vmap.h
> @@ -1,4 +1,4 @@
> -#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>
> @@ -25,17 +25,14 @@ void vfree(void *va);
>  
>  void __iomem *ioremap(paddr_t, size_t);
>  
> -static inline void iounmap(void __iomem *va)
> -{
> -    unsigned long addr = (unsigned long)(void __force *)va;
> -
> -    vunmap((void *)(addr & PAGE_MASK));
> -}
> +void iounmap(void __iomem *va);
>  
>  void *arch_vmap_virt_end(void);
>  static inline void vm_init(void)
>  {
> +#if defined(VMAP_VIRT_START)
>      vm_init_type(VMAP_DEFAULT, (void *)VMAP_VIRT_START, 
> arch_vmap_virt_end());
> +#endif
>  }

Why the (seemingly unrelated) #ifdef-ary here? You've eliminated
the problematic caller already. Didn't you mean to add wider scope
#ifdef CONFIG_HAS_VMAP to this header?

Jan