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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

> 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-*.

Jan