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

uki: Add .ucode section #98

Merged
merged 1 commit into from
Apr 15, 2024
Merged

uki: Add .ucode section #98

merged 1 commit into from
Apr 15, 2024

Conversation

tfg13
Copy link

@tfg13 tfg13 commented Mar 27, 2024

This documents the .ucode section implemented in systemd/systemd#31872, see there for most details.

It is relatively straightforward, just an initrd that is guaranteed to be handed to the kernel first.

This documents the `.ucode` section implemented in systemd/systemd#31872, see there for most details.

It is relatively straightforward, just an initrd that is guaranteed to
be handed to the kernel first.
@keszybz
Copy link
Member

keszybz commented Apr 15, 2024

LGTM too. We might want to add some details here, but it's good to merge the short description too.

@keszybz keszybz merged commit 8c5256a into uapi-group:main Apr 15, 2024
1 check passed
@poettering
Copy link
Collaborator

Uh, was about to reply here.

So I was wondering what the goal is with multiple microcode files, i.e. kernel only looks in the first initrd for microcode, hence we cannot just paste together microcode cpios like we paste together regular initrd cpios.

Hence, if there's an add-on with a .ucode file, and an embedded UKI, what's the strategy this should be handled in?

We could say:

  1. Multiple .ucode sections are not supported. If specified then one of the add-ons wins (probably the one ordered first alphabetically).
  2. Multiple .ucode sections are supported, we expect that an EFI stub pastes them together in a smart way, i.e. actually merges them together by first removing their individual header/trailers and then generating a new one for them all?
  3. Similar, but dictate that the .uncode section should come without header/trailer, so that it's easier to paste them together and just add a header/trailer to them? (this has the benefit that the efi stub wouldn't have to parse cpios because it doesn't have to remove header/trailer, in order to combine them)

Opinions?

@DaanDeMeyer
Copy link
Member

Uh, was about to reply here.

So I was wondering what the goal is with multiple microcode files, i.e. kernel only looks in the first initrd for microcode, hence we cannot just paste together microcode cpios like we paste together regular initrd cpios.

Hence, if there's an add-on with a .ucode file, and an embedded UKI, what's the strategy this should be handled in?

We could say:

  1. Multiple .ucode sections are not supported. If specified then one of the add-ons wins (probably the one ordered first alphabetically).
  2. Multiple .ucode sections are supported, we expect that an EFI stub pastes them together in a smart way, i.e. actually merges them together by first removing their individual header/trailers and then generating a new one for them all?
  3. Similar, but dictate that the .uncode section should come without header/trailer, so that it's easier to paste them together and just add a header/trailer to them? (this has the benefit that the efi stub wouldn't have to parse cpios because it doesn't have to remove header/trailer, in order to combine them)

Opinions?

@tfg13 I think you were going to submit another PR here to specify the exact mechanics of microcode addons. See Lennart's comments for all that needs to be taken into account

@poettering
Copy link
Collaborator

So i think right now, most distros generate a single cpio of both intel and amd microcode when updating initrds, right? i.e. rpms/debs ship unpacked firmware, and it's the initrd generator that turns this into a cpio, right?

@poettering
Copy link
Collaborator

I think the spec should say that .ucode should not be compressed (and maybe conversely that .initrd typically is compressed).

(iirc, that's how this works in the kernel, right? microcode cpios should not be compressed, but real initrds should be)

@tfg13
Copy link
Author

tfg13 commented Apr 15, 2024

So i think right now, most distros generate a single cpio of both intel and amd microcode when updating initrds, right? i.e. rpms/debs ship unpacked firmware, and it's the initrd generator that turns this into a cpio, right?

From looking around, this seems mostly true:

For the cpio generation:

I suppose this means we can get away with only supporting one ucode image. So we just have to resolve the embedded ucode vs. addon file conflict. That would point to option 1. Does this make sense to people?

I think the spec should say that .ucode should not be compressed (and maybe conversely that .initrd typically is compressed).

Correct, I just added a comment in the stub-pr, can add a similar one here.

(iirc, that's how this works in the kernel, right? microcode cpios should not be compressed, but real initrds should be)

That is what https://www.kernel.org/doc/html/next/x86/microcode.html says at least

@YHNdnzj
Copy link

YHNdnzj commented Apr 15, 2024

i.e. kernel only looks in the first initrd for microcode, hence we cannot just paste together microcode cpios like we paste together regular initrd cpios.

I really think this statement requires proof. Everything I've found on this says otherwise: archiso happily passes intel-ucode.img and amd-ucode.img individually and it works. Also, the way I read kernel's earlycpio code is that it keeps finding until a match or invalid magic is hit.

@tfg13
Copy link
Author

tfg13 commented Apr 17, 2024

I really think this statement requires proof. Everything I've found on this says otherwise: archiso happily passes intel-ucode.img and amd-ucode.img individually and it works. Also, the way I read kernel's earlycpio code is that it keeps finding until a match or invalid magic is hit.

You are correct, I just tested it. The kernel happily loads microcode from the second (uncompressed) initrd. I also confirmed it cannot see the microcode if it is placed after a compressed image.

@tfg13
Copy link
Author

tfg13 commented Apr 17, 2024

@poettering do you have opinions where this leaves us wrt to this discussion? Turns out the kernel will load ucode from initrds other than the first, unless they are after another compressed initrd.

Since the "primary" initrd is likely compressed, I think the main idea behind this new section still makes sense.

I am still leaning towards keeping things simple (for now?) and only doing one ucode section. However, there doesn't seem to be a fundamental reason why we can't allow multiple (in the future?) - we don't need to mess with the cpio in the stub or require them to be special formats, the normal concatenation will work fine.

@tfg13
Copy link
Author

tfg13 commented Apr 18, 2024

Early draft of implementing addons with the "just one gets passed" policy: tfg13/systemd#2

After writing the code, I would also be open to doing multiple ucode initrds right away - it might actually simplify the code, the version with only one has some dancing with the measurements that seems a bit fragile longer term (I decided to only measure the one that actually gets passed to the kernel, not sure that is correct (but not super important for the policy discussion)).

@poettering
Copy link
Collaborator

poettering commented Apr 19, 2024

So, did I understand this this right, you verified the kernel logic basically reads microcode from any of the the initrds passed, except that it stops when it hits the first compressed initrd?

If it's easy I'd probably actually say:

  1. allow only one .ucode section per UKI/add-on file
  2. permit multiple add-ons and the UKI to all have .ucode
  3. paste ucode sections from all add-ons (ordered by name) together, and then append the one from the UKI file.

@poettering
Copy link
Collaborator

hmm, what actually happens if a cpio archive contains some microcode file X and a later cpio contains the very same file X on the same path?

My understanding is that:

  1. the microcode patching stuff stops on the first X found and uses that. First wins.
  2. the later, regular initrd decompression logic would probably unpack X each time it is seen, so that in the end the last X found is in place in the unpacked tmpfs. Hence last wins?

That's very confusing I guess. But I guess no up to us, but simply kernel behaviour?

@tfg13
Copy link
Author

tfg13 commented Apr 19, 2024

So, did I understand this this right, you verified the kernel logic basically reads microcode from any of the the initrds passed, except that it stops when it hits the first compressed initrd?

Correct.

the microcode patching stuff stops on the first X found and uses that. First wins.

Correct. I just checked again, it only cares about the first match, here: https://github.com/torvalds/linux/blob/2668e3ae2ef36d5e7c52f818ad7d90822c037de4/arch/x86/kernel/cpu/microcode/core.c#L218
find_cpio_data can be called in a loop to find more instances of the same path in later initrds, but the microcode loading path doesn't use that. (and even then it can't see into compressed ones)

the later, regular initrd decompression logic would probably unpack X each time it is seen, so that in the end the last X found is in place in the unpacked tmpfs. Hence last wins?

Yes that is my understanding.

@tfg13
Copy link
Author

tfg13 commented Apr 19, 2024

If it's easy I'd probably actually say:

  1. allow only one .ucode section per UKI/add-on file
  2. permit multiple add-ons and the UKI to all have .ucode
  3. paste ucode sections from all add-ons (ordered by name) together, and then append the one from the UKI file.

Sure, I can do that. Will rework the draft addon PR. (this doesn't change systemd/systemd#31872)

@tfg13
Copy link
Author

tfg13 commented May 1, 2024

Implemented in systemd/systemd#32463

Note I changed the order from what was discussed here:

  1. Global addons
  2. UKI addons
  3. Embedded .ucode section

to

  1. Embedded .ucode section
  2. Global addons
  3. UKI addons

This was done because both cmdline and devicetree use this order, I think it makes more sense (one can put an addon file to change what a (distro-) supplied UKI does, and it greatly simplifies the measurement code (I have more confidence in it being correct).

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

Successfully merging this pull request may close these issues.

5 participants