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

From: Jan Beulich <jbeulich@suse.com>
To: Luca Fancellu <luca.fancellu@arm.com>
Cc: xen-devel@lists.xenproject.org,
	Bertrand Marquis <bertrand.marquis@arm.com>,
	wei.chen@arm.com, Andrew Cooper <andrew.cooper3@citrix.com>,
	George Dunlap <george.dunlap@citrix.com>,
	Ian Jackson <iwj@xenproject.org>, Julien Grall <julien@xen.org>,
	Wei Liu <wl@xen.org>, Stefano Stabellini <sstabellini@kernel.org>
Subject: Re: [PATCH v5 3/3] docs/doxygen: doxygen documentation for grant_table.h
Date: Thu, 6 May 2021 16:49:31 +0200	[thread overview]
Message-ID: <b66c7973-385d-2380-21f6-8948cca7f5c7@suse.com> (raw)
In-Reply-To: <BFE99431-6929-4A14-BE57-BFD6D6AA2997@arm.com>

On 06.05.2021 16:43, Luca Fancellu wrote:
>> On 6 May 2021, at 10:58, Jan Beulich <jbeulich@suse.com> wrote:
>> On 06.05.2021 10:48, Luca Fancellu wrote:
>>>> On 4 May 2021, at 23:27, Stefano Stabellini <sstabellini@kernel.org> 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.
> 
> Hi Jan,
> 
> Just to be sure that we are on the same page, something like this could be ok?
> 
>  * fully virtualised memory.
>  *
>  * GRANT TABLE REPRESENTATION
>  *
> + * A grant table comprises a packed array of grant entries in one or more
> + * page frames shared between Xen and a guest.
> + *
>  * Some rough guidelines on accessing and updating grant-table entries
>  * in a concurrency-safe manner. For more information, Linux contains a
> […]
>  * Changing a GTF_permit_access from read-only to writable:
>  *
>  *  Use SMP-safe bit-setting instruction.
>  *
> + * 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
>  * @{

I think so, yes. Thanks.

Jan