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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.08.2020 00:56, Stefano Stabellini wrote:
> 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:
>>>>> @@ -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.

While I'd slightly prefer 2, I'll be okay with your choice.

Jan