Re: [PATCH v8 1/2] x86/boot: Align mbi2.c stack to 16 bytes

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.10.2024 10:34, Frediano Ziglio wrote:
> On Wed, Oct 9, 2024 at 12:13 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>
>> On 09.10.2024 12:15, Frediano Ziglio wrote:
>>> On Wed, Oct 9, 2024 at 9:20 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>>> On 09.10.2024 10:04, Frediano Ziglio wrote:
>>>>> --- a/xen/arch/x86/efi/Makefile
>>>>> +++ b/xen/arch/x86/efi/Makefile
>>>>> @@ -11,6 +11,8 @@ $(obj)/boot.init.o: $(obj)/buildid.o
>>>>>  $(call 
>>>>> cc-option-add,cflags-stack-boundary,CC,-mpreferred-stack-boundary=4)
>>>>>  $(addprefix $(obj)/,$(EFIOBJ-y)): CFLAGS_stack_boundary := 
>>>>> $(cflags-stack-boundary)
>>>>>
>>>>> +$(obj)/mbi2.o: CFLAGS_stack_boundary := $(cflags-stack-boundary)
>>>>> +
>>>>>  obj-y := common-stub.o stub.o
>>>>>  obj-$(XEN_BUILD_EFI) := $(filter-out %.init.o,$(EFIOBJ-y))
>>>>>  obj-bin-$(XEN_BUILD_EFI) := $(filter %.init.o,$(EFIOBJ-y))
>>>>
>>>> You're duplicating code, which is better to avoid when possible. Is there
>>>> a reason the earlier commit didn't simply add mbi2.o to $(EFIOBJ-y)? That
>>>> way the existing logic would have covered that file as well. And really I
>>>> think it should have been mbi2.init.o (or else adding it into $(obj-bin-y)
>>>> is wrong), which probably wants correcting at the same time (ISTR actually
>>>> having requested that during an earlier review round).
>>>
>>> This was my first attempt, but it fails poorly, as EFIOBJ-y comes with
>>> the addition of creating some file links that causes mbi2.c to be
>>> overridden.
>>
>> I can't see $(EFIOBJ-y) affecting symlink creation. What I can see is that
>> the variable is used in the setting of clean-files, which indeed is a 
>> problem.
>> Still imo the solution then is to introduce another variable to substitute 
>> the
>> uses of $(EFIOBJ-y) in arch/x86/efi/Makefile. E.g.
>>
>> EFIOBJ-all := $(EFIOBJ-y) mbi2.init.o
>>
> 
> what about simply
> 
> diff --git a/xen/arch/x86/efi/Makefile b/xen/arch/x86/efi/Makefile
> index 7e2b5c07de..f2ce739f57 100644
> --- a/xen/arch/x86/efi/Makefile
> +++ b/xen/arch/x86/efi/Makefile
> @@ -9,7 +9,7 @@ $(obj)/%.o: $(src)/%.ihex FORCE
> $(obj)/boot.init.o: $(obj)/buildid.o
> 
> $(call cc-option-add,cflags-stack-boundary,CC,-mpreferred-stack-boundary=4)
> -$(addprefix $(obj)/,$(EFIOBJ-y)): CFLAGS_stack_boundary :=
> $(cflags-stack-boundary)
> +$(addprefix $(obj)/,$(EFIOBJ-y) mbi2.o): CFLAGS_stack_boundary :=
> $(cflags-stack-boundary)
> 
> obj-y := common-stub.o stub.o
> obj-$(XEN_BUILD_EFI) := $(filter-out %.init.o,$(EFIOBJ-y))

Yes, but see below for the other adjustment to make.

>>> If I remember, you suggested changing to obj-bin-y. Still, maybe is
>>> not the best place. It was added to obj-bin-y because it should be
>>> included either if XEN_BUILD_EFI is "y" or not.
>>
>> No, that doesn't explain the addition to obj-bin-y; this would equally be
>> achieved by adding to obj-y. The difference between the two variables is
>> whether objects are to be subject to LTO. And the typical case then is that
>> init-only objects aren't worth that extra build overhead. Hence the common
>> pattern is (besides files with assembly sources) for *.init.o to be added to
>> obj-bin-*.
> 
> Then I would stick to obj-bin-y.

Correct, yet it wants to be mbi2.init.o there.

Jan