[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v5 3/3] docs/doxygen: doxygen documentation for grant_table.h
> On 6 May 2021, at 10:58, Jan Beulich <jbeulich@xxxxxxxx> wrote: > > 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. 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 >
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |