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
