Re: [PATCH] build: add crypto/ to SUBDIRS

[Jan Beulich <jbeulich@xxxxxxxx>]

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