In it's current form it's rather cryptic for uninitiated, so this patch
documents everything in there to make it more palatable.

No functional change.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
 xen/include/xen/vmap.h | 100 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 100 insertions(+)

diff --git a/xen/include/xen/vmap.h b/xen/include/xen/vmap.h
index 0c16baa85f31..2ab8030d6d00 100644
--- a/xen/include/xen/vmap.h
+++ b/xen/include/xen/vmap.h
@@ -1,34 +1,131 @@
+/*
+ * Interface to map physical memory onto contiguous virtual memory areas.
+ *
+ * Two ranges of linear address space are reserved for this purpose: A general
+ * 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)
 #define __XEN_VMAP_H__
 
 #include <xen/mm-frame.h>
 #include <xen/page-size.h>
 
+/** Identifiers for the linear ranges tracked by vmap */
 enum vmap_region {
+    /*
+     * Region used for general purpose RW mappings. Mapping/allocating memory
+     * here can induce extra allocations for the supporting page tables.
+     */
     VMAP_DEFAULT,
+    /**
+     * Region used for loading livepatches. Can't use VMAP_DEFAULT because it
+     * must live close to the running Xen image. The caller also ensures all
+     * page tables are already in place with adequate PTE flags.
+     */
     VMAP_XEN,
+    /** Sentinel value for bounds checking */
     VMAP_REGION_NR,
 };
 
+/**
+ * Runtime initialiser for each vmap region type
+ *
+ * Must only be called once per vmap region type.
+ *
+ * @param type  Designation of the region to initialise.
+ * @param start Start address of the `type` region.
+ * @param end   End address (not inclusive) of the `type` region
+ */
 void vm_init_type(enum vmap_region type, void *start, void *end);
 
+/**
+ * Maps a range of physical ranges onto a single virtual range
+ *
+ * `mfn` is an array of `nr` physical ranges, each of which is `granularity`
+ * pages wide. `type` defines which vmap region to use for the mapping and
+ * `flags` is the PTE flags the page table leaves are meant to have.
+ *
+ * Typically used via the vmap() and vmap_contig() helpers.
+ *
+ * @param mfn          Array of mfns
+ * @param granularity  Number of contiguous pages each mfn represents
+ * @param nr           Number of mfns in the `mfn` array
+ * @param align        Alignment of the virtual area to map
+ * @param flags        PTE flags for the leaves of the PT tree.
+ * @param type         Which region to create the mappings on
+ */
 void *__vmap(const mfn_t *mfn, unsigned int granularity, unsigned int nr,
              unsigned int align, unsigned int flags, enum vmap_region type);
+
+/**
+ * Map an arrray of pages contiguously into the VMAP_DEFAULT vmap region
+ *
+ * @param[in] mfn Pointer to the base of an array of mfns
+ * @param[in] nr  Number of mfns in the array
+ */
 void *vmap(const mfn_t *mfn, unsigned int nr);
+
+/**
+ * Maps physically contiguous pages onto the VMAP_DEFAULT vmap region
+ *
+ * Used when the directmap is unavailable (i.e: due to secret hiding)
+ *
+ * @param mfn Base mfn of the physical region
+ * @param nr  Number of mfns in the physical region
+ */
 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
+ *
+ * The system remembers internally how wide the mapping is and unmaps it all.
+ *
+ * @param size Pointer to the base of an array of mfns
+ * @return Pointer to the mapped area on success; NULL otherwise.
+ */
 void *vmalloc(size_t size);
+
+/** Same as vmalloc(), but for the VMAP_XEN vmap region. */
 void *vmalloc_xen(size_t size);
 
+/** Same as vmalloc(), but set the contents to zero before returning */
 void *vzalloc(size_t size);
+
+/**
+ * Unmap and free memory from vmalloc(), vmalloc_xen() or vzalloc()
+ *
+ * @param va Virtual base address of the range to free and unmap
+ */
 void vfree(void *va);
 
+/**
+ * Analogous to vmap_contig(), but for IO memory
+ *
+ * Unlike vmap_contig(), it ensures architecturally correct cacheability
+ * settings are set for the mapped IO memory.
+ *
+ * @param pa  Physical base address of the MMIO region.
+ * @param len Length of the MMIO region in octets.
+ * @return Pointer to the mapped area on success; NULL otherwise.
+ */
 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)
 {
     unsigned long addr = (unsigned long)(void __force *)va;
@@ -36,7 +133,10 @@ 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)
 {
     vm_init_type(VMAP_DEFAULT, (void *)VMAP_VIRT_START, arch_vmap_virt_end());
-- 
2.34.1

On 02.07.2024 15:03, Alejandro Vallejo wrote:
> --- a/xen/include/xen/vmap.h
> +++ b/xen/include/xen/vmap.h
> @@ -1,34 +1,131 @@
> +/*
> + * Interface to map physical memory onto contiguous virtual memory areas.
> + *
> + * Two ranges of linear address space are reserved for this purpose: A general
> + * 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)
>  #define __XEN_VMAP_H__
>  
>  #include <xen/mm-frame.h>
>  #include <xen/page-size.h>
>  
> +/** Identifiers for the linear ranges tracked by vmap */

Here and below: Why /** ? Seeing ...

>  enum vmap_region {
> +    /*
> +     * Region used for general purpose RW mappings. Mapping/allocating memory
> +     * here can induce extra allocations for the supporting page tables.
> +     */

... this vs ...

>      VMAP_DEFAULT,
> +    /**
> +     * Region used for loading livepatches. Can't use VMAP_DEFAULT because it
> +     * must live close to the running Xen image. The caller also ensures all
> +     * page tables are already in place with adequate PTE flags.
> +     */

... this, it's not even looking to be consistent.

>      VMAP_XEN,
> +    /** Sentinel value for bounds checking */
>      VMAP_REGION_NR,
>  };
>  
> +/**
> + * Runtime initialiser for each vmap region type
> + *
> + * Must only be called once per vmap region type.
> + *
> + * @param type  Designation of the region to initialise.
> + * @param start Start address of the `type` region.
> + * @param end   End address (not inclusive) of the `type` region
> + */
>  void vm_init_type(enum vmap_region type, void *start, void *end);
>  
> +/**
> + * Maps a range of physical ranges onto a single virtual range

Largely related to it not really being clear what "a range of physical ranges"
actually is, maybe "a set of physical ranges"?

> + * `mfn` is an array of `nr` physical ranges, each of which is `granularity`
> + * pages wide. `type` defines which vmap region to use for the mapping and
> + * `flags` is the PTE flags the page table leaves are meant to have.
> + *
> + * Typically used via the vmap() and vmap_contig() helpers.
> + *
> + * @param mfn          Array of mfns
> + * @param granularity  Number of contiguous pages each mfn represents
> + * @param nr           Number of mfns in the `mfn` array
> + * @param align        Alignment of the virtual area to map
> + * @param flags        PTE flags for the leaves of the PT tree.
> + * @param type         Which region to create the mappings on
> + */
>  void *__vmap(const mfn_t *mfn, unsigned int granularity, unsigned int nr,
>               unsigned int align, unsigned int flags, enum vmap_region type);
> +
> +/**
> + * Map an arrray of pages contiguously into the VMAP_DEFAULT vmap region

Nit: One r too many in "array".

> + * @param[in] mfn Pointer to the base of an array of mfns
> + * @param[in] nr  Number of mfns in the array
> + */
>  void *vmap(const mfn_t *mfn, unsigned int nr);
> +
> +/**
> + * Maps physically contiguous pages onto the VMAP_DEFAULT vmap region
> + *
> + * Used when the directmap is unavailable (i.e: due to secret hiding)

Please omit this. There's no reason to suggest it can't also be used for
other purposes.

> + * @param mfn Base mfn of the physical region
> + * @param nr  Number of mfns in the physical region
> + */
>  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
> + *
> + * The system remembers internally how wide the mapping is and unmaps it all.

This is a verbatim copy of what you say for vunmap(), yet it looks unrelated
here. Maybe you meant to edit it after copy-and-paste? Or maybe this was
meant to go ...

> + * @param size Pointer to the base of an array of mfns
> + * @return Pointer to the mapped area on success; NULL otherwise.
> + */
>  void *vmalloc(size_t size);
> +
> +/** Same as vmalloc(), but for the VMAP_XEN vmap region. */
>  void *vmalloc_xen(size_t size);
>  
> +/** Same as vmalloc(), but set the contents to zero before returning */
>  void *vzalloc(size_t size);
> +
> +/**
> + * Unmap and free memory from vmalloc(), vmalloc_xen() or vzalloc()
> + *
> + * @param va Virtual base address of the range to free and unmap
> + */
>  void vfree(void *va);

... here?

Also, if you want this to be considered for 4.19, please don't forget to Cc
Oleksii.

Jan

On Wed Jul 3, 2024 at 8:41 AM BST, Jan Beulich wrote:
> On 02.07.2024 15:03, Alejandro Vallejo wrote:
> > --- a/xen/include/xen/vmap.h
> > +++ b/xen/include/xen/vmap.h
> > @@ -1,34 +1,131 @@
> > +/*
> > + * Interface to map physical memory onto contiguous virtual memory areas.
> > + *
> > + * Two ranges of linear address space are reserved for this purpose: A general
> > + * 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)
> >  #define __XEN_VMAP_H__
> >  
> >  #include <xen/mm-frame.h>
> >  #include <xen/page-size.h>
> >  
> > +/** Identifiers for the linear ranges tracked by vmap */
>
> Here and below: Why /** ? Seeing ...
>
> >  enum vmap_region {
> > +    /*
> > +     * Region used for general purpose RW mappings. Mapping/allocating memory
> > +     * here can induce extra allocations for the supporting page tables.
> > +     */
>
> ... this vs ...
>
> >      VMAP_DEFAULT,
> > +    /**
> > +     * Region used for loading livepatches. Can't use VMAP_DEFAULT because it
> > +     * must live close to the running Xen image. The caller also ensures all
> > +     * page tables are already in place with adequate PTE flags.
> > +     */
>
> ... this, it's not even looking to be consistent.
>
> >      VMAP_XEN,
> > +    /** Sentinel value for bounds checking */
> >      VMAP_REGION_NR,
> >  };

Double stars are Doxygen style for documenting interfaces. I don't mind getting
rid of those. Some functions exhibit them in the arm port, but it's true
virtually nothing in common Xen does. VMAP_XEN's comment was meant to have it
too, I just missed it :)

> >  
> > +/**
> > + * Runtime initialiser for each vmap region type
> > + *
> > + * Must only be called once per vmap region type.
> > + *
> > + * @param type  Designation of the region to initialise.
> > + * @param start Start address of the `type` region.
> > + * @param end   End address (not inclusive) of the `type` region
> > + */
> >  void vm_init_type(enum vmap_region type, void *start, void *end);
> >  
> > +/**
> > + * Maps a range of physical ranges onto a single virtual range
>
> Largely related to it not really being clear what "a range of physical ranges"
> actually is, maybe "a set of physical ranges"?

Ack. I like your phrasing better.

>
> > + * `mfn` is an array of `nr` physical ranges, each of which is `granularity`
> > + * pages wide. `type` defines which vmap region to use for the mapping and
> > + * `flags` is the PTE flags the page table leaves are meant to have.
> > + *
> > + * Typically used via the vmap() and vmap_contig() helpers.
> > + *
> > + * @param mfn          Array of mfns
> > + * @param granularity  Number of contiguous pages each mfn represents
> > + * @param nr           Number of mfns in the `mfn` array
> > + * @param align        Alignment of the virtual area to map
> > + * @param flags        PTE flags for the leaves of the PT tree.
> > + * @param type         Which region to create the mappings on
> > + */
> >  void *__vmap(const mfn_t *mfn, unsigned int granularity, unsigned int nr,
> >               unsigned int align, unsigned int flags, enum vmap_region type);
> > +
> > +/**
> > + * Map an arrray of pages contiguously into the VMAP_DEFAULT vmap region
>
> Nit: One r too many in "array".

Sure.

>
> > + * @param[in] mfn Pointer to the base of an array of mfns
> > + * @param[in] nr  Number of mfns in the array
> > + */
> >  void *vmap(const mfn_t *mfn, unsigned int nr);
> > +
> > +/**
> > + * Maps physically contiguous pages onto the VMAP_DEFAULT vmap region
> > + *
> > + * Used when the directmap is unavailable (i.e: due to secret hiding)
>
> Please omit this. There's no reason to suggest it can't also be used for
> other purposes.

Sure.

>
> > + * @param mfn Base mfn of the physical region
> > + * @param nr  Number of mfns in the physical region
> > + */
> >  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
> > + *
> > + * The system remembers internally how wide the mapping is and unmaps it all.
>
> This is a verbatim copy of what you say for vunmap(), yet it looks unrelated
> here. Maybe you meant to edit it after copy-and-paste? Or maybe this was
> meant to go ...

Yeah, that shouldn't be there and was meant to go...

>
> > + * @param size Pointer to the base of an array of mfns
> > + * @return Pointer to the mapped area on success; NULL otherwise.
> > + */
> >  void *vmalloc(size_t size);
> > +
> > +/** Same as vmalloc(), but for the VMAP_XEN vmap region. */
> >  void *vmalloc_xen(size_t size);
> >  
> > +/** Same as vmalloc(), but set the contents to zero before returning */
> >  void *vzalloc(size_t size);
> > +
> > +/**
> > + * Unmap and free memory from vmalloc(), vmalloc_xen() or vzalloc()
> > + *
> > + * @param va Virtual base address of the range to free and unmap
> > + */
> >  void vfree(void *va);
>
> ... here?

... there.

>
> Also, if you want this to be considered for 4.19, please don't forget to Cc
> Oleksii.
>
> Jan

Will do on v2.

Cheers,
Alejandro