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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.04.2021 17:37, Luca Fancellu wrote:
> --- 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

I continue to object to this giving the impression that grant tables
are something Arm64 specific.

> @@ -66,6 +67,7 @@
>   *     compiler barrier will still be required.
>   *
>   * Introducing a valid entry into the grant table:
> + * @code
>   *  1. Write ent->domid.
>   *  2. Write ent->frame:
>   *      GTF_permit_access:   Frame to which access is permitted.
> @@ -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).

Since neither in the cover letter nor in the description here I could
spot a link to the resulting generated doc, I wonder what the
inconsistencies above are about: You add @code/@endcode (and no blank
lines) to parts of what's being described, and _instead_ a blank line
to another part. I think the goal should be that not only the
generated doc ends up looking "nice", but that the source code also
remains consistent. Otherwise, someone like me coming across this
later on might easily conclude that there was a @code/@endcode pair
missed.

> @@ -97,18 +104,23 @@
>   *      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).
>   *

Why did this not also get enclosed by @code/@endcode?

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

And these two?

> + * @addtogroup grant_table Grant Tables

And no blank (comment) line ahead of this?

> @@ -120,24 +132,26 @@ typedef uint32_t grant_ref_t;
>   * [GST]: This field is written by the guest and read by Xen.
>   */
>  
> -/*
> - * Version 1 of the grant table entry structure is maintained purely
> - * for backwards compatibility.  New guests should use version 2.
> - */
>  #if __XEN_INTERFACE_VERSION__ < 0x0003020a
>  #define grant_entry_v1 grant_entry
>  #define grant_entry_v1_t grant_entry_t
>  #endif
> +/**
> + * Version 1 of the grant table entry structure is maintained purely
> + * for backwards compatibility.  New guests should use version 2.
> + */

In case I didn't say so already before - I think this would be a good
opportunity to drop the comment pointing at v2. With v2 optionally
unavailable altogether, this can't possibly be a generally valid
course of action.

> @@ -451,11 +465,6 @@ 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 {
>      /* IN parameters. */
>      struct gnttab_copy_ptr {
> @@ -471,6 +480,12 @@ struct gnttab_copy {
>      /* OUT parameters. */
>      int16_t       status;
>  };
> +
> +#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)

I guess I didn't express myself precisely enough: I think these
would better live _immediately_ next to the field that uses them.

Jan