[v8] x86/boot: Improve MBI2 structure check (was: Reduce assembly code)

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

Posted by Frediano Ziglio 1 year, 4 months ago

Doing previous testing with an Adler Lake Intel machine the following
patch (improving MBI structure checking) started to fail.
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@cloud.com>
---
 xen/arch/x86/efi/Makefile | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/arch/x86/efi/Makefile b/xen/arch/x86/efi/Makefile
index 7e2b5c07de..c2cad86856 100644
--- 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))
-- 
2.34.1

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

Posted by Jan Beulich 1 year, 4 months ago

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@cloud.com>

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

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

Posted by Frediano Ziglio 1 year, 4 months ago

On Wed, Oct 9, 2024 at 9:20 AM Jan Beulich <jbeulich@suse.com> 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@cloud.com>
>
> 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

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

Posted by Jan Beulich 1 year, 4 months ago

On 09.10.2024 12:15, Frediano Ziglio wrote:
> On Wed, Oct 9, 2024 at 9:20 AM Jan Beulich <jbeulich@suse.com> 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

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

Posted by Frediano Ziglio 1 year, 4 months ago

On Wed, Oct 9, 2024 at 12:13 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 09.10.2024 12:15, Frediano Ziglio wrote:
> > On Wed, Oct 9, 2024 at 9:20 AM Jan Beulich <jbeulich@suse.com> 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

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

Posted by Jan Beulich 1 year, 4 months ago

On 10.10.2024 10:34, Frediano Ziglio wrote:
> On Wed, Oct 9, 2024 at 12:13 PM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 09.10.2024 12:15, Frediano Ziglio wrote:
>>> On Wed, Oct 9, 2024 at 9:20 AM Jan Beulich <jbeulich@suse.com> 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