[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Michal Orzel <michal.orzel@xxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Mon, 27 Feb 2023 16:00:21 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=3OszXZPctgpmr94+qLqKPiPURi8jAc9g+E/ezLbDIm4=; b=CsTLui51g3vEulYMhH0vdYdrbWvihGAZg+OEh5WeSUcpupdhdv0n0nIaTuV1uErZld7888JuVcuu45iwOT/Gn2Vd2ku8GnZzdnBFlxBkSSRSTMTQ3J0c7x4RwrnwFvznDKvrDl6CHYUo1Gx22Er4h3L+V4iDgqcBctLRZ6Jv041eAjPzWIwggs1JKx5VIg7Kn+All6AZqG3nN+/SgKyXVqofjwYbCCLg39N59RaELdX0lQmG4X70mur/Bzu14VSIeKobDt47ouJ4U/YM1hqGS7DFncLKSlunJQCNhakGA4yedEWvx57QOECxFGmmNOeQMC92m2y0AmKaXTCun3PiKw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=nRC5qHIioGtzUqXVRIwn2yvnb82hDVzM7S0siVnGz77pjCx69x1mXE7MmcVk2xI6avWjk9ZZHv4nzqi/oco8h7HahHS0c4WdqT1EXDhEdvL1bdpqvXKLDAPauAhgY2paR23X7LaHSSiWl9sHWR9XNCrRE9CXDl/qhSuxZXjnqIkO2CcfSdpklzOytecjOTZGvZ5JM1PDhDHqapXDJNFY3bh71JDqFbEd8+Ds2vb3sJ+7gKVLc6PRpaD8/BCchfu4lUy5bUBLqrAxyA95XcKN6TfV2VjuluLFDhCZ8UvW5h3oy6svvjD4kHV3e3yHq2mnBC6rZB08oF94LM7ZbhDbWA==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • Delivery-date: Mon, 27 Feb 2023 15:00:40 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.