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 package linux-firmware and use it on metal-* variants #3296

Merged
merged 4 commits into from
Aug 29, 2023

Conversation

foersleo
Copy link
Contributor

Issue number:

Closes #3246

Description of changes:

With introduction of Metal variants we opened up Bottlerocket to a wider variety of Hardware configurations. In the past we have added driver support for some server grade hardware, mostly in the 10G+ NIC realm. While adding the drivers, we overlooked that some of these devices may need firmware binaries to be available in order to update firmware and function with the driver version shipped with the kernel.

Add firmware from linux-firmware to our metal variants for drivers we support in Bottlerocket. I have made an effort to cut down the list of binaries to only the ones we currently supply drivers for, as the linux-firmware package is quite large in full. I was able to cut the size of the installed size on disk down from 426 MB for the full package to 29 MB for the stripped down version carrying only the needed firmware.

We could strip the size on disk down even further when using compressed firmware images. Currently ZSTD and XZ compression are supported. While ZSTD does not change the size significantly XZ compression can further reduce the size from 29 MB (uncompressed) to 25 MB (xz compressed). However, doing that requires us to enable additional kernel config options CONFIG_FW_LOADER_COMPRESS and CONFIG_FW_LOADER_COMPRESS_XZ. That optimization can probably be part of a separate commit.

The resulting firmware package carries firmware for the following list of drivers and their corresponding config options we have configured for our metal kernels:

cxgb4 - CONFIG_CHELSIO_T4
tg3 - CONFIG_TIGON3
bnx2x - CONFIG_BNX2X
netxen_nic - CONFIG_NETXEN_NIC
qed - CONFIG_QED
myri10ge - CONFIG_MYRI10GE
i915 - CONFIG_DRM_I915
ice - CONFIG_ICE

Testing done:

I have done some build testing. Unfortunately assessing if all the firmware is in the correct place and can be reached by the drivers is beyond what I can do as I do not have access to these devices.

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

@foersleo
Copy link
Contributor Author

foersleo commented Jul 26, 2023

I have not put the source tar in the lookaside cache yet. Hence, the failing test runs. I want to check the licensing on these firmware blobs we ship first. The linux-firmware package is an interesting place with regards to licenses.

Edit: So all the binaries that will currently go into this package are marked as Redistributable with no modification permitted.

packages/linux-firmware/linux-firmware.spec Outdated Show resolved Hide resolved
packages/linux-firmware/linux-firmware.spec Outdated Show resolved Hide resolved
packages/linux-firmware/linux-firmware.spec Outdated Show resolved Hide resolved
packages/linux-firmware/linux-firmware.spec Outdated Show resolved Hide resolved
packages/linux-firmware/linux-firmware.spec Outdated Show resolved Hide resolved
packages/linux-firmware/linux-firmware.spec Outdated Show resolved Hide resolved
packages/linux-firmware/linux-firmware.spec Outdated Show resolved Hide resolved
packages/linux-firmware/linux-firmware.spec Outdated Show resolved Hide resolved
@foersleo
Copy link
Contributor Author

⬆️ Force push was just rebasing on top of develop before tending comments.

@foersleo
Copy link
Contributor Author

⬆️ Force push tended most comments by Ben. The only one I have not fixed yet is the one about the SPDX license specs because of the above mentioned reasons.

I went also ahead and removed unneeded license files in the patches in commit 2 of the series. That helps uncluttering the license directory in the installation and only ships the license files for firmware we actually ship.

Copy link
Contributor

@zmrow zmrow left a comment

Choose a reason for hiding this comment

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

From the variant perspective this looks good - I don't have any additional comments about the package or patches!

Copy link
Contributor

@yeazelm yeazelm left a comment

Choose a reason for hiding this comment

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

I'm happy with the outcome of trimming this size down. I am curious if we know how difficult it will be to keep up with these updates as things are added to the linux-firmware bundle upstream. Is there anything special we need to track to ensure updates don't accidentally start bloating with more things or will we just need to be diligent to check each update doesn't include things we don't need at the time of reviewing the update?

packages/linux-firmware/linux-firmware.spec Outdated Show resolved Hide resolved
@foersleo
Copy link
Contributor Author

I'm happy with the outcome of trimming this size down. I am curious if we know how difficult it will be to keep up with these updates as things are added to the linux-firmware bundle upstream. Is there anything special we need to track to ensure updates don't accidentally start bloating with more things or will we just need to be diligent to check each update doesn't include things we don't need at the time of reviewing the update?

That is a great question and I am a bit concerned about that myself. In all honesty I do not know yet. It looks like there is a release for linux-firmware about every 4-6 weeks (roughly once a month). I will have a stab at what it takes to update once the next release is out.

In the extreme case where these updates are too burdensome to do every month, we could just go with a linux-firmware update when we introduce a new major kernel. As discussed in a comment to Ben's question about dependency to specific kernel versions, the firmware images would be fine for the kernels we currently support. I do not expect major driver updates that would rely on newer firmware files (The file names are usually hard-coded in the drivers) within a stable series.

In an ideal world we would spend some time to build tooling to automatically strip down the files according to some kernel config file. Given the structure of the firmware repo I'd assume this to be quite challenging with lots of corner cases. But it is something we might want to try our hands on when we find some time

@foersleo
Copy link
Contributor Author

Firmware was confirmed to be at the right spot and the driver finding it properly by Max Böhm #3246 (comment)

Thanks a lot Max for helping out here!

@markusboehme
Copy link
Member

I wanted to review those patches as applied to linux-firmware and there's a bit of an interesting problem there: They do apply fine for me via patch and git apply, but then I lose their history and commentary. When trying to apply them via git am, at least 0003-linux-firmware-bt-wifi-Remove-firmware-for-Bluetooth.patch fails. The first failure is for the first line of the deleted LICENSE.nxp which does seem to use CRLF line endings. The patch seems to have converted those to just LF, and hence doesn't match.

I know Git can perform conversion of line endings, but it seems unexpected here. Can you check whether the original patches as generated by git format-patch were correct and this only happened when committing the patch files to the Bottlerocket repo? If so, I wonder whether disabling EOL conversion for *.patch files via .gitattributes (by treating those as binary files) would be desirable.

For the review, I just pass --ignore-whitespace to git am.

Copy link
Member

@markusboehme markusboehme left a comment

Choose a reason for hiding this comment

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

Thanks for the diligent work combing through all this! I hope it all plays well with updates and won't need to be redone. 🥲

I'm happy to approve once we reach a conclusion about the SPDX identifiers and the line endings (not caring at this point could be one).

packages/linux-firmware/linux-firmware.spec Outdated Show resolved Hide resolved
@foersleo
Copy link
Contributor Author

foersleo commented Aug 2, 2023

I wanted to review those patches as applied to linux-firmware and there's a bit of an interesting problem there: They do apply fine for me via patch and git apply, but then I lose their history and commentary. When trying to apply them via git am, at least 0003-linux-firmware-bt-wifi-Remove-firmware-for-Bluetooth.patch fails. The first failure is for the first line of the deleted LICENSE.nxp which does seem to use CRLF line endings. The patch seems to have converted those to just LF, and hence doesn't match.

I know Git can perform conversion of line endings, but it seems unexpected here. Can you check whether the original patches as generated by git format-patch were correct and this only happened when committing the patch files to the Bottlerocket repo? If so, I wonder whether disabling EOL conversion for *.patch files via .gitattributes (by treating those as binary files) would be desirable.

For the review, I just pass --ignore-whitespace to git am.

The line endings seems to be an interesting issue with git format-patch and git am. This is not an artifact of adding patches to a separate repository. The original patches do not apply to a clean checkout/unpack of linux firmware when trying to apply via git am without extra flags. We can work around this by passing flags --ignore-whitespace (as pointed out by you), or --keep-cr which will apply the patches, but create a warning about quoted CRLF:

$ for p in ../../linux-firmware-20230625/*.patch; do git am --keep-cr $p; done
Applying: linux-firmware: snd: remove firmware for snd/audio devices
Applying: linux-firmware: video: Remove firmware for video/broadcast devices
warning: quoted CRLF detected
Applying: linux-firmware: bt/wifi: Remove firmware for Bluetooth/WiFi devices
Applying: linux-firmware: scsi: Remove firmware for SCSI devices
Applying: linux-firmware: usb: remove firmware for USB/Serial/PCMCIA devices
Applying: linux-firmware: ethernet: Remove firmware for ethernet/IB devices
Applying: linux-firmware: Remove firmware for Accelarator devices
Applying: linux-firmware: gpu: Remove firmware for GPU devices
Applying: linux-firmware: various: Remove firmware for various devices
Applying: linux-firmware: amd-ucode: Remove amd microcode

Stackoverflow has some more detail about what is happening and what git tries to do to work around this. The quoted commit message in one of the answers is particularly enlightening about the odds and ins of CRLF. The gist of which is: git-am by default does not trust line endings coming in through mail because SMTP transport is CRLF-unsafe.

Unfortunately the proposed --transfer-encoding option seems to be not available with git format-patch so it seems there is not really anything we can do about this on the patch creation side.

@foersleo
Copy link
Contributor Author

foersleo commented Aug 2, 2023

⬆️ Force push simplified %prep as suggested by @markusboehme and dropped %changelog as suggested by @yeazelm

@markusboehme
Copy link
Member

@foersleo Thanks for looking closer into it! TIL about git am --keep-cr which seems to be the better alternative.

@foersleo
Copy link
Contributor Author

foersleo commented Aug 2, 2023

Converting this one to draft for now to reduce risk of accidental merge while I am sorting out the questions regarding the Licenses.

@foersleo
Copy link
Contributor Author

⬆️ Force push included fixing up the list of SPDX identifiers as discussed with OSPO and added the force-upstream flag to the package as introduced in #3333 .
Unfortunately I did mix in some catch up with the upstream develop branch and so the comparison to the previous state is littered with a lot of other changes.

Will do an additional rebase shortly to base it on top of current develop branch.

@foersleo foersleo marked this pull request as ready for review August 21, 2023 17:11
Copy link
Member

@markusboehme markusboehme left a comment

Choose a reason for hiding this comment

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

Still looking good to me with the updated SPDX license specifiers.

Create linux-firmware package and import latest source from upstream.
Adjust spec file to package all firmware files available.

Signed-off-by: Leonard Foerster <[email protected]>
Bottlerocket is an OS with a comparably cut-down kernel config and as
such does not provide the wide range of drivers one can find in general
purpose linux distributions. Many of these devices, Bottlerocket does
not support have firmware binaries shipped through linux-firmware.

Drop those firmware blobs we do not ship drivers for in Bottlerocket. I
have tried to section the removal into groups of drivers with
commonalities (e.g. Wifi and Bluetooth devices). Each patch has a list
mapping each driver to a specific kernel config option so we can easily
find which firmware binaries to add back into Bottlerocket if we add
drivers.

The patches only touch the WHENCE file and not the actual binaries as
rpm does not like to apply git binary patches. The WHENCE file is a
record of all the firmware binaries in the package with additional
information for each file. On installation the copy-firmware.sh script
uses the information in the file to determine which files to copy, so it
is enough to remove information from WHENCE in order to remove binaries
from the installed directories.

Signed-off-by: Leonard Foerster <[email protected]>
With the metal-* variants we are targetting a wider variety of hardware
platforms. In order to enable users to properly use Bottlerocket on
platforms we provide drivers for we also need to supply firmware
binaries for some of these devices. Ship these in the metal variants
through new package linux-firmware.

Signed-off-by: Leonard Foerster <[email protected]>
As we want to adhere to SPDX identifiers the Licensing originally given
by AL/Fedora is not suitable. Adjust the Licenses to cover the firmware
we actually package.

Signed-off-by: Leonard Foerster <[email protected]>
@foersleo
Copy link
Contributor Author

⬆️ tended the comments mentioned by Arnaldo.

In addition I removed the force-upstream falg from the linux-firmware package. Discussing it with Ben we should be fine hosting a copy in the look-aside cache and we want to do that for as many packages as possible.

@foersleo
Copy link
Contributor Author

Uploaded the source artifacts to lookaside cache and checked the automated test builds succeed. Merging.

@foersleo foersleo merged commit e318340 into bottlerocket-os:develop Aug 29, 2023
42 checks passed
@foersleo foersleo deleted the linux-firmware branch August 29, 2023 11:51
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.

Bare Metal image: "qed" network driver exists in image but driver firmware file is missing
6 participants