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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> On 15 Nov 2024, at 11:12, Jan Beulich <jbeulich@xxxxxxxx> 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