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
