| Message ID | 20241009080439.2411730-2-frediano.ziglio@cloud.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/boot: Improve MBI2 structure check (was: Reduce assembly code) | expand |
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
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
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
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
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
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))
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(+)