Re: [Buildroot] [PATCH] boot/uboot: fix dependency on xilinx-prebuilt

[Luca Ceresoli via buildroot <buildroot@buildroot.org>]

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

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot