| Message ID | 20210426153741.26904-4-luca.fancellu@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Use Doxygen and sphinx for html documentation | expand |
On 26.04.2021 17:37, Luca Fancellu wrote: > --- 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 I continue to object to this giving the impression that grant tables are something Arm64 specific. > @@ -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). Since neither in the cover letter nor in the description here I could spot a link to the resulting generated doc, I wonder what the inconsistencies above are about: You add @code/@endcode (and no blank lines) to parts of what's being described, and _instead_ a blank line to another part. I think the goal should be that not only the generated doc ends up looking "nice", but that the source code also remains consistent. Otherwise, someone like me coming across this later on might easily conclude that there was a @code/@endcode pair missed. > @@ -97,18 +104,23 @@ > * 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). > * Why did this not also get enclosed by @code/@endcode? > * 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. And these two? > + * @addtogroup grant_table Grant Tables And no blank (comment) line ahead of this? > @@ -120,24 +132,26 @@ typedef uint32_t grant_ref_t; > * [GST]: This field is written by the guest and read by Xen. > */ > > -/* > - * Version 1 of the grant table entry structure is maintained purely > - * for backwards compatibility. New guests should use version 2. > - */ > #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. > + */ In case I didn't say so already before - I think this would be a good opportunity to drop the comment pointing at v2. With v2 optionally unavailable altogether, this can't possibly be a generally valid course of action. > @@ -451,11 +465,6 @@ 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 { > @@ -471,6 +480,12 @@ struct gnttab_copy { > /* OUT parameters. */ > int16_t status; > }; > + > +#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) I guess I didn't express myself precisely enough: I think these would better live _immediately_ next to the field that uses them. Jan
On 27/04/2021 14:57, Jan Beulich wrote: >> 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 > > I continue to object to this giving the impression that grant tables > are something Arm64 specific. +1, grant tables is common interface so why do they need to be described per arch? I haven't looked at the history of the patch, but if there was some rationale about this approach, then this ought to be written in the commit message or at least after (---). Cheers,
> On 27 Apr 2021, at 14:57, Jan Beulich <jbeulich@suse.com> wrote: > > On 26.04.2021 17:37, Luca Fancellu wrote: >> --- 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 > > I continue to object to this giving the impression that grant tables > are something Arm64 specific. Hi Jan, I’ll put it in a folder named “common" > >> @@ -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). > > Since neither in the cover letter nor in the description here I could > spot a link to the resulting generated doc, I wonder what the > inconsistencies above are about: You add @code/@endcode (and no blank > lines) to parts of what's being described, and _instead_ a blank line > to another part. I think the goal should be that not only the > generated doc ends up looking "nice", but that the source code also > remains consistent. Otherwise, someone like me coming across this > later on might easily conclude that there was a @code/@endcode pair > missed. Yes I’ll explain better in the commit message, that part and other parts are enclosed by @code/@endcode because they are formatted using spaces. If the block is not enclosed the spaces are missing in the html page resulting In a mess. If you can suggest an alias for the @code/@endcode command, I can create it so that the user looking the source code can understand better what's going on. An example: @keepformat/@endkeepformat OR @keepindent/@endkeepindent Here a link to the documentation right now: https://luca.fancellu.gitlab.io/xen-docs/hypercall-interfaces/arm64/grant_tables.html > >> @@ -97,18 +104,23 @@ >> * 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). >> * > > Why did this not also get enclosed by @code/@endcode? In this case there are no spaces that contributes to the indentation. > >> * 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. > > And these two? These two lines makes the resulting html page looks better, the source code however seems not too impacted by the change though. > >> + * @addtogroup grant_table Grant Tables > > And no blank (comment) line ahead of this? Here there is no need for a blank line in the comment, but if in your opinion the source code will look better I can add it > >> @@ -120,24 +132,26 @@ typedef uint32_t grant_ref_t; >> * [GST]: This field is written by the guest and read by Xen. >> */ >> >> -/* >> - * Version 1 of the grant table entry structure is maintained purely >> - * for backwards compatibility. New guests should use version 2. >> - */ >> #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. >> + */ > > In case I didn't say so already before - I think this would be a good > opportunity to drop the comment pointing at v2. With v2 optionally > unavailable altogether, this can't possibly be a generally valid > course of action. Could you explain me better that? Do you want to get rid of this comment? /** * Version 1 of the grant table entry structure is maintained purely * for backwards compatibility. New guests should use version 2. */ In this case I don’t think this commit is the right place to do that, I can just put it back where it was so that the documentation simply doesn’t show that. > >> @@ -451,11 +465,6 @@ 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 { >> @@ -471,6 +480,12 @@ struct gnttab_copy { >> /* OUT parameters. */ >> int16_t status; >> }; >> + >> +#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) > > I guess I didn't express myself precisely enough: I think these > would better live _immediately_ next to the field that uses them. Ok I didn’t get that, I’ll put them right next to the field Cheers, Luca > > Jan
On 28.04.2021 16:59, Luca Fancellu wrote: >> On 27 Apr 2021, at 14:57, Jan Beulich <jbeulich@suse.com> wrote: >> On 26.04.2021 17:37, Luca Fancellu wrote: >>> @@ -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). >> >> Since neither in the cover letter nor in the description here I could >> spot a link to the resulting generated doc, I wonder what the >> inconsistencies above are about: You add @code/@endcode (and no blank >> lines) to parts of what's being described, and _instead_ a blank line >> to another part. I think the goal should be that not only the >> generated doc ends up looking "nice", but that the source code also >> remains consistent. Otherwise, someone like me coming across this >> later on might easily conclude that there was a @code/@endcode pair >> missed. > > Yes I’ll explain better in the commit message, that part and other parts are > enclosed by @code/@endcode because they are formatted using spaces. > If the block is not enclosed the spaces are missing in the html page resulting > In a mess. > If you can suggest an alias for the @code/@endcode command, I can create > it so that the user looking the source code can understand better what's going on. > An example: @keepformat/@endkeepformat OR @keepindent/@endkeepindent Oh, are you suggesting @code / @endcode isn't something doxygen mandates? In this case either of your alternative suggestions would look better to me. Which one depend on whether the goal to to merely keep indentation or whether formatting should be kept altogether. >>> @@ -97,18 +104,23 @@ >>> * 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). >>> * >> >> Why did this not also get enclosed by @code/@endcode? > > In this case there are no spaces that contributes to the indentation. But if, for consistency, @code / @endcode were added here, all would still be well? >>> * 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. >> >> And these two? > > These two lines makes the resulting html page looks better, the source code however > seems not too impacted by the change though. I was rather asking about the absence of @code / @endcode here. >>> + * @addtogroup grant_table Grant Tables >> >> And no blank (comment) line ahead of this? > > Here there is no need for a blank line in the comment, but if in your opinion the source > code will look better I can add it I think so, yes. >>> @@ -120,24 +132,26 @@ typedef uint32_t grant_ref_t; >>> * [GST]: This field is written by the guest and read by Xen. >>> */ >>> >>> -/* >>> - * Version 1 of the grant table entry structure is maintained purely >>> - * for backwards compatibility. New guests should use version 2. >>> - */ >>> #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. >>> + */ >> >> In case I didn't say so already before - I think this would be a good >> opportunity to drop the comment pointing at v2. With v2 optionally >> unavailable altogether, this can't possibly be a generally valid >> course of action. > > Could you explain me better that? Do you want to get rid of this comment? Especially the second sentence is misleading. If new code used v2, it would not work on Xen with v2 support turned off. > /** > * Version 1 of the grant table entry structure is maintained purely > * for backwards compatibility. New guests should use version 2. > */ > > In this case I don’t think this commit is the right place to do that, I can just > put it back where it was so that the documentation simply doesn’t show that. Keeping misleading information out of the docs is imo rather desirable. Of course we should, independently of that, also address the misleading information in the source code. I can accept that doing the adjustment right in this patch may not be ideal. I don't suppose I could talk you into adding a prereq patch dropping at least that 2nd sentence? Jan
> On 29 Apr 2021, at 10:04, Jan Beulich <jbeulich@suse.com> wrote: > > On 28.04.2021 16:59, Luca Fancellu wrote: >>> On 27 Apr 2021, at 14:57, Jan Beulich <jbeulich@suse.com> wrote: >>> On 26.04.2021 17:37, Luca Fancellu wrote: >>>> @@ -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). >>> >>> Since neither in the cover letter nor in the description here I could >>> spot a link to the resulting generated doc, I wonder what the >>> inconsistencies above are about: You add @code/@endcode (and no blank >>> lines) to parts of what's being described, and _instead_ a blank line >>> to another part. I think the goal should be that not only the >>> generated doc ends up looking "nice", but that the source code also >>> remains consistent. Otherwise, someone like me coming across this >>> later on might easily conclude that there was a @code/@endcode pair >>> missed. >> >> Yes I’ll explain better in the commit message, that part and other parts are >> enclosed by @code/@endcode because they are formatted using spaces. >> If the block is not enclosed the spaces are missing in the html page resulting >> In a mess. >> If you can suggest an alias for the @code/@endcode command, I can create >> it so that the user looking the source code can understand better what's going on. >> An example: @keepformat/@endkeepformat OR @keepindent/@endkeepindent > > Oh, are you suggesting @code / @endcode isn't something doxygen mandates? > In this case either of your alternative suggestions would look better to > me. Which one depend on whether the goal to to merely keep indentation or > whether formatting should be kept altogether. Hi Jan, Sure, I can go with @keepindent/@endkeepindent > >>>> @@ -97,18 +104,23 @@ >>>> * 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). >>>> * >>> >>> Why did this not also get enclosed by @code/@endcode? >> >> In this case there are no spaces that contributes to the indentation. > > But if, for consistency, @code / @endcode were added here, all would > still be well? Yes it would be ok, in the html page you will see a box with this text inside. If you see the url I sent in previous mail, you can see the rendering of the @code/@endcode in the html page to have an idea on how it looks. > >>>> * 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. >>> >>> And these two? >> >> These two lines makes the resulting html page looks better, the source code however >> seems not too impacted by the change though. > > I was rather asking about the absence of @code / @endcode here. I didn’t use it because there is no space indentation to be kept, it looks better either in the source code and in the html page in this way > >>>> + * @addtogroup grant_table Grant Tables >>> >>> And no blank (comment) line ahead of this? >> >> Here there is no need for a blank line in the comment, but if in your opinion the source >> code will look better I can add it > > I think so, yes. I will add it in the next patch > >>>> @@ -120,24 +132,26 @@ typedef uint32_t grant_ref_t; >>>> * [GST]: This field is written by the guest and read by Xen. >>>> */ >>>> >>>> -/* >>>> - * Version 1 of the grant table entry structure is maintained purely >>>> - * for backwards compatibility. New guests should use version 2. >>>> - */ >>>> #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. >>>> + */ >>> >>> In case I didn't say so already before - I think this would be a good >>> opportunity to drop the comment pointing at v2. With v2 optionally >>> unavailable altogether, this can't possibly be a generally valid >>> course of action. >> >> Could you explain me better that? Do you want to get rid of this comment? > > Especially the second sentence is misleading. If new code used v2, it > would not work on Xen with v2 support turned off. Can you suggest what to write here? I’m not very familiar with this xen Interface > >> /** >> * Version 1 of the grant table entry structure is maintained purely >> * for backwards compatibility. New guests should use version 2. >> */ >> >> In this case I don’t think this commit is the right place to do that, I can just >> put it back where it was so that the documentation simply doesn’t show that. > > Keeping misleading information out of the docs is imo rather desirable. > Of course we should, independently of that, also address the misleading > information in the source code. I can accept that doing the adjustment > right in this patch may not be ideal. I don't suppose I could talk you > into adding a prereq patch dropping at least that 2nd sentence? > I will push a patch to change this comment and I will rebase this serie on Top of that. Cheers, Luca > Jan
On 29.04.2021 11:50, Luca Fancellu wrote: >> On 29 Apr 2021, at 10:04, Jan Beulich <jbeulich@suse.com> wrote: >> On 28.04.2021 16:59, Luca Fancellu wrote: >>>> On 27 Apr 2021, at 14:57, Jan Beulich <jbeulich@suse.com> wrote: >>>> On 26.04.2021 17:37, Luca Fancellu wrote: >>>>> @@ -120,24 +132,26 @@ typedef uint32_t grant_ref_t; >>>>> * [GST]: This field is written by the guest and read by Xen. >>>>> */ >>>>> >>>>> -/* >>>>> - * Version 1 of the grant table entry structure is maintained purely >>>>> - * for backwards compatibility. New guests should use version 2. >>>>> - */ >>>>> #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. >>>>> + */ >>>> >>>> In case I didn't say so already before - I think this would be a good >>>> opportunity to drop the comment pointing at v2. With v2 optionally >>>> unavailable altogether, this can't possibly be a generally valid >>>> course of action. >>> >>> Could you explain me better that? Do you want to get rid of this comment? >> >> Especially the second sentence is misleading. If new code used v2, it >> would not work on Xen with v2 support turned off. > > Can you suggest what to write here? I’m not very familiar with this xen > Interface I've sent a patch myself just a few seconds ago. Jan
> On 29 Apr 2021, at 14:11, Jan Beulich <jbeulich@suse.com> wrote: > > On 29.04.2021 11:50, Luca Fancellu wrote: >>> On 29 Apr 2021, at 10:04, Jan Beulich <jbeulich@suse.com> wrote: >>> On 28.04.2021 16:59, Luca Fancellu wrote: >>>>> On 27 Apr 2021, at 14:57, Jan Beulich <jbeulich@suse.com> wrote: >>>>> On 26.04.2021 17:37, Luca Fancellu wrote: >>>>>> @@ -120,24 +132,26 @@ typedef uint32_t grant_ref_t; >>>>>> * [GST]: This field is written by the guest and read by Xen. >>>>>> */ >>>>>> >>>>>> -/* >>>>>> - * Version 1 of the grant table entry structure is maintained purely >>>>>> - * for backwards compatibility. New guests should use version 2. >>>>>> - */ >>>>>> #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. >>>>>> + */ >>>>> >>>>> In case I didn't say so already before - I think this would be a good >>>>> opportunity to drop the comment pointing at v2. With v2 optionally >>>>> unavailable altogether, this can't possibly be a generally valid >>>>> course of action. >>>> >>>> Could you explain me better that? Do you want to get rid of this comment? >>> >>> Especially the second sentence is misleading. If new code used v2, it >>> would not work on Xen with v2 support turned off. >> >> Can you suggest what to write here? I’m not very familiar with this xen >> Interface > > I've sent a patch myself just a few seconds ago. Hi Jan, That’s great, I’m going to push the v4 for this serie soon, now that the comment is fixed, I can include it in the docs too, do you agree? Cheers, Luca > > Jan
On 29.04.2021 15:15, Luca Fancellu wrote: > That’s great, I’m going to push the v4 for this serie soon, now that the comment is fixed, > I can include it in the docs too, do you agree? Of course (provided the patch will not get objected to). 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..9b5ccf07a4 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 */ +/** + * @file + * @brief Interface for granting foreign access to page frames, and receiving + * page-ownership transfers. + */ + #ifndef __XEN_PUBLIC_GRANT_TABLE_H__ #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,18 +104,23 @@ * 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. + * @addtogroup grant_table Grant Tables + * @{ */ -/* +/** * Reference to a grant entry in a specified domain's grant table. */ typedef uint32_t grant_ref_t; @@ -120,24 +132,26 @@ typedef uint32_t grant_ref_t; * [GST]: This field is written by the guest and read by Xen. */ -/* - * Version 1 of the grant table entry structure is maintained purely - * for backwards compatibility. New guests should use version 2. - */ #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; }; @@ -228,7 +242,7 @@ struct grant_entry_header { }; typedef struct grant_entry_header grant_entry_header_t; -/* +/** * Version 2 of the grant entry structure. */ union grant_entry_v2 { @@ -433,7 +447,7 @@ typedef struct gnttab_transfer gnttab_transfer_t; DEFINE_XEN_GUEST_HANDLE(gnttab_transfer_t); -/* +/** * 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,11 +465,6 @@ 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 { @@ -471,6 +480,12 @@ struct gnttab_copy { /* OUT parameters. */ int16_t status; }; + +#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) + typedef struct gnttab_copy gnttab_copy_t; DEFINE_XEN_GUEST_HANDLE(gnttab_copy_t); @@ -579,7 +594,7 @@ 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. */ @@ -588,8 +603,8 @@ struct gnttab_cache_flush { 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 */ + 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 +688,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> --- v3 changes: - removed tags to skip anonymous union/struct - moved back comment pointed out by Jan - moved down defines related to struct gnttab_copy as pointed out by Jan 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 | 75 ++++++++++++------- 4 files changed, 57 insertions(+), 28 deletions(-) create mode 100644 docs/hypercall-interfaces/arm64/grant_tables.rst