Re: [PATCH v2 3/3] docs/doxygen: doxygen documentation for grant_table.h

[Luca Fancellu <luca.fancellu@xxxxxxx>]

> On 20 Apr 2021, at 10:14, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> On 20.04.2021 10:46, Luca Fancellu wrote:
>>> On 19 Apr 2021, at 12:05, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>> 
>>> On 19.04.2021 11:12, Luca Fancellu wrote:
>>>> Modification to include/public/grant_table.h:
>>>> 
>>>> 1) Add doxygen tags to:
>>>> - Create Grant tables section
>>>> - include variables in the generated documentation
>>>> 2) Add .rst file for grant table for Arm64
>>> 
>>> I'm missing some reasoning about at least some of the changes done
>>> to grant_table.h. Looking at this and the earlier patches I also
>>> couldn't spot any general outline of what is acceptable or even
>>> necessary in such a header to be understood by doxygen. Without
>>> this written down somewhere (or, if documented elsewhere, a
>>> pointer provided to that doc) I'm afraid things might get screwed
>>> up again later on.
>> 
>> Doxygen is a tool that generates documentation based on parsing the source 
>> code comments, it recognises some
>> commands in the comments and builds the documentation sticking to what you 
>> wrote.
>> 
>> Here the doxygen docs: https://www.doxygen.nl/manual/docblocks.html
>> 
>> Basically it doesn’t react to all comments, it parses only some well crafted 
>> comments as explained in its documentation.
> 
> Providing this link somewhere might be helpful. However, also seeing
> some of your further comments, it feels like to edit a public header
> enabled for doxygen one has to know fair parts of this documentation.
> While I'm certainly in favor of having a way to generate docs from
> the headers, I'm afraid I don't think such knowledge should become a
> prereq to e.g. adding a new sub-function of a hypercall. So far I was
> assuming that the formatting requirements would be quite limited, and
> that it would hence be possible to just glean them from existing code.
> But e.g. the "/**<" notation isn't going to be obvious to spot.

Yes I see, some knowledge of the documentation tool is required but I’m sure 
that if anyone is able to
put his/her hands on the xen code, he/she will be able to use some of the 
doxygen tag too, as you can
see from this patch the usage is easy and some less obvious thing can be 
understood looking the source
code and the corresponding generated doc page.


> 
>>>> --- a/docs/hypercall-interfaces/arm64.rst
>>>> +++ b/docs/hypercall-interfaces/arm64.rst
>>>> @@ -8,6 +8,7 @@ Starting points
>>>> .. toctree::
>>>>   :maxdepth: 2
>>>> 
>>>> +   arm64/grant_tables
>>>> 
>>>> 
>>>> Functions
>>>> diff --git a/docs/hypercall-interfaces/arm64/grant_tables.rst 
>>>> b/docs/hypercall-interfaces/arm64/grant_tables.rst
>>>> new file mode 100644
>>>> index 0000000000..8955ec5812
>>>> --- /dev/null
>>>> +++ b/docs/hypercall-interfaces/arm64/grant_tables.rst
>>>> @@ -0,0 +1,8 @@
>>>> +.. SPDX-License-Identifier: CC-BY-4.0
>>>> +
>>>> +Grant Tables
>>>> +============
>>>> +
>>>> +.. doxygengroup:: grant_table
>>> 
>>> Why is this Arm64-specific?
>> 
>> This particular one is Arm64 specific, but it doesn’t mean that grant tables 
>> are arm specific, it is only that for now I’m
>> Introducing a partial documentation in the arm side. Any other user can in 
>> the future add more documentation for
>> each platform.
> 
> I'm of the pretty strong opinion that common hypercalls should be
> documented as common, and hence not live in an arch-specific
> section.

Yes sure, I agree, but this is the first step to enable a kind of 
documentation, it’s a work in progress.
On a future serie this can be modify. For now I’ve tried to reproduce this 
page: https://xenbits.xen.org/docs/unstable/
In the section “Hypercall Interfaces"

> 
>>>> @@ -73,20 +75,25 @@
>>>> *                           frame, or zero if none.
>>>> *  3. Write memory barrier (WMB).
>>>> *  4. Write ent->flags, inc. valid type.
>>>> + * @endcode
>>>> *
>>>> * Invalidating an unused GTF_permit_access entry:
>>>> + * @code
>>>> *  1. flags = ent->flags.
>>>> *  2. Observe that !(flags & (GTF_reading|GTF_writing)).
>>>> *  3. Check result of SMP-safe CMPXCHG(&ent->flags, flags, 0).
>>>> *  NB. No need for WMB as reuse of entry is control-dependent on success of
>>>> *      step 3, and all architectures guarantee ordering of ctrl-dep writes.
>>>> + * @endcode
>>>> *
>>>> * Invalidating an in-use GTF_permit_access entry:
>>>> + *
>>>> *  This cannot be done directly. Request assistance from the domain 
>>>> controller
>>>> *  which can set a timeout on the use of a grant entry and take necessary
>>>> *  action. (NB. This is not yet implemented!).
>>>> *
>>>> * Invalidating an unused GTF_accept_transfer entry:
>>>> + * @code
>>>> *  1. flags = ent->flags.
>>>> *  2. Observe that !(flags & GTF_transfer_committed). [*]
>>>> *  3. Check result of SMP-safe CMPXCHG(&ent->flags, flags, 0).
>>>> @@ -97,47 +104,55 @@
>>>> *      transferred frame is written. It is safe for the guest to spin 
>>>> waiting
>>>> *      for this to occur (detect by observing GTF_transfer_completed in
>>>> *      ent->flags).
>>>> + * @endcode
>>>> *
>>>> * Invalidating a committed GTF_accept_transfer entry:
>>>> *  1. Wait for (ent->flags & GTF_transfer_completed).
>>>> *
>>>> * Changing a GTF_permit_access from writable to read-only:
>>>> + *
>>>> *  Use SMP-safe CMPXCHG to set GTF_readonly, while checking !GTF_writing.
>>>> *
>>>> * Changing a GTF_permit_access from read-only to writable:
>>>> + *
>>>> *  Use SMP-safe bit-setting instruction.
>>> 
>>> For example - are the blank lines you add necessary or merely nice
>>> to have in your personal opinion?
>> 
>> The blank lines makes the docs output more good looking
> 
> I'm inclined to suggest to split beautification from basic enabling.

I will take this into consideration

> 
>>>> - */
>>>> -
>>>> -/*
>>>> - * Reference to a grant entry in a specified domain's grant table.
>>>> - */
>>>> -typedef uint32_t grant_ref_t;
>>> 
>>> Why does this get moved ...
>>> 
>>>> -
>>>> -/*
>>>> + *
>>>> * A grant table comprises a packed array of grant entries in one or more
>>>> * page frames shared between Xen and a guest.
>>>> + *
>>>> * [XEN]: This field is written by Xen and read by the sharing guest.
>>>> + *
>>>> * [GST]: This field is written by the guest and read by Xen.
>>>> + *
>>>> + * @addtogroup grant_table Grant Tables
>>>> + * @{
>>>> */
>>>> 
>>>> -/*
>>>> - * Version 1 of the grant table entry structure is maintained purely
>>>> - * for backwards compatibility.  New guests should use version 2.
>>>> +/**
>>>> + * Reference to a grant entry in a specified domain's grant table.
>>>> */
>>>> +typedef uint32_t grant_ref_t;
>>> 
>>> ... here, past a comment unrelated to it?
>> 
>> The comment “Version 1 of the grant table entry […]” was moved on top of the 
>> struct grant_entry_v1, in this way
>> Doxygen associate the comment to that structure.
>> 
>> The comment “Reference to a grant entry in a specified domain's grant 
>> table.” Is moved on top of typedef uint32_t grant_ref_t
>> for the same reason above
> 
> But it's the other comment ("A grant table comprises ...") that I
> was asking about.

I thought it was part of the brief about grant tables, am I wrong? For that 
reason I moved it.

> 
>>>> @@ -243,23 +258,27 @@ union grant_entry_v2 {
>>>>     * In that case, the frame field has the same semantics as the
>>>>     * field of the same name in the V1 entry structure.
>>>>     */
>>>> +    /** @cond skip anonymous struct/union for doxygen */
>>>>    struct {
>>>>        grant_entry_header_t hdr;
>>>>        uint32_t pad0;
>>>>        uint64_t frame;
>>>>    } full_page;
>>>> +    /** @endcond */
>>>> 
>>>>    /*
>>>>     * If the grant type is GTF_grant_access and GTF_sub_page is set,
>>>>     * @domid is allowed to access bytes [@page_off,@page_off+@length)
>>>>     * in frame @frame.
>>>>     */
>>>> +    /** @cond skip anonymous struct/union for doxygen */
>>>>    struct {
>>>>        grant_entry_header_t hdr;
>>>>        uint16_t page_off;
>>>>        uint16_t length;
>>>>        uint64_t frame;
>>>>    } sub_page;
>>>> +    /** @endcond */
>>>> 
>>>>    /*
>>>>     * If the grant is GTF_transitive, @domid is allowed to use the
>>>> @@ -270,12 +289,14 @@ union grant_entry_v2 {
>>>>     * The current version of Xen does not allow transitive grants
>>>>     * to be mapped.
>>>>     */
>>>> +    /** @cond skip anonymous struct/union for doxygen */
>>>>    struct {
>>>>        grant_entry_header_t hdr;
>>>>        domid_t trans_domid;
>>>>        uint16_t pad0;
>>>>        grant_ref_t gref;
>>>>    } transitive;
>>>> +    /** @endcond */
>>> 
>>> While already better than the introduction of strange struct tags,
>>> I'm still not convinced we want this extra clutter (sorry). Plus -
>>> don't these additions mean the sub-structures then won't be
>>> represented in the generated doc, rendering it (partly) useless?
>> 
>> Are you suggesting to remove the structure from docs?
> 
> Just yet I'm not suggesting anything here - I was merely guessing at
> the effect of "@cond" and the associated "skip ..." to mean that this
> construct makes doxygen skip the enclosed section. If that's not the
> effect, then maybe the comment wants rewording? (And then - what does
> @cond mean?)

Yes you were right, in the documentation the structure grant_entry_v2 won’t 
display the
anonymous union.

> 
>>>> @@ -433,7 +454,12 @@ typedef struct gnttab_transfer gnttab_transfer_t;
>>>> DEFINE_XEN_GUEST_HANDLE(gnttab_transfer_t);
>>>> 
>>>> 
>>>> -/*
>>>> +#define _GNTCOPY_source_gref      (0)
>>>> +#define GNTCOPY_source_gref       (1<<_GNTCOPY_source_gref)
>>>> +#define _GNTCOPY_dest_gref        (1)
>>>> +#define GNTCOPY_dest_gref         (1<<_GNTCOPY_dest_gref)
>>>> +
>>>> +/**
>>>> * GNTTABOP_copy: Hypervisor based copy
>>>> * source and destinations can be eithers MFNs or, for foreign domains,
>>>> * grant references. the foreign domain has to grant read/write access
>>>> @@ -451,18 +477,15 @@ DEFINE_XEN_GUEST_HANDLE(gnttab_transfer_t);
>>>> * bytes to be copied.
>>>> */
>>>> 
>>>> -#define _GNTCOPY_source_gref      (0)
>>>> -#define GNTCOPY_source_gref       (1<<_GNTCOPY_source_gref)
>>>> -#define _GNTCOPY_dest_gref        (1)
>>>> -#define GNTCOPY_dest_gref         (1<<_GNTCOPY_dest_gref)
>>>> -
>>>> struct gnttab_copy {
>>> 
>>> Again the question - why the movement?
>> 
>> Doxygen takes the comment just above the data structure to build the docs, 
>> so here we are moving the
>> Comment just on top of the described structure.
> 
> Well, okay, this then is an explanation _that_ the #define-s want
> moving, but not an explanation for where they got moved (father
> away from what they actually relate to). Personally I consider it
> good practice to have such #define-s next to the field they relate
> to (and we have such placement elsewhere). Perhaps worth
> considering to move them down rather than up?

Sure they can be moved down without problem.

> 
> Jan
> 
>>>> @@ -579,17 +602,19 @@ struct gnttab_swap_grant_ref {
>>>> typedef struct gnttab_swap_grant_ref gnttab_swap_grant_ref_t;
>>>> DEFINE_XEN_GUEST_HANDLE(gnttab_swap_grant_ref_t);
>>>> 
>>>> -/*
>>>> +/**
>>>> * Issue one or more cache maintenance operations on a portion of a
>>>> * page granted to the calling domain by a foreign domain.
>>>> */
>>>> struct gnttab_cache_flush {
>>>> +    /** @cond skip anonymous struct/union for doxygen */
>>>>    union {
>>>>        uint64_t dev_bus_addr;
>>>>        grant_ref_t ref;
>>>>    } a;
>>>> -    uint16_t offset; /* offset from start of grant */
>>>> -    uint16_t length; /* size within the grant */
>>>> +    /** @endcond */
>>>> +    uint16_t offset; /**< offset from start of grant */
>>>> +    uint16_t length; /**< size within the grant */
>>> 
>>> Skipping just part of a struct is perhaps even more confusing than
>>> omitting it altogether.
>>> 
>>> Also, what's the significance of "/**<" ?
>> 
>> It is a doxygen pattern that tells it to use the comment as a field related 
>> documentation.
>> If you build the documentation you will find the result, I encourage you to 
>> see it to
>> realize the power of the tool and the benefits that Xen can get with it.
>> 
>> Cheers,
>> Luca