Re: [PATCH 08/14] kernel-doc: public/memory.h

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 7 Aug 2020, Jan Beulich wrote:
> On 07.08.2020 01:49, Stefano Stabellini wrote:
> > From: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
> > 
> > Convert in-code comments to kernel-doc format wherever possible.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
> > ---
> >  xen/include/public/memory.h | 232 ++++++++++++++++++++++++------------
> >  1 file changed, 155 insertions(+), 77 deletions(-)
> > 
> > diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
> > index 21057ed78e..4c57ed213c 100644
> > --- a/xen/include/public/memory.h
> > +++ b/xen/include/public/memory.h
> > @@ -30,7 +30,9 @@
> >  #include "xen.h"
> >  #include "physdev.h"
> >  
> > -/*
> > +/**
> > + * DOC: XENMEM_increase_reservation and XENMEM_decrease_reservation
> > + *
> >   * Increase or decrease the specified domain's memory reservation. Returns 
> > the
> >   * number of extents successfully allocated or freed.
> >   * arg == addr of struct xen_memory_reservation.
> > @@ -40,29 +42,37 @@
> >  #define XENMEM_populate_physmap     6
> >  
> >  #if __XEN_INTERFACE_VERSION__ >= 0x00030209
> > -/*
> > - * Maximum # bits addressable by the user of the allocated region (e.g., 
> > I/O
> > - * devices often have a 32-bit limitation even in 64-bit systems). If zero
> > - * then the user has no addressing restriction. This field is not used by
> > - * XENMEM_decrease_reservation.
> > +/**
> > + * DOC: XENMEMF_*
> > + *
> > + * - XENMEMF_address_bits, XENMEMF_get_address_bits:
> > + *       Maximum # bits addressable by the user of the allocated region
> > + *       (e.g., I/O devices often have a 32-bit limitation even in 64-bit
> > + *       systems). If zero then the user has no addressing restriction. 
> > This
> > + *       field is not used by XENMEM_decrease_reservation.
> > + * - XENMEMF_node, XENMEMF_get_node: NUMA node to allocate from
> > + * - XENMEMF_populate_on_demand: Flag to populate physmap with 
> > populate-on-demand entries
> > + * - XENMEMF_exact_node_request, XENMEMF_exact_node: Flag to request 
> > allocation only from the node specified
> 
> Nit: overly long line

I'll fix


> > + * - XENMEMF_vnode: Flag to indicate the node specified is virtual node
> >   */
> >  #define XENMEMF_address_bits(x)     (x)
> >  #define XENMEMF_get_address_bits(x) ((x) & 0xffu)
> > -/* NUMA node to allocate from. */
> >  #define XENMEMF_node(x)     (((x) + 1) << 8)
> >  #define XENMEMF_get_node(x) ((((x) >> 8) - 1) & 0xffu)
> > -/* Flag to populate physmap with populate-on-demand entries */
> >  #define XENMEMF_populate_on_demand (1<<16)
> > -/* Flag to request allocation only from the node specified */
> >  #define XENMEMF_exact_node_request  (1<<17)
> >  #define XENMEMF_exact_node(n) (XENMEMF_node(n) | 
> > XENMEMF_exact_node_request)
> > -/* Flag to indicate the node specified is virtual node */
> >  #define XENMEMF_vnode  (1<<18)
> >  #endif
> >  
> > +/**
> > + * struct xen_memory_reservation
> > + */
> >  struct xen_memory_reservation {
> >  
> > -    /*
> > +    /**
> > +     * @extent_start:
> > +     *
> 
> Take the opportunity and drop the stray blank line?
 
Sure


> > @@ -200,90 +236,115 @@ DEFINE_XEN_GUEST_HANDLE(xen_machphys_mfn_list_t);
> >   */
> >  #define XENMEM_machphys_compat_mfn_list     25
> >  
> > -/*
> > +#define XENMEM_machphys_mapping     12
> > +/**
> > + * struct xen_machphys_mapping - XENMEM_machphys_mapping
> > + *
> >   * Returns the location in virtual address space of the machine_to_phys
> >   * mapping table. Architectures which do not have a m2p table, or which do 
> > not
> >   * map it by default into guest address space, do not implement this 
> > command.
> >   * arg == addr of xen_machphys_mapping_t.
> >   */
> > -#define XENMEM_machphys_mapping     12
> >  struct xen_machphys_mapping {
> > +    /** @v_start: Start virtual address */
> >      xen_ulong_t v_start, v_end; /* Start and end virtual addresses.   */
> > -    xen_ulong_t max_mfn;        /* Maximum MFN that can be looked up. */
> > +    /** @v_end: End virtual addresses */
> > +    xen_ulong_t v_end;
> > +    /** @max_mfn: Maximum MFN that can be looked up */
> > +    xen_ulong_t max_mfn;
> >  };
> >  typedef struct xen_machphys_mapping xen_machphys_mapping_t;
> >  DEFINE_XEN_GUEST_HANDLE(xen_machphys_mapping_t);
> >  
> > -/* Source mapping space. */
> > +/**
> > + * DOC: Source mapping space.
> > + *
> > + * - XENMAPSPACE_shared_info:  shared info page
> > + * - XENMAPSPACE_grant_table:  grant table page
> > + * - XENMAPSPACE_gmfn:         GMFN
> > + * - XENMAPSPACE_gmfn_range:   GMFN range, XENMEM_add_to_physmap only.
> > + * - XENMAPSPACE_gmfn_foreign: GMFN from another dom,
> > + *                             XENMEM_add_to_physmap_batch only.
> > + * - XENMAPSPACE_dev_mmio:     device mmio region ARM only; the region is 
> > mapped
> > + *                             in Stage-2 using the Normal 
> > MemoryInner/Outer
> > + *                             Write-Back Cacheable memory attribute.
> > + */
> >  /* ` enum phys_map_space { */
> 
> Isn't this and ...
> 
> > -#define XENMAPSPACE_shared_info  0 /* shared info page */
> > -#define XENMAPSPACE_grant_table  1 /* grant table page */
> > -#define XENMAPSPACE_gmfn         2 /* GMFN */
> > -#define XENMAPSPACE_gmfn_range   3 /* GMFN range, XENMEM_add_to_physmap 
> > only. */
> > -#define XENMAPSPACE_gmfn_foreign 4 /* GMFN from another dom,
> > -                                    * XENMEM_add_to_physmap_batch only. */
> > -#define XENMAPSPACE_dev_mmio     5 /* device mmio region
> > -                                      ARM only; the region is mapped in
> > -                                      Stage-2 using the Normal Memory
> > -                                      Inner/Outer Write-Back Cacheable
> > -                                      memory attribute. */
> > +#define XENMAPSPACE_shared_info  0
> > +#define XENMAPSPACE_grant_table  1
> > +#define XENMAPSPACE_gmfn         2
> > +#define XENMAPSPACE_gmfn_range   3
> > +#define XENMAPSPACE_gmfn_foreign 4
> > +#define XENMAPSPACE_dev_mmio     5
> >  /* ` } */
> 
> ... this also something that wants converting?

For clarity, I take you are talking about these two enum-related
comments:

/* ` enum phys_map_space { */
[... various #defines ... ]
/* ` } */

Is this something we want to convert to kernel-doc? I don't know. I
couldn't see an obvious value in doing it, in the sense that it doesn't
necessarely make things clearer.

I took a second look at the header and the following would work:

/**
 * DOC: Source mapping space.
 *
 * enum phys_map_space {
 *
 * - XENMAPSPACE_shared_info:  shared info page
 * - XENMAPSPACE_grant_table:  grant table page
 * - XENMAPSPACE_gmfn:         GMFN
 * - XENMAPSPACE_gmfn_range:   GMFN range, XENMEM_add_to_physmap only.
 * - XENMAPSPACE_gmfn_foreign: GMFN from another dom,
 *                             XENMEM_add_to_physmap_batch only.
 * - XENMAPSPACE_dev_mmio:     device mmio region ARM only; the region is mapped
 *                             in Stage-2 using the Normal MemoryInner/Outer
 *                             Write-Back Cacheable memory attribute.
 * }
 */

Note the blank line after "enum phys_map_space {" is required.


All in all I am in favor of *not* converting the enum comment to
kernel-doc, but I'd be OK with it anyway.