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

[Luca Fancellu <luca.fancellu@xxxxxxx>]

> On 4 May 2021, at 23:27, Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:
> 
> On Tue, 4 May 2021, 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
>> - Used @keepindent/@endkeepindent to enclose comment
>>   section that are indented using spaces, to keep
>>   the indentation.
>> 2) Add .rst file for grant table for Arm64
> 
> Thanks Luca for your hard work on this. It is going to make things a lot
> better!
> 
> I reviewed this patch while looking at
> https://luca.fancellu.gitlab.io/xen-docs/hypercall-interfaces/arm64/grant_tables.html
> 
> In short, I think this changes look fine except for a trivial code style
> issue on the very last comment at the bottom of the patch.
> 
> All my questions below are basically about some other in-code comments,
> currently existing in the code, but not outputted in the html file.
> Is there an easy way to add them?
> 
> 
> 
>> Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
>> ---
>> v5 changes:
>> - Move GNTCOPY_* define next to the flags field
>> v4 changes:
>> - Used @keepindent/@endkeepindent doxygen commands
>>  to keep text with spaces indentation.
>> - drop changes to grant_entry_v1 comment, it will
>>  be changed and included in the docs in a future patch
>> - Move docs .rst to "common" folder
>> v3 changes:
>> - removed tags to skip anonymous union/struct
>> - moved back comment pointed out by Jan
>> - moved down defines related to struct gnttab_copy
>>  as pointed out by Jan
>> v2 changes:
>> - Revert back to anonymous union/struct
>> - add doxygen tags to skip anonymous union/struct
>> ---
>> docs/hypercall-interfaces/arm64.rst           |  1 +
>> .../common/grant_tables.rst                   |  8 +++
>> docs/xen-doxygen/doxy_input.list              |  1 +
>> xen/include/public/grant_table.h              | 66 ++++++++++++-------
>> 4 files changed, 52 insertions(+), 24 deletions(-)
>> create mode 100644 docs/hypercall-interfaces/common/grant_tables.rst
>> 
>> diff --git a/docs/hypercall-interfaces/arm64.rst 
>> b/docs/hypercall-interfaces/arm64.rst
>> index 5e701a2adc..cb4c0d13de 100644
>> --- a/docs/hypercall-interfaces/arm64.rst
>> +++ b/docs/hypercall-interfaces/arm64.rst
>> @@ -8,6 +8,7 @@ Starting points
>> .. toctree::
>>    :maxdepth: 2
>> 
>> +   common/grant_tables
>> 
>> 
>> Functions
>> diff --git a/docs/hypercall-interfaces/common/grant_tables.rst 
>> b/docs/hypercall-interfaces/common/grant_tables.rst
>> new file mode 100644
>> index 0000000000..8955ec5812
>> --- /dev/null
>> +++ b/docs/hypercall-interfaces/common/grant_tables.rst
>> @@ -0,0 +1,8 @@
>> +.. SPDX-License-Identifier: CC-BY-4.0
>> +
>> +Grant Tables
>> +============
>> +
>> +.. doxygengroup:: grant_table
>> +   :project: Xen
>> +   :members:
>> diff --git a/docs/xen-doxygen/doxy_input.list 
>> b/docs/xen-doxygen/doxy_input.list
>> index e69de29bb2..233d692fa7 100644
>> --- a/docs/xen-doxygen/doxy_input.list
>> +++ b/docs/xen-doxygen/doxy_input.list
>> @@ -0,0 +1 @@
>> +xen/include/public/grant_table.h
>> diff --git a/xen/include/public/grant_table.h 
>> b/xen/include/public/grant_table.h
>> index 84b1d26b36..e1fb91dfc6 100644
>> --- a/xen/include/public/grant_table.h
>> +++ b/xen/include/public/grant_table.h
>> @@ -25,15 +25,19 @@
>>  * Copyright (c) 2004, K A Fraser
>>  */
>> 
>> +/**
>> + * @file
>> + * @brief Interface for granting foreign access to page frames, and 
>> receiving
>> + * page-ownership transfers.
>> + */
>> +
>> #ifndef __XEN_PUBLIC_GRANT_TABLE_H__
>> #define __XEN_PUBLIC_GRANT_TABLE_H__
>> 
>> #include "xen.h"
>> 
>> -/*
>> - * `incontents 150 gnttab Grant Tables
>> - *
>> - * Xen's grant tables provide a generic mechanism to memory sharing
>> +/**
>> + * @brief Xen's grant tables provide a generic mechanism to memory sharing
>>  * between domains. This shared memory interface underpins the split
>>  * device drivers for block and network IO.
>>  *
>> @@ -51,13 +55,10 @@
>>  * know the real machine address of a page it is sharing. This makes
>>  * it possible to share memory correctly with domains running in
>>  * fully virtualised memory.
>> - */
>> -
>> -/***********************************
>> + *
>>  * GRANT TABLE REPRESENTATION
>> - */
>> -
>> -/* Some rough guidelines on accessing and updating grant-table entries
>> + *
>> + * Some rough guidelines on accessing and updating grant-table entries
>>  * in a concurrency-safe manner. For more information, Linux contains a
>>  * reference implementation for guest OSes (drivers/xen/grant_table.c, see
>>  * 
>> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=drivers/xen/grant-table.c;hb=HEAD
>> @@ -66,6 +67,7 @@
>>  *     compiler barrier will still be required.
>>  *
>>  * Introducing a valid entry into the grant table:
>> + * @keepindent
>>  *  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.
>> + * @endkeepindent
>>  *
>>  * Invalidating an unused GTF_permit_access entry:
>> + * @keepindent
>>  *  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.
>> + * @endkeepindent
>>  *
>>  * 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:
>> + * @keepindent
>>  *  1. flags = ent->flags.
>>  *  2. Observe that !(flags & GTF_transfer_committed). [*]
>>  *  3. Check result of SMP-safe CMPXCHG(&ent->flags, flags, 0).
>> @@ -97,18 +104,24 @@
>>  *      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).
>> + * @endkeepindent
>>  *
>>  * 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.
>> + *
>> + * @addtogroup grant_table Grant Tables
>> + * @{
>>  */
>> 
>> -/*
>> +/**
>>  * Reference to a grant entry in a specified domain's grant table.
>>  */
>> typedef uint32_t grant_ref_t;
> 
> Just below this typedef there is the following comment:
> 
> /*
> * 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.
> */
> 
> I noticed it doesn't appear in the output html. Any way we can retain it
> somewhere? Maybe we have to move it together with the larger comment
> above?

I agree with you, this comment should appear in the html docs, but to do so
It has to be moved together with the larger comment above.

In the original patch it was like that but I had to revert it back due to 
objection, my proposal is
to put it together with the larger comment and write something like this to
maintain a good readability:

   *  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.
+ *
+ * A grant table comprises a packed array of grant entries in one or more
+ * page frames shared between Xen and a guest.
+ *
+ * Data structure fields or defines described below have the following tags:
+ * * [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
   * @{

If we all agree I will put it in the v6

> 
> 
>> @@ -129,15 +142,17 @@ typedef uint32_t grant_ref_t;
>> #define grant_entry_v1_t grant_entry_t
>> #endif
>> struct grant_entry_v1 {
>> -    /* GTF_xxx: various type and flag information.  [XEN,GST] */
>> +    /** GTF_xxx: various type and flag information.  [XEN,GST] */
>>     uint16_t flags;
>> -    /* The domain being granted foreign privileges. [GST] */
>> +    /** The domain being granted foreign privileges. [GST] */
>>     domid_t  domid;
>> -    /*
>> +    /**
>> +     * @keepindent
>>      * GTF_permit_access: GFN that @domid is allowed to map and access. [GST]
>>      * GTF_accept_transfer: GFN that @domid is allowed to transfer into. 
>> [GST]
>>      * GTF_transfer_completed: MFN whose ownership transferred by @domid
>>      *                         (non-translated guests only). [XEN]
>> +     * @endkeepindent
>>      */
>>     uint32_t frame;
>> };
>> @@ -228,7 +243,7 @@ struct grant_entry_header {
>> };
>> typedef struct grant_entry_header grant_entry_header_t;
> 
> 
> Also this comment is missing from the output:
> 
> /*
> * Type of grant entry.
> *  GTF_invalid: This grant entry grants no privileges.
> *  GTF_permit_access: Allow @domid to map/access @frame.
> *  GTF_accept_transfer: Allow @domid to transfer ownership of one page frame
> *                       to this guest. Xen writes the page number to @frame.
> *  GTF_transitive: Allow @domid to transitively access a subrange of
> *                  @trans_grant in @trans_domid.  No mappings are allowed.
> */
> 
> Is there a way to keep it?
> 
> 
> Similarly for the other subflags descriptions.

Here we could split the comment and do something like this:

/**< GTF_invalid: This grant entry grants no privileges. */
#define GTF_invalid         (0U<<0)

/**< GTF_permit_access: Allow \@domid to map/access \@frame. */
#define GTF_permit_access   (1U<<0) 

/**
* GTF_accept_transfer: Allow \@domid to transfer 
* ownership of one page frame to this guest. Xen 
* writes the page number to \@frame.
*/
#define GTF_accept_transfer (2U<<0)

/**
* GTF_transitive: Allow @domid to transitively access a subrange of
* \@trans_grant in \@trans_domid.  No mappings are allowed.
*/
#define GTF_transitive      (3U<<0)

In this way each define will have its own description and I think both source 
code
and html docs would be improved.

> 
> 
>> -/*
>> +/**
>>  * Version 2 of the grant entry structure.
>>  */
>> union grant_entry_v2 {
>> @@ -433,7 +448,7 @@ typedef struct gnttab_transfer gnttab_transfer_t;
>> DEFINE_XEN_GUEST_HANDLE(gnttab_transfer_t);
> 
> What about the comments for each members of the union? Basically I am
> trying to ask if we can output almost all comments currently living in
> this header.

Yes we can, every comment that is in place near the commented data structure
needs just to have the /* changed to /** to be part of the html documentation.

There are other cases as above where the comment could be split, I can do that 
in the v6,
I need just to know if we agree on a solution like that so I don’t have to do 
work that I will
trash later.

> 
> 
>> -/*
>> +/**
>>  * 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,11 +466,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 {
>> @@ -468,6 +478,10 @@ struct gnttab_copy {
>>     } source, dest;
>>     uint16_t      len;
>>     uint16_t      flags;          /* GNTCOPY_* */
>> +#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 think this is OK
> 
> 
>>     /* OUT parameters. */
>>     int16_t       status;
>> };
>> @@ -579,7 +593,7 @@ 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.
>>  */
>> @@ -588,8 +602,8 @@ struct gnttab_cache_flush {
>>         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 */
>> +    uint16_t offset; /**< offset from start of grant */
>> +    uint16_t length; /**< size within the grant */
>> #define GNTTAB_CACHE_CLEAN          (1u<<0)
>> #define GNTTAB_CACHE_INVAL          (1u<<1)
>> #define GNTTAB_CACHE_SOURCE_GREF    (1u<<31)
>> @@ -673,6 +687,10 @@ DEFINE_XEN_GUEST_HANDLE(gnttab_cache_flush_t);
>>     "operation not done; try again"             \
>> }
>> 
>> +/**
>> + * @}
>> +*/
> 
> Alignment of the *

Sure I’ll fix it in the v6

I’ll wait a couple of days for the reply, then I’ll assume everything above is 
agreed and I’ll
push the v6.


Cheers,
Luca

> 
> 
>> #endif /* __XEN_PUBLIC_GRANT_TABLE_H__ */
>> 
>> /*
>> -- 
>> 2.17.1