VMAP is widely used in ALTERNATIVE feature, CPUERRATA feature, Grant
Table feature, LIVEPATCH feature etc, to remap a range of memory with new
memory attributes. Since this is highly dependent on virtual address
translation, we choose to fold VMAP in MMU system.

In this patch, we introduce a new Kconfig CONFIG_HAS_VMAP, and make it only
support in MMU system on ARM architecture. And we make features like
ALTERNATIVE, CPUERRATA, LIVEPATCH, Grant Table, etc, now depend on VMAP.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
Signed-off-by: Wei Chen <wei.chen@arm.com>
---
v2:
- new commit
---
v3:
- make LIVEPATCH/ALTERNATIVE/CPUERRATA/Grant Table/LIVEPATCH depend on HAS_VMAP
- function call should be wrapped in context, then we could remove inline stubs
---
 xen/arch/arm/Kconfig   |  3 ++-
 xen/arch/arm/Makefile  |  2 +-
 xen/arch/arm/setup.c   |  7 +++++++
 xen/arch/arm/smpboot.c |  2 ++
 xen/arch/x86/Kconfig   |  1 +
 xen/arch/x86/setup.c   |  2 ++
 xen/common/Kconfig     |  5 +++++
 xen/common/Makefile    |  2 +-
 xen/common/vmap.c      |  7 +++++++
 xen/include/xen/vmap.h | 11 ++++-------
 10 files changed, 32 insertions(+), 10 deletions(-)

diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index 22b28b8ba2..a88500fb50 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -11,7 +11,7 @@ config ARM_64
 
 config ARM
 	def_bool y
-	select HAS_ALTERNATIVE
+	select HAS_ALTERNATIVE if HAS_VMAP
 	select HAS_DEVICE_TREE
 	select HAS_PASSTHROUGH
 	select HAS_PDX
@@ -63,6 +63,7 @@ config HAS_MMU
 	bool "Memory Management Unit support in a VMSA system"
 	default y
 	select HAS_PMAP
+	select HAS_VMAP
 	help
 	  In a VMSA system, a Memory Management Unit (MMU) provides fine-grained control of
 	  a memory system through a set of virtual to physical address mappings and associated memory
diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index c1babdba6a..d01528cac6 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -10,7 +10,7 @@ obj-$(CONFIG_HAS_VPCI) += vpci.o
 
 obj-$(CONFIG_HAS_ALTERNATIVE) += alternative.o
 obj-y += bootfdt.init.o
-obj-y += cpuerrata.o
+obj-$(CONFIG_HAS_VMAP) += cpuerrata.o
 obj-y += cpufeature.o
 obj-y += decode.o
 obj-y += device.o
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 50259552a0..34923d9984 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -812,7 +812,9 @@ void __init start_xen(unsigned long boot_phys_offset,
      */
     system_state = SYS_STATE_boot;
 
+#ifdef CONFIG_HAS_VMAP
     vm_init();
+#endif
 
     if ( acpi_disabled )
     {
@@ -844,11 +846,13 @@ void __init start_xen(unsigned long boot_phys_offset,
     nr_cpu_ids = smp_get_max_cpus();
     printk(XENLOG_INFO "SMP: Allowing %u CPUs\n", nr_cpu_ids);
 
+#ifdef CONFIG_HAS_VMAP
     /*
      * Some errata relies on SMCCC version which is detected by psci_init()
      * (called from smp_init_cpus()).
      */
     check_local_cpu_errata();
+#endif
 
     check_local_cpu_features();
 
@@ -915,12 +919,15 @@ void __init start_xen(unsigned long boot_phys_offset,
 
     do_initcalls();
 
+
+#ifdef CONFIG_HAS_VMAP
     /*
      * It needs to be called after do_initcalls to be able to use
      * stop_machine (tasklets initialized via an initcall).
      */
     apply_alternatives_all();
     enable_errata_workarounds();
+#endif
     enable_cpu_features();
 
     /* Create initial domain 0. */
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index 8bcdbea66c..0796e534ec 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -388,7 +388,9 @@ void start_secondary(void)
 
     local_abort_enable();
 
+#ifdef CONFIG_HAS_VMAP
     check_local_cpu_errata();
+#endif
     check_local_cpu_features();
 
     printk(XENLOG_DEBUG "CPU %u booted.\n", smp_processor_id());
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 406445a358..033cc2332e 100644
--- 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
 	select HAS_VPCI if HVM
 	select NEEDS_LIBELF
 
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 74e3915a4d..9f06879225 100644
--- 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
 
     bsp_stack = cpu_alloc_stack(0);
     if ( !bsp_stack )
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 3d2123a783..2c29e89b75 100644
--- 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
 	---help---
 	  Grant table provides a generic mechanism to memory sharing
 	  between domains. This shared memory interface underpins the
@@ -65,6 +66,9 @@ config HAS_SCHED_GRANULARITY
 config HAS_UBSAN
 	bool
 
+config HAS_VMAP
+	bool
+
 config MEM_ACCESS_ALWAYS_ON
 	bool
 
@@ -367,6 +371,7 @@ config LIVEPATCH
 	bool "Live patching support"
 	default X86
 	depends on "$(XEN_HAS_BUILD_ID)" = "y"
+	depends on HAS_VMAP
 	select CC_SPLIT_SECTIONS
 	---help---
 	  Allows a running Xen hypervisor to be dynamically patched using
diff --git a/xen/common/Makefile b/xen/common/Makefile
index 46049eac35..4803282d62 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -51,7 +51,7 @@ obj-$(CONFIG_TRACEBUFFER) += trace.o
 obj-y += version.o
 obj-y += virtual_region.o
 obj-y += vm_event.o
-obj-y += vmap.o
+obj-$(CONFIG_HAS_VMAP) += vmap.o
 obj-y += vsprintf.o
 obj-y += wait.o
 obj-bin-y += warning.init.o
diff --git a/xen/common/vmap.c b/xen/common/vmap.c
index 4fd6b3067e..51e13e17ed 100644
--- 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));
+}
 #endif
diff --git a/xen/include/xen/vmap.h b/xen/include/xen/vmap.h
index b0f7632e89..d7ef4df452 100644
--- 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
 }
 
 #endif /* __XEN_VMAP_H__ */
-- 
2.25.1

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

Hi Jan

On 2023/6/26 14:00, Jan Beulich wrote:
> 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.
> 

I may misunderstand what you said in last serie and thought #ifdef-ary 
is preferred compared with inline stubs, referring here to recall
'''
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.
'''
So maybe the preferring order is "declaration > empty inline stubs > 
#ifdef-ary" ? As having a declaration is not enough for vm_init()(when
!HAS_VMAP), we provide static inline empty stub here.

>> --- 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?

vzalloc() is used in grant_table_init() in xen/common/grantable.c:2023

> 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.
> 

We have discussed in MPU design, normally, xen guest in MPU system
will be a linux guest with no pv(with CONFIG_XEN=n), so advanced 
features like grant table is in no use there.

>> --- 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?

ioremap/iounmap is using vmap, at least in ARM. So for this more
generic interface, I was intending to implement it on MPU system.
In commit "[PATCH v3 19/52] xen/arm: switch to use ioremap_xxx in common 
file", I'm trying to replace all direct vmap interface with ioremap_xxx 
in common files.
Then I, in commit "[PATCH v3 36/52] xen/mpu: implememt ioremap_xxx in 
MPU", will implement MPU version of ioremap_xxx.

> 
>> --- 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?
> 

plz correct me if I'm wrong, in MPU system with !HAS_VMAP, if we don't
#ifdef-ary here, we will have the following compilation error, I guess
it's because that vm_init() is a static inline stub:
```
./include/xen/vmap.h: In function ‘vm_init’:
./include/xen/vmap.h:33:40: error: ‘VMAP_VIRT_START’ undeclared (first 
use in this function); did you mean ‘XEN_VIRT_START’?
    33 |     vm_init_type(VMAP_DEFAULT, (void *)VMAP_VIRT_START, 
arch_vmap_virt_end());
       |                                        ^~~~~~~~~~~~~~~
       |                                        XEN_VIRT_START
```

> Jan
> 

On 28.06.2023 07:38, Penny Zheng wrote:
> On 2023/6/26 14:00, Jan Beulich wrote:
>> 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.
>>
> 
> I may misunderstand what you said in last serie and thought #ifdef-ary 
> is preferred compared with inline stubs, referring here to recall
> '''
> 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.
> '''
> So maybe the preferring order is "declaration > empty inline stubs > 
> #ifdef-ary" ?

Indeed.

> As having a declaration is not enough for vm_init()(when
> !HAS_VMAP), we provide static inline empty stub here.

Really you change the existing vm_init() to an empty stub already; no
need to have a 2nd one (if that was a consideration).

>>> --- 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?
> 
> vzalloc() is used in grant_table_init() in xen/common/grantable.c:2023

You'll need vzalloc() to fall back to xzalloc() anyway when !HAS_VMAP,
I suppose.

>> 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.
> 
> We have discussed in MPU design, normally, xen guest in MPU system
> will be a linux guest with no pv(with CONFIG_XEN=n), so advanced 
> features like grant table is in no use there.

Is this design decision written down anywhere? It looks pretty limiting
to me ...

>>> --- 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?
> 
> ioremap/iounmap is using vmap, at least in ARM. So for this more
> generic interface, I was intending to implement it on MPU system.
> In commit "[PATCH v3 19/52] xen/arm: switch to use ioremap_xxx in common 
> file", I'm trying to replace all direct vmap interface with ioremap_xxx 
> in common files.
> Then I, in commit "[PATCH v3 36/52] xen/mpu: implememt ioremap_xxx in 
> MPU", will implement MPU version of ioremap_xxx.

Which suggests that the movement _may_ be needed, but doing it here is
likely premature.

>>> --- 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?
>>
> 
> plz correct me if I'm wrong, in MPU system with !HAS_VMAP, if we don't
> #ifdef-ary here, we will have the following compilation error, I guess
> it's because that vm_init() is a static inline stub:
> ```
> ./include/xen/vmap.h: In function ‘vm_init’:
> ./include/xen/vmap.h:33:40: error: ‘VMAP_VIRT_START’ undeclared (first 
> use in this function); did you mean ‘XEN_VIRT_START’?
>     33 |     vm_init_type(VMAP_DEFAULT, (void *)VMAP_VIRT_START, 
> arch_vmap_virt_end());
>        |                                        ^~~~~~~~~~~~~~~
>        |                                        XEN_VIRT_START
> ```

Perhaps, but that doesn't mean the #ifdef-ary here is the only
solution. I'm afraid the approach so far doesn't really make clear
in what overall direction you want to move. Possibly it may help
if you split the patch ...

Jan

Hi,

On 28/06/2023 06:38, Penny Zheng wrote:
> On 2023/6/26 14:00, Jan Beulich wrote:
>> On 26.06.2023 05:34, Penny Zheng wrote:
>>> --- 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?
> 
> ioremap/iounmap is using vmap, at least in ARM. So for this more
> generic interface, I was intending to implement it on MPU system.
> In commit "[PATCH v3 19/52] xen/arm: switch to use ioremap_xxx in common 
> file", I'm trying to replace all direct vmap interface with ioremap_xxx 
> in common files.

While the implementation of ioremap() is based on vmap(), the intended 
usage is not the same. ioremap() is for MMIO regions while vmap() is for 
RAM.

So I don't think this is correct to replace vmap() with ioremap().

Cheers,
-- 
Julien Grall

Hi Julien

On 2023/6/28 15:59, Julien Grall wrote:
> Hi,
> 
> On 28/06/2023 06:38, Penny Zheng wrote:
>> On 2023/6/26 14:00, Jan Beulich wrote:
>>> On 26.06.2023 05:34, Penny Zheng wrote:
>>>> --- 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?
>>
>> ioremap/iounmap is using vmap, at least in ARM. So for this more
>> generic interface, I was intending to implement it on MPU system.
>> In commit "[PATCH v3 19/52] xen/arm: switch to use ioremap_xxx in 
>> common file", I'm trying to replace all direct vmap interface with 
>> ioremap_xxx in common files.
> 
> While the implementation of ioremap() is based on vmap(), the intended 
> usage is not the same. ioremap() is for MMIO regions while vmap() is for 
> RAM.
> 
> So I don't think this is correct to replace vmap() with ioremap().
> 

Sure, understood now.
So then the current usage of ioremap_xxx in xen/arch/arm/kernel.c, like
```
kernel_zimage_load()
...
kernel = ioremap_wc(paddr, len);
...
```
ioremap_wc() is used for remapping and then copying guest kernel image 
to guest memory. Since ioremap_xxx is for MMIO regions, maybe we shall 
provide a new pair of interfaces for RAM, like ramremap_xxx, to replace 
the ioremap_xxx here?

> Cheers,

Hi,

On 28/06/2023 09:41, Penny Zheng wrote:
> On 2023/6/28 15:59, Julien Grall wrote:
>> Hi,
>>
>> On 28/06/2023 06:38, Penny Zheng wrote:
>>> On 2023/6/26 14:00, Jan Beulich wrote:
>>>> On 26.06.2023 05:34, Penny Zheng wrote:
>>>>> --- 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?
>>>
>>> ioremap/iounmap is using vmap, at least in ARM. So for this more
>>> generic interface, I was intending to implement it on MPU system.
>>> In commit "[PATCH v3 19/52] xen/arm: switch to use ioremap_xxx in 
>>> common file", I'm trying to replace all direct vmap interface with 
>>> ioremap_xxx in common files.
>>
>> While the implementation of ioremap() is based on vmap(), the intended 
>> usage is not the same. ioremap() is for MMIO regions while vmap() is 
>> for RAM.
>>
>> So I don't think this is correct to replace vmap() with ioremap().
>>
> 
> Sure, understood now.
> So then the current usage of ioremap_xxx in xen/arch/arm/kernel.c, like
> ```
> kernel_zimage_load()
> ...
> kernel = ioremap_wc(paddr, len);
> ...
> ```
> ioremap_wc() is used for remapping and then copying guest kernel image 
> to guest memory. Since ioremap_xxx is for MMIO regions, maybe we shall 
> provide a new pair of interfaces for RAM, like ramremap_xxx, to replace 
> the ioremap_xxx here?

I always wondered why we were using ioremap_wc() there. But I have never 
made up my mind for this one. We would need to look at the history to 
understand why it was used.

Anyway, if we want to replace it, then I think this would be a plain 
vmap() rather inventing a new API.

Cheers,

-- 
Julien Grall