diff mbox series

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

Message ID 20241115105036.218418-2-luca.fancellu@arm.com (mailing list archive)
State New
Headers show
Series Prerequisite patches for R82 upstreaming | expand

Commit Message

Luca Fancellu Nov. 15, 2024, 10:50 a.m. UTC
When HAS_VMAP is disabled, the xv{malloc,zalloc,...} functions
should fall back to the simple x{malloc,zalloc,...} variant,
implement that because MPU systems won't have virtual memory.

Additionally remove VMAP_VIRT_START from vmap.h guards since
MPU systems won't have it defined and move iounmap function
to the vmap.c file, because it uses the vunmap function that
won't be compiled in when !HAS_VMAP.

Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
This is a rework of this one: https://patchwork.kernel.org/project/xen-devel/patch/20230626033443.2943270-16-Penny.Zheng@arm.com/
where I hope I've understood correctly what Jan Beulich was suggesting here:
https://patchwork.kernel.org/project/xen-devel/patch/20230626033443.2943270-16-Penny.Zheng@arm.com/#25409119
---
 xen/common/vmap.c          |  7 +++++++
 xen/include/xen/vmap.h     |  9 ++-------
 xen/include/xen/xvmalloc.h | 36 +++++++++++++++++++++++++++++++-----
 3 files changed, 40 insertions(+), 12 deletions(-)

Comments

Jan Beulich Nov. 15, 2024, 11:12 a.m. UTC | #1
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
Andrew Cooper Nov. 15, 2024, noon UTC | #2
On 15/11/2024 10:50 am, Luca Fancellu wrote:
> diff --git a/xen/include/xen/xvmalloc.h b/xen/include/xen/xvmalloc.h
> index 440d85a284bb..802be6687085 100644
> --- 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)
> +
>  /* Free any of the above. */
>  void xvfree(void *va);
>  
> +/* Underlying functions */
> +void *_xvmalloc(size_t size, unsigned int align);
> +void *_xvzalloc(size_t size, unsigned int align);
> +void *_xvrealloc(void *va, size_t size, unsigned int align);
> +
> +#else
> +
> +static inline void xvfree(void *va)
> +{
> +    xfree(va);
> +}
> +
> +void *_xvmalloc(size_t size, unsigned int align)
> +{
> +    return _xmalloc(size, align);
> +}
> +
> +void *_xvzalloc(size_t size, unsigned int align)
> +{
> +    return _xzalloc(size, align);
> +}
> +
> +void *_xvrealloc(void *va, size_t size, unsigned int align)
> +{
> +    return _xrealloc(va, size, align);
> +}
> +
> +#endif

Does this really compile with the wrappers not being static inline ?

That aside, could we not do this using conditional aliases, rather than
wrapping the functions?  It would certainly be shorter, code wise.

~Andrew
Luca Fancellu Nov. 15, 2024, 12:12 p.m. UTC | #3
> On 15 Nov 2024, at 12:00, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> 
> On 15/11/2024 10:50 am, Luca Fancellu wrote:
>> diff --git a/xen/include/xen/xvmalloc.h b/xen/include/xen/xvmalloc.h
>> index 440d85a284bb..802be6687085 100644
>> --- 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)
>> +
>> /* Free any of the above. */
>> void xvfree(void *va);
>> 
>> +/* Underlying functions */
>> +void *_xvmalloc(size_t size, unsigned int align);
>> +void *_xvzalloc(size_t size, unsigned int align);
>> +void *_xvrealloc(void *va, size_t size, unsigned int align);
>> +
>> +#else
>> +
>> +static inline void xvfree(void *va)
>> +{
>> +    xfree(va);
>> +}
>> +
>> +void *_xvmalloc(size_t size, unsigned int align)
>> +{
>> +    return _xmalloc(size, align);
>> +}
>> +
>> +void *_xvzalloc(size_t size, unsigned int align)
>> +{
>> +    return _xzalloc(size, align);
>> +}
>> +
>> +void *_xvrealloc(void *va, size_t size, unsigned int align)
>> +{
>> +    return _xrealloc(va, size, align);
>> +}
>> +
>> +#endif
> 
> Does this really compile with the wrappers not being static inline ?

wow, yes it is compiling and I’m surprised about that, this was clearly a mistake, I’ll fix

> 
> That aside, could we not do this using conditional aliases, rather than
> wrapping the functions?  It would certainly be shorter, code wise.

I’ll ping you to understand better what you mean here


> 
> ~Andrew
Luca Fancellu Nov. 15, 2024, 12:15 p.m. UTC | #4
> On 15 Nov 2024, at 12:00, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> 
> On 15/11/2024 10:50 am, Luca Fancellu wrote:
>> diff --git a/xen/include/xen/xvmalloc.h b/xen/include/xen/xvmalloc.h
>> index 440d85a284bb..802be6687085 100644
>> --- 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)
>> +
>> /* Free any of the above. */
>> void xvfree(void *va);
>> 
>> +/* Underlying functions */
>> +void *_xvmalloc(size_t size, unsigned int align);
>> +void *_xvzalloc(size_t size, unsigned int align);
>> +void *_xvrealloc(void *va, size_t size, unsigned int align);
>> +
>> +#else
>> +
>> +static inline void xvfree(void *va)
>> +{
>> +    xfree(va);
>> +}
>> +
>> +void *_xvmalloc(size_t size, unsigned int align)
>> +{
>> +    return _xmalloc(size, align);
>> +}
>> +
>> +void *_xvzalloc(size_t size, unsigned int align)
>> +{
>> +    return _xzalloc(size, align);
>> +}
>> +
>> +void *_xvrealloc(void *va, size_t size, unsigned int align)
>> +{
>> +    return _xrealloc(va, size, align);
>> +}
>> +
>> +#endif
> 
> Does this really compile with the wrappers not being static inline ?
> 
> That aside, could we not do this using conditional aliases, rather than
> wrapping the functions?  It would certainly be shorter, code wise.

Do you mean something like below?

#define xvfree xfree
#define _xvmalloc _xmalloc
[…]

> 
> ~Andrew
Andrew Cooper Nov. 15, 2024, 12:27 p.m. UTC | #5
On 15/11/2024 12:15 pm, Luca Fancellu wrote:
>
>> On 15 Nov 2024, at 12:00, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>
>> On 15/11/2024 10:50 am, Luca Fancellu wrote:
>>> diff --git a/xen/include/xen/xvmalloc.h b/xen/include/xen/xvmalloc.h
>>> index 440d85a284bb..802be6687085 100644
>>> --- 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)
>>> +
>>> /* Free any of the above. */
>>> void xvfree(void *va);
>>>
>>> +/* Underlying functions */
>>> +void *_xvmalloc(size_t size, unsigned int align);
>>> +void *_xvzalloc(size_t size, unsigned int align);
>>> +void *_xvrealloc(void *va, size_t size, unsigned int align);
>>> +
>>> +#else
>>> +
>>> +static inline void xvfree(void *va)
>>> +{
>>> +    xfree(va);
>>> +}
>>> +
>>> +void *_xvmalloc(size_t size, unsigned int align)
>>> +{
>>> +    return _xmalloc(size, align);
>>> +}
>>> +
>>> +void *_xvzalloc(size_t size, unsigned int align)
>>> +{
>>> +    return _xzalloc(size, align);
>>> +}
>>> +
>>> +void *_xvrealloc(void *va, size_t size, unsigned int align)
>>> +{
>>> +    return _xrealloc(va, size, align);
>>> +}
>>> +
>>> +#endif
>> Does this really compile with the wrappers not being static inline ?
>>
>> That aside, could we not do this using conditional aliases, rather than
>> wrapping the functions?  It would certainly be shorter, code wise.
> Do you mean something like below?
>
> #define xvfree xfree
> #define _xvmalloc _xmalloc
> […]

I mean __attribute__((__alias__("")))

There are two examples in tree already.  See efi_compat_get_info() being
aliased to efi_get_info()

In this case, in the !HAS_VMAP case, we'd just declare _xmalloc() to
have an alias called _xvmalloc() too.

This avoids needing to wrap every function in the headers.

~Andrew
Luca Fancellu Nov. 15, 2024, 12:46 p.m. UTC | #6
> On 15 Nov 2024, at 11:12, Jan Beulich <jbeulich@suse.com> wrote:
> 
> 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().

I’ll try to keep it as it was originally, I gave a brief look into the R82 branch and it should be fine.
I’m planning to define vmap_config(), vunmap(), ioremap(), iounmap() in a vmap-mpu.c under arch/arm/mpu

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

Sure, I’ll wrap what can’t be used in MPU case with HAS_VMAP, I would like to keep out:

void *vmap_contig(mfn_t mfn, unsigned int nr);

void vunmap(const void *va);

void __iomem *ioremap(paddr_t pa, size_t len);

static inline void iounmap(void __iomem *va)

static inline void vm_init(void)

In order to don’t put too many #ifdef, are you ok if I move the declarations in order to have these close to each other. like below:

diff --git a/xen/include/xen/vmap.h b/xen/include/xen/vmap.h
index c1dd7ac22f30..940b7655ed8f 100644
--- a/xen/include/xen/vmap.h
+++ b/xen/include/xen/vmap.h
@@ -11,6 +11,8 @@
 #include <xen/mm-frame.h>
 #include <xen/page-size.h>
 
+#ifdef CONFIG_HAS_VMAP
+
 /* Identifiers for the linear ranges tracked by vmap */
 enum vmap_region {
     /*
@@ -68,25 +70,6 @@ void *__vmap(const mfn_t *mfn, unsigned int granularity, unsigned int nr,
  */
 void *vmap(const mfn_t *mfn, unsigned int nr);
 
-/*
- * Maps physically contiguous pages onto the VMAP_DEFAULT vmap region
- *
- * @param mfn Base mfn of the physical region
- * @param nr  Number of mfns in the physical region
- * @return Pointer to the mapped area on success; NULL otherwise.
- */
-void *vmap_contig(mfn_t mfn, unsigned int nr);
-
-/*
- * Unmaps a range of virtually contiguous memory from one of the vmap regions
- *
- * The system remembers internally how wide the mapping is and unmaps it all.
- * It also can determine the vmap region type from the `va`.
- *
- * @param va Virtual base address of the range to unmap
- */
-void vunmap(const void *va);
-
 /*
  * Allocate `size` octets of possibly non-contiguous physical memory and map
  * them contiguously in the VMAP_DEFAULT vmap region
@@ -112,6 +95,33 @@ void *vzalloc(size_t size);
  */
 void vfree(void *va);
 
+/* Return the number of pages in the mapping starting at address 'va' */
+unsigned int vmap_size(const void *va);
+
+/* Pointer to 1 octet past the end of the VMAP_DEFAULT virtual area */
+void *arch_vmap_virt_end(void);
+
+#else /* !CONFIG_HAS_VMAP */
+
+/*
+ * Maps physically contiguous pages onto the VMAP_DEFAULT vmap region
+ *
+ * @param mfn Base mfn of the physical region
+ * @param nr  Number of mfns in the physical region
+ * @return Pointer to the mapped area on success; NULL otherwise.
+ */
+void *vmap_contig(mfn_t mfn, unsigned int nr);
+
+/*
+ * Unmaps a range of virtually contiguous memory from one of the vmap regions
+ *
+ * The system remembers internally how wide the mapping is and unmaps it all.
+ * It also can determine the vmap region type from the `va`.
+ *
+ * @param va Virtual base address of the range to unmap
+ */
+void vunmap(const void *va);
+
 /*
  * Analogous to vmap_contig(), but for IO memory
  *
@@ -124,9 +134,6 @@ void vfree(void *va);
  */
 void __iomem *ioremap(paddr_t pa, size_t len);
 
-/* Return the number of pages in the mapping starting at address 'va' */
-unsigned int vmap_size(const void *va);
-
 /* Analogous to vunmap(), but for IO memory mapped via ioremap() */
 static inline void iounmap(void __iomem *va)
 {
@@ -135,9 +142,6 @@ static inline void iounmap(void __iomem *va)
     vunmap((void *)(addr & PAGE_MASK));
 }
 
-/* Pointer to 1 octet past the end of the VMAP_DEFAULT virtual area */
-void *arch_vmap_virt_end(void);
-
 /* Initialises the VMAP_DEFAULT virtual range */
 static inline void vm_init(void)
 {
@@ -146,4 +150,6 @@ static inline void vm_init(void)
 #endif
 }
 
+#endif /* CONFIG_HAS_VMAP */
+
 #endif /* __XEN_VMAP_H__ */


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

sure I will

> 
> Jan
Jan Beulich Nov. 15, 2024, 1:18 p.m. UTC | #7
On 15.11.2024 13:46, Luca Fancellu wrote:
> 
> 
>> On 15 Nov 2024, at 11:12, Jan Beulich <jbeulich@suse.com> wrote:
>>
>> 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().
> 
> I’ll try to keep it as it was originally, I gave a brief look into the R82 branch and it should be fine.
> I’m planning to define vmap_config(), vunmap(), ioremap(), iounmap() in a vmap-mpu.c under arch/arm/mpu
> 
>>
>>> --- 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.
> 
> Sure, I’ll wrap what can’t be used in MPU case with HAS_VMAP, I would like to keep out:
> 
> void *vmap_contig(mfn_t mfn, unsigned int nr);
> 
> void vunmap(const void *va);
> 
> void __iomem *ioremap(paddr_t pa, size_t len);
> 
> static inline void iounmap(void __iomem *va)
> 
> static inline void vm_init(void)
> 
> In order to don’t put too many #ifdef, are you ok if I move the declarations in order to have these close to each other. like below:

Some re-arrangement ought to be fine, especially when the #ifdef is
accompanied by a comment. I can't see how there can be #else though.

Jan

> --- a/xen/include/xen/vmap.h
> +++ b/xen/include/xen/vmap.h
> @@ -11,6 +11,8 @@
>  #include <xen/mm-frame.h>
>  #include <xen/page-size.h>
>  
> +#ifdef CONFIG_HAS_VMAP
> +
>  /* Identifiers for the linear ranges tracked by vmap */
>  enum vmap_region {
>      /*
> @@ -68,25 +70,6 @@ void *__vmap(const mfn_t *mfn, unsigned int granularity, unsigned int nr,
>   */
>  void *vmap(const mfn_t *mfn, unsigned int nr);
>  
> -/*
> - * Maps physically contiguous pages onto the VMAP_DEFAULT vmap region
> - *
> - * @param mfn Base mfn of the physical region
> - * @param nr  Number of mfns in the physical region
> - * @return Pointer to the mapped area on success; NULL otherwise.
> - */
> -void *vmap_contig(mfn_t mfn, unsigned int nr);
> -
> -/*
> - * Unmaps a range of virtually contiguous memory from one of the vmap regions
> - *
> - * The system remembers internally how wide the mapping is and unmaps it all.
> - * It also can determine the vmap region type from the `va`.
> - *
> - * @param va Virtual base address of the range to unmap
> - */
> -void vunmap(const void *va);
> -
>  /*
>   * Allocate `size` octets of possibly non-contiguous physical memory and map
>   * them contiguously in the VMAP_DEFAULT vmap region
> @@ -112,6 +95,33 @@ void *vzalloc(size_t size);
>   */
>  void vfree(void *va);
>  
> +/* Return the number of pages in the mapping starting at address 'va' */
> +unsigned int vmap_size(const void *va);
> +
> +/* Pointer to 1 octet past the end of the VMAP_DEFAULT virtual area */
> +void *arch_vmap_virt_end(void);
> +
> +#else /* !CONFIG_HAS_VMAP */
> +
> +/*
> + * Maps physically contiguous pages onto the VMAP_DEFAULT vmap region
> + *
> + * @param mfn Base mfn of the physical region
> + * @param nr  Number of mfns in the physical region
> + * @return Pointer to the mapped area on success; NULL otherwise.
> + */
> +void *vmap_contig(mfn_t mfn, unsigned int nr);
> +
> +/*
> + * Unmaps a range of virtually contiguous memory from one of the vmap regions
> + *
> + * The system remembers internally how wide the mapping is and unmaps it all.
> + * It also can determine the vmap region type from the `va`.
> + *
> + * @param va Virtual base address of the range to unmap
> + */
> +void vunmap(const void *va);
> +
>  /*
>   * Analogous to vmap_contig(), but for IO memory
>   *
> @@ -124,9 +134,6 @@ void vfree(void *va);
>   */
>  void __iomem *ioremap(paddr_t pa, size_t len);
>  
> -/* Return the number of pages in the mapping starting at address 'va' */
> -unsigned int vmap_size(const void *va);
> -
>  /* Analogous to vunmap(), but for IO memory mapped via ioremap() */
>  static inline void iounmap(void __iomem *va)
>  {
> @@ -135,9 +142,6 @@ static inline void iounmap(void __iomem *va)
>      vunmap((void *)(addr & PAGE_MASK));
>  }
>  
> -/* Pointer to 1 octet past the end of the VMAP_DEFAULT virtual area */
> -void *arch_vmap_virt_end(void);
> -
>  /* Initialises the VMAP_DEFAULT virtual range */
>  static inline void vm_init(void)
>  {
> @@ -146,4 +150,6 @@ static inline void vm_init(void)
>  #endif
>  }
>  
> +#endif /* CONFIG_HAS_VMAP */
> +
>  #endif /* __XEN_VMAP_H__ */
Luca Fancellu Nov. 15, 2024, 2:15 p.m. UTC | #8
>> 
>> Sure, I’ll wrap what can’t be used in MPU case with HAS_VMAP, I would like to keep out:
>> 
>> void *vmap_contig(mfn_t mfn, unsigned int nr);
>> 
>> void vunmap(const void *va);
>> 
>> void __iomem *ioremap(paddr_t pa, size_t len);
>> 
>> static inline void iounmap(void __iomem *va)
>> 
>> static inline void vm_init(void)
>> 
>> In order to don’t put too many #ifdef, are you ok if I move the declarations in order to have these close to each other. like below:
> 
> Some re-arrangement ought to be fine, especially when the #ifdef is
> accompanied by a comment. I can't see how there can be #else though.

Yes right, clearly not tested, thanks I’ll do the changes

> 
> Jan
Luca Fancellu Nov. 15, 2024, 2:56 p.m. UTC | #9
Hi Andrew,

>>>> +
>>>> +void *_xvrealloc(void *va, size_t size, unsigned int align)
>>>> +{
>>>> +    return _xrealloc(va, size, align);
>>>> +}
>>>> +
>>>> +#endif
>>> Does this really compile with the wrappers not being static inline ?
>>> 
>>> That aside, could we not do this using conditional aliases, rather than
>>> wrapping the functions?  It would certainly be shorter, code wise.
>> Do you mean something like below?
>> 
>> #define xvfree xfree
>> #define _xvmalloc _xmalloc
>> […]
> 
> I mean __attribute__((__alias__("")))
> 
> There are two examples in tree already.  See efi_compat_get_info() being
> aliased to efi_get_info()
> 
> In this case, in the !HAS_VMAP case, we'd just declare _xmalloc() to
> have an alias called _xvmalloc() too.
> 
> This avoids needing to wrap every function in the headers.

I’m getting:

error: ‘xvfree’ aliased to undefined symbol ‘xfree’

looking into the documentation it says:

“It is an error if the alias target is not defined in the same translation unit as the alias."

So I think I can’t use this one here.

Should I continue to do in the other way? Or using #define?

> 
> ~Andrew
diff mbox series

Patch

diff --git a/xen/common/vmap.c b/xen/common/vmap.c
index 47225fecc067..294280dcd08c 100644
--- 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));
+}
diff --git a/xen/include/xen/vmap.h b/xen/include/xen/vmap.h
index c1dd7ac22f30..9e1d794c2548 100644
--- 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__
 
 #include <xen/mm-frame.h>
@@ -128,12 +128,7 @@  void __iomem *ioremap(paddr_t pa, size_t len);
 unsigned int vmap_size(const void *va);
 
 /* Analogous to vunmap(), but for IO memory mapped via ioremap() */
-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);
 
 /* Pointer to 1 octet past the end of the VMAP_DEFAULT virtual area */
 void *arch_vmap_virt_end(void);
diff --git a/xen/include/xen/xvmalloc.h b/xen/include/xen/xvmalloc.h
index 440d85a284bb..802be6687085 100644
--- 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)
+
 /* Free any of the above. */
 void xvfree(void *va);
 
+/* Underlying functions */
+void *_xvmalloc(size_t size, unsigned int align);
+void *_xvzalloc(size_t size, unsigned int align);
+void *_xvrealloc(void *va, size_t size, unsigned int align);
+
+#else
+
+static inline void xvfree(void *va)
+{
+    xfree(va);
+}
+
+void *_xvmalloc(size_t size, unsigned int align)
+{
+    return _xmalloc(size, align);
+}
+
+void *_xvzalloc(size_t size, unsigned int align)
+{
+    return _xzalloc(size, align);
+}
+
+void *_xvrealloc(void *va, size_t size, unsigned int align)
+{
+    return _xrealloc(va, size, align);
+}
+
+#endif
+
 /* Free an allocation, and zero the pointer to it. */
 #define XVFREE(p) do { \
     xvfree(p);         \
     (p) = NULL;        \
 } while ( false )
 
-/* Underlying functions */
-void *_xvmalloc(size_t size, unsigned int align);
-void *_xvzalloc(size_t size, unsigned int align);
-void *_xvrealloc(void *va, size_t size, unsigned int align);
-
 static inline void *_xvmalloc_array(
     size_t size, unsigned int align, unsigned long num)
 {