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
