| Message ID | 20240809-boot-uboot-fix-dependency-on-xilinx-prebuilt-v1-1-7014697db0c6@collins.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | boot/uboot: fix dependency on xilinx-prebuilt | expand |
Hi Brandon, > When building with `make -j` and PER_PACKAGE_DIRECTORIES, a race > condition occurs between xilinx-prebuilt and u-boot, resulting in the > following error. > objcopy -O binary -I elf32-little .../images/pmufw.elf .../images/pmufw.bin > objcopy: '.../images/pmufw.elf': No such file > U-Boot registers a KCONFIG_FIXUP_CMD that uses the pmufw.elf from > xilinx-prebuilt. But KCONFIG_FIXUP_CMDS does not use the normal > DEPENDENCIES, so xilinx-prebuilt is not guaranteed to have run before > fixup. > Instead move the `objcopy` call out to a PRE_BUILD_HOOK so it will run > after xilinx-prebuilt has finished. > Signed-off-by: Brandon Maier <brandon.maier@collins.com> Thank you for looking into this. I am adding Luca on copy as he developed the uboot.mk code you are modifying. Shouldn't line 448 of u-boot.mk prevent this issue? UBOOT_DEPENDENCIES += xilinx-prebuilt Do you have the following config when you see this error? BR2_TARGET_UBOOT_ZYNQMP_PMUFW_PREBUILT Best regards, Neal Frager AMD
Hi Neal, On Sat Aug 10, 2024 at 7:01 AM UTC, Frager, Neal via buildroot wrote: > Hi Brandon, > > > When building with `make -j` and PER_PACKAGE_DIRECTORIES, a race > > condition occurs between xilinx-prebuilt and u-boot, resulting in the > > following error. > > > objcopy -O binary -I elf32-little .../images/pmufw.elf .../images/pmufw.bin > > objcopy: '.../images/pmufw.elf': No such file > > > U-Boot registers a KCONFIG_FIXUP_CMD that uses the pmufw.elf from > > xilinx-prebuilt. But KCONFIG_FIXUP_CMDS does not use the normal > > DEPENDENCIES, so xilinx-prebuilt is not guaranteed to have run before > > fixup. > > > Instead move the `objcopy` call out to a PRE_BUILD_HOOK so it will run > > after xilinx-prebuilt has finished. > > > Signed-off-by: Brandon Maier <brandon.maier@collins.com> > > Thank you for looking into this. I am adding Luca on copy as he developed > the uboot.mk code you are modifying. > > Shouldn't line 448 of u-boot.mk prevent this issue? > UBOOT_DEPENDENCIES += xilinx-prebuilt It doesn't, the dependency graph looks something like this UBOOT_KCONFIG_DEPENDENCIES -> uboot-kconfig uboot-kconfig -> uboot-configure UBOOT_DEPENDENCIES -> uboot-configure The `objcopy` call is happening during `uboot-kconfig`, so UBOOT_DEPENDENCIES is not a prerequisite. There is a UBOOT_KCONFIG_DEPENDENCIES, but it seemed more appropriate that the `objcopy` should be a build step. > > Do you have the following config when you see this error? > BR2_TARGET_UBOOT_ZYNQMP_PMUFW_PREBUILT Yes, this happened when I was building configs/zynqmp_zcu106_defconfig but with PER_PACKAGE_DIRECTORIES enabled, to speed up build with. > > Best regards, > Neal Frager > AMD > _______________________________________________ > buildroot mailing list > buildroot@buildroot.org > https://lists.buildroot.org/mailman/listinfo/buildroot
Hi Brandon, > Hi Brandon, > > > When building with `make -j` and PER_PACKAGE_DIRECTORIES, a race > > condition occurs between xilinx-prebuilt and u-boot, resulting in the > > following error. > > > objcopy -O binary -I elf32-little .../images/pmufw.elf .../images/pmufw.bin > > objcopy: '.../images/pmufw.elf': No such file > > > U-Boot registers a KCONFIG_FIXUP_CMD that uses the pmufw.elf from > > xilinx-prebuilt. But KCONFIG_FIXUP_CMDS does not use the normal > > DEPENDENCIES, so xilinx-prebuilt is not guaranteed to have run before > > fixup. > > > Instead move the `objcopy` call out to a PRE_BUILD_HOOK so it will run > > after xilinx-prebuilt has finished. > > > Signed-off-by: Brandon Maier <brandon.maier@collins.com> > > Thank you for looking into this. I am adding Luca on copy as he developed > the uboot.mk code you are modifying. > > Shouldn't line 448 of u-boot.mk prevent this issue? > UBOOT_DEPENDENCIES += xilinx-prebuilt > It doesn't, the dependency graph looks something like this > UBOOT_KCONFIG_DEPENDENCIES -> uboot-kconfig > uboot-kconfig -> uboot-configure > UBOOT_DEPENDENCIES -> uboot-configure > The `objcopy` call is happening during `uboot-kconfig`, so > UBOOT_DEPENDENCIES is not a prerequisite. Thanks for clarifying this. The issue is clear now. > There is a UBOOT_KCONFIG_DEPENDENCIES, but it seemed more appropriate > that the `objcopy` should be a build step. Yes, I see. objcopy is now a pre-build step. > > Do you have the following config when you see this error? > BR2_TARGET_UBOOT_ZYNQMP_PMUFW_PREBUILT > Yes, this happened when I was building configs/zynqmp_zcu106_defconfig > but with PER_PACKAGE_DIRECTORIES enabled, to speed up build with. Thanks for catching this issue! I was completely unaware. I boot tested a zcu102 and kv260 and all looks good, thanks! Tested-by: Neal Frager <neal.frager@amd.com> Reviewed-by: Neal Frager <neal.frager@amd.com> Best regards, Neal Frager AMD
Hello Brandon, On Fri, 09 Aug 2024 16:30:18 +0000 Brandon Maier via buildroot <buildroot@buildroot.org> wrote: > When building with `make -j` and PER_PACKAGE_DIRECTORIES, a race > condition occurs between xilinx-prebuilt and u-boot, resulting in the > following error. > > objcopy -O binary -I elf32-little .../images/pmufw.elf .../images/pmufw.bin > objcopy: '.../images/pmufw.elf': No such file > > U-Boot registers a KCONFIG_FIXUP_CMD that uses the pmufw.elf from > xilinx-prebuilt. But KCONFIG_FIXUP_CMDS does not use the normal > DEPENDENCIES, so xilinx-prebuilt is not guaranteed to have run before > fixup. > > Instead move the `objcopy` call out to a PRE_BUILD_HOOK so it will run > after xilinx-prebuilt has finished. > > Signed-off-by: Brandon Maier <brandon.maier@collins.com> Thanks for fixing this, I have applied to master. I have a few questions/suggestions below. > +ifeq ($(suffix $(UBOOT_ZYNQMP_PMUFW_PATH)),.elf) > +UBOOT_ZYNQMP_PMUFW_PATH_FINAL = $(basename $(UBOOT_ZYNQMP_PMUFW_PATH)).bin > +define UBOOT_ZYNQMP_PMUFW_CONVERT > + objcopy -O binary -I elf32-little $(UBOOT_ZYNQMP_PMUFW_PATH) $(UBOOT_ZYNQMP_PMUFW_PATH_FINAL) Should we be using $(TARGET_OBJCOPY) instead of objcopy to use the target toolchain objcopy? > +endef > +UBOOT_PRE_BUILD_HOOKS += UBOOT_ZYNQMP_PMUFW_CONVERT > +else > +UBOOT_ZYNQMP_PMUFW_PATH_FINAL = $(UBOOT_ZYNQMP_PMUFW_PATH) > +endif > define UBOOT_ZYNQMP_KCONFIG_PMUFW > - $(if $(filter %.elf,$(UBOOT_ZYNQMP_PMUFW_PATH)), > - objcopy -O binary -I elf32-little $(UBOOT_ZYNQMP_PMUFW_BASENAME).elf $(UBOOT_ZYNQMP_PMUFW_BASENAME).bin > - $(call KCONFIG_SET_OPT,CONFIG_PMUFW_INIT_FILE,"$(UBOOT_ZYNQMP_PMUFW_BASENAME).bin"), > - $(call KCONFIG_SET_OPT,CONFIG_PMUFW_INIT_FILE,"$(UBOOT_ZYNQMP_PMUFW_PATH)")) > + $(call KCONFIG_SET_OPT,CONFIG_PMUFW_INIT_FILE,"$(UBOOT_ZYNQMP_PMUFW_PATH_FINAL)") This means this KCONFIG_SET_OPT is not unconditionally if I'm not wrong (so even on platforms that are not Xilinx ones). Shouldn't it only be done if $(UBOOT_ZYNQMP_PMUFW_PATH_FINAL) is not empty? Best regards, Thomas
Hi Thomas, > When building with `make -j` and PER_PACKAGE_DIRECTORIES, a race > condition occurs between xilinx-prebuilt and u-boot, resulting in the > following error. > > objcopy -O binary -I elf32-little .../images/pmufw.elf .../images/pmufw.bin > objcopy: '.../images/pmufw.elf': No such file > > U-Boot registers a KCONFIG_FIXUP_CMD that uses the pmufw.elf from > xilinx-prebuilt. But KCONFIG_FIXUP_CMDS does not use the normal > DEPENDENCIES, so xilinx-prebuilt is not guaranteed to have run before > fixup. > > Instead move the `objcopy` call out to a PRE_BUILD_HOOK so it will run > after xilinx-prebuilt has finished. > > Signed-off-by: Brandon Maier <brandon.maier@collins.com> > Thanks for fixing this, I have applied to master. I have a few > questions/suggestions below. > +ifeq ($(suffix $(UBOOT_ZYNQMP_PMUFW_PATH)),.elf) > +UBOOT_ZYNQMP_PMUFW_PATH_FINAL = $(basename $(UBOOT_ZYNQMP_PMUFW_PATH)).bin > +define UBOOT_ZYNQMP_PMUFW_CONVERT > + objcopy -O binary -I elf32-little $(UBOOT_ZYNQMP_PMUFW_PATH) $(UBOOT_ZYNQMP_PMUFW_PATH_FINAL) > Should we be using $(TARGET_OBJCOPY) instead of objcopy to use the > target toolchain objcopy? $(TARGET_OBJCOPY) would still be wrong. The pmufw.elf is a microblaze image. > +endef > +UBOOT_PRE_BUILD_HOOKS += UBOOT_ZYNQMP_PMUFW_CONVERT > +else > +UBOOT_ZYNQMP_PMUFW_PATH_FINAL = $(UBOOT_ZYNQMP_PMUFW_PATH) > +endif > define UBOOT_ZYNQMP_KCONFIG_PMUFW > - $(if $(filter %.elf,$(UBOOT_ZYNQMP_PMUFW_PATH)), > - objcopy -O binary -I elf32-little $(UBOOT_ZYNQMP_PMUFW_BASENAME).elf $(UBOOT_ZYNQMP_PMUFW_BASENAME).bin > - $(call KCONFIG_SET_OPT,CONFIG_PMUFW_INIT_FILE,"$(UBOOT_ZYNQMP_PMUFW_BASENAME).bin"), > - $(call KCONFIG_SET_OPT,CONFIG_PMUFW_INIT_FILE,"$(UBOOT_ZYNQMP_PMUFW_PATH)")) > + $(call KCONFIG_SET_OPT,CONFIG_PMUFW_INIT_FILE,"$(UBOOT_ZYNQMP_PMUFW_PATH_FINAL)") > This means this KCONFIG_SET_OPT is not unconditionally if I'm not > wrong (so even on platforms that are not Xilinx ones). Shouldn't it > only be done if $(UBOOT_ZYNQMP_PMUFW_PATH_FINAL) is not empty? I have to think about this, but maybe Brandon has an answer. Best regards, Neal Frager AMD
Hi Thomas, On Thu Aug 15, 2024 at 9:14 AM UTC, Thomas Petazzoni via buildroot wrote: > Hello Brandon, > > On Fri, 09 Aug 2024 16:30:18 +0000 > Brandon Maier via buildroot <buildroot@buildroot.org> wrote: > > > When building with `make -j` and PER_PACKAGE_DIRECTORIES, a race > > condition occurs between xilinx-prebuilt and u-boot, resulting in the > > following error. > > > > objcopy -O binary -I elf32-little .../images/pmufw.elf .../images/pmufw.bin > > objcopy: '.../images/pmufw.elf': No such file > > > > U-Boot registers a KCONFIG_FIXUP_CMD that uses the pmufw.elf from > > xilinx-prebuilt. But KCONFIG_FIXUP_CMDS does not use the normal > > DEPENDENCIES, so xilinx-prebuilt is not guaranteed to have run before > > fixup. > > > > Instead move the `objcopy` call out to a PRE_BUILD_HOOK so it will run > > after xilinx-prebuilt has finished. > > > > Signed-off-by: Brandon Maier <brandon.maier@collins.com> > > Thanks for fixing this, I have applied to master. I have a few > questions/suggestions below. > > > +ifeq ($(suffix $(UBOOT_ZYNQMP_PMUFW_PATH)),.elf) > > +UBOOT_ZYNQMP_PMUFW_PATH_FINAL = $(basename $(UBOOT_ZYNQMP_PMUFW_PATH)).bin > > +define UBOOT_ZYNQMP_PMUFW_CONVERT > > + objcopy -O binary -I elf32-little $(UBOOT_ZYNQMP_PMUFW_PATH) $(UBOOT_ZYNQMP_PMUFW_PATH_FINAL) > > Should we be using $(TARGET_OBJCOPY) instead of objcopy to use the > target toolchain objcopy? I'm not certain, as Neal pointed out the PMU.elf is a microblaze. I'm guessing this works today as `objcopy` probably doesn't need to support the target architecture to convert an ELF to a binary. If that is true, then maybe we should use $(TARGET_OBJCOPY) anyway. So we aren't relying on the build system's version of `objcopy`. > > > +endef > > +UBOOT_PRE_BUILD_HOOKS += UBOOT_ZYNQMP_PMUFW_CONVERT > > +else > > +UBOOT_ZYNQMP_PMUFW_PATH_FINAL = $(UBOOT_ZYNQMP_PMUFW_PATH) > > +endif > > define UBOOT_ZYNQMP_KCONFIG_PMUFW > > - $(if $(filter %.elf,$(UBOOT_ZYNQMP_PMUFW_PATH)), > > - objcopy -O binary -I elf32-little $(UBOOT_ZYNQMP_PMUFW_BASENAME).elf $(UBOOT_ZYNQMP_PMUFW_BASENAME).bin > > - $(call KCONFIG_SET_OPT,CONFIG_PMUFW_INIT_FILE,"$(UBOOT_ZYNQMP_PMUFW_BASENAME).bin"), > > - $(call KCONFIG_SET_OPT,CONFIG_PMUFW_INIT_FILE,"$(UBOOT_ZYNQMP_PMUFW_PATH)")) > > + $(call KCONFIG_SET_OPT,CONFIG_PMUFW_INIT_FILE,"$(UBOOT_ZYNQMP_PMUFW_PATH_FINAL)") > > This means this KCONFIG_SET_OPT is not unconditionally if I'm not > wrong (so even on platforms that are not Xilinx ones). Shouldn't it > only be done if $(UBOOT_ZYNQMP_PMUFW_PATH_FINAL) is not empty? This whole section is inside a `ifeq ($(BR2_TARGET_UBOOT_ZYNQMP),y)` block, so it will only run on Xilinx ZynqMP platforms. However it's possible to set the PMUFW to an empty string and then it will set CONFIG_PMUFW_INIT_FILE to an empty string. If a developer set CONFIG_PMUFW_INIT_FILE in a custom U-Boot defconfig or frag, then this could overwrite it. Though I'm not sure if there's any practical use-case where this is an issue. I can send a patch to add the empty string check though, it seems harmless to add. Thanks, Brandon Maier > > Best regards, > > Thomas
Hello Brandon, On Thu, 15 Aug 2024 13:58:07 +0000 Brandon Maier via buildroot <buildroot@buildroot.org> wrote: > Hi Thomas, > > On Thu Aug 15, 2024 at 9:14 AM UTC, Thomas Petazzoni via buildroot wrote: > > Hello Brandon, > > > > On Fri, 09 Aug 2024 16:30:18 +0000 > > Brandon Maier via buildroot <buildroot@buildroot.org> wrote: > > > > > When building with `make -j` and PER_PACKAGE_DIRECTORIES, a race > > > condition occurs between xilinx-prebuilt and u-boot, resulting in the > > > following error. > > > > > > objcopy -O binary -I elf32-little .../images/pmufw.elf .../images/pmufw.bin > > > objcopy: '.../images/pmufw.elf': No such file > > > > > > U-Boot registers a KCONFIG_FIXUP_CMD that uses the pmufw.elf from > > > xilinx-prebuilt. But KCONFIG_FIXUP_CMDS does not use the normal > > > DEPENDENCIES, so xilinx-prebuilt is not guaranteed to have run before > > > fixup. > > > > > > Instead move the `objcopy` call out to a PRE_BUILD_HOOK so it will run > > > after xilinx-prebuilt has finished. Thanks for having fixed this! I apologize for not having reviewed this earlier -- vacation time + inbox full :( > > > +ifeq ($(suffix $(UBOOT_ZYNQMP_PMUFW_PATH)),.elf) > > > +UBOOT_ZYNQMP_PMUFW_PATH_FINAL = $(basename $(UBOOT_ZYNQMP_PMUFW_PATH)).bin > > > +define UBOOT_ZYNQMP_PMUFW_CONVERT > > > + objcopy -O binary -I elf32-little $(UBOOT_ZYNQMP_PMUFW_PATH) $(UBOOT_ZYNQMP_PMUFW_PATH_FINAL) > > > > Should we be using $(TARGET_OBJCOPY) instead of objcopy to use the > > target toolchain objcopy? > > I'm not certain, as Neal pointed out the PMU.elf is a microblaze. I'm > guessing this works today as `objcopy` probably doesn't need to support > the target architecture to convert an ELF to a binary. > > If that is true, then maybe we should use $(TARGET_OBJCOPY) anyway. So > we aren't relying on the build system's version of `objcopy`. While both are "wrong but working", I agree using $(TARGET_OBJCOPY) would make the process more reproducible and predictable. While there, adding an explanatory comment would be a good idea. > > > +endef > > > +UBOOT_PRE_BUILD_HOOKS += UBOOT_ZYNQMP_PMUFW_CONVERT > > > +else > > > +UBOOT_ZYNQMP_PMUFW_PATH_FINAL = $(UBOOT_ZYNQMP_PMUFW_PATH) > > > +endif > > > define UBOOT_ZYNQMP_KCONFIG_PMUFW > > > - $(if $(filter %.elf,$(UBOOT_ZYNQMP_PMUFW_PATH)), > > > - objcopy -O binary -I elf32-little $(UBOOT_ZYNQMP_PMUFW_BASENAME).elf $(UBOOT_ZYNQMP_PMUFW_BASENAME).bin > > > - $(call KCONFIG_SET_OPT,CONFIG_PMUFW_INIT_FILE,"$(UBOOT_ZYNQMP_PMUFW_BASENAME).bin"), > > > - $(call KCONFIG_SET_OPT,CONFIG_PMUFW_INIT_FILE,"$(UBOOT_ZYNQMP_PMUFW_PATH)")) > > > + $(call KCONFIG_SET_OPT,CONFIG_PMUFW_INIT_FILE,"$(UBOOT_ZYNQMP_PMUFW_PATH_FINAL)") > > > > This means this KCONFIG_SET_OPT is not unconditionally if I'm not > > wrong (so even on platforms that are not Xilinx ones). Shouldn't it > > only be done if $(UBOOT_ZYNQMP_PMUFW_PATH_FINAL) is not empty? > > This whole section is inside a `ifeq ($(BR2_TARGET_UBOOT_ZYNQMP),y)` > block, so it will only run on Xilinx ZynqMP platforms. However it's > possible to set the PMUFW to an empty string and then it will set > CONFIG_PMUFW_INIT_FILE to an empty string. > > If a developer set CONFIG_PMUFW_INIT_FILE in a custom U-Boot defconfig > or frag, then this could overwrite it. Though I'm not sure if there's > any practical use-case where this is an issue. > > I can send a patch to add the empty string check though, it seems > harmless to add. By a cursory look at the U-Boot code, it looks like the U-Boot build would fail. If that's the case, I don't think we need an extra check here: empty string would be just one of all possible incorrect settings that just cause a build failure. IOW, I'd only add a check if it can cause subtle issues that are hard to notice and debug, like a "successful" build (returning zero) producing non-working binaries. Luca
diff --git a/boot/uboot/uboot.mk b/boot/uboot/uboot.mk index 45ad1c880f..cdb9f435f7 100644 --- a/boot/uboot/uboot.mk +++ b/boot/uboot/uboot.mk @@ -460,13 +460,17 @@ endif #ifneq ($(findstring ://,$(UBOOT_ZYNQMP_PMUFW)),) endif #BR2_TARGET_UBOOT_ZYNQMP_PMUFW_PREBUILT -UBOOT_ZYNQMP_PMUFW_BASENAME = $(basename $(UBOOT_ZYNQMP_PMUFW_PATH)) - +ifeq ($(suffix $(UBOOT_ZYNQMP_PMUFW_PATH)),.elf) +UBOOT_ZYNQMP_PMUFW_PATH_FINAL = $(basename $(UBOOT_ZYNQMP_PMUFW_PATH)).bin +define UBOOT_ZYNQMP_PMUFW_CONVERT + objcopy -O binary -I elf32-little $(UBOOT_ZYNQMP_PMUFW_PATH) $(UBOOT_ZYNQMP_PMUFW_PATH_FINAL) +endef +UBOOT_PRE_BUILD_HOOKS += UBOOT_ZYNQMP_PMUFW_CONVERT +else +UBOOT_ZYNQMP_PMUFW_PATH_FINAL = $(UBOOT_ZYNQMP_PMUFW_PATH) +endif define UBOOT_ZYNQMP_KCONFIG_PMUFW - $(if $(filter %.elf,$(UBOOT_ZYNQMP_PMUFW_PATH)), - objcopy -O binary -I elf32-little $(UBOOT_ZYNQMP_PMUFW_BASENAME).elf $(UBOOT_ZYNQMP_PMUFW_BASENAME).bin - $(call KCONFIG_SET_OPT,CONFIG_PMUFW_INIT_FILE,"$(UBOOT_ZYNQMP_PMUFW_BASENAME).bin"), - $(call KCONFIG_SET_OPT,CONFIG_PMUFW_INIT_FILE,"$(UBOOT_ZYNQMP_PMUFW_PATH)")) + $(call KCONFIG_SET_OPT,CONFIG_PMUFW_INIT_FILE,"$(UBOOT_ZYNQMP_PMUFW_PATH_FINAL)") endef UBOOT_ZYNQMP_PM_CFG = $(call qstrip,$(BR2_TARGET_UBOOT_ZYNQMP_PM_CFG))
When building with `make -j` and PER_PACKAGE_DIRECTORIES, a race condition occurs between xilinx-prebuilt and u-boot, resulting in the following error. objcopy -O binary -I elf32-little .../images/pmufw.elf .../images/pmufw.bin objcopy: '.../images/pmufw.elf': No such file U-Boot registers a KCONFIG_FIXUP_CMD that uses the pmufw.elf from xilinx-prebuilt. But KCONFIG_FIXUP_CMDS does not use the normal DEPENDENCIES, so xilinx-prebuilt is not guaranteed to have run before fixup. Instead move the `objcopy` call out to a PRE_BUILD_HOOK so it will run after xilinx-prebuilt has finished. Signed-off-by: Brandon Maier <brandon.maier@collins.com> --- boot/uboot/uboot.mk | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) --- base-commit: 038af921dc90d54a0bf5dabad8b03327cfe44247 change-id: 20240809-boot-uboot-fix-dependency-on-xilinx-prebuilt-63f18e883df7 Best regards,