| Message ID | 20210406103603.8530-4-luca.fancellu@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Use Doxygen and sphinx for html documentation | expand |
On 06.04.2021 12:36, Luca Fancellu wrote: > Modification to include/public/grant_table.h: > > 1) Change anonymous structure to be named structure, > because doxygen can't deal with them. Especially in the form presented (adding further name space clutter for consumers to fall over) I object to this, most notably ... > @@ -584,7 +599,7 @@ DEFINE_XEN_GUEST_HANDLE(gnttab_swap_grant_ref_t); > * page granted to the calling domain by a foreign domain. > */ > struct gnttab_cache_flush { > - union { > + union a { ... this one. Jan
On Tue, 6 Apr 2021, Jan Beulich wrote: > On 06.04.2021 12:36, Luca Fancellu wrote: > > Modification to include/public/grant_table.h: > > > > 1) Change anonymous structure to be named structure, > > because doxygen can't deal with them. > > Especially in the form presented (adding further name space clutter > for consumers to fall over) I object to this, most notably ... > > > @@ -584,7 +599,7 @@ DEFINE_XEN_GUEST_HANDLE(gnttab_swap_grant_ref_t); > > * page granted to the calling domain by a foreign domain. > > */ > > struct gnttab_cache_flush { > > - union { > > + union a { > > ... this one. Hi Jan, It is unfortunate that none of these tools support anonymous structs or unions well. (You might recall we also had issues with the older kernel-doc series too, although a bit different.) It is difficult to provide a good name here, a suggestion would be more than welcome. I agree with you that calling it "a" is a bad idea: "a" becomes a globally-visible union name. Maybe we could call it: StructName_MemberName, so in this case: union gnttab_cache_flush_a It makes sure it is unique and doesn't risk clashing with anything else. We can follow this pattern elsewhere as well. Any better suggestions?
On 06.04.2021 23:46, Stefano Stabellini wrote: > On Tue, 6 Apr 2021, Jan Beulich wrote: >> On 06.04.2021 12:36, Luca Fancellu wrote: >>> Modification to include/public/grant_table.h: >>> >>> 1) Change anonymous structure to be named structure, >>> because doxygen can't deal with them. >> >> Especially in the form presented (adding further name space clutter >> for consumers to fall over) I object to this, most notably ... >> >>> @@ -584,7 +599,7 @@ DEFINE_XEN_GUEST_HANDLE(gnttab_swap_grant_ref_t); >>> * page granted to the calling domain by a foreign domain. >>> */ >>> struct gnttab_cache_flush { >>> - union { >>> + union a { >> >> ... this one. > > It is unfortunate that none of these tools support anonymous structs or > unions well. (You might recall we also had issues with the older > kernel-doc series too, although a bit different.) While I wouldn't veto such changes, I think it is a very bad approach to make adjustments like this to cover for a docs tool shortcoming. Is it entirely unreasonable to have the tool fixed? In fact, if the issue was run into before, isn't it a bad sign that the tool hasn't been fixed already, and we merely need to require a certain minimum version? > It is difficult to provide a good name here, a suggestion would be more > than welcome. I agree with you that calling it "a" is a bad idea: "a" > becomes a globally-visible union name. > > Maybe we could call it: StructName_MemberName, so in this case: > > union gnttab_cache_flush_a > > It makes sure it is unique and doesn't risk clashing with anything else. > We can follow this pattern elsewhere as well. > > Any better suggestions? First and foremost any new additions ought to use a xen_, Xen_, or XEN_ prefix. For the specific case here, since "a" is already a rather bad choice for a member name (What does it stand for? In lieu of any better name we typically use "u" in such cases.), the badness shouldn't be extended. Sadly the ref as being one way of expressing the target MFN is also not accompanied by a GFN (as it ought to be), but an address. Otherwise I would have suggested to abstract the similar union also used by struct gnttab_copy. In the end I can't see many alternatives to something like xen_gnttab_cache_flush_target or xen_gnttab_cache_flush_ref_or_addr. Jan
> On 7 Apr 2021, at 09:10, Jan Beulich <jbeulich@suse.com> wrote: > > On 06.04.2021 23:46, Stefano Stabellini wrote: >> On Tue, 6 Apr 2021, Jan Beulich wrote: >>> On 06.04.2021 12:36, Luca Fancellu wrote: >>>> Modification to include/public/grant_table.h: >>>> >>>> 1) Change anonymous structure to be named structure, >>>> because doxygen can't deal with them. >>> >>> Especially in the form presented (adding further name space clutter >>> for consumers to fall over) I object to this, most notably ... >>> >>>> @@ -584,7 +599,7 @@ DEFINE_XEN_GUEST_HANDLE(gnttab_swap_grant_ref_t); >>>> * page granted to the calling domain by a foreign domain. >>>> */ >>>> struct gnttab_cache_flush { >>>> - union { >>>> + union a { >>> >>> ... this one. >> >> It is unfortunate that none of these tools support anonymous structs or >> unions well. (You might recall we also had issues with the older >> kernel-doc series too, although a bit different.) > > While I wouldn't veto such changes, I think it is a very bad approach > to make adjustments like this to cover for a docs tool shortcoming. > Is it entirely unreasonable to have the tool fixed? In fact, if the > issue was run into before, isn't it a bad sign that the tool hasn't > been fixed already, and we merely need to require a certain minimum > version? > >> It is difficult to provide a good name here, a suggestion would be more >> than welcome. I agree with you that calling it "a" is a bad idea: "a" >> becomes a globally-visible union name. >> >> Maybe we could call it: StructName_MemberName, so in this case: >> >> union gnttab_cache_flush_a >> >> It makes sure it is unique and doesn't risk clashing with anything else. >> We can follow this pattern elsewhere as well. >> >> Any better suggestions? > > First and foremost any new additions ought to use a xen_, Xen_, or > XEN_ prefix. For the specific case here, since "a" is already a > rather bad choice for a member name (What does it stand for? In lieu > of any better name we typically use "u" in such cases.), the badness > shouldn't be extended. Sadly the ref as being one way of expressing > the target MFN is also not accompanied by a GFN (as it ought to be), > but an address. Otherwise I would have suggested to abstract the > similar union also used by struct gnttab_copy. In the end I can't > see many alternatives to something like xen_gnttab_cache_flush_target > or xen_gnttab_cache_flush_ref_or_addr. Hi Jan, Thank you for your feedback, I agree with you all that “a” is not really a good name, I will be happy to change it if we define a pattern. Just to be sure that we are in the same page, are you suggesting to modify the name In this way? struct gnttab_cache_flush { - union { + union xen_gnttab_cache_flush_a { uint64_t dev_bus_addr; grant_ref_t ref; } a; Following this kind of pattern: xen_<upper struct name>_<member name> ? Cheers, Luca > > Jan
On 07.04.2021 10:42, Luca Fancellu wrote: > Just to be sure that we are in the same page, are you suggesting to modify the name > In this way? > > struct gnttab_cache_flush { > - union { > + union xen_gnttab_cache_flush_a { > uint64_t dev_bus_addr; > grant_ref_t ref; > } a; > > Following this kind of pattern: xen_<upper struct name>_<member name> ? While in general I would be fine with this scheme, for field names like "a" or "u" it doesn't fit well imo. I'm also unconvinced this would be scalable to the case where there's further struct/union nesting. Jan
Hi, On 06/04/2021 11:36, Luca Fancellu wrote: > Modification to include/public/grant_table.h: > > 1) Change anonymous structure to be named structure, > because doxygen can't deal with them. What do you mean by can't deal with them? I had a quick try with doxygen build and couldn't find any failure (although I haven't looked at the output). Cheers,
> On 7 Apr 2021, at 14:13, Julien Grall <julien@xen.org> wrote: > > Hi, > > On 06/04/2021 11:36, Luca Fancellu wrote: >> Modification to include/public/grant_table.h: >> 1) Change anonymous structure to be named structure, >> because doxygen can't deal with them. > > What do you mean by can't deal with them? I had a quick try with doxygen build and couldn't find any failure (although I haven't looked at the output). > Hi Julien, The problem is that Doxygen can’t generate proper documentation for anonymous union/structure, it ends up with warning and/or producing wrong documentation like changing names or giving field description to the wrong field. Cheers, Luca > Cheers, > > -- > Julien Grall
Hi Luca, On 07/04/2021 14:19, Luca Fancellu wrote: > > >> On 7 Apr 2021, at 14:13, Julien Grall <julien@xen.org> wrote: >> >> Hi, >> >> On 06/04/2021 11:36, Luca Fancellu wrote: >>> Modification to include/public/grant_table.h: >>> 1) Change anonymous structure to be named structure, >>> because doxygen can't deal with them. >> >> What do you mean by can't deal with them? I had a quick try with doxygen build and couldn't find any failure (although I haven't looked at the output). >> > > Hi Julien, > > The problem is that Doxygen can’t generate proper documentation for anonymous union/structure, it ends up with warning and/or producing wrong documentation like > changing names or giving field description to the wrong field. I might do something wrong because I cannot spot any significant difference in the doxygen ouput if I switch back to anonymous union. Would you mind to post more details (such as a diff) on how doxygen doesn't generate properly documentation? Cheers,
> On 7 Apr 2021, at 14:56, Julien Grall <julien@xen.org> wrote: > > Hi Luca, > > On 07/04/2021 14:19, Luca Fancellu wrote: >>> On 7 Apr 2021, at 14:13, Julien Grall <julien@xen.org> wrote: >>> >>> Hi, >>> >>> On 06/04/2021 11:36, Luca Fancellu wrote: >>>> Modification to include/public/grant_table.h: >>>> 1) Change anonymous structure to be named structure, >>>> because doxygen can't deal with them. >>> >>> What do you mean by can't deal with them? I had a quick try with doxygen build and couldn't find any failure (although I haven't looked at the output). >>> >> Hi Julien, >> The problem is that Doxygen can’t generate proper documentation for anonymous union/structure, it ends up with warning and/or producing wrong documentation like >> changing names or giving field description to the wrong field. > > I might do something wrong because I cannot spot any significant difference in the doxygen ouput if I switch back to anonymous union. Would you mind to post more details (such as a diff) on how doxygen doesn't generate properly documentation? > Hi Julien, Here the explanation of the problem: https://vovkos.github.io/doxyrest/manual/unnamed-structs.html Clearly the proposed solution is not suitable because they are just hiding the anonymous union/struct field from the documentation. I did two test: 1) with the anonymous structure: struct gnttab_cache_flush { union { uint64_t dev_bus_addr; grant_ref_t ref; } a; I get a warning from sphinx (because the XML output of Doxygen is not in a good shape) when I generate the documentation, here follow the output: $ make -C docs XEN_TARGET_ARCH="arm64" sphinx-html make: Entering directory '/home/user/prj_xen/xen/docs' Generating xen.doxyfile mv xen.doxyfile.tmp xen.doxyfile Generating doxygen_input.h /usr/bin/doxygen xen.doxyfile Generating hypercall-interfaces/index.rst XEN_ROOT=/home/user/prj_xen/xen /usr/bin/sphinx-build -b html . sphinx/html Running Sphinx v1.6.7 making output directory... loading pickled environment... not yet created building [mo]: targets for 0 po files that are out of date building [html]: targets for 21 source files that are out of date updating environment: 21 added, 0 changed, 0 removed reading sources... [100%] misc/xen-makefiles/makefiles /home/user/prj_xen/xen/docs/hypercall-interfaces/arm64/grant_tables.rst:6: WARNING: Invalid definition: Expected end of definition. [error at 18] gnttab_cache_flush.a ------------------^ looking for now-outdated files... none found pickling environment... done checking consistency... /home/user/prj_xen/xen/docs/hypercall-interfaces/arm32.rst: WARNING: document isn't included in any toctree /home/user/prj_xen/xen/docs/hypercall-interfaces/x86_64.rst: WARNING: document isn't included in any toctree /home/user/prj_xen/xen/docs/misc/kconfig.rst: WARNING: document isn't included in any toctree /home/user/prj_xen/xen/docs/misc/kconfig-language.rst: WARNING: document isn't included in any toctree /home/user/prj_xen/xen/docs/misc/kconfig-macro-language.rst: WARNING: document isn't included in any toctree /home/user/prj_xen/xen/docs/misc/xen-makefiles/makefiles.rst: WARNING: document isn't included in any toctree done preparing documents... done writing output... [100%] misc/xen-makefiles/makefiles generating indices... genindex writing additional pages... search copying images... [100%] admin-guide/xen-overview.drawio.svg copying static files... done copying extra files... done dumping search index in English (code: en) ... done dumping object inventory... done build succeeded, 7 warnings. make: Leaving directory '/home/user/prj_xen/xen/docs’ And checking the generated html page html/hypercall-interfaces/arm64/grant_tables.html you can see that there is a union without name or fields right above "union __guest_handle_gnttab_cache_flush_t". 2) without the anonymous structure: struct gnttab_cache_flush { - union { + union a { uint64_t dev_bus_addr; grant_ref_t ref; } a; I don’t get the warning from sphinx and looking in the html/hypercall-interfaces/arm64/grant_tables.html page I can see the proper documentation for the union a right above "union __guest_handle_gnttab_cache_flush_t”. Let me know if you get different results. Cheers, Luca > Cheers, > > -- > Julien Grall
Luca Fancellu writes ("Re: [PATCH 3/3] docs/doxygen: doxygen documentation for grant_table.h"):
> The problem is that Doxygen can’t generate proper documentation for anonymous union/structure, it ends up with warning and/or producing wrong documentation like
> changing names or giving field description to the wrong field.
This does not seem like it would be an impossibly hard feature to add
to doxygen.
Ian.
Hi Ian, > On 7 Apr 2021, at 16:19, Ian Jackson <iwj@xenproject.org> wrote: > > Luca Fancellu writes ("Re: [PATCH 3/3] docs/doxygen: doxygen documentation for grant_table.h"): >> The problem is that Doxygen can’t generate proper documentation for anonymous union/structure, it ends up with warning and/or producing wrong documentation like >> changing names or giving field description to the wrong field. > > This does not seem like it would be an impossibly hard feature to add > to doxygen. Modifying doxygen is not really in our planned efforts and if someone does it that would put an hard constraint on the version of doxygen possible to use. But is adding names to anonymous elements really an issue here ? If we agree on names or on a convention for name the result will not impact the code or backward compatibility. Regards Bertrand > > Ian.
On 07.04.2021 17:29, Bertrand Marquis wrote: >> On 7 Apr 2021, at 16:19, Ian Jackson <iwj@xenproject.org> wrote: >> >> Luca Fancellu writes ("Re: [PATCH 3/3] docs/doxygen: doxygen documentation for grant_table.h"): >>> The problem is that Doxygen can’t generate proper documentation for anonymous union/structure, it ends up with warning and/or producing wrong documentation like >>> changing names or giving field description to the wrong field. >> >> This does not seem like it would be an impossibly hard feature to add >> to doxygen. > > Modifying doxygen is not really in our planned efforts and if someone does it that would put an hard constraint on the version of doxygen possible to use. > > But is adding names to anonymous elements really an issue here ? It's clutter in the code base, making things harder to read (even if just slightly). It's certainly odd to make such source changes just for a doc tool. If changing doxygen is not an option for you, how about pre-processing the header and inserting the names the tool wants, before handing the result as input to it? Jan
On 07/04/2021 16:29, Bertrand Marquis wrote: > Hi Ian, > >> On 7 Apr 2021, at 16:19, Ian Jackson <iwj@xenproject.org> wrote: >> >> Luca Fancellu writes ("Re: [PATCH 3/3] docs/doxygen: doxygen documentation for grant_table.h"): >>> The problem is that Doxygen can’t generate proper documentation for anonymous union/structure, it ends up with warning and/or producing wrong documentation like >>> changing names or giving field description to the wrong field. >> >> This does not seem like it would be an impossibly hard feature to add >> to doxygen. > > Modifying doxygen is not really in our planned efforts and if someone does it that would put an hard constraint on the version of doxygen possible to use. Are you saying that anyone who want to use doxygen has to waive off the use of anonymous union/struct? Is it the only thing doxygen can't deal with? > But is adding names to anonymous elements really an issue here ? > If we agree on names or on a convention for name the result will not impact the code or backward compatibility. I think the naming is only the tip of the problem. One advantage of anymous union/struct is you make clear they are not meant to be used outside of the context. So they should mostly be seen as an easy way to access some part of the "parent" structure directly. Therefore, IMHO, they don't deserve to be documented separately. In fact, this is the first thing I noticed when building the documentation because 'union a' was in global index. Cheers,
Hi Julien, > On 7 Apr 2021, at 16:55, Julien Grall <julien@xen.org> wrote: > > > > On 07/04/2021 16:29, Bertrand Marquis wrote: >> Hi Ian, >>> On 7 Apr 2021, at 16:19, Ian Jackson <iwj@xenproject.org> wrote: >>> >>> Luca Fancellu writes ("Re: [PATCH 3/3] docs/doxygen: doxygen documentation for grant_table.h"): >>>> The problem is that Doxygen can’t generate proper documentation for anonymous union/structure, it ends up with warning and/or producing wrong documentation like >>>> changing names or giving field description to the wrong field. >>> >>> This does not seem like it would be an impossibly hard feature to add >>> to doxygen. >> Modifying doxygen is not really in our planned efforts and if someone does it that would put an hard constraint on the version of doxygen possible to use. > > Are you saying that anyone who want to use doxygen has to waive off the use of anonymous union/struct? Is it the only thing doxygen can't deal with? That is the main one we came into while doing this but there might be other going forward, hard to be sure at this stage. > >> But is adding names to anonymous elements really an issue here ? >> If we agree on names or on a convention for name the result will not impact the code or backward compatibility. > > I think the naming is only the tip of the problem. One advantage of anymous union/struct is you make clear they are not meant to be used outside of the context. So they should mostly be seen as an easy way to access some part of the "parent" structure directly. Therefore, IMHO, they don't deserve to be documented separately. Somehow in the documentation when you have a union you will need to document that it is a union and the possible entries. Having a name to refer to it sounds to me a lot easier than making it anonymous. One way or an other most standards like MISRA are forbidding anonymous entries as they cannot be referred to. > > In fact, this is the first thing I noticed when building the documentation because 'union a' was in global index. Definitely I agree the “a” is not a good solution and we need to find meaningful names. But this is in fact true for the sub-element in the structure (from which the name was taken), using “a” as an identifier is not really explanatory of what that is. “u” for union can be see as a standard though. This is why i think we should put names which a meaning but this is not always easy to find. Cheers Bertrand > > Cheers, > > -- > Julien Grall
Hi, > On 7 Apr 2021, at 16:54, Jan Beulich <jbeulich@suse.com> wrote: > > On 07.04.2021 17:29, Bertrand Marquis wrote: >>> On 7 Apr 2021, at 16:19, Ian Jackson <iwj@xenproject.org> wrote: >>> >>> Luca Fancellu writes ("Re: [PATCH 3/3] docs/doxygen: doxygen documentation for grant_table.h"): >>>> The problem is that Doxygen can’t generate proper documentation for anonymous union/structure, it ends up with warning and/or producing wrong documentation like >>>> changing names or giving field description to the wrong field. >>> >>> This does not seem like it would be an impossibly hard feature to add >>> to doxygen. >> >> Modifying doxygen is not really in our planned efforts and if someone does it that would put an hard constraint on the version of doxygen possible to use. >> >> But is adding names to anonymous elements really an issue here ? > > It's clutter in the code base, making things harder to read (even if > just slightly). It's certainly odd to make such source changes just > for a doc tool. If changing doxygen is not an option for you, how > about pre-processing the header and inserting the names the tool > wants, before handing the result as input to it? Introducing a new tool or find a pre-processing solution is for sure possible but will be more complex and less error prone. Also as said in my mail to Julien, passing this issue of anonymous entries now might just push the problem as we might have to solve it later if we want to become MISRA compliant (which is also something we are looking in FuSa). Regards Bertrand > > Jan >
Bertrand Marquis writes ("Re: [PATCH 3/3] docs/doxygen: doxygen documentation for grant_table.h"):
> Somehow in the documentation when you have a union you will need to document that it is a union and the possible entries.
I would expect to find the documentation for an anonymous struct or
union folded into the documentation of the containing structure, just
as it is in the source.
> One way or an other most standards like MISRA are forbidding anonymous entries as they cannot be referred to.
An anonymous union or struct like this is always the type of a single
field in a containing aggegate type. So if one needs to speak of it,
one can specify the container's type and the field. So it *can* be
named.
I am assuming we don't have *unused* anonymous structs and unions.
Ian.
On Wed, 7 Apr 2021, Jan Beulich wrote: > On 07.04.2021 10:42, Luca Fancellu wrote: > > Just to be sure that we are in the same page, are you suggesting to modify the name > > In this way? > > > > struct gnttab_cache_flush { > > - union { > > + union xen_gnttab_cache_flush_a { > > uint64_t dev_bus_addr; > > grant_ref_t ref; > > } a; > > > > Following this kind of pattern: xen_<upper struct name>_<member name> ? > > While in general I would be fine with this scheme, for field names like > "a" or "u" it doesn't fit well imo. "a" is a bad name anyway, even for the member. We can take the opportunity to find a better name. Almost anything would be better than "a". Maybe "refaddr"? > I'm also unconvinced this would be > scalable to the case where there's further struct/union nesting. How many of these instances of multilevel nesting do we have? Luca might know. Probably not many? They could be special-cased.
On 07.04.2021 23:26, Stefano Stabellini wrote: > On Wed, 7 Apr 2021, Jan Beulich wrote: >> On 07.04.2021 10:42, Luca Fancellu wrote: >>> Just to be sure that we are in the same page, are you suggesting to modify the name >>> In this way? >>> >>> struct gnttab_cache_flush { >>> - union { >>> + union xen_gnttab_cache_flush_a { >>> uint64_t dev_bus_addr; >>> grant_ref_t ref; >>> } a; >>> >>> Following this kind of pattern: xen_<upper struct name>_<member name> ? >> >> While in general I would be fine with this scheme, for field names like >> "a" or "u" it doesn't fit well imo. > > "a" is a bad name anyway, even for the member. We can take the > opportunity to find a better name. Almost anything would be better than > "a". Maybe "refaddr"? We need to be careful with changing _anything_ in the public interface. Consumers importing our headers directly (as was e.g. done for the old linux-2.6.18-xen.hg tree) could break with a field name change as much as with any other changes to what had been made available to them. Jan
> On 7 Apr 2021, at 22:26, Stefano Stabellini <sstabellini@kernel.org> wrote: > > On Wed, 7 Apr 2021, Jan Beulich wrote: >> On 07.04.2021 10:42, Luca Fancellu wrote: >>> Just to be sure that we are in the same page, are you suggesting to modify the name >>> In this way? >>> >>> struct gnttab_cache_flush { >>> - union { >>> + union xen_gnttab_cache_flush_a { >>> uint64_t dev_bus_addr; >>> grant_ref_t ref; >>> } a; >>> >>> Following this kind of pattern: xen_<upper struct name>_<member name> ? >> >> While in general I would be fine with this scheme, for field names like >> "a" or "u" it doesn't fit well imo. > > "a" is a bad name anyway, even for the member. We can take the > opportunity to find a better name. Almost anything would be better than > "a". Maybe "refaddr"? > > >> I'm also unconvinced this would be >> scalable to the case where there's further struct/union nesting. > > How many of these instances of multilevel nesting do we have? Luca might > know. Probably not many? They could be special-cased. There are not many multilevel nesting instances of anonymous struct/union and the maximum level of nesting I found in the public headers is 2: union { union/struct { … } <name> } <name> I also see that in the majority of cases the unions have not meaningful names like “a” or “u” as member name, instead struct names are fine, It could be fine to keep the meaningful name the same for the struct type name and use the pattern for the non-meaningful ones as long as the names doesn’t create compilation errors? Example: struct upper_level { union { struct { } meaningful_name1; struct { } meaningful_name2; } u; }; becomes: struct upper_level { union upper_level_u { struct meaningful_name1 { } meaningful_name1; struct meaningful_name2 { } meaningful_name2; } u; }; Doing this will help a lot the documentation side because the html page will show the "struct upper_level" with inside the “union upper_level_u" and inside again the two struct “meaningful_name1" and “meaningful_name2", and from the point of view of the developer it can tell her/him exactly the name of the member to access when writing code (apart from the upper_level_u that can be accessed by u, but we can’t clearly change it). If this sounds difficult to understand when reading, please generate the documentation and have a look on the page in one side and the source code in another. Moreover the change should be back-compatible since we are not changing member names. Cheers, Luca
On 08.04.2021 13:02, Luca Fancellu wrote: > > >> On 7 Apr 2021, at 22:26, Stefano Stabellini <sstabellini@kernel.org> wrote: >> >> On Wed, 7 Apr 2021, Jan Beulich wrote: >>> On 07.04.2021 10:42, Luca Fancellu wrote: >>>> Just to be sure that we are in the same page, are you suggesting to modify the name >>>> In this way? >>>> >>>> struct gnttab_cache_flush { >>>> - union { >>>> + union xen_gnttab_cache_flush_a { >>>> uint64_t dev_bus_addr; >>>> grant_ref_t ref; >>>> } a; >>>> >>>> Following this kind of pattern: xen_<upper struct name>_<member name> ? >>> >>> While in general I would be fine with this scheme, for field names like >>> "a" or "u" it doesn't fit well imo. >> >> "a" is a bad name anyway, even for the member. We can take the >> opportunity to find a better name. Almost anything would be better than >> "a". Maybe "refaddr"? >> >> >>> I'm also unconvinced this would be >>> scalable to the case where there's further struct/union nesting. >> >> How many of these instances of multilevel nesting do we have? Luca might >> know. Probably not many? They could be special-cased. > > There are not many multilevel nesting instances of anonymous struct/union and the maximum level of nesting I found in the public headers is 2: > > union { > union/struct { > … > } <name> > } <name> > > I also see that in the majority of cases the unions have not meaningful names like “a” or “u” as member name, instead struct names are fine, > It could be fine to keep the meaningful name the same for the struct type name and use the pattern for the non-meaningful ones as long > as the names doesn’t create compilation errors? > > Example: > > struct upper_level { > union { > struct { > > } meaningful_name1; > struct { > > } meaningful_name2; > } u; > }; > > becomes: > > struct upper_level { > union upper_level_u { > struct meaningful_name1 { > > } meaningful_name1; > struct meaningful_name2 { > > } meaningful_name2; > } u; > }; As you say - as long as there aren't any compilation issues. Two top level struct-s could have identically named struct/union fields without tag, in which case your approach outlined above will fail. And even if there was no such case right now, the case would need to be covered in whatever naming model was to be used (except, of course, the one without any names). Jan
Hi Luca, On 08/04/2021 12:02, Luca Fancellu wrote: > > >> On 7 Apr 2021, at 22:26, Stefano Stabellini <sstabellini@kernel.org> wrote: >> >> On Wed, 7 Apr 2021, Jan Beulich wrote: >>> On 07.04.2021 10:42, Luca Fancellu wrote: >>>> Just to be sure that we are in the same page, are you suggesting to modify the name >>>> In this way? >>>> >>>> struct gnttab_cache_flush { >>>> - union { >>>> + union xen_gnttab_cache_flush_a { >>>> uint64_t dev_bus_addr; >>>> grant_ref_t ref; >>>> } a; >>>> >>>> Following this kind of pattern: xen_<upper struct name>_<member name> ? >>> >>> While in general I would be fine with this scheme, for field names like >>> "a" or "u" it doesn't fit well imo. >> >> "a" is a bad name anyway, even for the member. We can take the >> opportunity to find a better name. Almost anything would be better than >> "a". Maybe "refaddr"? >> >> >>> I'm also unconvinced this would be >>> scalable to the case where there's further struct/union nesting. >> >> How many of these instances of multilevel nesting do we have? Luca might >> know. Probably not many? They could be special-cased. > > There are not many multilevel nesting instances of anonymous struct/union and the maximum level of nesting I found in the public headers is 2: > > union { > union/struct { > … > } <name> > } <name> > > I also see that in the majority of cases the unions have not meaningful names like “a” or “u” as member name, instead struct names are fine, > It could be fine to keep the meaningful name the same for the struct type name and use the pattern for the non-meaningful ones as long > as the names doesn’t create compilation errors? > > Example: > > struct upper_level { > union { > struct { > > } meaningful_name1; > struct { > > } meaningful_name2; > } u; > }; > > becomes: > > struct upper_level { > union upper_level_u { > struct meaningful_name1 { > > } meaningful_name1; > struct meaningful_name2 { > > } meaningful_name2; > } u; > }; If I understand correctly your proposal, the name of the structure would be the name of the field. The name of the fields are usually pretty generic so you will likely end up to redefine the structure name. Unless we want to provide random name, the only safe naming would be to define the structure as upper_level_u_meaningful_name{1, 2}. But, this is going to be pretty awful to read. But I am still a bit puzzled by the fact doxygen is not capable to deal with anynomous/unamed union. How do other projects deal with them? > > Doing this will help a lot the documentation side because the html page will show the "struct upper_level" with inside the “union upper_level_u" and inside again > the two struct “meaningful_name1" and “meaningful_name2", and from the point of view of the developer it can tell her/him exactly the name of the member to > access when writing code (apart from the upper_level_u that can be accessed by u, but we can’t clearly change it). I don't quite understand the last point. Wouldn't the developper see the field name? So how is it going to be different from seeing the structure name? > If this sounds difficult to understand when reading, please generate the documentation and have a look on the page in one side and the source code in another. Just to be clear, do you mean understanding what you wrote or a developper trying to understand the code? Cheers,
On 08/04/2021 12:40, Julien Grall wrote: > Hi Luca, > > On 08/04/2021 12:02, Luca Fancellu wrote: >> >> >>> On 7 Apr 2021, at 22:26, Stefano Stabellini <sstabellini@kernel.org> >>> wrote: >>> >>> On Wed, 7 Apr 2021, Jan Beulich wrote: >>>> On 07.04.2021 10:42, Luca Fancellu wrote: >>>>> Just to be sure that we are in the same page, are you suggesting to >>>>> modify the name >>>>> In this way? >>>>> >>>>> struct gnttab_cache_flush { >>>>> - union { >>>>> + union xen_gnttab_cache_flush_a { >>>>> uint64_t dev_bus_addr; >>>>> grant_ref_t ref; >>>>> } a; >>>>> >>>>> Following this kind of pattern: xen_<upper struct name>_<member >>>>> name> ? >>>> >>>> While in general I would be fine with this scheme, for field names like >>>> "a" or "u" it doesn't fit well imo. >>> >>> "a" is a bad name anyway, even for the member. We can take the >>> opportunity to find a better name. Almost anything would be better than >>> "a". Maybe "refaddr"? >>> >>> >>>> I'm also unconvinced this would be >>>> scalable to the case where there's further struct/union nesting. >>> >>> How many of these instances of multilevel nesting do we have? Luca might >>> know. Probably not many? They could be special-cased. >> >> There are not many multilevel nesting instances of anonymous >> struct/union and the maximum level of nesting I found in the public >> headers is 2: >> >> union { >> union/struct { >> … >> } <name> >> } <name> >> >> I also see that in the majority of cases the unions have not >> meaningful names like “a” or “u” as member name, instead struct names >> are fine, >> It could be fine to keep the meaningful name the same for the struct >> type name and use the pattern for the non-meaningful ones as long >> as the names doesn’t create compilation errors? >> >> Example: >> >> struct upper_level { >> union { >> struct { >> >> } meaningful_name1; >> struct { >> >> } meaningful_name2; >> } u; >> }; >> >> becomes: >> >> struct upper_level { >> union upper_level_u { >> struct meaningful_name1 { >> >> } meaningful_name1; >> struct meaningful_name2 { >> >> } meaningful_name2; >> } u; >> }; > > If I understand correctly your proposal, the name of the structure would > be the name of the field. The name of the fields are usually pretty > generic so you will likely end up to redefine the structure name. > > Unless we want to provide random name, the only safe naming would be to > define the structure as upper_level_u_meaningful_name{1, 2}. But, this > is going to be pretty awful to read. > > But I am still a bit puzzled by the fact doxygen is not capable to deal > with anynomous/unamed union. How do other projects deal with them? While going through the list of anynomous union in Xen, I noticed we also have something like: struct test { union { int a; int b; }; }; We can't name them because of syntactic reasons. What's your plan for them? Cheers,
> On 8 Apr 2021, at 12:40, Julien Grall <julien@xen.org> wrote: > > Hi Luca, > > On 08/04/2021 12:02, Luca Fancellu wrote: >>> On 7 Apr 2021, at 22:26, Stefano Stabellini <sstabellini@kernel.org> wrote: >>> >>> On Wed, 7 Apr 2021, Jan Beulich wrote: >>>> On 07.04.2021 10:42, Luca Fancellu wrote: >>>>> Just to be sure that we are in the same page, are you suggesting to modify the name >>>>> In this way? >>>>> >>>>> struct gnttab_cache_flush { >>>>> - union { >>>>> + union xen_gnttab_cache_flush_a { >>>>> uint64_t dev_bus_addr; >>>>> grant_ref_t ref; >>>>> } a; >>>>> >>>>> Following this kind of pattern: xen_<upper struct name>_<member name> ? >>>> >>>> While in general I would be fine with this scheme, for field names like >>>> "a" or "u" it doesn't fit well imo. >>> >>> "a" is a bad name anyway, even for the member. We can take the >>> opportunity to find a better name. Almost anything would be better than >>> "a". Maybe "refaddr"? >>> >>> >>>> I'm also unconvinced this would be >>>> scalable to the case where there's further struct/union nesting. >>> >>> How many of these instances of multilevel nesting do we have? Luca might >>> know. Probably not many? They could be special-cased. >> There are not many multilevel nesting instances of anonymous struct/union and the maximum level of nesting I found in the public headers is 2: >> union { >> union/struct { >> … >> } <name> >> } <name> >> I also see that in the majority of cases the unions have not meaningful names like “a” or “u” as member name, instead struct names are fine, >> It could be fine to keep the meaningful name the same for the struct type name and use the pattern for the non-meaningful ones as long >> as the names doesn’t create compilation errors? >> Example: >> struct upper_level { >> union { >> struct { >> >> } meaningful_name1; >> struct { >> >> } meaningful_name2; >> } u; >> }; >> becomes: >> struct upper_level { >> union upper_level_u { >> struct meaningful_name1 { >> >> } meaningful_name1; >> struct meaningful_name2 { >> >> } meaningful_name2; >> } u; >> }; > > If I understand correctly your proposal, the name of the structure would be the name of the field. The name of the fields are usually pretty generic so you will likely end up to redefine the structure name. > > Unless we want to provide random name, the only safe naming would be to define the structure as upper_level_u_meaningful_name{1, 2}. But, this is going to be pretty awful to read. > > But I am still a bit puzzled by the fact doxygen is not capable to deal with anynomous/unamed union. How do other projects deal with them? > >> Doing this will help a lot the documentation side because the html page will show the "struct upper_level" with inside the “union upper_level_u" and inside again >> the two struct “meaningful_name1" and “meaningful_name2", and from the point of view of the developer it can tell her/him exactly the name of the member to >> access when writing code (apart from the upper_level_u that can be accessed by u, but we can’t clearly change it). > > I don't quite understand the last point. Wouldn't the developper see the field name? So how is it going to be different from seeing the structure name? The developer, that is using the documentation generated with sphinx+doxygen, will see the structure name and not the field name because this is the way sphinx+doxygen is rendering the code structures. You can see it in the generated documentation using this serie. > >> If this sounds difficult to understand when reading, please generate the documentation and have a look on the page in one side and the source code in another. > > Just to be clear, do you mean understanding what you wrote or a developper trying to understand the code? I meant understanding what I wrote, because I know it’s difficult to describe the concept without visualising the html page, it would have been much easier to attach an image to clarify. > > Cheers, > > -- > Julien Grall
> On 8 Apr 2021, at 12:50, Julien Grall <julien@xen.org> wrote: > > > > On 08/04/2021 12:40, Julien Grall wrote: >> Hi Luca, >> On 08/04/2021 12:02, Luca Fancellu wrote: >>> >>> >>>> On 7 Apr 2021, at 22:26, Stefano Stabellini <sstabellini@kernel.org> wrote: >>>> >>>> On Wed, 7 Apr 2021, Jan Beulich wrote: >>>>> On 07.04.2021 10:42, Luca Fancellu wrote: >>>>>> Just to be sure that we are in the same page, are you suggesting to modify the name >>>>>> In this way? >>>>>> >>>>>> struct gnttab_cache_flush { >>>>>> - union { >>>>>> + union xen_gnttab_cache_flush_a { >>>>>> uint64_t dev_bus_addr; >>>>>> grant_ref_t ref; >>>>>> } a; >>>>>> >>>>>> Following this kind of pattern: xen_<upper struct name>_<member name> ? >>>>> >>>>> While in general I would be fine with this scheme, for field names like >>>>> "a" or "u" it doesn't fit well imo. >>>> >>>> "a" is a bad name anyway, even for the member. We can take the >>>> opportunity to find a better name. Almost anything would be better than >>>> "a". Maybe "refaddr"? >>>> >>>> >>>>> I'm also unconvinced this would be >>>>> scalable to the case where there's further struct/union nesting. >>>> >>>> How many of these instances of multilevel nesting do we have? Luca might >>>> know. Probably not many? They could be special-cased. >>> >>> There are not many multilevel nesting instances of anonymous struct/union and the maximum level of nesting I found in the public headers is 2: >>> >>> union { >>> union/struct { >>> … >>> } <name> >>> } <name> >>> >>> I also see that in the majority of cases the unions have not meaningful names like “a” or “u” as member name, instead struct names are fine, >>> It could be fine to keep the meaningful name the same for the struct type name and use the pattern for the non-meaningful ones as long >>> as the names doesn’t create compilation errors? >>> >>> Example: >>> >>> struct upper_level { >>> union { >>> struct { >>> >>> } meaningful_name1; >>> struct { >>> >>> } meaningful_name2; >>> } u; >>> }; >>> >>> becomes: >>> >>> struct upper_level { >>> union upper_level_u { >>> struct meaningful_name1 { >>> >>> } meaningful_name1; >>> struct meaningful_name2 { >>> >>> } meaningful_name2; >>> } u; >>> }; >> If I understand correctly your proposal, the name of the structure would be the name of the field. The name of the fields are usually pretty generic so you will likely end up to redefine the structure name. >> Unless we want to provide random name, the only safe naming would be to define the structure as upper_level_u_meaningful_name{1, 2}. But, this is going to be pretty awful to read. >> But I am still a bit puzzled by the fact doxygen is not capable to deal with anynomous/unamed union. How do other projects deal with them? > > While going through the list of anynomous union in Xen, I noticed we also have something like: > > struct test { > union { > int a; > int b; > }; > }; > > We can't name them because of syntactic reasons. What's your plan for them? I would say that if the fields a and b are not meant to be described in the documentation, they can be hidden, so there is no need to change the structure itself but we might just add some comment containing Doxygen tags for skipping them. In Zephyr they have some kind of structures like that. > > Cheers, > > -- > Julien Grall
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..b506e09ed3 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,7 +258,7 @@ 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. */ - struct { + struct full_page { grant_entry_header_t hdr; uint32_t pad0; uint64_t frame; @@ -254,7 +269,7 @@ union grant_entry_v2 { * @domid is allowed to access bytes [@page_off,@page_off+@length) * in frame @frame. */ - struct { + struct sub_page { grant_entry_header_t hdr; uint16_t page_off; uint16_t length; @@ -270,7 +285,7 @@ union grant_entry_v2 { * The current version of Xen does not allow transitive grants * to be mapped. */ - struct { + struct transitive { grant_entry_header_t hdr; domid_t trans_domid; uint16_t pad0; @@ -459,7 +474,7 @@ DEFINE_XEN_GUEST_HANDLE(gnttab_transfer_t); struct gnttab_copy { /* IN parameters. */ struct gnttab_copy_ptr { - union { + union gnttab_copy_ptr_u { grant_ref_t ref; xen_pfn_t gmfn; } u; @@ -584,7 +599,7 @@ DEFINE_XEN_GUEST_HANDLE(gnttab_swap_grant_ref_t); * page granted to the calling domain by a foreign domain. */ struct gnttab_cache_flush { - union { + union a { uint64_t dev_bus_addr; grant_ref_t ref; } a; @@ -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) Change anonymous structure to be named structure, because doxygen can't deal with them. 2) Add doxygen tags to: - Create Grant tables section - include variables in the generated documentation 3) Add .rst file for grant table for Arm64 Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> --- docs/hypercall-interfaces/arm64.rst | 1 + .../arm64/grant_tables.rst | 8 ++ docs/xen-doxygen/doxy_input.list | 1 + xen/include/public/grant_table.h | 79 ++++++++++++------- 4 files changed, 59 insertions(+), 30 deletions(-) create mode 100644 docs/hypercall-interfaces/arm64/grant_tables.rst