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

[Frediano Ziglio <frediano.ziglio@xxxxxxxxx>]

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))


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

Frediano