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
