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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.10.2024 10:04, Frediano Ziglio wrote:
> Doing previous testing with an Adler Lake Intel machine the following
> patch (improving MBI structure checking) started to fail.

In patch descriptions please don't refer to "this patch" or "the following
patch"; describe a commit in a self-contained way, with references to
what's already committed mentioning commit hash and title, whereas
references to what hasn't been committed using merely the title (and maybe
a link to its most recent posting). I'm not sure though that the other
patch really matters here beyond having exposed an issue that was there
(latently) anyway.

> Excluding it makes the tests succeed however there was not apparent
> reason (looking at the code) for the failure.
> So I instrumented code to output the structure and tested code with
> this extracted data with and without the following patch and results
> were the same.
> Compiled assembly code from lab was also fine beside not keeping
> the 16-byte alignment for the stack.
> Turning on stack alignment solve the problem on Adler Lake machine.
> 
> Signed-off-by: Frediano Ziglio <frediano.ziglio@xxxxxxxxx>

This really wants a Fixes: tag then.

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

Jan