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

[Luca Fancellu <luca.fancellu@xxxxxxx>]

> 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.

Hi Jan,

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.

> 
>> --- 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.

> 
>> @@ -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

> 
>> - */
>> -
>> -/*
>> - * 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

> 
>> @@ -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?

> 
>> @@ -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.

> 
>> @@ -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

> 
> Jan