| Message ID | 20230227095315.6483-1-michal.orzel@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | build: add crypto/ to SUBDIRS | expand |
On 27.02.2023 10:53, Michal Orzel wrote: > --- a/xen/Makefile > +++ b/xen/Makefile > @@ -589,7 +589,7 @@ $(TARGET): outputmakefile FORCE > $(Q)$(MAKE) $(build)=. arch/$(TARGET_ARCH)/include/asm/asm-offsets.h > $(Q)$(MAKE) $(build)=. MKRELOC=$(MKRELOC) 'ALL_OBJS=$(ALL_OBJS-y)' 'ALL_LIBS=$(ALL_LIBS-y)' $@ > > -SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test > +SUBDIRS = xsm arch/$(TARGET_ARCH) common crypto drivers lib test > define all_sources > ( find include -type f -name '*.h' -print; \ > find $(SUBDIRS) -type f -name '*.[chS]' -print ) As long as it's arch/$(TARGET_ARCH) that's used here, crypto should imo also only be included when selected (or at the very least only when an arch might select it, which afaics is possible on x86 only right now). It would also help if in the description you made explicit that SUBDIRS isn't used for anything else (the name, after all, is pretty generic). Which actually points at an issue: I suppose the variable would actually better be used elsewhere as well, e.g. in the _clean: rule and perhaps also in the setting of ALL_OBJS-y. (That'll require splitting the variable, to that e.g. _clean would use $(SUBDIRS), $(SUBDIRS-y), and $(SUBDIRS-) collectively.) It is, imo, that lack of consolidation which has caused crypto to be missing from SUBDIRS. Jan
Hi Jan, On 27/02/2023 11:10, Jan Beulich wrote: > > > On 27.02.2023 10:53, Michal Orzel wrote: >> --- a/xen/Makefile >> +++ b/xen/Makefile >> @@ -589,7 +589,7 @@ $(TARGET): outputmakefile FORCE >> $(Q)$(MAKE) $(build)=. arch/$(TARGET_ARCH)/include/asm/asm-offsets.h >> $(Q)$(MAKE) $(build)=. MKRELOC=$(MKRELOC) 'ALL_OBJS=$(ALL_OBJS-y)' 'ALL_LIBS=$(ALL_LIBS-y)' $@ >> >> -SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test >> +SUBDIRS = xsm arch/$(TARGET_ARCH) common crypto drivers lib test >> define all_sources >> ( find include -type f -name '*.h' -print; \ >> find $(SUBDIRS) -type f -name '*.[chS]' -print ) > > As long as it's arch/$(TARGET_ARCH) that's used here, crypto should imo > also only be included when selected (or at the very least only when an > arch might select it, which afaics is possible on x86 only right now). > > It would also help if in the description you made explicit that SUBDIRS > isn't used for anything else (the name, after all, is pretty generic). > Which actually points at an issue: I suppose the variable would actually > better be used elsewhere as well, e.g. in the _clean: rule and perhaps > also in the setting of ALL_OBJS-y. (That'll require splitting the > variable, to that e.g. _clean would use $(SUBDIRS), $(SUBDIRS-y), and > $(SUBDIRS-) collectively.) It is, imo, that lack of consolidation which > has caused crypto to be missing from SUBDIRS. > I think what you wrote can be split into 2 parts: the part being a goal of this patch and the cleanup/improvements that would be beneficial but not related to this patch. The second part involves more code and there are parts to be discussed: 1) If we decide to create ALL_OBJS-y from SUBDIRS, then we would need to carve out test/ dir that is not part of ALL_OBJS-y and add it to SUBDIRS later on. Also, the order of ALL_OBJS-y matters for linking, so we would need to transfer the responsibility to SUBDIRS. The natural placement of SUBDIRS (including SUBDIRS-y, etc.) would be right above ALL_OBJS-y. However, when doing clean (next point), need-config is set to n and SUBDIRS would be empty. This means it would need to be defined somewhere at the top of the Makefile (thus harder to make sure the linking order is correct). 2) If we deicide to use SUBDIRS for _clean rule, then we would need some foreach loop, right? Apart from that, there are other directories that are not part of SUBDIRS i.e. include/, tools/. Together with SUBDIRS variable, it would create confusion (these dirs are also sub-directories, so why not having them listed in this variable?). Also, I can see that we do clean not only for $(TARGET_ARCH) but for all the existing architectures. I would prefer to stick for now to the goal of this patch which is to add crypto/ so that it is taken into account for the tags/csope generation. Would the following change be ok for that purpose? diff --git a/xen/Makefile b/xen/Makefile index 2d55bb9401f4..05bf301bd7ab 100644 --- a/xen/Makefile +++ b/xen/Makefile @@ -589,7 +589,9 @@ $(TARGET): outputmakefile FORCE $(Q)$(MAKE) $(build)=. arch/$(TARGET_ARCH)/include/asm/asm-offsets.h $(Q)$(MAKE) $(build)=. MKRELOC=$(MKRELOC) 'ALL_OBJS=$(ALL_OBJS-y)' 'ALL_LIBS=$(ALL_LIBS-y)' $@ -SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test +SUBDIRS-$(CONFIG_CRYPTO) += crypto +SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test $(SUBDIRS-y) + define all_sources ( find include -type f -name '*.h' -print; \ find $(SUBDIRS) -type f -name '*.[chS]' -print ) ~Michal
On 27.02.2023 14:41, Michal Orzel wrote: > Hi Jan, > > On 27/02/2023 11:10, Jan Beulich wrote: >> >> >> On 27.02.2023 10:53, Michal Orzel wrote: >>> --- a/xen/Makefile >>> +++ b/xen/Makefile >>> @@ -589,7 +589,7 @@ $(TARGET): outputmakefile FORCE >>> $(Q)$(MAKE) $(build)=. arch/$(TARGET_ARCH)/include/asm/asm-offsets.h >>> $(Q)$(MAKE) $(build)=. MKRELOC=$(MKRELOC) 'ALL_OBJS=$(ALL_OBJS-y)' 'ALL_LIBS=$(ALL_LIBS-y)' $@ >>> >>> -SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test >>> +SUBDIRS = xsm arch/$(TARGET_ARCH) common crypto drivers lib test >>> define all_sources >>> ( find include -type f -name '*.h' -print; \ >>> find $(SUBDIRS) -type f -name '*.[chS]' -print ) >> >> As long as it's arch/$(TARGET_ARCH) that's used here, crypto should imo >> also only be included when selected (or at the very least only when an >> arch might select it, which afaics is possible on x86 only right now). >> >> It would also help if in the description you made explicit that SUBDIRS >> isn't used for anything else (the name, after all, is pretty generic). >> Which actually points at an issue: I suppose the variable would actually >> better be used elsewhere as well, e.g. in the _clean: rule and perhaps >> also in the setting of ALL_OBJS-y. (That'll require splitting the >> variable, to that e.g. _clean would use $(SUBDIRS), $(SUBDIRS-y), and >> $(SUBDIRS-) collectively.) It is, imo, that lack of consolidation which >> has caused crypto to be missing from SUBDIRS. >> > I think what you wrote can be split into 2 parts: the part being a goal of this patch > and the cleanup/improvements that would be beneficial but not related to this patch. > The second part involves more code and there are parts to be discussed: > > 1) If we decide to create ALL_OBJS-y from SUBDIRS, then we would need to carve out test/ dir > that is not part of ALL_OBJS-y and add it to SUBDIRS later on. Also, the order of ALL_OBJS-y matters > for linking, so we would need to transfer the responsibility to SUBDIRS. The natural placement of > SUBDIRS (including SUBDIRS-y, etc.) would be right above ALL_OBJS-y. However, when doing clean (next point), > need-config is set to n and SUBDIRS would be empty. This means it would need to be defined somewhere at the > top of the Makefile (thus harder to make sure the linking order is correct). > > 2) If we deicide to use SUBDIRS for _clean rule, then we would need some foreach loop, right? > Apart from that, there are other directories that are not part of SUBDIRS i.e. include/, tools/. > Together with SUBDIRS variable, it would create confusion (these dirs are also sub-directories, so why > not having them listed in this variable?). Also, I can see that we do clean not only for $(TARGET_ARCH) > but for all the existing architectures. I understand that the changes would be more involved, but I disagree with your "two parts" statement: If what I've outlined was already the case, your patch would not even exist (because crypto/ would already be taken care of wherever needed). > I would prefer to stick for now to the goal of this patch which is to add crypto/ so that it is taken > into account for the tags/csope generation. Would the following change be ok for that purpose? > > diff --git a/xen/Makefile b/xen/Makefile > index 2d55bb9401f4..05bf301bd7ab 100644 > --- a/xen/Makefile > +++ b/xen/Makefile > @@ -589,7 +589,9 @@ $(TARGET): outputmakefile FORCE > $(Q)$(MAKE) $(build)=. arch/$(TARGET_ARCH)/include/asm/asm-offsets.h > $(Q)$(MAKE) $(build)=. MKRELOC=$(MKRELOC) 'ALL_OBJS=$(ALL_OBJS-y)' 'ALL_LIBS=$(ALL_LIBS-y)' $@ > > -SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test > +SUBDIRS-$(CONFIG_CRYPTO) += crypto > +SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test $(SUBDIRS-y) > + > define all_sources > ( find include -type f -name '*.h' -print; \ > find $(SUBDIRS) -type f -name '*.[chS]' -print ) The fact that, afaict, this won't do is related to the outline I gave. You've referred yourself to need-config. Most if not all of the goals SUBDIRS is (currently) relevant for are listed in no-dot-config-targets. Hence your change above is effectively a no-op, because CONFIG_CRYPTO will simply be unset in the common case. Therefore if an easy change is wanted, the original version of it would be it. An intermediate approach might be to add crypto to SUBDIRS only when TARGET_ARCH=x86. But neither would preclude the same issue from being introduced again, when a new subdir appears. Jan
On 27/02/2023 14:54, Jan Beulich wrote: > > > On 27.02.2023 14:41, Michal Orzel wrote: >> Hi Jan, >> >> On 27/02/2023 11:10, Jan Beulich wrote: >>> >>> >>> On 27.02.2023 10:53, Michal Orzel wrote: >>>> --- a/xen/Makefile >>>> +++ b/xen/Makefile >>>> @@ -589,7 +589,7 @@ $(TARGET): outputmakefile FORCE >>>> $(Q)$(MAKE) $(build)=. arch/$(TARGET_ARCH)/include/asm/asm-offsets.h >>>> $(Q)$(MAKE) $(build)=. MKRELOC=$(MKRELOC) 'ALL_OBJS=$(ALL_OBJS-y)' 'ALL_LIBS=$(ALL_LIBS-y)' $@ >>>> >>>> -SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test >>>> +SUBDIRS = xsm arch/$(TARGET_ARCH) common crypto drivers lib test >>>> define all_sources >>>> ( find include -type f -name '*.h' -print; \ >>>> find $(SUBDIRS) -type f -name '*.[chS]' -print ) >>> >>> As long as it's arch/$(TARGET_ARCH) that's used here, crypto should imo >>> also only be included when selected (or at the very least only when an >>> arch might select it, which afaics is possible on x86 only right now). >>> >>> It would also help if in the description you made explicit that SUBDIRS >>> isn't used for anything else (the name, after all, is pretty generic). >>> Which actually points at an issue: I suppose the variable would actually >>> better be used elsewhere as well, e.g. in the _clean: rule and perhaps >>> also in the setting of ALL_OBJS-y. (That'll require splitting the >>> variable, to that e.g. _clean would use $(SUBDIRS), $(SUBDIRS-y), and >>> $(SUBDIRS-) collectively.) It is, imo, that lack of consolidation which >>> has caused crypto to be missing from SUBDIRS. >>> >> I think what you wrote can be split into 2 parts: the part being a goal of this patch >> and the cleanup/improvements that would be beneficial but not related to this patch. >> The second part involves more code and there are parts to be discussed: >> >> 1) If we decide to create ALL_OBJS-y from SUBDIRS, then we would need to carve out test/ dir >> that is not part of ALL_OBJS-y and add it to SUBDIRS later on. Also, the order of ALL_OBJS-y matters >> for linking, so we would need to transfer the responsibility to SUBDIRS. The natural placement of >> SUBDIRS (including SUBDIRS-y, etc.) would be right above ALL_OBJS-y. However, when doing clean (next point), >> need-config is set to n and SUBDIRS would be empty. This means it would need to be defined somewhere at the >> top of the Makefile (thus harder to make sure the linking order is correct). >> >> 2) If we deicide to use SUBDIRS for _clean rule, then we would need some foreach loop, right? >> Apart from that, there are other directories that are not part of SUBDIRS i.e. include/, tools/. >> Together with SUBDIRS variable, it would create confusion (these dirs are also sub-directories, so why >> not having them listed in this variable?). Also, I can see that we do clean not only for $(TARGET_ARCH) >> but for all the existing architectures. > > I understand that the changes would be more involved, but I disagree with > your "two parts" statement: If what I've outlined was already the case, > your patch would not even exist (because crypto/ would already be taken > care of wherever needed). > >> I would prefer to stick for now to the goal of this patch which is to add crypto/ so that it is taken >> into account for the tags/csope generation. Would the following change be ok for that purpose? >> >> diff --git a/xen/Makefile b/xen/Makefile >> index 2d55bb9401f4..05bf301bd7ab 100644 >> --- a/xen/Makefile >> +++ b/xen/Makefile >> @@ -589,7 +589,9 @@ $(TARGET): outputmakefile FORCE >> $(Q)$(MAKE) $(build)=. arch/$(TARGET_ARCH)/include/asm/asm-offsets.h >> $(Q)$(MAKE) $(build)=. MKRELOC=$(MKRELOC) 'ALL_OBJS=$(ALL_OBJS-y)' 'ALL_LIBS=$(ALL_LIBS-y)' $@ >> >> -SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test >> +SUBDIRS-$(CONFIG_CRYPTO) += crypto >> +SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test $(SUBDIRS-y) >> + >> define all_sources >> ( find include -type f -name '*.h' -print; \ >> find $(SUBDIRS) -type f -name '*.[chS]' -print ) > > The fact that, afaict, this won't do is related to the outline I gave. > You've referred yourself to need-config. Most if not all of the goals > SUBDIRS is (currently) relevant for are listed in no-dot-config-targets. > Hence your change above is effectively a no-op, because CONFIG_CRYPTO > will simply be unset in the common case. Therefore if an easy change is > wanted, the original version of it would be it. An intermediate > approach might be to add crypto to SUBDIRS only when TARGET_ARCH=x86. > But neither would preclude the same issue from being introduced again, > when a new subdir appears. I understand your concerns. However, I cannot see how your suggested changes e.g. making use of SUBDIRS in ALL_OBJS and _clean would solve our issue. They would reduce the repetitions (hence I called them cleanup/improvements) but as we want to keep tags,cscope,clean as no-dot-config targets, we would still not be able to use: SUBDIRS-$(CONFIG_CRYPTO) += crypto and use it in all_sources (because as you pointed out, it will be a no-op). Also, why do we care so much about guarding crypto/ if we take into account so many architecture/config dependent files for tags/cscope (e.g. in drivers, lib/x86, etc.)? If these are no-dot-config targets, then we should take everything, apart from really big directories like arch/ ones. ~Michal
On 27.02.2023 15:46, Michal Orzel wrote: > On 27/02/2023 14:54, Jan Beulich wrote: >> On 27.02.2023 14:41, Michal Orzel wrote: >>> On 27/02/2023 11:10, Jan Beulich wrote: >>>> On 27.02.2023 10:53, Michal Orzel wrote: >>>>> --- a/xen/Makefile >>>>> +++ b/xen/Makefile >>>>> @@ -589,7 +589,7 @@ $(TARGET): outputmakefile FORCE >>>>> $(Q)$(MAKE) $(build)=. arch/$(TARGET_ARCH)/include/asm/asm-offsets.h >>>>> $(Q)$(MAKE) $(build)=. MKRELOC=$(MKRELOC) 'ALL_OBJS=$(ALL_OBJS-y)' 'ALL_LIBS=$(ALL_LIBS-y)' $@ >>>>> >>>>> -SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test >>>>> +SUBDIRS = xsm arch/$(TARGET_ARCH) common crypto drivers lib test >>>>> define all_sources >>>>> ( find include -type f -name '*.h' -print; \ >>>>> find $(SUBDIRS) -type f -name '*.[chS]' -print ) >>>> >>>> As long as it's arch/$(TARGET_ARCH) that's used here, crypto should imo >>>> also only be included when selected (or at the very least only when an >>>> arch might select it, which afaics is possible on x86 only right now). >>>> >>>> It would also help if in the description you made explicit that SUBDIRS >>>> isn't used for anything else (the name, after all, is pretty generic). >>>> Which actually points at an issue: I suppose the variable would actually >>>> better be used elsewhere as well, e.g. in the _clean: rule and perhaps >>>> also in the setting of ALL_OBJS-y. (That'll require splitting the >>>> variable, to that e.g. _clean would use $(SUBDIRS), $(SUBDIRS-y), and >>>> $(SUBDIRS-) collectively.) It is, imo, that lack of consolidation which >>>> has caused crypto to be missing from SUBDIRS. >>>> >>> I think what you wrote can be split into 2 parts: the part being a goal of this patch >>> and the cleanup/improvements that would be beneficial but not related to this patch. >>> The second part involves more code and there are parts to be discussed: >>> >>> 1) If we decide to create ALL_OBJS-y from SUBDIRS, then we would need to carve out test/ dir >>> that is not part of ALL_OBJS-y and add it to SUBDIRS later on. Also, the order of ALL_OBJS-y matters >>> for linking, so we would need to transfer the responsibility to SUBDIRS. The natural placement of >>> SUBDIRS (including SUBDIRS-y, etc.) would be right above ALL_OBJS-y. However, when doing clean (next point), >>> need-config is set to n and SUBDIRS would be empty. This means it would need to be defined somewhere at the >>> top of the Makefile (thus harder to make sure the linking order is correct). >>> >>> 2) If we deicide to use SUBDIRS for _clean rule, then we would need some foreach loop, right? >>> Apart from that, there are other directories that are not part of SUBDIRS i.e. include/, tools/. >>> Together with SUBDIRS variable, it would create confusion (these dirs are also sub-directories, so why >>> not having them listed in this variable?). Also, I can see that we do clean not only for $(TARGET_ARCH) >>> but for all the existing architectures. >> >> I understand that the changes would be more involved, but I disagree with >> your "two parts" statement: If what I've outlined was already the case, >> your patch would not even exist (because crypto/ would already be taken >> care of wherever needed). >> >>> I would prefer to stick for now to the goal of this patch which is to add crypto/ so that it is taken >>> into account for the tags/csope generation. Would the following change be ok for that purpose? >>> >>> diff --git a/xen/Makefile b/xen/Makefile >>> index 2d55bb9401f4..05bf301bd7ab 100644 >>> --- a/xen/Makefile >>> +++ b/xen/Makefile >>> @@ -589,7 +589,9 @@ $(TARGET): outputmakefile FORCE >>> $(Q)$(MAKE) $(build)=. arch/$(TARGET_ARCH)/include/asm/asm-offsets.h >>> $(Q)$(MAKE) $(build)=. MKRELOC=$(MKRELOC) 'ALL_OBJS=$(ALL_OBJS-y)' 'ALL_LIBS=$(ALL_LIBS-y)' $@ >>> >>> -SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test >>> +SUBDIRS-$(CONFIG_CRYPTO) += crypto >>> +SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test $(SUBDIRS-y) >>> + >>> define all_sources >>> ( find include -type f -name '*.h' -print; \ >>> find $(SUBDIRS) -type f -name '*.[chS]' -print ) >> >> The fact that, afaict, this won't do is related to the outline I gave. >> You've referred yourself to need-config. Most if not all of the goals >> SUBDIRS is (currently) relevant for are listed in no-dot-config-targets. >> Hence your change above is effectively a no-op, because CONFIG_CRYPTO >> will simply be unset in the common case. Therefore if an easy change is >> wanted, the original version of it would be it. An intermediate >> approach might be to add crypto to SUBDIRS only when TARGET_ARCH=x86. >> But neither would preclude the same issue from being introduced again, >> when a new subdir appears. > I understand your concerns. However, I cannot see how your suggested changes > e.g. making use of SUBDIRS in ALL_OBJS and _clean would solve our issue. > They would reduce the repetitions (hence I called them cleanup/improvements) > but as we want to keep tags,cscope,clean as no-dot-config targets, I, for one, didn't take this is a goal (perhaps for clean, but there ... > we would still not be able to use: > SUBDIRS-$(CONFIG_CRYPTO) += crypto > and use it in all_sources (because as you pointed out, it will be a no-op). ... the problem is obvious to solve, as outlined before). > Also, why do we care so much about guarding crypto/ if we take into account > so many architecture/config dependent files for tags/cscope (e.g. in drivers, > lib/x86, etc.)? Good question, which I'm afraid I have to answer with asking back: > If these are no-dot-config targets, then we should take everything, apart > from really big directories like arch/ ones. Why is arch/ other than arch/$(TARGET_ARCH) to be ignored? And if it is, why would crypto, used by only x86, not be ignored? As always I'd prefer firm rules, not arbitrary and/or fuzzy boundaries. Furthermore, considering e.g. cscope: Personally I view it as actively wrong to constrain it to just one arch. That'll lead to mistakes as soon as you want to make any cross-arch or even tree-wide change. Hence it would probably want treating just like clean. I can't comment on the various tags (case-insensitive) goals, as I don't know what they're about. Jan
On 27/02/2023 16:00, Jan Beulich wrote: > > > On 27.02.2023 15:46, Michal Orzel wrote: >> On 27/02/2023 14:54, Jan Beulich wrote: >>> On 27.02.2023 14:41, Michal Orzel wrote: >>>> On 27/02/2023 11:10, Jan Beulich wrote: >>>>> On 27.02.2023 10:53, Michal Orzel wrote: >>>>>> --- a/xen/Makefile >>>>>> +++ b/xen/Makefile >>>>>> @@ -589,7 +589,7 @@ $(TARGET): outputmakefile FORCE >>>>>> $(Q)$(MAKE) $(build)=. arch/$(TARGET_ARCH)/include/asm/asm-offsets.h >>>>>> $(Q)$(MAKE) $(build)=. MKRELOC=$(MKRELOC) 'ALL_OBJS=$(ALL_OBJS-y)' 'ALL_LIBS=$(ALL_LIBS-y)' $@ >>>>>> >>>>>> -SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test >>>>>> +SUBDIRS = xsm arch/$(TARGET_ARCH) common crypto drivers lib test >>>>>> define all_sources >>>>>> ( find include -type f -name '*.h' -print; \ >>>>>> find $(SUBDIRS) -type f -name '*.[chS]' -print ) >>>>> >>>>> As long as it's arch/$(TARGET_ARCH) that's used here, crypto should imo >>>>> also only be included when selected (or at the very least only when an >>>>> arch might select it, which afaics is possible on x86 only right now). >>>>> >>>>> It would also help if in the description you made explicit that SUBDIRS >>>>> isn't used for anything else (the name, after all, is pretty generic). >>>>> Which actually points at an issue: I suppose the variable would actually >>>>> better be used elsewhere as well, e.g. in the _clean: rule and perhaps >>>>> also in the setting of ALL_OBJS-y. (That'll require splitting the >>>>> variable, to that e.g. _clean would use $(SUBDIRS), $(SUBDIRS-y), and >>>>> $(SUBDIRS-) collectively.) It is, imo, that lack of consolidation which >>>>> has caused crypto to be missing from SUBDIRS. >>>>> >>>> I think what you wrote can be split into 2 parts: the part being a goal of this patch >>>> and the cleanup/improvements that would be beneficial but not related to this patch. >>>> The second part involves more code and there are parts to be discussed: >>>> >>>> 1) If we decide to create ALL_OBJS-y from SUBDIRS, then we would need to carve out test/ dir >>>> that is not part of ALL_OBJS-y and add it to SUBDIRS later on. Also, the order of ALL_OBJS-y matters >>>> for linking, so we would need to transfer the responsibility to SUBDIRS. The natural placement of >>>> SUBDIRS (including SUBDIRS-y, etc.) would be right above ALL_OBJS-y. However, when doing clean (next point), >>>> need-config is set to n and SUBDIRS would be empty. This means it would need to be defined somewhere at the >>>> top of the Makefile (thus harder to make sure the linking order is correct). >>>> >>>> 2) If we deicide to use SUBDIRS for _clean rule, then we would need some foreach loop, right? >>>> Apart from that, there are other directories that are not part of SUBDIRS i.e. include/, tools/. >>>> Together with SUBDIRS variable, it would create confusion (these dirs are also sub-directories, so why >>>> not having them listed in this variable?). Also, I can see that we do clean not only for $(TARGET_ARCH) >>>> but for all the existing architectures. >>> >>> I understand that the changes would be more involved, but I disagree with >>> your "two parts" statement: If what I've outlined was already the case, >>> your patch would not even exist (because crypto/ would already be taken >>> care of wherever needed). >>> >>>> I would prefer to stick for now to the goal of this patch which is to add crypto/ so that it is taken >>>> into account for the tags/csope generation. Would the following change be ok for that purpose? >>>> >>>> diff --git a/xen/Makefile b/xen/Makefile >>>> index 2d55bb9401f4..05bf301bd7ab 100644 >>>> --- a/xen/Makefile >>>> +++ b/xen/Makefile >>>> @@ -589,7 +589,9 @@ $(TARGET): outputmakefile FORCE >>>> $(Q)$(MAKE) $(build)=. arch/$(TARGET_ARCH)/include/asm/asm-offsets.h >>>> $(Q)$(MAKE) $(build)=. MKRELOC=$(MKRELOC) 'ALL_OBJS=$(ALL_OBJS-y)' 'ALL_LIBS=$(ALL_LIBS-y)' $@ >>>> >>>> -SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test >>>> +SUBDIRS-$(CONFIG_CRYPTO) += crypto >>>> +SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test $(SUBDIRS-y) >>>> + >>>> define all_sources >>>> ( find include -type f -name '*.h' -print; \ >>>> find $(SUBDIRS) -type f -name '*.[chS]' -print ) >>> >>> The fact that, afaict, this won't do is related to the outline I gave. >>> You've referred yourself to need-config. Most if not all of the goals >>> SUBDIRS is (currently) relevant for are listed in no-dot-config-targets. >>> Hence your change above is effectively a no-op, because CONFIG_CRYPTO >>> will simply be unset in the common case. Therefore if an easy change is >>> wanted, the original version of it would be it. An intermediate >>> approach might be to add crypto to SUBDIRS only when TARGET_ARCH=x86. >>> But neither would preclude the same issue from being introduced again, >>> when a new subdir appears. >> I understand your concerns. However, I cannot see how your suggested changes >> e.g. making use of SUBDIRS in ALL_OBJS and _clean would solve our issue. >> They would reduce the repetitions (hence I called them cleanup/improvements) >> but as we want to keep tags,cscope,clean as no-dot-config targets, > > I, for one, didn't take this is a goal (perhaps for clean, but there ... So you would suggest to make these targets (except clean) .config dependent? > >> we would still not be able to use: >> SUBDIRS-$(CONFIG_CRYPTO) += crypto >> and use it in all_sources (because as you pointed out, it will be a no-op). > > ... the problem is obvious to solve, as outlined before). > >> Also, why do we care so much about guarding crypto/ if we take into account >> so many architecture/config dependent files for tags/cscope (e.g. in drivers, >> lib/x86, etc.)? > > Good question, which I'm afraid I have to answer with asking back: > >> If these are no-dot-config targets, then we should take everything, apart >> from really big directories like arch/ ones. > > Why is arch/ other than arch/$(TARGET_ARCH) to be ignored? And if it is, > why would crypto, used by only x86, not be ignored? As always I'd prefer > firm rules, not arbitrary and/or fuzzy boundaries. > > Furthermore, considering e.g. cscope: Personally I view it as actively > wrong to constrain it to just one arch. That'll lead to mistakes as > soon as you want to make any cross-arch or even tree-wide change. Hence > it would probably want treating just like clean. I can't comment on the > various tags (case-insensitive) goals, as I don't know what they're > about. The useful thing about generating tags/cscope per-arch is that you can limit the number of findings. Each symbol leads to one definition and not to multiple ones. But this is still not ideal solution because doing this per-arch and not per-config leads to inconsistency. Also, as you wrote, sometimes it is useful to have all the symbols loaded when doing tree-wide changes. So in ideal world we should have an option to use approach or another. I reckon this is why Linux has a separate script for that which allows to set all-arch/per-arch, config-dependent/config-independent. Anyway, I do not have any ready-made solution for this problem. The only benefit of this patch would be that the symbols for crypto/ would be visible in cscope/tags (TBOOT uses the crypto functions but there are no definitions present which may be suprising for people using ctags). But I can understand if you do not want to take this patch in, due to all the observations above. ~Michal
On 27.02.2023 16:46, Michal Orzel wrote: > On 27/02/2023 16:00, Jan Beulich wrote: >> On 27.02.2023 15:46, Michal Orzel wrote: >>> On 27/02/2023 14:54, Jan Beulich wrote: >>>> On 27.02.2023 14:41, Michal Orzel wrote: >>>>> On 27/02/2023 11:10, Jan Beulich wrote: >>>>>> On 27.02.2023 10:53, Michal Orzel wrote: >>>>>>> --- a/xen/Makefile >>>>>>> +++ b/xen/Makefile >>>>>>> @@ -589,7 +589,7 @@ $(TARGET): outputmakefile FORCE >>>>>>> $(Q)$(MAKE) $(build)=. arch/$(TARGET_ARCH)/include/asm/asm-offsets.h >>>>>>> $(Q)$(MAKE) $(build)=. MKRELOC=$(MKRELOC) 'ALL_OBJS=$(ALL_OBJS-y)' 'ALL_LIBS=$(ALL_LIBS-y)' $@ >>>>>>> >>>>>>> -SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test >>>>>>> +SUBDIRS = xsm arch/$(TARGET_ARCH) common crypto drivers lib test >>>>>>> define all_sources >>>>>>> ( find include -type f -name '*.h' -print; \ >>>>>>> find $(SUBDIRS) -type f -name '*.[chS]' -print ) >>>>>> >>>>>> As long as it's arch/$(TARGET_ARCH) that's used here, crypto should imo >>>>>> also only be included when selected (or at the very least only when an >>>>>> arch might select it, which afaics is possible on x86 only right now). >>>>>> >>>>>> It would also help if in the description you made explicit that SUBDIRS >>>>>> isn't used for anything else (the name, after all, is pretty generic). >>>>>> Which actually points at an issue: I suppose the variable would actually >>>>>> better be used elsewhere as well, e.g. in the _clean: rule and perhaps >>>>>> also in the setting of ALL_OBJS-y. (That'll require splitting the >>>>>> variable, to that e.g. _clean would use $(SUBDIRS), $(SUBDIRS-y), and >>>>>> $(SUBDIRS-) collectively.) It is, imo, that lack of consolidation which >>>>>> has caused crypto to be missing from SUBDIRS. >>>>>> >>>>> I think what you wrote can be split into 2 parts: the part being a goal of this patch >>>>> and the cleanup/improvements that would be beneficial but not related to this patch. >>>>> The second part involves more code and there are parts to be discussed: >>>>> >>>>> 1) If we decide to create ALL_OBJS-y from SUBDIRS, then we would need to carve out test/ dir >>>>> that is not part of ALL_OBJS-y and add it to SUBDIRS later on. Also, the order of ALL_OBJS-y matters >>>>> for linking, so we would need to transfer the responsibility to SUBDIRS. The natural placement of >>>>> SUBDIRS (including SUBDIRS-y, etc.) would be right above ALL_OBJS-y. However, when doing clean (next point), >>>>> need-config is set to n and SUBDIRS would be empty. This means it would need to be defined somewhere at the >>>>> top of the Makefile (thus harder to make sure the linking order is correct). >>>>> >>>>> 2) If we deicide to use SUBDIRS for _clean rule, then we would need some foreach loop, right? >>>>> Apart from that, there are other directories that are not part of SUBDIRS i.e. include/, tools/. >>>>> Together with SUBDIRS variable, it would create confusion (these dirs are also sub-directories, so why >>>>> not having them listed in this variable?). Also, I can see that we do clean not only for $(TARGET_ARCH) >>>>> but for all the existing architectures. >>>> >>>> I understand that the changes would be more involved, but I disagree with >>>> your "two parts" statement: If what I've outlined was already the case, >>>> your patch would not even exist (because crypto/ would already be taken >>>> care of wherever needed). >>>> >>>>> I would prefer to stick for now to the goal of this patch which is to add crypto/ so that it is taken >>>>> into account for the tags/csope generation. Would the following change be ok for that purpose? >>>>> >>>>> diff --git a/xen/Makefile b/xen/Makefile >>>>> index 2d55bb9401f4..05bf301bd7ab 100644 >>>>> --- a/xen/Makefile >>>>> +++ b/xen/Makefile >>>>> @@ -589,7 +589,9 @@ $(TARGET): outputmakefile FORCE >>>>> $(Q)$(MAKE) $(build)=. arch/$(TARGET_ARCH)/include/asm/asm-offsets.h >>>>> $(Q)$(MAKE) $(build)=. MKRELOC=$(MKRELOC) 'ALL_OBJS=$(ALL_OBJS-y)' 'ALL_LIBS=$(ALL_LIBS-y)' $@ >>>>> >>>>> -SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test >>>>> +SUBDIRS-$(CONFIG_CRYPTO) += crypto >>>>> +SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test $(SUBDIRS-y) >>>>> + >>>>> define all_sources >>>>> ( find include -type f -name '*.h' -print; \ >>>>> find $(SUBDIRS) -type f -name '*.[chS]' -print ) >>>> >>>> The fact that, afaict, this won't do is related to the outline I gave. >>>> You've referred yourself to need-config. Most if not all of the goals >>>> SUBDIRS is (currently) relevant for are listed in no-dot-config-targets. >>>> Hence your change above is effectively a no-op, because CONFIG_CRYPTO >>>> will simply be unset in the common case. Therefore if an easy change is >>>> wanted, the original version of it would be it. An intermediate >>>> approach might be to add crypto to SUBDIRS only when TARGET_ARCH=x86. >>>> But neither would preclude the same issue from being introduced again, >>>> when a new subdir appears. >>> I understand your concerns. However, I cannot see how your suggested changes >>> e.g. making use of SUBDIRS in ALL_OBJS and _clean would solve our issue. >>> They would reduce the repetitions (hence I called them cleanup/improvements) >>> but as we want to keep tags,cscope,clean as no-dot-config targets, >> >> I, for one, didn't take this is a goal (perhaps for clean, but there ... > So you would suggest to make these targets (except clean) .config dependent? > > >> >>> we would still not be able to use: >>> SUBDIRS-$(CONFIG_CRYPTO) += crypto >>> and use it in all_sources (because as you pointed out, it will be a no-op). >> >> ... the problem is obvious to solve, as outlined before). >> >>> Also, why do we care so much about guarding crypto/ if we take into account >>> so many architecture/config dependent files for tags/cscope (e.g. in drivers, >>> lib/x86, etc.)? >> >> Good question, which I'm afraid I have to answer with asking back: >> >>> If these are no-dot-config targets, then we should take everything, apart >>> from really big directories like arch/ ones. >> >> Why is arch/ other than arch/$(TARGET_ARCH) to be ignored? And if it is, >> why would crypto, used by only x86, not be ignored? As always I'd prefer >> firm rules, not arbitrary and/or fuzzy boundaries. >> >> Furthermore, considering e.g. cscope: Personally I view it as actively >> wrong to constrain it to just one arch. That'll lead to mistakes as >> soon as you want to make any cross-arch or even tree-wide change. Hence >> it would probably want treating just like clean. I can't comment on the >> various tags (case-insensitive) goals, as I don't know what they're >> about. > The useful thing about generating tags/cscope per-arch is that you can limit > the number of findings. Each symbol leads to one definition and not to multiple > ones. But this is still not ideal solution because doing this per-arch > and not per-config leads to inconsistency. Also, as you wrote, sometimes it is useful > to have all the symbols loaded when doing tree-wide changes. So in ideal world we should > have an option to use approach or another. I reckon this is why Linux > has a separate script for that which allows to set all-arch/per-arch, config-dependent/config-independent. > > Anyway, I do not have any ready-made solution for this problem. > The only benefit of this patch would be that the symbols for crypto/ would be visible > in cscope/tags (TBOOT uses the crypto functions but there are no definitions present > which may be suprising for people using ctags). > But I can understand if you do not want to take this patch in, due to all the observations above. Well, I'm not outright opposed. But I definitely want to hear another maintainer's view before deciding. Jan
On 27.02.2023 16:57, Jan Beulich wrote: > Well, I'm not outright opposed. But I definitely want to hear another > maintainer's view before deciding. Oh, and I should have added: If to be taken, the description would need extending to explain why the simple route is taken here, and why it is deemed okay to consider crypto for as little or as much as is, in the eventual final version, going to be covered in the final version (i.e. in the version as submitted it would want clarifying why it is okay to include for Arm despite being unused there). Jan
On 27/02/2023 16:57, Jan Beulich wrote: > > > On 27.02.2023 16:46, Michal Orzel wrote: >> On 27/02/2023 16:00, Jan Beulich wrote: >>> On 27.02.2023 15:46, Michal Orzel wrote: >>>> On 27/02/2023 14:54, Jan Beulich wrote: >>>>> On 27.02.2023 14:41, Michal Orzel wrote: >>>>>> On 27/02/2023 11:10, Jan Beulich wrote: >>>>>>> On 27.02.2023 10:53, Michal Orzel wrote: >>>>>>>> --- a/xen/Makefile >>>>>>>> +++ b/xen/Makefile >>>>>>>> @@ -589,7 +589,7 @@ $(TARGET): outputmakefile FORCE >>>>>>>> $(Q)$(MAKE) $(build)=. arch/$(TARGET_ARCH)/include/asm/asm-offsets.h >>>>>>>> $(Q)$(MAKE) $(build)=. MKRELOC=$(MKRELOC) 'ALL_OBJS=$(ALL_OBJS-y)' 'ALL_LIBS=$(ALL_LIBS-y)' $@ >>>>>>>> >>>>>>>> -SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test >>>>>>>> +SUBDIRS = xsm arch/$(TARGET_ARCH) common crypto drivers lib test >>>>>>>> define all_sources >>>>>>>> ( find include -type f -name '*.h' -print; \ >>>>>>>> find $(SUBDIRS) -type f -name '*.[chS]' -print ) >>>>>>> >>>>>>> As long as it's arch/$(TARGET_ARCH) that's used here, crypto should imo >>>>>>> also only be included when selected (or at the very least only when an >>>>>>> arch might select it, which afaics is possible on x86 only right now). >>>>>>> >>>>>>> It would also help if in the description you made explicit that SUBDIRS >>>>>>> isn't used for anything else (the name, after all, is pretty generic). >>>>>>> Which actually points at an issue: I suppose the variable would actually >>>>>>> better be used elsewhere as well, e.g. in the _clean: rule and perhaps >>>>>>> also in the setting of ALL_OBJS-y. (That'll require splitting the >>>>>>> variable, to that e.g. _clean would use $(SUBDIRS), $(SUBDIRS-y), and >>>>>>> $(SUBDIRS-) collectively.) It is, imo, that lack of consolidation which >>>>>>> has caused crypto to be missing from SUBDIRS. >>>>>>> >>>>>> I think what you wrote can be split into 2 parts: the part being a goal of this patch >>>>>> and the cleanup/improvements that would be beneficial but not related to this patch. >>>>>> The second part involves more code and there are parts to be discussed: >>>>>> >>>>>> 1) If we decide to create ALL_OBJS-y from SUBDIRS, then we would need to carve out test/ dir >>>>>> that is not part of ALL_OBJS-y and add it to SUBDIRS later on. Also, the order of ALL_OBJS-y matters >>>>>> for linking, so we would need to transfer the responsibility to SUBDIRS. The natural placement of >>>>>> SUBDIRS (including SUBDIRS-y, etc.) would be right above ALL_OBJS-y. However, when doing clean (next point), >>>>>> need-config is set to n and SUBDIRS would be empty. This means it would need to be defined somewhere at the >>>>>> top of the Makefile (thus harder to make sure the linking order is correct). >>>>>> >>>>>> 2) If we deicide to use SUBDIRS for _clean rule, then we would need some foreach loop, right? >>>>>> Apart from that, there are other directories that are not part of SUBDIRS i.e. include/, tools/. >>>>>> Together with SUBDIRS variable, it would create confusion (these dirs are also sub-directories, so why >>>>>> not having them listed in this variable?). Also, I can see that we do clean not only for $(TARGET_ARCH) >>>>>> but for all the existing architectures. >>>>> >>>>> I understand that the changes would be more involved, but I disagree with >>>>> your "two parts" statement: If what I've outlined was already the case, >>>>> your patch would not even exist (because crypto/ would already be taken >>>>> care of wherever needed). >>>>> >>>>>> I would prefer to stick for now to the goal of this patch which is to add crypto/ so that it is taken >>>>>> into account for the tags/csope generation. Would the following change be ok for that purpose? >>>>>> >>>>>> diff --git a/xen/Makefile b/xen/Makefile >>>>>> index 2d55bb9401f4..05bf301bd7ab 100644 >>>>>> --- a/xen/Makefile >>>>>> +++ b/xen/Makefile >>>>>> @@ -589,7 +589,9 @@ $(TARGET): outputmakefile FORCE >>>>>> $(Q)$(MAKE) $(build)=. arch/$(TARGET_ARCH)/include/asm/asm-offsets.h >>>>>> $(Q)$(MAKE) $(build)=. MKRELOC=$(MKRELOC) 'ALL_OBJS=$(ALL_OBJS-y)' 'ALL_LIBS=$(ALL_LIBS-y)' $@ >>>>>> >>>>>> -SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test >>>>>> +SUBDIRS-$(CONFIG_CRYPTO) += crypto >>>>>> +SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test $(SUBDIRS-y) >>>>>> + >>>>>> define all_sources >>>>>> ( find include -type f -name '*.h' -print; \ >>>>>> find $(SUBDIRS) -type f -name '*.[chS]' -print ) >>>>> >>>>> The fact that, afaict, this won't do is related to the outline I gave. >>>>> You've referred yourself to need-config. Most if not all of the goals >>>>> SUBDIRS is (currently) relevant for are listed in no-dot-config-targets. >>>>> Hence your change above is effectively a no-op, because CONFIG_CRYPTO >>>>> will simply be unset in the common case. Therefore if an easy change is >>>>> wanted, the original version of it would be it. An intermediate >>>>> approach might be to add crypto to SUBDIRS only when TARGET_ARCH=x86. >>>>> But neither would preclude the same issue from being introduced again, >>>>> when a new subdir appears. >>>> I understand your concerns. However, I cannot see how your suggested changes >>>> e.g. making use of SUBDIRS in ALL_OBJS and _clean would solve our issue. >>>> They would reduce the repetitions (hence I called them cleanup/improvements) >>>> but as we want to keep tags,cscope,clean as no-dot-config targets, >>> >>> I, for one, didn't take this is a goal (perhaps for clean, but there ... >> So you would suggest to make these targets (except clean) .config dependent? >> >> >>> >>>> we would still not be able to use: >>>> SUBDIRS-$(CONFIG_CRYPTO) += crypto >>>> and use it in all_sources (because as you pointed out, it will be a no-op). >>> >>> ... the problem is obvious to solve, as outlined before). >>> >>>> Also, why do we care so much about guarding crypto/ if we take into account >>>> so many architecture/config dependent files for tags/cscope (e.g. in drivers, >>>> lib/x86, etc.)? >>> >>> Good question, which I'm afraid I have to answer with asking back: >>> >>>> If these are no-dot-config targets, then we should take everything, apart >>>> from really big directories like arch/ ones. >>> >>> Why is arch/ other than arch/$(TARGET_ARCH) to be ignored? And if it is, >>> why would crypto, used by only x86, not be ignored? As always I'd prefer >>> firm rules, not arbitrary and/or fuzzy boundaries. >>> >>> Furthermore, considering e.g. cscope: Personally I view it as actively >>> wrong to constrain it to just one arch. That'll lead to mistakes as >>> soon as you want to make any cross-arch or even tree-wide change. Hence >>> it would probably want treating just like clean. I can't comment on the >>> various tags (case-insensitive) goals, as I don't know what they're >>> about. >> The useful thing about generating tags/cscope per-arch is that you can limit >> the number of findings. Each symbol leads to one definition and not to multiple >> ones. But this is still not ideal solution because doing this per-arch >> and not per-config leads to inconsistency. Also, as you wrote, sometimes it is useful >> to have all the symbols loaded when doing tree-wide changes. So in ideal world we should >> have an option to use approach or another. I reckon this is why Linux >> has a separate script for that which allows to set all-arch/per-arch, config-dependent/config-independent. >> >> Anyway, I do not have any ready-made solution for this problem. >> The only benefit of this patch would be that the symbols for crypto/ would be visible >> in cscope/tags (TBOOT uses the crypto functions but there are no definitions present >> which may be suprising for people using ctags). >> But I can understand if you do not want to take this patch in, due to all the observations above. > > Well, I'm not outright opposed. But I definitely want to hear another > maintainer's view before deciding. While waiting for other maintainers vote, I would like to propose an intermediate approach that would at least result in unified approach (related to what you wrote about constraints): In general, there are 2 *main* approaches when dealing with code indexing. The first is a "tree-wide" approach, where we do not take Kconfig into account. The second is a "config-aware" approach, where we take into account Kconfig options. At the moment, in Xen, the approach taken is not uniform because we take all the directories into account except arch/ where we take only $(TARGET_ARCH) into indexing. This makes it difficult to reason about. What is the rule then - how big is the directory? The intermediate solution is to switch for now to "tree-wide" approach by modifying the SUBDIRS from: SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test to: SUBDIRS = xsm arch common drivers lib test crypto This way, the approach is clear for everyone and we do not make any exceptions. Also the name of SUBDIRS and all_sources makes sense as they are read as "global" by default as oppose to e.g. "all_compiled_sources". In the future, we can then add support for "config-aware" approach by taking into account Kconfig which involes more code. Benefits: - clear approach, no fuzzy boundaries - all in - crypto can be included right away as well as any new subdir without requiring to fix the infrastructure first ~Michal
On 28.02.2023 09:14, Michal Orzel wrote: > > > On 27/02/2023 16:57, Jan Beulich wrote: >> >> >> On 27.02.2023 16:46, Michal Orzel wrote: >>> On 27/02/2023 16:00, Jan Beulich wrote: >>>> On 27.02.2023 15:46, Michal Orzel wrote: >>>>> On 27/02/2023 14:54, Jan Beulich wrote: >>>>>> On 27.02.2023 14:41, Michal Orzel wrote: >>>>>>> On 27/02/2023 11:10, Jan Beulich wrote: >>>>>>>> On 27.02.2023 10:53, Michal Orzel wrote: >>>>>>>>> --- a/xen/Makefile >>>>>>>>> +++ b/xen/Makefile >>>>>>>>> @@ -589,7 +589,7 @@ $(TARGET): outputmakefile FORCE >>>>>>>>> $(Q)$(MAKE) $(build)=. arch/$(TARGET_ARCH)/include/asm/asm-offsets.h >>>>>>>>> $(Q)$(MAKE) $(build)=. MKRELOC=$(MKRELOC) 'ALL_OBJS=$(ALL_OBJS-y)' 'ALL_LIBS=$(ALL_LIBS-y)' $@ >>>>>>>>> >>>>>>>>> -SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test >>>>>>>>> +SUBDIRS = xsm arch/$(TARGET_ARCH) common crypto drivers lib test >>>>>>>>> define all_sources >>>>>>>>> ( find include -type f -name '*.h' -print; \ >>>>>>>>> find $(SUBDIRS) -type f -name '*.[chS]' -print ) >>>>>>>> >>>>>>>> As long as it's arch/$(TARGET_ARCH) that's used here, crypto should imo >>>>>>>> also only be included when selected (or at the very least only when an >>>>>>>> arch might select it, which afaics is possible on x86 only right now). >>>>>>>> >>>>>>>> It would also help if in the description you made explicit that SUBDIRS >>>>>>>> isn't used for anything else (the name, after all, is pretty generic). >>>>>>>> Which actually points at an issue: I suppose the variable would actually >>>>>>>> better be used elsewhere as well, e.g. in the _clean: rule and perhaps >>>>>>>> also in the setting of ALL_OBJS-y. (That'll require splitting the >>>>>>>> variable, to that e.g. _clean would use $(SUBDIRS), $(SUBDIRS-y), and >>>>>>>> $(SUBDIRS-) collectively.) It is, imo, that lack of consolidation which >>>>>>>> has caused crypto to be missing from SUBDIRS. >>>>>>>> >>>>>>> I think what you wrote can be split into 2 parts: the part being a goal of this patch >>>>>>> and the cleanup/improvements that would be beneficial but not related to this patch. >>>>>>> The second part involves more code and there are parts to be discussed: >>>>>>> >>>>>>> 1) If we decide to create ALL_OBJS-y from SUBDIRS, then we would need to carve out test/ dir >>>>>>> that is not part of ALL_OBJS-y and add it to SUBDIRS later on. Also, the order of ALL_OBJS-y matters >>>>>>> for linking, so we would need to transfer the responsibility to SUBDIRS. The natural placement of >>>>>>> SUBDIRS (including SUBDIRS-y, etc.) would be right above ALL_OBJS-y. However, when doing clean (next point), >>>>>>> need-config is set to n and SUBDIRS would be empty. This means it would need to be defined somewhere at the >>>>>>> top of the Makefile (thus harder to make sure the linking order is correct). >>>>>>> >>>>>>> 2) If we deicide to use SUBDIRS for _clean rule, then we would need some foreach loop, right? >>>>>>> Apart from that, there are other directories that are not part of SUBDIRS i.e. include/, tools/. >>>>>>> Together with SUBDIRS variable, it would create confusion (these dirs are also sub-directories, so why >>>>>>> not having them listed in this variable?). Also, I can see that we do clean not only for $(TARGET_ARCH) >>>>>>> but for all the existing architectures. >>>>>> >>>>>> I understand that the changes would be more involved, but I disagree with >>>>>> your "two parts" statement: If what I've outlined was already the case, >>>>>> your patch would not even exist (because crypto/ would already be taken >>>>>> care of wherever needed). >>>>>> >>>>>>> I would prefer to stick for now to the goal of this patch which is to add crypto/ so that it is taken >>>>>>> into account for the tags/csope generation. Would the following change be ok for that purpose? >>>>>>> >>>>>>> diff --git a/xen/Makefile b/xen/Makefile >>>>>>> index 2d55bb9401f4..05bf301bd7ab 100644 >>>>>>> --- a/xen/Makefile >>>>>>> +++ b/xen/Makefile >>>>>>> @@ -589,7 +589,9 @@ $(TARGET): outputmakefile FORCE >>>>>>> $(Q)$(MAKE) $(build)=. arch/$(TARGET_ARCH)/include/asm/asm-offsets.h >>>>>>> $(Q)$(MAKE) $(build)=. MKRELOC=$(MKRELOC) 'ALL_OBJS=$(ALL_OBJS-y)' 'ALL_LIBS=$(ALL_LIBS-y)' $@ >>>>>>> >>>>>>> -SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test >>>>>>> +SUBDIRS-$(CONFIG_CRYPTO) += crypto >>>>>>> +SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test $(SUBDIRS-y) >>>>>>> + >>>>>>> define all_sources >>>>>>> ( find include -type f -name '*.h' -print; \ >>>>>>> find $(SUBDIRS) -type f -name '*.[chS]' -print ) >>>>>> >>>>>> The fact that, afaict, this won't do is related to the outline I gave. >>>>>> You've referred yourself to need-config. Most if not all of the goals >>>>>> SUBDIRS is (currently) relevant for are listed in no-dot-config-targets. >>>>>> Hence your change above is effectively a no-op, because CONFIG_CRYPTO >>>>>> will simply be unset in the common case. Therefore if an easy change is >>>>>> wanted, the original version of it would be it. An intermediate >>>>>> approach might be to add crypto to SUBDIRS only when TARGET_ARCH=x86. >>>>>> But neither would preclude the same issue from being introduced again, >>>>>> when a new subdir appears. >>>>> I understand your concerns. However, I cannot see how your suggested changes >>>>> e.g. making use of SUBDIRS in ALL_OBJS and _clean would solve our issue. >>>>> They would reduce the repetitions (hence I called them cleanup/improvements) >>>>> but as we want to keep tags,cscope,clean as no-dot-config targets, >>>> >>>> I, for one, didn't take this is a goal (perhaps for clean, but there ... >>> So you would suggest to make these targets (except clean) .config dependent? >>> >>> >>>> >>>>> we would still not be able to use: >>>>> SUBDIRS-$(CONFIG_CRYPTO) += crypto >>>>> and use it in all_sources (because as you pointed out, it will be a no-op). >>>> >>>> ... the problem is obvious to solve, as outlined before). >>>> >>>>> Also, why do we care so much about guarding crypto/ if we take into account >>>>> so many architecture/config dependent files for tags/cscope (e.g. in drivers, >>>>> lib/x86, etc.)? >>>> >>>> Good question, which I'm afraid I have to answer with asking back: >>>> >>>>> If these are no-dot-config targets, then we should take everything, apart >>>>> from really big directories like arch/ ones. >>>> >>>> Why is arch/ other than arch/$(TARGET_ARCH) to be ignored? And if it is, >>>> why would crypto, used by only x86, not be ignored? As always I'd prefer >>>> firm rules, not arbitrary and/or fuzzy boundaries. >>>> >>>> Furthermore, considering e.g. cscope: Personally I view it as actively >>>> wrong to constrain it to just one arch. That'll lead to mistakes as >>>> soon as you want to make any cross-arch or even tree-wide change. Hence >>>> it would probably want treating just like clean. I can't comment on the >>>> various tags (case-insensitive) goals, as I don't know what they're >>>> about. >>> The useful thing about generating tags/cscope per-arch is that you can limit >>> the number of findings. Each symbol leads to one definition and not to multiple >>> ones. But this is still not ideal solution because doing this per-arch >>> and not per-config leads to inconsistency. Also, as you wrote, sometimes it is useful >>> to have all the symbols loaded when doing tree-wide changes. So in ideal world we should >>> have an option to use approach or another. I reckon this is why Linux >>> has a separate script for that which allows to set all-arch/per-arch, config-dependent/config-independent. >>> >>> Anyway, I do not have any ready-made solution for this problem. >>> The only benefit of this patch would be that the symbols for crypto/ would be visible >>> in cscope/tags (TBOOT uses the crypto functions but there are no definitions present >>> which may be suprising for people using ctags). >>> But I can understand if you do not want to take this patch in, due to all the observations above. >> >> Well, I'm not outright opposed. But I definitely want to hear another >> maintainer's view before deciding. > > While waiting for other maintainers vote, I would like to propose an intermediate approach > that would at least result in unified approach (related to what you wrote about constraints): > > In general, there are 2 *main* approaches when dealing with code indexing. > The first is a "tree-wide" approach, where we do not take Kconfig into account. > The second is a "config-aware" approach, where we take into account Kconfig options. > > At the moment, in Xen, the approach taken is not uniform because we take all the directories > into account except arch/ where we take only $(TARGET_ARCH) into indexing. This makes it difficult > to reason about. What is the rule then - how big is the directory? > > The intermediate solution is to switch for now to "tree-wide" approach by modifying the SUBDIRS > from: > SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test > to: > SUBDIRS = xsm arch common drivers lib test crypto > > This way, the approach is clear for everyone and we do not make any exceptions. Also the name of > SUBDIRS and all_sources makes sense as they are read as "global" by default as oppose to e.g. "all_compiled_sources". > In the future, we can then add support for "config-aware" approach by taking into account Kconfig which involes > more code. > > Benefits: > - clear approach, no fuzzy boundaries - all in > - crypto can be included right away as well as any new subdir without requiring to fix the infrastructure first I'm okay with that proposal (albeit if you make a patch, please have "crypto" at least ahead of "test"), but it'll need agreement by people actually using any of the affected make goals. Jan
On Tue, 28 Feb 2023, Jan Beulich wrote: > On 28.02.2023 09:14, Michal Orzel wrote: > > On 27/02/2023 16:57, Jan Beulich wrote: > >> On 27.02.2023 16:46, Michal Orzel wrote: > >>> On 27/02/2023 16:00, Jan Beulich wrote: > >>>> On 27.02.2023 15:46, Michal Orzel wrote: > >>>>> On 27/02/2023 14:54, Jan Beulich wrote: > >>>>>> On 27.02.2023 14:41, Michal Orzel wrote: > >>>>>>> On 27/02/2023 11:10, Jan Beulich wrote: > >>>>>>>> On 27.02.2023 10:53, Michal Orzel wrote: > >>>>>>>>> --- a/xen/Makefile > >>>>>>>>> +++ b/xen/Makefile > >>>>>>>>> @@ -589,7 +589,7 @@ $(TARGET): outputmakefile FORCE > >>>>>>>>> $(Q)$(MAKE) $(build)=. arch/$(TARGET_ARCH)/include/asm/asm-offsets.h > >>>>>>>>> $(Q)$(MAKE) $(build)=. MKRELOC=$(MKRELOC) 'ALL_OBJS=$(ALL_OBJS-y)' 'ALL_LIBS=$(ALL_LIBS-y)' $@ > >>>>>>>>> > >>>>>>>>> -SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test > >>>>>>>>> +SUBDIRS = xsm arch/$(TARGET_ARCH) common crypto drivers lib test > >>>>>>>>> define all_sources > >>>>>>>>> ( find include -type f -name '*.h' -print; \ > >>>>>>>>> find $(SUBDIRS) -type f -name '*.[chS]' -print ) > >>>>>>>> > >>>>>>>> As long as it's arch/$(TARGET_ARCH) that's used here, crypto should imo > >>>>>>>> also only be included when selected (or at the very least only when an > >>>>>>>> arch might select it, which afaics is possible on x86 only right now). > >>>>>>>> > >>>>>>>> It would also help if in the description you made explicit that SUBDIRS > >>>>>>>> isn't used for anything else (the name, after all, is pretty generic). > >>>>>>>> Which actually points at an issue: I suppose the variable would actually > >>>>>>>> better be used elsewhere as well, e.g. in the _clean: rule and perhaps > >>>>>>>> also in the setting of ALL_OBJS-y. (That'll require splitting the > >>>>>>>> variable, to that e.g. _clean would use $(SUBDIRS), $(SUBDIRS-y), and > >>>>>>>> $(SUBDIRS-) collectively.) It is, imo, that lack of consolidation which > >>>>>>>> has caused crypto to be missing from SUBDIRS. > >>>>>>>> > >>>>>>> I think what you wrote can be split into 2 parts: the part being a goal of this patch > >>>>>>> and the cleanup/improvements that would be beneficial but not related to this patch. > >>>>>>> The second part involves more code and there are parts to be discussed: > >>>>>>> > >>>>>>> 1) If we decide to create ALL_OBJS-y from SUBDIRS, then we would need to carve out test/ dir > >>>>>>> that is not part of ALL_OBJS-y and add it to SUBDIRS later on. Also, the order of ALL_OBJS-y matters > >>>>>>> for linking, so we would need to transfer the responsibility to SUBDIRS. The natural placement of > >>>>>>> SUBDIRS (including SUBDIRS-y, etc.) would be right above ALL_OBJS-y. However, when doing clean (next point), > >>>>>>> need-config is set to n and SUBDIRS would be empty. This means it would need to be defined somewhere at the > >>>>>>> top of the Makefile (thus harder to make sure the linking order is correct). > >>>>>>> > >>>>>>> 2) If we deicide to use SUBDIRS for _clean rule, then we would need some foreach loop, right? > >>>>>>> Apart from that, there are other directories that are not part of SUBDIRS i.e. include/, tools/. > >>>>>>> Together with SUBDIRS variable, it would create confusion (these dirs are also sub-directories, so why > >>>>>>> not having them listed in this variable?). Also, I can see that we do clean not only for $(TARGET_ARCH) > >>>>>>> but for all the existing architectures. > >>>>>> > >>>>>> I understand that the changes would be more involved, but I disagree with > >>>>>> your "two parts" statement: If what I've outlined was already the case, > >>>>>> your patch would not even exist (because crypto/ would already be taken > >>>>>> care of wherever needed). > >>>>>> > >>>>>>> I would prefer to stick for now to the goal of this patch which is to add crypto/ so that it is taken > >>>>>>> into account for the tags/csope generation. Would the following change be ok for that purpose? > >>>>>>> > >>>>>>> diff --git a/xen/Makefile b/xen/Makefile > >>>>>>> index 2d55bb9401f4..05bf301bd7ab 100644 > >>>>>>> --- a/xen/Makefile > >>>>>>> +++ b/xen/Makefile > >>>>>>> @@ -589,7 +589,9 @@ $(TARGET): outputmakefile FORCE > >>>>>>> $(Q)$(MAKE) $(build)=. arch/$(TARGET_ARCH)/include/asm/asm-offsets.h > >>>>>>> $(Q)$(MAKE) $(build)=. MKRELOC=$(MKRELOC) 'ALL_OBJS=$(ALL_OBJS-y)' 'ALL_LIBS=$(ALL_LIBS-y)' $@ > >>>>>>> > >>>>>>> -SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test > >>>>>>> +SUBDIRS-$(CONFIG_CRYPTO) += crypto > >>>>>>> +SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test $(SUBDIRS-y) > >>>>>>> + > >>>>>>> define all_sources > >>>>>>> ( find include -type f -name '*.h' -print; \ > >>>>>>> find $(SUBDIRS) -type f -name '*.[chS]' -print ) > >>>>>> > >>>>>> The fact that, afaict, this won't do is related to the outline I gave. > >>>>>> You've referred yourself to need-config. Most if not all of the goals > >>>>>> SUBDIRS is (currently) relevant for are listed in no-dot-config-targets. > >>>>>> Hence your change above is effectively a no-op, because CONFIG_CRYPTO > >>>>>> will simply be unset in the common case. Therefore if an easy change is > >>>>>> wanted, the original version of it would be it. An intermediate > >>>>>> approach might be to add crypto to SUBDIRS only when TARGET_ARCH=x86. > >>>>>> But neither would preclude the same issue from being introduced again, > >>>>>> when a new subdir appears. > >>>>> I understand your concerns. However, I cannot see how your suggested changes > >>>>> e.g. making use of SUBDIRS in ALL_OBJS and _clean would solve our issue. > >>>>> They would reduce the repetitions (hence I called them cleanup/improvements) > >>>>> but as we want to keep tags,cscope,clean as no-dot-config targets, > >>>> > >>>> I, for one, didn't take this is a goal (perhaps for clean, but there ... > >>> So you would suggest to make these targets (except clean) .config dependent? > >>> > >>> > >>>> > >>>>> we would still not be able to use: > >>>>> SUBDIRS-$(CONFIG_CRYPTO) += crypto > >>>>> and use it in all_sources (because as you pointed out, it will be a no-op). > >>>> > >>>> ... the problem is obvious to solve, as outlined before). > >>>> > >>>>> Also, why do we care so much about guarding crypto/ if we take into account > >>>>> so many architecture/config dependent files for tags/cscope (e.g. in drivers, > >>>>> lib/x86, etc.)? > >>>> > >>>> Good question, which I'm afraid I have to answer with asking back: > >>>> > >>>>> If these are no-dot-config targets, then we should take everything, apart > >>>>> from really big directories like arch/ ones. > >>>> > >>>> Why is arch/ other than arch/$(TARGET_ARCH) to be ignored? And if it is, > >>>> why would crypto, used by only x86, not be ignored? As always I'd prefer > >>>> firm rules, not arbitrary and/or fuzzy boundaries. > >>>> > >>>> Furthermore, considering e.g. cscope: Personally I view it as actively > >>>> wrong to constrain it to just one arch. That'll lead to mistakes as > >>>> soon as you want to make any cross-arch or even tree-wide change. Hence > >>>> it would probably want treating just like clean. I can't comment on the > >>>> various tags (case-insensitive) goals, as I don't know what they're > >>>> about. > >>> The useful thing about generating tags/cscope per-arch is that you can limit > >>> the number of findings. Each symbol leads to one definition and not to multiple > >>> ones. But this is still not ideal solution because doing this per-arch > >>> and not per-config leads to inconsistency. Also, as you wrote, sometimes it is useful > >>> to have all the symbols loaded when doing tree-wide changes. So in ideal world we should > >>> have an option to use approach or another. I reckon this is why Linux > >>> has a separate script for that which allows to set all-arch/per-arch, config-dependent/config-independent. > >>> > >>> Anyway, I do not have any ready-made solution for this problem. > >>> The only benefit of this patch would be that the symbols for crypto/ would be visible > >>> in cscope/tags (TBOOT uses the crypto functions but there are no definitions present > >>> which may be suprising for people using ctags). > >>> But I can understand if you do not want to take this patch in, due to all the observations above. > >> > >> Well, I'm not outright opposed. But I definitely want to hear another > >> maintainer's view before deciding. > > > > While waiting for other maintainers vote, I would like to propose an intermediate approach > > that would at least result in unified approach (related to what you wrote about constraints): > > > > In general, there are 2 *main* approaches when dealing with code indexing. > > The first is a "tree-wide" approach, where we do not take Kconfig into account. > > The second is a "config-aware" approach, where we take into account Kconfig options. > > > > At the moment, in Xen, the approach taken is not uniform because we take all the directories > > into account except arch/ where we take only $(TARGET_ARCH) into indexing. This makes it difficult > > to reason about. What is the rule then - how big is the directory? > > > > The intermediate solution is to switch for now to "tree-wide" approach by modifying the SUBDIRS > > from: > > SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test > > to: > > SUBDIRS = xsm arch common drivers lib test crypto > > > > This way, the approach is clear for everyone and we do not make any exceptions. Also the name of > > SUBDIRS and all_sources makes sense as they are read as "global" by default as oppose to e.g. "all_compiled_sources". > > In the future, we can then add support for "config-aware" approach by taking into account Kconfig which involes > > more code. > > > > Benefits: > > - clear approach, no fuzzy boundaries - all in > > - crypto can be included right away as well as any new subdir without requiring to fix the infrastructure first > > I'm okay with that proposal (albeit if you make a patch, please have "crypto" > at least ahead of "test"), but it'll need agreement by people actually using > any of the affected make goals. I am OK with this too
diff --git a/xen/Makefile b/xen/Makefile index 2d55bb9401f4..27a2034b593e 100644 --- a/xen/Makefile +++ b/xen/Makefile @@ -589,7 +589,7 @@ $(TARGET): outputmakefile FORCE $(Q)$(MAKE) $(build)=. arch/$(TARGET_ARCH)/include/asm/asm-offsets.h $(Q)$(MAKE) $(build)=. MKRELOC=$(MKRELOC) 'ALL_OBJS=$(ALL_OBJS-y)' 'ALL_LIBS=$(ALL_LIBS-y)' $@ -SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test +SUBDIRS = xsm arch/$(TARGET_ARCH) common crypto drivers lib test define all_sources ( find include -type f -name '*.h' -print; \ find $(SUBDIRS) -type f -name '*.[chS]' -print )
This is done so that the crypto/ source files are listed in all_sources and thus taken into account for cscope,tags,... targets. Signed-off-by: Michal Orzel <michal.orzel@amd.com> --- xen/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)