Re: [PATCH for-4.19?] xen/vmap: Document the vmap header

[Jan Beulich <jbeulich@xxxxxxxx>]

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