| Message ID | 20210419091231.55684-4-luca.fancellu@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Use Doxygen and sphinx for html documentation | expand |
On 19.04.2021 11:12, Luca Fancellu wrote: > Modification to include/public/grant_table.h: > > 1) Add doxygen tags to: > - Create Grant tables section > - include variables in the generated documentation > 2) Add .rst file for grant table for Arm64 I'm missing some reasoning about at least some of the changes done to grant_table.h. Looking at this and the earlier patches I also couldn't spot any general outline of what is acceptable or even necessary in such a header to be understood by doxygen. Without this written down somewhere (or, if documented elsewhere, a pointer provided to that doc) I'm afraid things might get screwed up again later on. > --- a/docs/hypercall-interfaces/arm64.rst > +++ b/docs/hypercall-interfaces/arm64.rst > @@ -8,6 +8,7 @@ Starting points > .. toctree:: > :maxdepth: 2 > > + arm64/grant_tables > > > Functions > diff --git a/docs/hypercall-interfaces/arm64/grant_tables.rst b/docs/hypercall-interfaces/arm64/grant_tables.rst > new file mode 100644 > index 0000000000..8955ec5812 > --- /dev/null > +++ b/docs/hypercall-interfaces/arm64/grant_tables.rst > @@ -0,0 +1,8 @@ > +.. SPDX-License-Identifier: CC-BY-4.0 > + > +Grant Tables > +============ > + > +.. doxygengroup:: grant_table Why is this Arm64-specific? > @@ -73,20 +75,25 @@ > * frame, or zero if none. > * 3. Write memory barrier (WMB). > * 4. Write ent->flags, inc. valid type. > + * @endcode > * > * Invalidating an unused GTF_permit_access entry: > + * @code > * 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. > + * @endcode > * > * 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: > + * @code > * 1. flags = ent->flags. > * 2. Observe that !(flags & GTF_transfer_committed). [*] > * 3. Check result of SMP-safe CMPXCHG(&ent->flags, flags, 0). > @@ -97,47 +104,55 @@ > * 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). > + * @endcode > * > * 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. For example - are the blank lines you add necessary or merely nice to have in your personal opinion? > - */ > - > -/* > - * Reference to a grant entry in a specified domain's grant table. > - */ > -typedef uint32_t grant_ref_t; Why does this get moved ... > - > -/* > + * > * 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. > + * > + * @addtogroup grant_table Grant Tables > + * @{ > */ > > -/* > - * Version 1 of the grant table entry structure is maintained purely > - * for backwards compatibility. New guests should use version 2. > +/** > + * Reference to a grant entry in a specified domain's grant table. > */ > +typedef uint32_t grant_ref_t; ... here, past a comment unrelated to it? > @@ -243,23 +258,27 @@ union grant_entry_v2 { > * In that case, the frame field has the same semantics as the > * field of the same name in the V1 entry structure. > */ > + /** @cond skip anonymous struct/union for doxygen */ > struct { > grant_entry_header_t hdr; > uint32_t pad0; > uint64_t frame; > } full_page; > + /** @endcond */ > > /* > * If the grant type is GTF_grant_access and GTF_sub_page is set, > * @domid is allowed to access bytes [@page_off,@page_off+@length) > * in frame @frame. > */ > + /** @cond skip anonymous struct/union for doxygen */ > struct { > grant_entry_header_t hdr; > uint16_t page_off; > uint16_t length; > uint64_t frame; > } sub_page; > + /** @endcond */ > > /* > * If the grant is GTF_transitive, @domid is allowed to use the > @@ -270,12 +289,14 @@ union grant_entry_v2 { > * The current version of Xen does not allow transitive grants > * to be mapped. > */ > + /** @cond skip anonymous struct/union for doxygen */ > struct { > grant_entry_header_t hdr; > domid_t trans_domid; > uint16_t pad0; > grant_ref_t gref; > } transitive; > + /** @endcond */ While already better than the introduction of strange struct tags, I'm still not convinced we want this extra clutter (sorry). Plus - don't these additions mean the sub-structures then won't be represented in the generated doc, rendering it (partly) useless? > @@ -433,7 +454,12 @@ typedef struct gnttab_transfer gnttab_transfer_t; > DEFINE_XEN_GUEST_HANDLE(gnttab_transfer_t); > > > -/* > +#define _GNTCOPY_source_gref (0) > +#define GNTCOPY_source_gref (1<<_GNTCOPY_source_gref) > +#define _GNTCOPY_dest_gref (1) > +#define GNTCOPY_dest_gref (1<<_GNTCOPY_dest_gref) > + > +/** > * GNTTABOP_copy: Hypervisor based copy > * source and destinations can be eithers MFNs or, for foreign domains, > * grant references. the foreign domain has to grant read/write access > @@ -451,18 +477,15 @@ DEFINE_XEN_GUEST_HANDLE(gnttab_transfer_t); > * bytes to be copied. > */ > > -#define _GNTCOPY_source_gref (0) > -#define GNTCOPY_source_gref (1<<_GNTCOPY_source_gref) > -#define _GNTCOPY_dest_gref (1) > -#define GNTCOPY_dest_gref (1<<_GNTCOPY_dest_gref) > - > struct gnttab_copy { Again the question - why the movement? > @@ -579,17 +602,19 @@ struct gnttab_swap_grant_ref { > typedef struct gnttab_swap_grant_ref gnttab_swap_grant_ref_t; > DEFINE_XEN_GUEST_HANDLE(gnttab_swap_grant_ref_t); > > -/* > +/** > * Issue one or more cache maintenance operations on a portion of a > * page granted to the calling domain by a foreign domain. > */ > struct gnttab_cache_flush { > + /** @cond skip anonymous struct/union for doxygen */ > union { > uint64_t dev_bus_addr; > grant_ref_t ref; > } a; > - uint16_t offset; /* offset from start of grant */ > - uint16_t length; /* size within the grant */ > + /** @endcond */ > + uint16_t offset; /**< offset from start of grant */ > + uint16_t length; /**< size within the grant */ Skipping just part of a struct is perhaps even more confusing than omitting it altogether. Also, what's the significance of "/**<" ? Jan
> On 19 Apr 2021, at 12:05, Jan Beulich <jbeulich@suse.com> wrote: > > On 19.04.2021 11:12, Luca Fancellu wrote: >> Modification to include/public/grant_table.h: >> >> 1) Add doxygen tags to: >> - Create Grant tables section >> - include variables in the generated documentation >> 2) Add .rst file for grant table for Arm64 > > I'm missing some reasoning about at least some of the changes done > to grant_table.h. Looking at this and the earlier patches I also > couldn't spot any general outline of what is acceptable or even > necessary in such a header to be understood by doxygen. Without > this written down somewhere (or, if documented elsewhere, a > pointer provided to that doc) I'm afraid things might get screwed > up again later on. Hi Jan, Doxygen is a tool that generates documentation based on parsing the source code comments, it recognises some commands in the comments and builds the documentation sticking to what you wrote. Here the doxygen docs: https://www.doxygen.nl/manual/docblocks.html Basically it doesn’t react to all comments, it parses only some well crafted comments as explained in its documentation. > >> --- a/docs/hypercall-interfaces/arm64.rst >> +++ b/docs/hypercall-interfaces/arm64.rst >> @@ -8,6 +8,7 @@ Starting points >> .. toctree:: >> :maxdepth: 2 >> >> + arm64/grant_tables >> >> >> Functions >> diff --git a/docs/hypercall-interfaces/arm64/grant_tables.rst b/docs/hypercall-interfaces/arm64/grant_tables.rst >> new file mode 100644 >> index 0000000000..8955ec5812 >> --- /dev/null >> +++ b/docs/hypercall-interfaces/arm64/grant_tables.rst >> @@ -0,0 +1,8 @@ >> +.. SPDX-License-Identifier: CC-BY-4.0 >> + >> +Grant Tables >> +============ >> + >> +.. doxygengroup:: grant_table > > Why is this Arm64-specific? This particular one is Arm64 specific, but it doesn’t mean that grant tables are arm specific, it is only that for now I’m Introducing a partial documentation in the arm side. Any other user can in the future add more documentation for each platform. > >> @@ -73,20 +75,25 @@ >> * frame, or zero if none. >> * 3. Write memory barrier (WMB). >> * 4. Write ent->flags, inc. valid type. >> + * @endcode >> * >> * Invalidating an unused GTF_permit_access entry: >> + * @code >> * 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. >> + * @endcode >> * >> * 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: >> + * @code >> * 1. flags = ent->flags. >> * 2. Observe that !(flags & GTF_transfer_committed). [*] >> * 3. Check result of SMP-safe CMPXCHG(&ent->flags, flags, 0). >> @@ -97,47 +104,55 @@ >> * 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). >> + * @endcode >> * >> * 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. > > For example - are the blank lines you add necessary or merely nice > to have in your personal opinion? The blank lines makes the docs output more good looking > >> - */ >> - >> -/* >> - * Reference to a grant entry in a specified domain's grant table. >> - */ >> -typedef uint32_t grant_ref_t; > > Why does this get moved ... > >> - >> -/* >> + * >> * 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. >> + * >> + * @addtogroup grant_table Grant Tables >> + * @{ >> */ >> >> -/* >> - * Version 1 of the grant table entry structure is maintained purely >> - * for backwards compatibility. New guests should use version 2. >> +/** >> + * Reference to a grant entry in a specified domain's grant table. >> */ >> +typedef uint32_t grant_ref_t; > > ... here, past a comment unrelated to it? The comment “Version 1 of the grant table entry […]” was moved on top of the struct grant_entry_v1, in this way Doxygen associate the comment to that structure. The comment “Reference to a grant entry in a specified domain's grant table.” Is moved on top of typedef uint32_t grant_ref_t for the same reason above > >> @@ -243,23 +258,27 @@ union grant_entry_v2 { >> * In that case, the frame field has the same semantics as the >> * field of the same name in the V1 entry structure. >> */ >> + /** @cond skip anonymous struct/union for doxygen */ >> struct { >> grant_entry_header_t hdr; >> uint32_t pad0; >> uint64_t frame; >> } full_page; >> + /** @endcond */ >> >> /* >> * If the grant type is GTF_grant_access and GTF_sub_page is set, >> * @domid is allowed to access bytes [@page_off,@page_off+@length) >> * in frame @frame. >> */ >> + /** @cond skip anonymous struct/union for doxygen */ >> struct { >> grant_entry_header_t hdr; >> uint16_t page_off; >> uint16_t length; >> uint64_t frame; >> } sub_page; >> + /** @endcond */ >> >> /* >> * If the grant is GTF_transitive, @domid is allowed to use the >> @@ -270,12 +289,14 @@ union grant_entry_v2 { >> * The current version of Xen does not allow transitive grants >> * to be mapped. >> */ >> + /** @cond skip anonymous struct/union for doxygen */ >> struct { >> grant_entry_header_t hdr; >> domid_t trans_domid; >> uint16_t pad0; >> grant_ref_t gref; >> } transitive; >> + /** @endcond */ > > While already better than the introduction of strange struct tags, > I'm still not convinced we want this extra clutter (sorry). Plus - > don't these additions mean the sub-structures then won't be > represented in the generated doc, rendering it (partly) useless? Are you suggesting to remove the structure from docs? > >> @@ -433,7 +454,12 @@ typedef struct gnttab_transfer gnttab_transfer_t; >> DEFINE_XEN_GUEST_HANDLE(gnttab_transfer_t); >> >> >> -/* >> +#define _GNTCOPY_source_gref (0) >> +#define GNTCOPY_source_gref (1<<_GNTCOPY_source_gref) >> +#define _GNTCOPY_dest_gref (1) >> +#define GNTCOPY_dest_gref (1<<_GNTCOPY_dest_gref) >> + >> +/** >> * GNTTABOP_copy: Hypervisor based copy >> * source and destinations can be eithers MFNs or, for foreign domains, >> * grant references. the foreign domain has to grant read/write access >> @@ -451,18 +477,15 @@ DEFINE_XEN_GUEST_HANDLE(gnttab_transfer_t); >> * bytes to be copied. >> */ >> >> -#define _GNTCOPY_source_gref (0) >> -#define GNTCOPY_source_gref (1<<_GNTCOPY_source_gref) >> -#define _GNTCOPY_dest_gref (1) >> -#define GNTCOPY_dest_gref (1<<_GNTCOPY_dest_gref) >> - >> struct gnttab_copy { > > Again the question - why the movement? Doxygen takes the comment just above the data structure to build the docs, so here we are moving the Comment just on top of the described structure. > >> @@ -579,17 +602,19 @@ struct gnttab_swap_grant_ref { >> typedef struct gnttab_swap_grant_ref gnttab_swap_grant_ref_t; >> DEFINE_XEN_GUEST_HANDLE(gnttab_swap_grant_ref_t); >> >> -/* >> +/** >> * Issue one or more cache maintenance operations on a portion of a >> * page granted to the calling domain by a foreign domain. >> */ >> struct gnttab_cache_flush { >> + /** @cond skip anonymous struct/union for doxygen */ >> union { >> uint64_t dev_bus_addr; >> grant_ref_t ref; >> } a; >> - uint16_t offset; /* offset from start of grant */ >> - uint16_t length; /* size within the grant */ >> + /** @endcond */ >> + uint16_t offset; /**< offset from start of grant */ >> + uint16_t length; /**< size within the grant */ > > Skipping just part of a struct is perhaps even more confusing than > omitting it altogether. > > Also, what's the significance of "/**<" ? It is a doxygen pattern that tells it to use the comment as a field related documentation. If you build the documentation you will find the result, I encourage you to see it to realize the power of the tool and the benefits that Xen can get with it. Cheers, Luca > > Jan
On 20.04.2021 10:46, Luca Fancellu wrote: >> On 19 Apr 2021, at 12:05, Jan Beulich <jbeulich@suse.com> wrote: >> >> On 19.04.2021 11:12, Luca Fancellu wrote: >>> Modification to include/public/grant_table.h: >>> >>> 1) Add doxygen tags to: >>> - Create Grant tables section >>> - include variables in the generated documentation >>> 2) Add .rst file for grant table for Arm64 >> >> I'm missing some reasoning about at least some of the changes done >> to grant_table.h. Looking at this and the earlier patches I also >> couldn't spot any general outline of what is acceptable or even >> necessary in such a header to be understood by doxygen. Without >> this written down somewhere (or, if documented elsewhere, a >> pointer provided to that doc) I'm afraid things might get screwed >> up again later on. > > Doxygen is a tool that generates documentation based on parsing the source code comments, it recognises some > commands in the comments and builds the documentation sticking to what you wrote. > > Here the doxygen docs: https://www.doxygen.nl/manual/docblocks.html > > Basically it doesn’t react to all comments, it parses only some well crafted comments as explained in its documentation. Providing this link somewhere might be helpful. However, also seeing some of your further comments, it feels like to edit a public header enabled for doxygen one has to know fair parts of this documentation. While I'm certainly in favor of having a way to generate docs from the headers, I'm afraid I don't think such knowledge should become a prereq to e.g. adding a new sub-function of a hypercall. So far I was assuming that the formatting requirements would be quite limited, and that it would hence be possible to just glean them from existing code. But e.g. the "/**<" notation isn't going to be obvious to spot. >>> --- a/docs/hypercall-interfaces/arm64.rst >>> +++ b/docs/hypercall-interfaces/arm64.rst >>> @@ -8,6 +8,7 @@ Starting points >>> .. toctree:: >>> :maxdepth: 2 >>> >>> + arm64/grant_tables >>> >>> >>> Functions >>> diff --git a/docs/hypercall-interfaces/arm64/grant_tables.rst b/docs/hypercall-interfaces/arm64/grant_tables.rst >>> new file mode 100644 >>> index 0000000000..8955ec5812 >>> --- /dev/null >>> +++ b/docs/hypercall-interfaces/arm64/grant_tables.rst >>> @@ -0,0 +1,8 @@ >>> +.. SPDX-License-Identifier: CC-BY-4.0 >>> + >>> +Grant Tables >>> +============ >>> + >>> +.. doxygengroup:: grant_table >> >> Why is this Arm64-specific? > > This particular one is Arm64 specific, but it doesn’t mean that grant tables are arm specific, it is only that for now I’m > Introducing a partial documentation in the arm side. Any other user can in the future add more documentation for > each platform. I'm of the pretty strong opinion that common hypercalls should be documented as common, and hence not live in an arch-specific section. >>> @@ -73,20 +75,25 @@ >>> * frame, or zero if none. >>> * 3. Write memory barrier (WMB). >>> * 4. Write ent->flags, inc. valid type. >>> + * @endcode >>> * >>> * Invalidating an unused GTF_permit_access entry: >>> + * @code >>> * 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. >>> + * @endcode >>> * >>> * 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: >>> + * @code >>> * 1. flags = ent->flags. >>> * 2. Observe that !(flags & GTF_transfer_committed). [*] >>> * 3. Check result of SMP-safe CMPXCHG(&ent->flags, flags, 0). >>> @@ -97,47 +104,55 @@ >>> * 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). >>> + * @endcode >>> * >>> * 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. >> >> For example - are the blank lines you add necessary or merely nice >> to have in your personal opinion? > > The blank lines makes the docs output more good looking I'm inclined to suggest to split beautification from basic enabling. >>> - */ >>> - >>> -/* >>> - * Reference to a grant entry in a specified domain's grant table. >>> - */ >>> -typedef uint32_t grant_ref_t; >> >> Why does this get moved ... >> >>> - >>> -/* >>> + * >>> * 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. >>> + * >>> + * @addtogroup grant_table Grant Tables >>> + * @{ >>> */ >>> >>> -/* >>> - * Version 1 of the grant table entry structure is maintained purely >>> - * for backwards compatibility. New guests should use version 2. >>> +/** >>> + * Reference to a grant entry in a specified domain's grant table. >>> */ >>> +typedef uint32_t grant_ref_t; >> >> ... here, past a comment unrelated to it? > > The comment “Version 1 of the grant table entry […]” was moved on top of the struct grant_entry_v1, in this way > Doxygen associate the comment to that structure. > > The comment “Reference to a grant entry in a specified domain's grant table.” Is moved on top of typedef uint32_t grant_ref_t > for the same reason above But it's the other comment ("A grant table comprises ...") that I was asking about. >>> @@ -243,23 +258,27 @@ union grant_entry_v2 { >>> * In that case, the frame field has the same semantics as the >>> * field of the same name in the V1 entry structure. >>> */ >>> + /** @cond skip anonymous struct/union for doxygen */ >>> struct { >>> grant_entry_header_t hdr; >>> uint32_t pad0; >>> uint64_t frame; >>> } full_page; >>> + /** @endcond */ >>> >>> /* >>> * If the grant type is GTF_grant_access and GTF_sub_page is set, >>> * @domid is allowed to access bytes [@page_off,@page_off+@length) >>> * in frame @frame. >>> */ >>> + /** @cond skip anonymous struct/union for doxygen */ >>> struct { >>> grant_entry_header_t hdr; >>> uint16_t page_off; >>> uint16_t length; >>> uint64_t frame; >>> } sub_page; >>> + /** @endcond */ >>> >>> /* >>> * If the grant is GTF_transitive, @domid is allowed to use the >>> @@ -270,12 +289,14 @@ union grant_entry_v2 { >>> * The current version of Xen does not allow transitive grants >>> * to be mapped. >>> */ >>> + /** @cond skip anonymous struct/union for doxygen */ >>> struct { >>> grant_entry_header_t hdr; >>> domid_t trans_domid; >>> uint16_t pad0; >>> grant_ref_t gref; >>> } transitive; >>> + /** @endcond */ >> >> While already better than the introduction of strange struct tags, >> I'm still not convinced we want this extra clutter (sorry). Plus - >> don't these additions mean the sub-structures then won't be >> represented in the generated doc, rendering it (partly) useless? > > Are you suggesting to remove the structure from docs? Just yet I'm not suggesting anything here - I was merely guessing at the effect of "@cond" and the associated "skip ..." to mean that this construct makes doxygen skip the enclosed section. If that's not the effect, then maybe the comment wants rewording? (And then - what does @cond mean?) >>> @@ -433,7 +454,12 @@ typedef struct gnttab_transfer gnttab_transfer_t; >>> DEFINE_XEN_GUEST_HANDLE(gnttab_transfer_t); >>> >>> >>> -/* >>> +#define _GNTCOPY_source_gref (0) >>> +#define GNTCOPY_source_gref (1<<_GNTCOPY_source_gref) >>> +#define _GNTCOPY_dest_gref (1) >>> +#define GNTCOPY_dest_gref (1<<_GNTCOPY_dest_gref) >>> + >>> +/** >>> * GNTTABOP_copy: Hypervisor based copy >>> * source and destinations can be eithers MFNs or, for foreign domains, >>> * grant references. the foreign domain has to grant read/write access >>> @@ -451,18 +477,15 @@ DEFINE_XEN_GUEST_HANDLE(gnttab_transfer_t); >>> * bytes to be copied. >>> */ >>> >>> -#define _GNTCOPY_source_gref (0) >>> -#define GNTCOPY_source_gref (1<<_GNTCOPY_source_gref) >>> -#define _GNTCOPY_dest_gref (1) >>> -#define GNTCOPY_dest_gref (1<<_GNTCOPY_dest_gref) >>> - >>> struct gnttab_copy { >> >> Again the question - why the movement? > > Doxygen takes the comment just above the data structure to build the docs, so here we are moving the > Comment just on top of the described structure. Well, okay, this then is an explanation _that_ the #define-s want moving, but not an explanation for where they got moved (father away from what they actually relate to). Personally I consider it good practice to have such #define-s next to the field they relate to (and we have such placement elsewhere). Perhaps worth considering to move them down rather than up? Jan >>> @@ -579,17 +602,19 @@ struct gnttab_swap_grant_ref { >>> typedef struct gnttab_swap_grant_ref gnttab_swap_grant_ref_t; >>> DEFINE_XEN_GUEST_HANDLE(gnttab_swap_grant_ref_t); >>> >>> -/* >>> +/** >>> * Issue one or more cache maintenance operations on a portion of a >>> * page granted to the calling domain by a foreign domain. >>> */ >>> struct gnttab_cache_flush { >>> + /** @cond skip anonymous struct/union for doxygen */ >>> union { >>> uint64_t dev_bus_addr; >>> grant_ref_t ref; >>> } a; >>> - uint16_t offset; /* offset from start of grant */ >>> - uint16_t length; /* size within the grant */ >>> + /** @endcond */ >>> + uint16_t offset; /**< offset from start of grant */ >>> + uint16_t length; /**< size within the grant */ >> >> Skipping just part of a struct is perhaps even more confusing than >> omitting it altogether. >> >> Also, what's the significance of "/**<" ? > > It is a doxygen pattern that tells it to use the comment as a field related documentation. > If you build the documentation you will find the result, I encourage you to see it to > realize the power of the tool and the benefits that Xen can get with it. > > Cheers, > Luca
> On 20 Apr 2021, at 10:14, Jan Beulich <jbeulich@suse.com> wrote: > > On 20.04.2021 10:46, Luca Fancellu wrote: >>> On 19 Apr 2021, at 12:05, Jan Beulich <jbeulich@suse.com> wrote: >>> >>> On 19.04.2021 11:12, Luca Fancellu wrote: >>>> Modification to include/public/grant_table.h: >>>> >>>> 1) Add doxygen tags to: >>>> - Create Grant tables section >>>> - include variables in the generated documentation >>>> 2) Add .rst file for grant table for Arm64 >>> >>> I'm missing some reasoning about at least some of the changes done >>> to grant_table.h. Looking at this and the earlier patches I also >>> couldn't spot any general outline of what is acceptable or even >>> necessary in such a header to be understood by doxygen. Without >>> this written down somewhere (or, if documented elsewhere, a >>> pointer provided to that doc) I'm afraid things might get screwed >>> up again later on. >> >> Doxygen is a tool that generates documentation based on parsing the source code comments, it recognises some >> commands in the comments and builds the documentation sticking to what you wrote. >> >> Here the doxygen docs: https://www.doxygen.nl/manual/docblocks.html >> >> Basically it doesn’t react to all comments, it parses only some well crafted comments as explained in its documentation. > > Providing this link somewhere might be helpful. However, also seeing > some of your further comments, it feels like to edit a public header > enabled for doxygen one has to know fair parts of this documentation. > While I'm certainly in favor of having a way to generate docs from > the headers, I'm afraid I don't think such knowledge should become a > prereq to e.g. adding a new sub-function of a hypercall. So far I was > assuming that the formatting requirements would be quite limited, and > that it would hence be possible to just glean them from existing code. > But e.g. the "/**<" notation isn't going to be obvious to spot. Yes I see, some knowledge of the documentation tool is required but I’m sure that if anyone is able to put his/her hands on the xen code, he/she will be able to use some of the doxygen tag too, as you can see from this patch the usage is easy and some less obvious thing can be understood looking the source code and the corresponding generated doc page. > >>>> --- a/docs/hypercall-interfaces/arm64.rst >>>> +++ b/docs/hypercall-interfaces/arm64.rst >>>> @@ -8,6 +8,7 @@ Starting points >>>> .. toctree:: >>>> :maxdepth: 2 >>>> >>>> + arm64/grant_tables >>>> >>>> >>>> Functions >>>> diff --git a/docs/hypercall-interfaces/arm64/grant_tables.rst b/docs/hypercall-interfaces/arm64/grant_tables.rst >>>> new file mode 100644 >>>> index 0000000000..8955ec5812 >>>> --- /dev/null >>>> +++ b/docs/hypercall-interfaces/arm64/grant_tables.rst >>>> @@ -0,0 +1,8 @@ >>>> +.. SPDX-License-Identifier: CC-BY-4.0 >>>> + >>>> +Grant Tables >>>> +============ >>>> + >>>> +.. doxygengroup:: grant_table >>> >>> Why is this Arm64-specific? >> >> This particular one is Arm64 specific, but it doesn’t mean that grant tables are arm specific, it is only that for now I’m >> Introducing a partial documentation in the arm side. Any other user can in the future add more documentation for >> each platform. > > I'm of the pretty strong opinion that common hypercalls should be > documented as common, and hence not live in an arch-specific > section. Yes sure, I agree, but this is the first step to enable a kind of documentation, it’s a work in progress. On a future serie this can be modify. For now I’ve tried to reproduce this page: https://xenbits.xen.org/docs/unstable/ In the section “Hypercall Interfaces" > >>>> @@ -73,20 +75,25 @@ >>>> * frame, or zero if none. >>>> * 3. Write memory barrier (WMB). >>>> * 4. Write ent->flags, inc. valid type. >>>> + * @endcode >>>> * >>>> * Invalidating an unused GTF_permit_access entry: >>>> + * @code >>>> * 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. >>>> + * @endcode >>>> * >>>> * 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: >>>> + * @code >>>> * 1. flags = ent->flags. >>>> * 2. Observe that !(flags & GTF_transfer_committed). [*] >>>> * 3. Check result of SMP-safe CMPXCHG(&ent->flags, flags, 0). >>>> @@ -97,47 +104,55 @@ >>>> * 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). >>>> + * @endcode >>>> * >>>> * 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. >>> >>> For example - are the blank lines you add necessary or merely nice >>> to have in your personal opinion? >> >> The blank lines makes the docs output more good looking > > I'm inclined to suggest to split beautification from basic enabling. I will take this into consideration > >>>> - */ >>>> - >>>> -/* >>>> - * Reference to a grant entry in a specified domain's grant table. >>>> - */ >>>> -typedef uint32_t grant_ref_t; >>> >>> Why does this get moved ... >>> >>>> - >>>> -/* >>>> + * >>>> * 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. >>>> + * >>>> + * @addtogroup grant_table Grant Tables >>>> + * @{ >>>> */ >>>> >>>> -/* >>>> - * Version 1 of the grant table entry structure is maintained purely >>>> - * for backwards compatibility. New guests should use version 2. >>>> +/** >>>> + * Reference to a grant entry in a specified domain's grant table. >>>> */ >>>> +typedef uint32_t grant_ref_t; >>> >>> ... here, past a comment unrelated to it? >> >> The comment “Version 1 of the grant table entry […]” was moved on top of the struct grant_entry_v1, in this way >> Doxygen associate the comment to that structure. >> >> The comment “Reference to a grant entry in a specified domain's grant table.” Is moved on top of typedef uint32_t grant_ref_t >> for the same reason above > > But it's the other comment ("A grant table comprises ...") that I > was asking about. I thought it was part of the brief about grant tables, am I wrong? For that reason I moved it. > >>>> @@ -243,23 +258,27 @@ union grant_entry_v2 { >>>> * In that case, the frame field has the same semantics as the >>>> * field of the same name in the V1 entry structure. >>>> */ >>>> + /** @cond skip anonymous struct/union for doxygen */ >>>> struct { >>>> grant_entry_header_t hdr; >>>> uint32_t pad0; >>>> uint64_t frame; >>>> } full_page; >>>> + /** @endcond */ >>>> >>>> /* >>>> * If the grant type is GTF_grant_access and GTF_sub_page is set, >>>> * @domid is allowed to access bytes [@page_off,@page_off+@length) >>>> * in frame @frame. >>>> */ >>>> + /** @cond skip anonymous struct/union for doxygen */ >>>> struct { >>>> grant_entry_header_t hdr; >>>> uint16_t page_off; >>>> uint16_t length; >>>> uint64_t frame; >>>> } sub_page; >>>> + /** @endcond */ >>>> >>>> /* >>>> * If the grant is GTF_transitive, @domid is allowed to use the >>>> @@ -270,12 +289,14 @@ union grant_entry_v2 { >>>> * The current version of Xen does not allow transitive grants >>>> * to be mapped. >>>> */ >>>> + /** @cond skip anonymous struct/union for doxygen */ >>>> struct { >>>> grant_entry_header_t hdr; >>>> domid_t trans_domid; >>>> uint16_t pad0; >>>> grant_ref_t gref; >>>> } transitive; >>>> + /** @endcond */ >>> >>> While already better than the introduction of strange struct tags, >>> I'm still not convinced we want this extra clutter (sorry). Plus - >>> don't these additions mean the sub-structures then won't be >>> represented in the generated doc, rendering it (partly) useless? >> >> Are you suggesting to remove the structure from docs? > > Just yet I'm not suggesting anything here - I was merely guessing at > the effect of "@cond" and the associated "skip ..." to mean that this > construct makes doxygen skip the enclosed section. If that's not the > effect, then maybe the comment wants rewording? (And then - what does > @cond mean?) Yes you were right, in the documentation the structure grant_entry_v2 won’t display the anonymous union. > >>>> @@ -433,7 +454,12 @@ typedef struct gnttab_transfer gnttab_transfer_t; >>>> DEFINE_XEN_GUEST_HANDLE(gnttab_transfer_t); >>>> >>>> >>>> -/* >>>> +#define _GNTCOPY_source_gref (0) >>>> +#define GNTCOPY_source_gref (1<<_GNTCOPY_source_gref) >>>> +#define _GNTCOPY_dest_gref (1) >>>> +#define GNTCOPY_dest_gref (1<<_GNTCOPY_dest_gref) >>>> + >>>> +/** >>>> * GNTTABOP_copy: Hypervisor based copy >>>> * source and destinations can be eithers MFNs or, for foreign domains, >>>> * grant references. the foreign domain has to grant read/write access >>>> @@ -451,18 +477,15 @@ DEFINE_XEN_GUEST_HANDLE(gnttab_transfer_t); >>>> * bytes to be copied. >>>> */ >>>> >>>> -#define _GNTCOPY_source_gref (0) >>>> -#define GNTCOPY_source_gref (1<<_GNTCOPY_source_gref) >>>> -#define _GNTCOPY_dest_gref (1) >>>> -#define GNTCOPY_dest_gref (1<<_GNTCOPY_dest_gref) >>>> - >>>> struct gnttab_copy { >>> >>> Again the question - why the movement? >> >> Doxygen takes the comment just above the data structure to build the docs, so here we are moving the >> Comment just on top of the described structure. > > Well, okay, this then is an explanation _that_ the #define-s want > moving, but not an explanation for where they got moved (father > away from what they actually relate to). Personally I consider it > good practice to have such #define-s next to the field they relate > to (and we have such placement elsewhere). Perhaps worth > considering to move them down rather than up? Sure they can be moved down without problem. > > Jan > >>>> @@ -579,17 +602,19 @@ struct gnttab_swap_grant_ref { >>>> typedef struct gnttab_swap_grant_ref gnttab_swap_grant_ref_t; >>>> DEFINE_XEN_GUEST_HANDLE(gnttab_swap_grant_ref_t); >>>> >>>> -/* >>>> +/** >>>> * Issue one or more cache maintenance operations on a portion of a >>>> * page granted to the calling domain by a foreign domain. >>>> */ >>>> struct gnttab_cache_flush { >>>> + /** @cond skip anonymous struct/union for doxygen */ >>>> union { >>>> uint64_t dev_bus_addr; >>>> grant_ref_t ref; >>>> } a; >>>> - uint16_t offset; /* offset from start of grant */ >>>> - uint16_t length; /* size within the grant */ >>>> + /** @endcond */ >>>> + uint16_t offset; /**< offset from start of grant */ >>>> + uint16_t length; /**< size within the grant */ >>> >>> Skipping just part of a struct is perhaps even more confusing than >>> omitting it altogether. >>> >>> Also, what's the significance of "/**<" ? >> >> It is a doxygen pattern that tells it to use the comment as a field related documentation. >> If you build the documentation you will find the result, I encourage you to see it to >> realize the power of the tool and the benefits that Xen can get with it. >> >> Cheers, >> Luca
On 20.04.2021 11:42, Luca Fancellu wrote: >> On 20 Apr 2021, at 10:14, Jan Beulich <jbeulich@suse.com> wrote: >> On 20.04.2021 10:46, Luca Fancellu wrote: >>>> On 19 Apr 2021, at 12:05, Jan Beulich <jbeulich@suse.com> wrote: >>>> On 19.04.2021 11:12, Luca Fancellu wrote: >>>>> - */ >>>>> - >>>>> -/* >>>>> - * Reference to a grant entry in a specified domain's grant table. >>>>> - */ >>>>> -typedef uint32_t grant_ref_t; >>>> >>>> Why does this get moved ... >>>> >>>>> - >>>>> -/* >>>>> + * >>>>> * 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. >>>>> + * >>>>> + * @addtogroup grant_table Grant Tables >>>>> + * @{ >>>>> */ >>>>> >>>>> -/* >>>>> - * Version 1 of the grant table entry structure is maintained purely >>>>> - * for backwards compatibility. New guests should use version 2. >>>>> +/** >>>>> + * Reference to a grant entry in a specified domain's grant table. >>>>> */ >>>>> +typedef uint32_t grant_ref_t; >>>> >>>> ... here, past a comment unrelated to it? >>> >>> The comment “Version 1 of the grant table entry […]” was moved on top of the struct grant_entry_v1, in this way >>> Doxygen associate the comment to that structure. >>> >>> The comment “Reference to a grant entry in a specified domain's grant table.” Is moved on top of typedef uint32_t grant_ref_t >>> for the same reason above >> >> But it's the other comment ("A grant table comprises ...") that I >> was asking about. > > I thought it was part of the brief about grant tables, am I wrong? For that reason I moved it. Well, the comment talks about grant table entries (the layout of which gets defined immediately below, as visible in the original patch), not grant references. >>>>> @@ -243,23 +258,27 @@ union grant_entry_v2 { >>>>> * In that case, the frame field has the same semantics as the >>>>> * field of the same name in the V1 entry structure. >>>>> */ >>>>> + /** @cond skip anonymous struct/union for doxygen */ >>>>> struct { >>>>> grant_entry_header_t hdr; >>>>> uint32_t pad0; >>>>> uint64_t frame; >>>>> } full_page; >>>>> + /** @endcond */ >>>>> >>>>> /* >>>>> * If the grant type is GTF_grant_access and GTF_sub_page is set, >>>>> * @domid is allowed to access bytes [@page_off,@page_off+@length) >>>>> * in frame @frame. >>>>> */ >>>>> + /** @cond skip anonymous struct/union for doxygen */ >>>>> struct { >>>>> grant_entry_header_t hdr; >>>>> uint16_t page_off; >>>>> uint16_t length; >>>>> uint64_t frame; >>>>> } sub_page; >>>>> + /** @endcond */ >>>>> >>>>> /* >>>>> * If the grant is GTF_transitive, @domid is allowed to use the >>>>> @@ -270,12 +289,14 @@ union grant_entry_v2 { >>>>> * The current version of Xen does not allow transitive grants >>>>> * to be mapped. >>>>> */ >>>>> + /** @cond skip anonymous struct/union for doxygen */ >>>>> struct { >>>>> grant_entry_header_t hdr; >>>>> domid_t trans_domid; >>>>> uint16_t pad0; >>>>> grant_ref_t gref; >>>>> } transitive; >>>>> + /** @endcond */ >>>> >>>> While already better than the introduction of strange struct tags, >>>> I'm still not convinced we want this extra clutter (sorry). Plus - >>>> don't these additions mean the sub-structures then won't be >>>> represented in the generated doc, rendering it (partly) useless? >>> >>> Are you suggesting to remove the structure from docs? >> >> Just yet I'm not suggesting anything here - I was merely guessing at >> the effect of "@cond" and the associated "skip ..." to mean that this >> construct makes doxygen skip the enclosed section. If that's not the >> effect, then maybe the comment wants rewording? (And then - what does >> @cond mean?) > > Yes you were right, in the documentation the structure grant_entry_v2 won’t display the > anonymous union. In which case I'm now completely unclear about your prior question. Jan
> On 20 Apr 2021, at 11:27, Jan Beulich <jbeulich@suse.com> wrote: > > On 20.04.2021 11:42, Luca Fancellu wrote: >>> On 20 Apr 2021, at 10:14, Jan Beulich <jbeulich@suse.com> wrote: >>> On 20.04.2021 10:46, Luca Fancellu wrote: >>>>> On 19 Apr 2021, at 12:05, Jan Beulich <jbeulich@suse.com> wrote: >>>>> On 19.04.2021 11:12, Luca Fancellu wrote: >>>>>> - */ >>>>>> - >>>>>> -/* >>>>>> - * Reference to a grant entry in a specified domain's grant table. >>>>>> - */ >>>>>> -typedef uint32_t grant_ref_t; >>>>> >>>>> Why does this get moved ... >>>>> >>>>>> - >>>>>> -/* >>>>>> + * >>>>>> * 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. >>>>>> + * >>>>>> + * @addtogroup grant_table Grant Tables >>>>>> + * @{ >>>>>> */ >>>>>> >>>>>> -/* >>>>>> - * Version 1 of the grant table entry structure is maintained purely >>>>>> - * for backwards compatibility. New guests should use version 2. >>>>>> +/** >>>>>> + * Reference to a grant entry in a specified domain's grant table. >>>>>> */ >>>>>> +typedef uint32_t grant_ref_t; >>>>> >>>>> ... here, past a comment unrelated to it? >>>> >>>> The comment “Version 1 of the grant table entry […]” was moved on top of the struct grant_entry_v1, in this way >>>> Doxygen associate the comment to that structure. >>>> >>>> The comment “Reference to a grant entry in a specified domain's grant table.” Is moved on top of typedef uint32_t grant_ref_t >>>> for the same reason above >>> >>> But it's the other comment ("A grant table comprises ...") that I >>> was asking about. >> >> I thought it was part of the brief about grant tables, am I wrong? For that reason I moved it. > > Well, the comment talks about grant table entries (the layout of which > gets defined immediately below, as visible in the original patch), not > grant references. Hi Jan, Of course this can be reverted back if it is wrong. I’ve prepared a page with the output of all my commits (some of them are not yet in the ML), so anyone can have a look on what is the output and how can sphinx+doxygen improve the quality of the documentation. Here: https://luca.fancellu.gitlab.io/xen-docs/hypercall-interfaces/arm64.html the document Is trying to reproduce this other one from the current docs: https://xenbits.xen.org/docs/unstable/hypercall/arm/index.html You will receive a warning from the browser because the gitlab certificate has problems when the username has a dot in it. > >>>>>> @@ -243,23 +258,27 @@ union grant_entry_v2 { >>>>>> * In that case, the frame field has the same semantics as the >>>>>> * field of the same name in the V1 entry structure. >>>>>> */ >>>>>> + /** @cond skip anonymous struct/union for doxygen */ >>>>>> struct { >>>>>> grant_entry_header_t hdr; >>>>>> uint32_t pad0; >>>>>> uint64_t frame; >>>>>> } full_page; >>>>>> + /** @endcond */ >>>>>> >>>>>> /* >>>>>> * If the grant type is GTF_grant_access and GTF_sub_page is set, >>>>>> * @domid is allowed to access bytes [@page_off,@page_off+@length) >>>>>> * in frame @frame. >>>>>> */ >>>>>> + /** @cond skip anonymous struct/union for doxygen */ >>>>>> struct { >>>>>> grant_entry_header_t hdr; >>>>>> uint16_t page_off; >>>>>> uint16_t length; >>>>>> uint64_t frame; >>>>>> } sub_page; >>>>>> + /** @endcond */ >>>>>> >>>>>> /* >>>>>> * If the grant is GTF_transitive, @domid is allowed to use the >>>>>> @@ -270,12 +289,14 @@ union grant_entry_v2 { >>>>>> * The current version of Xen does not allow transitive grants >>>>>> * to be mapped. >>>>>> */ >>>>>> + /** @cond skip anonymous struct/union for doxygen */ >>>>>> struct { >>>>>> grant_entry_header_t hdr; >>>>>> domid_t trans_domid; >>>>>> uint16_t pad0; >>>>>> grant_ref_t gref; >>>>>> } transitive; >>>>>> + /** @endcond */ >>>>> >>>>> While already better than the introduction of strange struct tags, >>>>> I'm still not convinced we want this extra clutter (sorry). Plus - >>>>> don't these additions mean the sub-structures then won't be >>>>> represented in the generated doc, rendering it (partly) useless? >>>> >>>> Are you suggesting to remove the structure from docs? >>> >>> Just yet I'm not suggesting anything here - I was merely guessing at >>> the effect of "@cond" and the associated "skip ..." to mean that this >>> construct makes doxygen skip the enclosed section. If that's not the >>> effect, then maybe the comment wants rewording? (And then - what does >>> @cond mean?) >> >> Yes you were right, in the documentation the structure grant_entry_v2 won’t display the >> anonymous union. > > In which case I'm now completely unclear about your prior question. We have to choose just if the struct is useful even if it’s partially documented or if it’s better to hide it completely from the docs since it can’t be fully documented. Cheers, Luca > > Jan
On 22.04.2021 09:39, Luca Fancellu wrote: >> On 20 Apr 2021, at 11:27, Jan Beulich <jbeulich@suse.com> wrote: >> On 20.04.2021 11:42, Luca Fancellu wrote: >>>> On 20 Apr 2021, at 10:14, Jan Beulich <jbeulich@suse.com> wrote: >>>> On 20.04.2021 10:46, Luca Fancellu wrote: >>>>>> On 19 Apr 2021, at 12:05, Jan Beulich <jbeulich@suse.com> wrote: >>>>>> On 19.04.2021 11:12, Luca Fancellu wrote: >>>>>>> - */ >>>>>>> - >>>>>>> -/* >>>>>>> - * Reference to a grant entry in a specified domain's grant table. >>>>>>> - */ >>>>>>> -typedef uint32_t grant_ref_t; >>>>>> >>>>>> Why does this get moved ... >>>>>> >>>>>>> - >>>>>>> -/* >>>>>>> + * >>>>>>> * 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. >>>>>>> + * >>>>>>> + * @addtogroup grant_table Grant Tables >>>>>>> + * @{ >>>>>>> */ >>>>>>> >>>>>>> -/* >>>>>>> - * Version 1 of the grant table entry structure is maintained purely >>>>>>> - * for backwards compatibility. New guests should use version 2. >>>>>>> +/** >>>>>>> + * Reference to a grant entry in a specified domain's grant table. >>>>>>> */ >>>>>>> +typedef uint32_t grant_ref_t; >>>>>> >>>>>> ... here, past a comment unrelated to it? >>>>> >>>>> The comment “Version 1 of the grant table entry […]” was moved on top of the struct grant_entry_v1, in this way >>>>> Doxygen associate the comment to that structure. >>>>> >>>>> The comment “Reference to a grant entry in a specified domain's grant table.” Is moved on top of typedef uint32_t grant_ref_t >>>>> for the same reason above >>>> >>>> But it's the other comment ("A grant table comprises ...") that I >>>> was asking about. >>> >>> I thought it was part of the brief about grant tables, am I wrong? For that reason I moved it. >> >> Well, the comment talks about grant table entries (the layout of which >> gets defined immediately below, as visible in the original patch), not >> grant references. > > Hi Jan, > > Of course this can be reverted back if it is wrong. > > I’ve prepared a page with the output of all my commits (some of them are not yet in the ML), > so anyone can have a look on what is the output and how can sphinx+doxygen improve > the quality of the documentation. > > Here: > > https://luca.fancellu.gitlab.io/xen-docs/hypercall-interfaces/arm64.html The doc looks fine in this regard, but the C header should remain properly ordered as well. >>>>>>> @@ -243,23 +258,27 @@ union grant_entry_v2 { >>>>>>> * In that case, the frame field has the same semantics as the >>>>>>> * field of the same name in the V1 entry structure. >>>>>>> */ >>>>>>> + /** @cond skip anonymous struct/union for doxygen */ >>>>>>> struct { >>>>>>> grant_entry_header_t hdr; >>>>>>> uint32_t pad0; >>>>>>> uint64_t frame; >>>>>>> } full_page; >>>>>>> + /** @endcond */ >>>>>>> >>>>>>> /* >>>>>>> * If the grant type is GTF_grant_access and GTF_sub_page is set, >>>>>>> * @domid is allowed to access bytes [@page_off,@page_off+@length) >>>>>>> * in frame @frame. >>>>>>> */ >>>>>>> + /** @cond skip anonymous struct/union for doxygen */ >>>>>>> struct { >>>>>>> grant_entry_header_t hdr; >>>>>>> uint16_t page_off; >>>>>>> uint16_t length; >>>>>>> uint64_t frame; >>>>>>> } sub_page; >>>>>>> + /** @endcond */ >>>>>>> >>>>>>> /* >>>>>>> * If the grant is GTF_transitive, @domid is allowed to use the >>>>>>> @@ -270,12 +289,14 @@ union grant_entry_v2 { >>>>>>> * The current version of Xen does not allow transitive grants >>>>>>> * to be mapped. >>>>>>> */ >>>>>>> + /** @cond skip anonymous struct/union for doxygen */ >>>>>>> struct { >>>>>>> grant_entry_header_t hdr; >>>>>>> domid_t trans_domid; >>>>>>> uint16_t pad0; >>>>>>> grant_ref_t gref; >>>>>>> } transitive; >>>>>>> + /** @endcond */ >>>>>> >>>>>> While already better than the introduction of strange struct tags, >>>>>> I'm still not convinced we want this extra clutter (sorry). Plus - >>>>>> don't these additions mean the sub-structures then won't be >>>>>> represented in the generated doc, rendering it (partly) useless? >>>>> >>>>> Are you suggesting to remove the structure from docs? >>>> >>>> Just yet I'm not suggesting anything here - I was merely guessing at >>>> the effect of "@cond" and the associated "skip ..." to mean that this >>>> construct makes doxygen skip the enclosed section. If that's not the >>>> effect, then maybe the comment wants rewording? (And then - what does >>>> @cond mean?) >>> >>> Yes you were right, in the documentation the structure grant_entry_v2 won’t display the >>> anonymous union. >> >> In which case I'm now completely unclear about your prior question. > > We have to choose just if the struct is useful even if it’s partially documented or if > it’s better to hide it completely from the docs since it can’t be fully documented. I've never suggested to hide it completely (aiui doing so would mean widening the scope of the @cond, i.e. there would still be extra clutter). Instead I was trying to suggest to make sure the struct gets fully documented despite the absence of struct tags. Jan
> On 22 Apr 2021, at 09:06, Jan Beulich <jbeulich@suse.com> wrote: > > On 22.04.2021 09:39, Luca Fancellu wrote: >>> On 20 Apr 2021, at 11:27, Jan Beulich <jbeulich@suse.com> wrote: >>> On 20.04.2021 11:42, Luca Fancellu wrote: >>>>> On 20 Apr 2021, at 10:14, Jan Beulich <jbeulich@suse.com> wrote: >>>>> On 20.04.2021 10:46, Luca Fancellu wrote: >>>>>>> On 19 Apr 2021, at 12:05, Jan Beulich <jbeulich@suse.com> wrote: >>>>>>> On 19.04.2021 11:12, Luca Fancellu wrote: >>>>>>>> - */ >>>>>>>> - >>>>>>>> -/* >>>>>>>> - * Reference to a grant entry in a specified domain's grant table. >>>>>>>> - */ >>>>>>>> -typedef uint32_t grant_ref_t; >>>>>>> >>>>>>> Why does this get moved ... >>>>>>> >>>>>>>> - >>>>>>>> -/* >>>>>>>> + * >>>>>>>> * 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. >>>>>>>> + * >>>>>>>> + * @addtogroup grant_table Grant Tables >>>>>>>> + * @{ >>>>>>>> */ >>>>>>>> >>>>>>>> -/* >>>>>>>> - * Version 1 of the grant table entry structure is maintained purely >>>>>>>> - * for backwards compatibility. New guests should use version 2. >>>>>>>> +/** >>>>>>>> + * Reference to a grant entry in a specified domain's grant table. >>>>>>>> */ >>>>>>>> +typedef uint32_t grant_ref_t; >>>>>>> >>>>>>> ... here, past a comment unrelated to it? >>>>>> >>>>>> The comment “Version 1 of the grant table entry […]” was moved on top of the struct grant_entry_v1, in this way >>>>>> Doxygen associate the comment to that structure. >>>>>> >>>>>> The comment “Reference to a grant entry in a specified domain's grant table.” Is moved on top of typedef uint32_t grant_ref_t >>>>>> for the same reason above >>>>> >>>>> But it's the other comment ("A grant table comprises ...") that I >>>>> was asking about. >>>> >>>> I thought it was part of the brief about grant tables, am I wrong? For that reason I moved it. >>> >>> Well, the comment talks about grant table entries (the layout of which >>> gets defined immediately below, as visible in the original patch), not >>> grant references. >> >> Hi Jan, >> >> Of course this can be reverted back if it is wrong. >> >> I’ve prepared a page with the output of all my commits (some of them are not yet in the ML), >> so anyone can have a look on what is the output and how can sphinx+doxygen improve >> the quality of the documentation. >> >> Here: >> >> https://luca.fancellu.gitlab.io/xen-docs/hypercall-interfaces/arm64.html > > The doc looks fine in this regard, but the C header should remain > properly ordered as well. Hi Jan, I’ve pushed outside the v3 addressing your comment > >>>>>>>> @@ -243,23 +258,27 @@ union grant_entry_v2 { >>>>>>>> * In that case, the frame field has the same semantics as the >>>>>>>> * field of the same name in the V1 entry structure. >>>>>>>> */ >>>>>>>> + /** @cond skip anonymous struct/union for doxygen */ >>>>>>>> struct { >>>>>>>> grant_entry_header_t hdr; >>>>>>>> uint32_t pad0; >>>>>>>> uint64_t frame; >>>>>>>> } full_page; >>>>>>>> + /** @endcond */ >>>>>>>> >>>>>>>> /* >>>>>>>> * If the grant type is GTF_grant_access and GTF_sub_page is set, >>>>>>>> * @domid is allowed to access bytes [@page_off,@page_off+@length) >>>>>>>> * in frame @frame. >>>>>>>> */ >>>>>>>> + /** @cond skip anonymous struct/union for doxygen */ >>>>>>>> struct { >>>>>>>> grant_entry_header_t hdr; >>>>>>>> uint16_t page_off; >>>>>>>> uint16_t length; >>>>>>>> uint64_t frame; >>>>>>>> } sub_page; >>>>>>>> + /** @endcond */ >>>>>>>> >>>>>>>> /* >>>>>>>> * If the grant is GTF_transitive, @domid is allowed to use the >>>>>>>> @@ -270,12 +289,14 @@ union grant_entry_v2 { >>>>>>>> * The current version of Xen does not allow transitive grants >>>>>>>> * to be mapped. >>>>>>>> */ >>>>>>>> + /** @cond skip anonymous struct/union for doxygen */ >>>>>>>> struct { >>>>>>>> grant_entry_header_t hdr; >>>>>>>> domid_t trans_domid; >>>>>>>> uint16_t pad0; >>>>>>>> grant_ref_t gref; >>>>>>>> } transitive; >>>>>>>> + /** @endcond */ >>>>>>> >>>>>>> While already better than the introduction of strange struct tags, >>>>>>> I'm still not convinced we want this extra clutter (sorry). Plus - >>>>>>> don't these additions mean the sub-structures then won't be >>>>>>> represented in the generated doc, rendering it (partly) useless? >>>>>> >>>>>> Are you suggesting to remove the structure from docs? >>>>> >>>>> Just yet I'm not suggesting anything here - I was merely guessing at >>>>> the effect of "@cond" and the associated "skip ..." to mean that this >>>>> construct makes doxygen skip the enclosed section. If that's not the >>>>> effect, then maybe the comment wants rewording? (And then - what does >>>>> @cond mean?) >>>> >>>> Yes you were right, in the documentation the structure grant_entry_v2 won’t display the >>>> anonymous union. >>> >>> In which case I'm now completely unclear about your prior question. >> >> We have to choose just if the struct is useful even if it’s partially documented or if >> it’s better to hide it completely from the docs since it can’t be fully documented. > > I've never suggested to hide it completely (aiui doing so would > mean widening the scope of the @cond, i.e. there would still be > extra clutter). Instead I was trying to suggest to make sure the > struct gets fully documented despite the absence of struct tags. In the v3 I preprocess the header to give a name to the anonymous struct so that it appears in the docs without touching the header. Cheers, Luca > > Jan
diff --git a/docs/hypercall-interfaces/arm64.rst b/docs/hypercall-interfaces/arm64.rst index 5e701a2adc..c30a7142b1 100644 --- a/docs/hypercall-interfaces/arm64.rst +++ b/docs/hypercall-interfaces/arm64.rst @@ -8,6 +8,7 @@ Starting points .. toctree:: :maxdepth: 2 + arm64/grant_tables Functions diff --git a/docs/hypercall-interfaces/arm64/grant_tables.rst b/docs/hypercall-interfaces/arm64/grant_tables.rst new file mode 100644 index 0000000000..8955ec5812 --- /dev/null +++ b/docs/hypercall-interfaces/arm64/grant_tables.rst @@ -0,0 +1,8 @@ +.. SPDX-License-Identifier: CC-BY-4.0 + +Grant Tables +============ + +.. doxygengroup:: grant_table + :project: Xen + :members: diff --git a/docs/xen-doxygen/doxy_input.list b/docs/xen-doxygen/doxy_input.list index e69de29bb2..233d692fa7 100644 --- a/docs/xen-doxygen/doxy_input.list +++ b/docs/xen-doxygen/doxy_input.list @@ -0,0 +1 @@ +xen/include/public/grant_table.h diff --git a/xen/include/public/grant_table.h b/xen/include/public/grant_table.h index 84b1d26b36..51c8d85822 100644 --- a/xen/include/public/grant_table.h +++ b/xen/include/public/grant_table.h @@ -25,15 +25,19 @@ * Copyright (c) 2004, K A Fraser */ -#ifndef __XEN_PUBLIC_GRANT_TABLE_H__ +/** + * @file + * @brief Interface for granting foreign access to page frames, and receiving + * page-ownership transfers. + */ + +#if !defined(__XEN_PUBLIC_GRANT_TABLE_H__) || defined(DOXYGEN) #define __XEN_PUBLIC_GRANT_TABLE_H__ #include "xen.h" -/* - * `incontents 150 gnttab Grant Tables - * - * Xen's grant tables provide a generic mechanism to memory sharing +/** + * @brief Xen's grant tables provide a generic mechanism to memory sharing * between domains. This shared memory interface underpins the split * device drivers for block and network IO. * @@ -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: + * @code * 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. + * @endcode * * Invalidating an unused GTF_permit_access entry: + * @code * 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. + * @endcode * * 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: + * @code * 1. flags = ent->flags. * 2. Observe that !(flags & GTF_transfer_committed). [*] * 3. Check result of SMP-safe CMPXCHG(&ent->flags, flags, 0). @@ -97,47 +104,55 @@ * 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). + * @endcode * * 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. - */ - -/* - * Reference to a grant entry in a specified domain's grant table. - */ -typedef uint32_t grant_ref_t; - -/* + * * 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. + * + * @addtogroup grant_table Grant Tables + * @{ */ -/* - * Version 1 of the grant table entry structure is maintained purely - * for backwards compatibility. New guests should use version 2. +/** + * Reference to a grant entry in a specified domain's grant table. */ +typedef uint32_t grant_ref_t; + #if __XEN_INTERFACE_VERSION__ < 0x0003020a #define grant_entry_v1 grant_entry #define grant_entry_v1_t grant_entry_t #endif +/** + * Version 1 of the grant table entry structure is maintained purely + * for backwards compatibility. New guests should use version 2. + */ struct grant_entry_v1 { - /* GTF_xxx: various type and flag information. [XEN,GST] */ + /** GTF_xxx: various type and flag information. [XEN,GST] */ uint16_t flags; - /* The domain being granted foreign privileges. [GST] */ + /** The domain being granted foreign privileges. [GST] */ domid_t domid; - /* + /** + * @code * GTF_permit_access: GFN that @domid is allowed to map and access. [GST] * GTF_accept_transfer: GFN that @domid is allowed to transfer into. [GST] * GTF_transfer_completed: MFN whose ownership transferred by @domid * (non-translated guests only). [XEN] + * @endcode */ uint32_t frame; }; @@ -243,23 +258,27 @@ union grant_entry_v2 { * In that case, the frame field has the same semantics as the * field of the same name in the V1 entry structure. */ + /** @cond skip anonymous struct/union for doxygen */ struct { grant_entry_header_t hdr; uint32_t pad0; uint64_t frame; } full_page; + /** @endcond */ /* * If the grant type is GTF_grant_access and GTF_sub_page is set, * @domid is allowed to access bytes [@page_off,@page_off+@length) * in frame @frame. */ + /** @cond skip anonymous struct/union for doxygen */ struct { grant_entry_header_t hdr; uint16_t page_off; uint16_t length; uint64_t frame; } sub_page; + /** @endcond */ /* * If the grant is GTF_transitive, @domid is allowed to use the @@ -270,12 +289,14 @@ union grant_entry_v2 { * The current version of Xen does not allow transitive grants * to be mapped. */ + /** @cond skip anonymous struct/union for doxygen */ struct { grant_entry_header_t hdr; domid_t trans_domid; uint16_t pad0; grant_ref_t gref; } transitive; + /** @endcond */ uint32_t __spacer[4]; /* Pad to a power of two */ }; @@ -433,7 +454,12 @@ typedef struct gnttab_transfer gnttab_transfer_t; DEFINE_XEN_GUEST_HANDLE(gnttab_transfer_t); -/* +#define _GNTCOPY_source_gref (0) +#define GNTCOPY_source_gref (1<<_GNTCOPY_source_gref) +#define _GNTCOPY_dest_gref (1) +#define GNTCOPY_dest_gref (1<<_GNTCOPY_dest_gref) + +/** * GNTTABOP_copy: Hypervisor based copy * source and destinations can be eithers MFNs or, for foreign domains, * grant references. the foreign domain has to grant read/write access @@ -451,18 +477,15 @@ DEFINE_XEN_GUEST_HANDLE(gnttab_transfer_t); * bytes to be copied. */ -#define _GNTCOPY_source_gref (0) -#define GNTCOPY_source_gref (1<<_GNTCOPY_source_gref) -#define _GNTCOPY_dest_gref (1) -#define GNTCOPY_dest_gref (1<<_GNTCOPY_dest_gref) - struct gnttab_copy { /* IN parameters. */ struct gnttab_copy_ptr { + /** @cond skip anonymous struct/union for doxygen */ union { grant_ref_t ref; xen_pfn_t gmfn; } u; + /** @endcond */ domid_t domid; uint16_t offset; } source, dest; @@ -579,17 +602,19 @@ struct gnttab_swap_grant_ref { typedef struct gnttab_swap_grant_ref gnttab_swap_grant_ref_t; DEFINE_XEN_GUEST_HANDLE(gnttab_swap_grant_ref_t); -/* +/** * Issue one or more cache maintenance operations on a portion of a * page granted to the calling domain by a foreign domain. */ struct gnttab_cache_flush { + /** @cond skip anonymous struct/union for doxygen */ union { uint64_t dev_bus_addr; grant_ref_t ref; } a; - uint16_t offset; /* offset from start of grant */ - uint16_t length; /* size within the grant */ + /** @endcond */ + uint16_t offset; /**< offset from start of grant */ + uint16_t length; /**< size within the grant */ #define GNTTAB_CACHE_CLEAN (1u<<0) #define GNTTAB_CACHE_INVAL (1u<<1) #define GNTTAB_CACHE_SOURCE_GREF (1u<<31) @@ -673,6 +698,10 @@ DEFINE_XEN_GUEST_HANDLE(gnttab_cache_flush_t); "operation not done; try again" \ } +/** + * @} +*/ + #endif /* __XEN_PUBLIC_GRANT_TABLE_H__ */ /*
Modification to include/public/grant_table.h: 1) Add doxygen tags to: - Create Grant tables section - include variables in the generated documentation 2) Add .rst file for grant table for Arm64 Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> --- v2 changes: - Revert back to anonymous union/struct - add doxygen tags to skip anonymous union/struct --- docs/hypercall-interfaces/arm64.rst | 1 + .../arm64/grant_tables.rst | 8 ++ docs/xen-doxygen/doxy_input.list | 1 + xen/include/public/grant_table.h | 97 ++++++++++++------- 4 files changed, 73 insertions(+), 34 deletions(-) create mode 100644 docs/hypercall-interfaces/arm64/grant_tables.rst