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

[Stefano Stabellini <sstabellini@kernel.org>]

On Mon, 17 Aug 2020, Jan Beulich wrote:
> On 07.08.2020 23:51, Stefano Stabellini wrote:
> > On Fri, 7 Aug 2020, Jan Beulich wrote:
> >> On 07.08.2020 01:49, Stefano Stabellini wrote:
> >>> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
> >>>
> >>> Convert in-code comments to kernel-doc format wherever possible.
> >>>
> >>> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> >>> ---
> >>>  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.
> 
> Iirc the enum comments were added for documentation purposes. This to
> me means there are two options at this point:
> - retain them in a way that the new doc model consumes them,
> - drop them at the same time as adding the new doc comments.
> 
> Their (presumed) value is that they identify #define-s which supposed
> to be enum-like without actually being able to use enums in the public
> headers (with some exceptions).

I understand. Then, it doesn't look like we want to keep them in the code
without converting them to kernel-doc. We could either:

1) remove them as part of this series
2) convert them to kernel-doc in the top comment as shown above

I could do either, but my preference is 1) because I think it leads to
clearer docs.