xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
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 11:58:26 +0200	[thread overview]
Message-ID: <8fada713-9ae5-ddd3-585b-0f090748fc49@suse.com> (raw)
In-Reply-To: <9E7D7B58-0ABA-4800-B2D3-9EE3E29CF599@arm.com>

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.

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


  reply	other threads:[~2021-05-06  9:58 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-04 13:31 [PATCH v5 0/3] Use Doxygen and sphinx for html documentation Luca Fancellu
2021-05-04 13:31 ` [PATCH v5 1/3] docs: add doxygen support " Luca Fancellu
2021-05-04 22:55   ` Stefano Stabellini
2021-05-05 10:51     ` Luca Fancellu
2021-05-04 13:31 ` [PATCH v5 2/3] docs: hypercalls sphinx skeleton for generated html Luca Fancellu
2021-05-04 22:30   ` Stefano Stabellini
2021-05-05  8:23     ` Luca Fancellu
2021-05-04 13:31 ` [PATCH v5 3/3] docs/doxygen: doxygen documentation for grant_table.h Luca Fancellu
2021-05-04 22:27   ` Stefano Stabellini
2021-05-06  8:48     ` Luca Fancellu
2021-05-06  9:58       ` Jan Beulich [this message]
2021-05-06 14:43         ` Luca Fancellu
2021-05-06 14:49           ` Jan Beulich
2021-05-11 21:58         ` Stefano Stabellini
2021-05-12  7:34           ` Luca Fancellu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=8fada713-9ae5-ddd3-585b-0f090748fc49@suse.com \
    --to=jbeulich@suse.com \
    --cc=andrew.cooper3@citrix.com \
    --cc=bertrand.marquis@arm.com \
    --cc=george.dunlap@citrix.com \
    --cc=iwj@xenproject.org \
    --cc=julien@xen.org \
    --cc=luca.fancellu@arm.com \
    --cc=sstabellini@kernel.org \
    --cc=wei.chen@arm.com \
    --cc=wl@xen.org \
    --cc=xen-devel@lists.xenproject.org \
    --subject='Re: [PATCH v5 3/3] docs/doxygen: doxygen documentation for grant_table.h' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).