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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.05.2021 10:48, Luca Fancellu wrote:
>> On 4 May 2021, at 23:27, Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:
>> On Tue, 4 May 2021, Luca Fancellu wrote:
>>> @@ -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.

I think if this part was moved to the top of this big comment while ...

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

... this part was, as suggested by you, left near its bottom, I could
agree.

However, you making this suggestion caused me to look more closely at
what the comments actually describe. If there's effort to make the
documentation easier accessible by extracting it from the header, I
wonder whether - like with the v1 vs v2 comment pointed out previously
as misleading - we shouldn't, as a prereq step, make an attempt to
actually have the documentation be correct. For example I found this:

/*
 * Version 1 and version 2 grant entries share a common prefix.  The
 * fields of the prefix are documented as part of struct
 * grant_entry_v1.
 */
struct grant_entry_header {
    uint16_t flags;
    domid_t  domid;
};

The comment is wrong. "flags" here is only holding what's tagged
[GST] for v1. The [XEN] tagged bits actually live in grant_status_t.
This can perhaps best be seen in gnttab_set_version()'s code
dealing with the first 8 entries. However, contrary to v2's
intentions, GTF_transfer_committed and GTF_transfer_completed (which
aren't properly tagged either way) get set by Xen in shared entries,
not status ones. Maybe this was considered "okay" because the frame
field also gets written in this case (i.e. the cache line will get
dirtied in any event).

Similarly I'd like to refer to my still pending "gnttab: GTF_sub_page
is a v2-only flag", which also corrects documentation in this regard.
And perhaps there's more.

An alternative to correcting the (as it seems) v2 related issues, it
may be worth considering to extract only v1 documentation in this
initial phase.

Jan