Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for 64-bit kernels #29

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jgartrel
Copy link

This is a verbatim rebase of the changes provided by @lurch to resolve #2. Thank you @lurch. This also will resolve issue #22 posted by @Zahrun. Tested on 64-bit Raspberry Pi OS running on a Pi4B, building kernel modules for the zfs-dkms and zfsutils-linux packages on bullseye with kernel 6.1.73-v8+.

It should fix the issues with rpi-source identified in raspberrypi/Raspberry-Pi-OS-64bit#4 (comment)

@lurch
Copy link

lurch commented Mar 13, 2024

Since I wrote that PR, there's also now the BCM2712 chip as used by the Raspberry Pi 5 🙂 (which can use either kernel8.img or kernel_2712.img )

Copy link
Member

@pelwell pelwell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this submission - I think it could be improved (see inline comments), but it's a good start.

There is one case where rpi-source will still fail, and that's when running a 32-bit OS on a 64-bit kernel. For that, the user would have to have installed crossbuild-essential-arm64, and include CROSS_COMPILE=aarch64-linux-gnu- in the make command. However, we can leave that for a separate PR.

rpi-source Outdated
@@ -24,8 +24,10 @@ import errno
# used by archive file and unpacked archive
DISK_USAGE_MB = 900

PROCESSOR_TYPES = list(range(0, 4))
PROCESSOR_TYPES_NAMES = ['BCM2835', 'BCM2836', 'BCM2837', 'BCM2711']
PROCESSOR_TYPES_NAMES = ('BCM2835', 'BCM2836', 'BCM2837', 'BCM2711')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add , 'BCM2712'.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved in bb4ba1e

PROCESSOR_TYPES_NAMES = ['BCM2835', 'BCM2836', 'BCM2837', 'BCM2711']
PROCESSOR_TYPES_NAMES = ('BCM2835', 'BCM2836', 'BCM2837', 'BCM2711')
PROCESSOR_TYPES = range(0, len(PROCESSOR_TYPES_NAMES))
ARCHITECTURE_TYPES_NAMES = ('32-bit', '64-bit')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'arm', 'arm64' fits the ARCHITECTURE_TYPES category better, and can be used later (see below).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems to me that 'arm' and 'arm64' may be confused with the reported machine platform arch of: 'aarch64'. I this pull request ARCHITECTURE_TYPE_NAMES is only used for an informative display, so I am not sure I see the value for making this less clear with 'arm' and 'arm64'. Perhaps, we should rework this along with the make command changes you referenced below in a separate PR. What do you think?

rpi-source Outdated
if architecture_type == 0:
sh("cd %s && make bcm2709_defconfig" % (linux_symlink,))
elif architecture_type == 1:
sh("cd %s && make bcmrpi3_defconfig" % (linux_symlink,))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BCM2710 should also be using bcm2711_defconfig - the bcmrpi3_defconfig is a misleading unofficial defconfig that may be deleted.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved in 2a5ba55

rpi-source Outdated
@@ -408,8 +438,13 @@ if args.default_config or not kernel.config:
info(".config (generating default)")
if processor_type == 0:
sh("cd %s && make bcmrpi_defconfig" % (linux_symlink,))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to put an explicit ARCH=%s with the value of architecture_type in each of these make lines, which should also remove the need for the trailing comma.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, with so much in common, isn't it better to use processor_type and possibly architecture_type to generate a defconfig_name, and use that in a single sh() line?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's probably a good idea. Perhaps, we should do that in a separate PR that optimizes the way this is all handled by rpi-source. What do you think?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like this:

 if args.default_config or not kernel.config:
     info(".config (generating default)")
+    defconfig = None
     if processor_type == 0:
-        sh("cd %s && make bcmrpi_defconfig" % (linux_symlink,))
+        defconfig = "bcmrpi_defconfig"
     elif processor_type == 1:
-        sh("cd %s && make bcm2709_defconfig" % (linux_symlink,))
+        defconfig = "bcm2709_defconfig"
     elif processor_type == 2:
         if architecture_type == 0:
-            sh("cd %s && make bcm2709_defconfig" % (linux_symlink,))
+            defconfig = "bcm2709_defconfig"
         elif architecture_type == 1:
-            sh("cd %s && make bcm2711_defconfig" % (linux_symlink,))
+            defconfig = "bcm2711_defconfig"
     elif processor_type == 3:
-        sh("cd %s && make bcm2711_defconfig" % (linux_symlink,))
+        defconfig = "bcm2711_defconfig"
     elif processor_type == 4:
         if page_size == 4096:
-            sh("cd %s && make bcm2711_defconfig" % (linux_symlink,))
+            defconfig = "bcm2711_defconfig"
         elif page_size == 16384:
-            sh("cd %s && make bcm2712_defconfig" % (linux_symlink,))
+            defconfig = "bcm2712_defconfig"
     else:
         fail("Unsupported processor_type %d" % processor_type)
+    if defconfig is not None:
+        sh("cd %s && make %s" % (linux_symlink, defconfig))
 else:
     info(".config")
     writef(os.path.join(linux_dir, '.config'), kernel.config)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved in bd2423c

@jgartrel
Copy link
Author

@pelwell , @lurch FYI, I have a bit of an issue testing all of the various configurations, especially the changes made to RPi3 and RPi5. If anyone could lend a hand with that or recommend a way that I could get that done, I would be grateful!

@lurch
Copy link

lurch commented Mar 22, 2024

I just tried testing this on a Pi 5 that's running 64-bit Bookworm, which is fully up-to-date with apt, but on which I haven't run rpi-update :

pi5@homepi5:~ $ git clone https://github.com/jgartrel/rpi-source
Cloning into 'rpi-source'...
remote: Enumerating objects: 154, done.
remote: Counting objects: 100% (63/63), done.
remote: Compressing objects: 100% (17/17), done.
remote: Total 154 (delta 52), reused 52 (delta 46), pack-reused 91
Receiving objects: 100% (154/154), 50.66 KiB | 625.00 KiB/s, done.
Resolving deltas: 100% (83/83), done.
pi5@homepi5:~ $ cd rpi-source/
pi5@homepi5:~/rpi-source $ ./rpi-source --skip-update

 *** SoC: BCM2712

 *** Arch: 64-bit

 *** Page Size: 16384
ERROR:
Can't find a source for this kernel

Help: https://github.com/RPi-Distro/rpi-source/blob/master/README.md

Looking at the script's source, that appears to be because /usr/share/doc/raspberrypi-bootloader/changelog.Debian.gz doesn't exist (which is correct, as raspberrypi-bootloader isn't installed in Bookworm). I tried doing a bit of poking around and it looks like /usr/share/doc/raspi-firmware/changelog.Debian.gz is the nearest equivalent, but that doesn't have the special firmware as of lines that the rpi-source script is expecting.

@jgartrel
Copy link
Author

@pelwell , Should we remove the recently added BCM2712 support in this pull request, then address the Bookworm incompatibility in a new issue / pull request?

@pelwell
Copy link
Member

pelwell commented Mar 22, 2024

I'm not against a split, but it looks to me that the 2712 support is essentially done, while the required changes for "Bookworm and no rpi-update" are still unknown - is that not a better fault line to split along?

@jgartrel
Copy link
Author

@pelwell , I am ok with that ... as long as we can accept the inability to fully test PI 5 in this pull request.

@pelwell
Copy link
Member

pelwell commented Mar 22, 2024

If a 2712 build of an rpi-update kernel works then I'm happy with this PR.

@jgartrel
Copy link
Author

@lurch , @pelwell I think we are good to go for final review here.

@pelwell
Copy link
Member

pelwell commented Mar 22, 2024

This still doesn't address the case where you have a 64-bit kernel and 32-bit userland, which is likely to affect many Pi 4 users. It needs to set ARCH=arm64 for all the make commands and CROSS_COMPILE=aarch64-linux-gnu- for most of them (you might as well set it for all).

@lurch
Copy link

lurch commented Mar 22, 2024

Given the similarity between the "kernel_suffix" stuff and the "defconfig" stuff, it might be worth moving the defconfig stuff into a function; but it obviously doesn't matter if you don't want to do that.

@jgartrel
Copy link
Author

Looking at the script's source, that appears to be because /usr/share/doc/raspberrypi-bootloader/changelog.Debian.gz doesn't exist (which is correct, as raspberrypi-bootloader isn't installed in Bookworm). I tried doing a bit of poking around and it looks like /usr/share/doc/raspi-firmware/changelog.Debian.gz is the nearest equivalent, but that doesn't have the special firmware as of lines that the rpi-source script is expecting.

@lurch, Looking into this a bit deeper, it looks like rpi-update leaves a .firmware.revision file (THANKS TO YOU!, 11 years ago). The "firmware revision" contained in the file can be linked to a .git_hash file in the https://github.com/raspberrypi/firmware repo, which then contains a hash that represents the exact source hash of https://github.com/raspberrypi/linux by which to retrieve the exact source for the kernel.

At first glance, the Raspbian kernel package does not leave such a breadcrumb trail, but kernel headers are packaged and released at the same time for those "stock" versions.

This means it's conceivable to install the proper kernel headers in bookworm (or any kernel release over the last 11 years) for either of the two cases: rpi-update or stock kernel distribution. I will spend a bit more time tonight looking to see if there is a breadcrumb trail in the stock kernel packages as well that might allow us to install the headers directly from source instead of relying on the kernel headers package.

@lurch
Copy link

lurch commented Mar 23, 2024

Yes, I also looked into the "kernel breadcrumbs" when writing yesterday's comment, and found that e.g.

zcat /usr/share/doc/linux-image-rpi-2712/changelog.Debian.gz | grep "Linux commit:" | head -1

does get us the kernel commit-hash that we need, but that's only the commit-id in the https://github.com/raspberrypi/linux repo (which AFAICT is the "source"), but it seems like rpi-source also needs the Module.symvers files from https://github.com/raspberrypi/firmware/tree/master/extra (which AFAICT is the "binary") and I couldn't see an obvious way of linking the linux-commit to the firmware-commit, other than iterating through all previous versions of https://github.com/raspberrypi/firmware/blob/master/extra/git_hash which didn't seem like a workable solution, so I stopped digging.
Like you said earlier, "address the Bookworm incompatibility in a new issue / pull request" 😆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants