Received-SPF: pass (google.com: domain of linux-kernel+bounces-143249-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Fri, 12 Apr 2024 12:23:42 -0700
From: Calvin Owens <calvin@wbinvd.org>
To: Nathan Chancellor <nathan@kernel.org>
Cc: linux-kernel@vger.kernel.org, Masahiro Yamada <masahiroy@kernel.org>,
	Nicolas Schier <nicolas@fjasle.eu>, linux-kbuild@vger.kernel.org
Subject: Re: [PATCH] kbuild: buildtar: Add arm support
Message-ID: <ZhmKPlMl_WJOPIXs@mozart.vkv.me>
References: <e7c14a0d329e28bdcda21376b54a43c85a4aaf3f.1712682861.git.calvin@wbinvd.org>
 <20240410170450.GA1828262@dev-arch.thelio-3990X>
 <ZhcZCwMmANQS39s2@mozart.vkv.me>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <ZhcZCwMmANQS39s2@mozart.vkv.me>

On Wednesday 04/10 at 15:56 -0700, Calvin Owens wrote:
> On Wednesday 04/10 at 10:04 -0700, Nathan Chancellor wrote:
> > Hi Calvin,
> > 
> > Thanks for the patch!
> > 
> > On Tue, Apr 09, 2024 at 10:17:07AM -0700, Calvin Owens wrote:
> > > Make 'make tar-pkg' and friends work on 32-bit arm.
> > > 
> > > Signed-off-by: Calvin Owens <calvin@wbinvd.org>
> > 
> > Technically speaking, buildtar works for 32-bit ARM right now (I use it
> > almost daily), this is just explicitly adding it to the supported list
> > to avoid the warning and putting zImage at vmlinuz-${KERNELRELEASE}
> > instead of vmlinux-kbuild-${KERNELRELEASE}, right?
> 
> Exactly. I assumed (maybe incorrectly?) the vmlinux-kbuild-* name was
> generic "unimplemented" filler that was meant to be replaced. It seems
> like the vmlinuz-* naming has sort of become a de facto standard in the
> tar-pkgs.
> 
> The context for me is a pile of scripts that build kernels and boot them
> with QEMU on arm and arm64: it's convenient if the tar-pkg structure is
> consistent between the two (and across other architectures too).
> 
> > That said, looks mostly fine to me, one comment below.
> > 
> > Before:
> > 
> >   './System.map' -> 'tar-install/boot/System.map-6.9.0-rc3-00023-g2c71fdf02a95'
> >   '.config' -> 'tar-install/boot/config-6.9.0-rc3-00023-g2c71fdf02a95'
> >   './vmlinux' -> 'tar-install/boot/vmlinux-6.9.0-rc3-00023-g2c71fdf02a95'
> >   'arch/arm/boot/zImage' -> 'tar-install/boot/vmlinux-kbuild-6.9.0-rc3-00023-g2c71fdf02a95'
> > 
> >   ** ** **  WARNING  ** ** **
> > 
> >   Your architecture did not define any architecture-dependent files
> >   to be placed into the tarball. Please add those to scripts/package/buildtar ...
> > 
> > After:
> > 
> >   './System.map' -> 'tar-install/boot/System.map-6.9.0-rc3-00023-g2c71fdf02a95-dirty'
> >   '.config' -> 'tar-install/boot/config-6.9.0-rc3-00023-g2c71fdf02a95-dirty'
> >   './vmlinux' -> 'tar-install/boot/vmlinux-6.9.0-rc3-00023-g2c71fdf02a95-dirty'
> >   './arch/arm/boot/zImage' -> 'tar-install/boot/vmlinuz-6.9.0-rc3-00023-g2c71fdf02a95-dirty'
> > 
> > and the location of zImage is the only thing that changes as expected.
> > 
> > > ---
> > >  scripts/package/buildtar | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/scripts/package/buildtar b/scripts/package/buildtar
> > > index 72c91a1b832f..0939f9eabbf2 100755
> > > --- a/scripts/package/buildtar
> > > +++ b/scripts/package/buildtar
> > > @@ -101,6 +101,9 @@ case "${ARCH}" in
> > >  			fi
> > >  		done
> > >  		;;
> > > +	arm)
> > > +		[ -f "${objtree}/arch/arm/boot/zImage" ] && cp -v -- "${objtree}/arch/arm/boot/zImage" "${tmpdir}/boot/vmlinuz-${KERNELRELEASE}"
> > 
> > While it probably does not matter too much, it would be more proper to
> > make this
> > 
> >   [ -f "${KBUILD_IMAGE}" ] && cp -v -- "${KBUILD_IMAGE}" "${tmpdir}/boot/vmlinuz-${KERNELRELEASE}"
> > 
> > as the current line does not work with CONFIG_XIP_KERNEL=y, since zImage
> > does not exist (KBUILD_IMAGE is arch/arm/boot/xipImage with this
> > configuration)
> > 
> >   $ ls arch/arm/boot
> >   compressed  dts  xipImage
> > 
> > resulting in buildtar failing because
> > 
> >   [ -f "${objtree}/arch/arm/boot/zImage" ]
> > 
> > fails and is the last statement that runs in the script (and the tar
> > package is not really complete in this configuration anyways).
> > 
> > Prior to this change, the correct image would get placed into the
> > tarball.
> > 
> >   'arch/arm/boot/xipImage' -> 'tar-install/boot/vmlinux-kbuild-6.9.0-rc3-00023-g2c71fdf02a95'
> 
> Makes sense, thanks. Although...
> 
> > > +		;;
> > >  	*)
> > >  		[ -f "${KBUILD_IMAGE}" ] && cp -v -- "${KBUILD_IMAGE}" "${tmpdir}/boot/vmlinux-kbuild-${KERNELRELEASE}"
> > >  		echo "" >&2
> 
> ...it ends up looking almost identical to the default case. Does it make
> make more sense to change the destination in the default case and remove
> the warning? I'm not sure if anything might rely on the current
> behavior, it goes all the way back (git sha 6d983feab809).

What I'm trying to say is: using KBUILD_IMAGE like you suggest allows
more of the existing cases to be combined, like the below (and probably
alpha too, at least).

Thanks,
Calvin

---8<---

diff --git a/scripts/package/buildtar b/scripts/package/buildtar
index 72c91a1b832f..66b4d8d308b6 100755
--- a/scripts/package/buildtar
+++ b/scripts/package/buildtar
@@ -54,8 +54,8 @@ cp -v -- "${objtree}/vmlinux" "${tmpdir}/boot/vmlinux-${KERNELRELEASE}"
 # Install arch-specific kernel image(s)
 #
 case "${ARCH}" in
-	x86|i386|x86_64)
-		[ -f "${objtree}/arch/x86/boot/bzImage" ] && cp -v -- "${objtree}/arch/x86/boot/bzImage" "${tmpdir}/boot/vmlinuz-${KERNELRELEASE}"
+	x86|i386|x86_64|arm)
+		[ -f "${KBUILD_IMAGE}" ] && cp -v -- "${KBUILD_IMAGE}" "${tmpdir}/boot/vmlinuz-${KERNELRELEASE}"
 		;;
 	alpha)
 		[ -f "${objtree}/arch/alpha/boot/vmlinux.gz" ] && cp -v -- "${objtree}/arch/alpha/boot/vmlinux.gz" "${tmpdir}/boot/vmlinuz-${KERNELRELEASE}"