| Message ID | e7c14a0d329e28bdcda21376b54a43c85a4aaf3f.1712682861.git.calvin@wbinvd.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kbuild: buildtar: Add arm support | expand |
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? 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' > + ;; > *) > [ -f "${KBUILD_IMAGE}" ] && cp -v -- "${KBUILD_IMAGE}" "${tmpdir}/boot/vmlinux-kbuild-${KERNELRELEASE}" > echo "" >&2 > -- > 2.39.2 > Cheers, Nathan
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). Thanks, Calvin > > -- > > 2.39.2 > > > > Cheers, > Nathan
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}"
On Fri, Apr 12, 2024 at 12:23:42PM -0700, Calvin Owens wrote: > 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. I think your first assumption is likely correct although I have not looked back at the history to confirm that. I am not as sure on the second statement, mainly just because not all kernel images are compressed so they wouldn't necessarily make sense as vmlinuz. I think it just happens that many of the most popular architectures have default compressed kernel images. > > 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). Yes, I think including that reasoning in the commit message makes sense, since it is justification for changing the status quo. > > > 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). I see you already sent v2, which I will review shortly, but doing this change certainly seems reasonable to me. We could add a comment above it like # Architectures with just a compressed KBUILD_IMAGE > ---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}"
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}" + ;; *) [ -f "${KBUILD_IMAGE}" ] && cp -v -- "${KBUILD_IMAGE}" "${tmpdir}/boot/vmlinux-kbuild-${KERNELRELEASE}" echo "" >&2
Make 'make tar-pkg' and friends work on 32-bit arm. Signed-off-by: Calvin Owens <calvin@wbinvd.org> --- scripts/package/buildtar | 3 +++ 1 file changed, 3 insertions(+)