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: Introduce .dtbauto and .hwids sections #123

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

anonymix007
Copy link

@anonymix007 anonymix007 changed the title UKI: Introduce .dtbauto sections UKI: Introduce .dtbauto sections Oct 22, 2024
@bluca
Copy link
Member

bluca commented Oct 22, 2024

What's the difference with the existing one?

@anonymix007
Copy link
Author

What's the difference with the existing one?

systemd/systemd#34158 (comment)
systemd/systemd#34158 (comment)

It would probably make sense to revert 5fab44a to reflect this

@poettering
Copy link
Collaborator

What's the difference with the existing one?

I asked for this. I think there should be two distinct types of PE sections. One "dumb" one that is applied without checking if it fits the local system. And "smart" one that is matched to the local system. So the logic would be:

  1. Look if there are any of the "smart" ones and if any apply to the local system. If so use first matching.
  2. Otherwise, look for the "dumb" one, which exists 0 or 1 times in the PE file. If so, apply it.
  3. Otherwise, don't bother.

I think this concept is quite powerful, as it makes the semantics very clear, i.e. what is applied "dumb" and what is applied "smart", and how the fallback logic works. This allows people to put together "one-off" policies, i.e. where they apply one dtb fix for a specific device, but otherwise a more generic one for all other devices.

In this model the "dumb" one would be ".dtb". And the smart one would be the ".dtbauto" one.

The spec so far left open what to do with multiple .dtb, but nothing ever implemented any handling of that really. Hence I think we can make spec changes still to say that .dtb shall be a singleton, and no matching is applied, and .dtbauto otoh can appear multiple times, and matching is applied.

@bluca
Copy link
Member

bluca commented Oct 23, 2024

What's the difference with the existing one?

I asked for this. I think there should be two distinct types of PE sections. One "dumb" one that is applied without checking if it fits the local system. And "smart" one that is matched to the local system. So the logic would be:

  1. Look if there are any of the "smart" ones and if any apply to the local system. If so use first matching.
  2. Otherwise, look for the "dumb" one, which exists 0 or 1 times in the PE file. If so, apply it.
  3. Otherwise, don't bother.

I think this concept is quite powerful, as it makes the semantics very clear, i.e. what is applied "dumb" and what is applied "smart", and how the fallback logic works. This allows people to put together "one-off" policies, i.e. where they apply one dtb fix for a specific device, but otherwise a more generic one for all other devices.

In this model the "dumb" one would be ".dtb". And the smart one would be the ".dtbauto" one.

The spec so far left open what to do with multiple .dtb, but nothing ever implemented any handling of that really. Hence I think we can make spec changes still to say that .dtb shall be a singleton, and no matching is applied, and .dtbauto otoh can appear multiple times, and matching is applied.

Why does it need to be a separate section at all? What's the use case?

@poettering
Copy link
Collaborator

Why does it need to be a separate section at all? What's the use case?

because currently we apply stuff dumb. and suddenly matching shall be added, and I find that a quite drastic redefinition. i'd rather have that split out.

And as I said, I think having one-off cases makes a lot of sense (i.e. "usually apply dtb x, but on system y apply dtb z")

@bluca
Copy link
Member

bluca commented Oct 23, 2024

Why does it need to be a separate section at all? What's the use case?

because currently we apply stuff dumb. and suddenly matching shall be added, and I find that a quite drastic redefinition. i'd rather have that split out.

And as I said, I think having one-off cases makes a lot of sense (i.e. "usually apply dtb x, but on system y apply dtb z")

I am not sure anybody is actually using the current DTB stuff though? Ubuntu was looking into it for the arm laptops, but then switched back to grub
Matching requires specific fields to be added to the DTB anyway, no? So it shouldn't be an issue to just base it on that?

@poettering
Copy link
Collaborator

my understanding was that people want both the smart matching stuff and the dumb no-matching stuff, already because the matching stuff only works if the firmware actually has something to match agains, i.e. smbios stuff or an existing dtb blob. Which as I understand is not really a given.

But dunno, I am not a dtb guy, I am not entirely sure what people actually do want.

@bluca
Copy link
Member

bluca commented Oct 23, 2024

Yeah it's really difficult without concrete and specific use cases, we are doing a lot of guessing, ideally we should get an idea from a real use case, and failing that we should try and get qemu-arm64+uboot+sdboot working again so that there's at least something close enough to a real use case. Unfortunately this setup broke for me last year, and haven't been able to look into it again

@poettering
Copy link
Collaborator

poettering commented Oct 23, 2024

So after talking to some people it appears to me that right now this is what happens:

  1. the .dtb that might be built into a UKI overrides any host provided one in a dumb fashion.
  2. the .dtb sections that might be found in EFI add-ons are merged ("overlayed") with the firmware or UKI provided dtb, via the uboot fixup protocol, also very much dumb. This only works on u-boot though.

And now the question is how matching fits into all of this... i.e. a .dtauto in an add-on should it be an overlay or should it be an override? i really don't know. Somebody, with a deep understanding of this should please make a complete proposal here, i.e.

  1. how shall devicetree stuff from the system, from the UKI and from add-ons be combined/overriden
  2. how should matching fit into all of this.

@poettering
Copy link
Collaborator

@jan-kiszka @calebccff, any chance you can comment on this?

@poettering
Copy link
Collaborator

(and maybe cc other devicetree people who might want to know about this and provide a comment)

@poettering
Copy link
Collaborator

@ardbiesheuvel or maybe you?

@bluca
Copy link
Member

bluca commented Oct 23, 2024

@xnox was working on this some time ago iirc

@xnox
Copy link

xnox commented Oct 23, 2024

Note there is fixup protocol too which may have been installed by an earlier bootloader in the boot chain (i.e. grub, or u-boot, or more weird things on riscv).

ideally it would be installed as a fresh one + call fixup protocol, if one is available.

@jan-kiszka
Copy link

From our experience, full dtb replacement is commonly needed, merging/overlaying will only complicate handling of such dtb updates.

Fixup is then needed when the firmware tuned things. The related protocol is still waiting for becoming a standard: ARM-software/ebbr#68. Or you have creative approaches like trying to perform fixups when a new dtb is registered against UEFI, ignoring the details of a proper fixup protocol (seen on Tegra with EDK2).

@anonymix007 anonymix007 force-pushed the uki-dtbauto branch 2 times, most recently from 2c44ac3 to bcf224d Compare October 24, 2024 13:09
@anonymix007 anonymix007 changed the title UKI: Introduce .dtbauto sections UKI: Introduce .dtbauto and .hwids sections Oct 24, 2024
@anonymix007 anonymix007 marked this pull request as ready for review December 12, 2024 11:50
@calebccff
Copy link

Could the exact matching behaviour also be explained here - specifically how are the dtbs in the .dtbauto section compared to the firmware provided dt?

@calebccff
Copy link

should it be an overlay or should it be an override?

overlays are pretty brittle and there isn't really a well-defined process for using them upstream yet (though there are some boards which have them). Replacing the DT entirely is the right move for now.

also cc @lumag

Copy link

@samueldr samueldr left a comment

Choose a reason for hiding this comment

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

I know this will be controversial. And probably ignored, but still, I believe it is needed.

I would like to ask for a pause, a sort of moratorium on adding this to the spec. I don't know for how long, but there needs to be more discussion around dtb file selection across the different parties involved with boot, including platform firmwares, other OS loaders, and the Linux kernel.

The rationale (and some discussion points) can be found in this thread on my personal fediverse/activitypub/“mastodon” account. The gist of it is that doing this properly is hard, and I strongly believe that supporting this in the long term in a specification shouldn't be taken lightly.

Especially since most of the semantics are not defined here, so already selecting the dtb to load is currently undefined. And if it was to be defined on systemd's semantics, the exact matching on only the first compatible field is incomplete.

Selecting the dtb file should depend, at the very least, on an interface defined by the authors of those dtb files, which is the Linux kernel here. Otherwise, selecting dtb file is currently, as it is in every projects, a best guesstimate.

Furthermore, just loading a dtb file is not enough. Without fixups, the work is incomplete. Not all platform firmwares provide the (new~ish) EFI_DT_FIXUP_PROTOCOL, and when it's not available, platform-defined values will end-up missing.

(TBF, that latter comment about fixups also applies to the .dtb section, which is pretty much undefined behaviour in this spec. The spec doesn't even state what should be done with them “for systems which need it”...)

Also disallow multiple .dtb sections as this use-case is now handled via .dtbauto
@anonymix007
Copy link
Author

I would like to ask for a pause, a sort of moratorium on adding this to the spec

Please correct me If I'm wrong, but AFAIU specification is intended to be a simplified description of what systemd-stub does. Current spec does not fully describe systemd-stub behavior and therefore needs to be updated accordingly unless there's a very good reason not to.

And

there needs to be more discussion

is not a reason at all. In fact, I kind of do agree about the discussion (though it won't be very productive without the actual implementation and I doubt), but it has absolutely nothing to do with this PR. I even think your solution (aka "do it in the kernel") may be better long-term, but it has a fatal flaw: it doesn't exist.

Selecting the dtb file should depend, at the very least, on an interface defined by the authors of those dtb files, which is the Linux kernel here. Otherwise, selecting dtb file is currently, as it is in every projects, a best guesstimate.

Better than nothing I guess.

Furthermore, just loading a dtb file is not enough. Without fixups, the work is incomplete.

Funnily enough, this also applies to your PoC.

Now to your article.

(AFAICT no suggested upstream source for the look-up table, and none part of systemd.)

The latter is intended though. Table is intended to be created by distro maintainers for tested devices. I have one that I'm using myself and it's publicly available here

Loads dtb according matching on compatible string from .dtbauto entries.

That's inaccurate, there's also .hwids section which makes .dtbauto to be kind of on par with dtbloader (no fixup, but also no path dependencies), not that we're at the competition anyway.

At first with an independent compatibility tool (for backward compat, and proving itself)

And how's that different from .dtbauto? As far as I can see, your PoC employs all the same basic techniques, though I might've missed something.

Then within the kernel (to solve this once and for all)

How many years do you expect this to take?

@anonymix007
Copy link
Author

Could the exact matching behaviour also be explained here - specifically how are the dtbs in the .dtbauto section compared to the firmware provided dt?

Will something like the following be enough?

devicetree's compatible property is matched against ...

@calebccff
Copy link

@anonymix007 the compatible property is an array, could we also clarify if all values are matched or just the first?

@anonymix007
Copy link
Author

the compatible property is an array, could we also clarify if all values are matched or just the first?

Sure, how about this?

first compatible property of the Devicetree is matched against ...

@trini
Copy link

trini commented Dec 18, 2024

It would be good to talk with https://github.com/devicetree-org before deciding what is / isn't valid and allowed to be depend upon for automatic selection to be a good idea. A lot of these problems and ideas and potential solutions are frustratingly old at this point. The first overlay manager is maybe a decade in the past at this point?

@alyssais
Copy link

(AFAICT no suggested upstream source for the look-up table, and none part of systemd.)

The latter is intended though. Table is intended to be created by distro maintainers for tested devices. I have one that I'm using myself and it's publicly available here

Separately by each distro? Surely it would be better to have a single common source?

@samueldr
Copy link

I would like to ask for a pause, a sort of moratorium on adding this to the spec

Please correct me If I'm wrong, but AFAIU specification is intended to be a simplified description of what systemd-stub does. Current spec does not fully describe systemd-stub behavior and therefore needs to be updated accordingly unless there's a very good reason not to.

Depending on this, the discussion will go an entirely different way. So I suppose we should make sure everyone here is on the same page about what UKI is.

My understanding was that:

  • UKI is a specification, of which systemd-stub is the reference implementation. The specification should be self-sufficient to make different implementations cross-compatible

But it seems like, from what you're saying, it is(?):

  • The UKI specification describes at a high-level how systemd-stub works.

I don't think it is the latter, since otherwise it would be weird to keep this away from the systemd-stub implementation and documentation. And rather, it would be a description of how to implement UKIs such that they can work outside of the reference implementation, and stay compatible with eachother.

@anonymix007
Copy link
Author

Separately by each distro? Surely it would be better to have a single common source?

What I actually meant is that it shouldn't be a part of systemd. So yes, it would be better have a single known good and well-tested source, it's just that it doesn't really fit the release schedule of systemd well.

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.

9 participants