xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
From: Luca Fancellu <luca.fancellu@arm.com>
To: Jan Beulich <jbeulich@suse.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 15:43:21 +0100	[thread overview]
Message-ID: <BFE99431-6929-4A14-BE57-BFD6D6AA2997@arm.com> (raw)
In-Reply-To: <8fada713-9ae5-ddd3-585b-0f090748fc49@suse.com>



> 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
 * @{


> 
> 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 14:44 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
2021-05-06 14:43         ` Luca Fancellu [this message]
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=BFE99431-6929-4A14-BE57-BFD6D6AA2997@arm.com \
    --to=luca.fancellu@arm.com \
    --cc=andrew.cooper3@citrix.com \
    --cc=bertrand.marquis@arm.com \
    --cc=george.dunlap@citrix.com \
    --cc=iwj@xenproject.org \
    --cc=jbeulich@suse.com \
    --cc=julien@xen.org \
    --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).