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 9:20 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> 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.
>

In this case it's referring to a not merged commit, so I cannot put
the hash, but I changed to state the subject.

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

Done.

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

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

Frediano