kata.kernel-uvm: keep kernelconfig in-tree by charludo · Pull Request #2249 · edgelesssys/contrast

charludo · 2026-03-12T14:02:06Z

Don't worry, most of those 15,070 lines do not require review... 😅

sespiros

Thanks for the changes. I did a pass. Also a question, why do we have to keep both the upstream kernel config in-tree under nodeinstaller/internal/kernelconfig and the modified one under packages/by-name/kata/kernel-uvm, is the intent for both to be reviewable in a diff for instance?

charludo · 2026-03-16T15:10:04Z

@sespiros I pretty heavily reworked this. The actual generating of the final config now happens at build time of the kernel-uvm package. This also allows, as you suggested, to pass additional config options, e.g. setting the ACPI debug mode, without an additional patch or config file.

Both The upstream and expected configs are still kept in-tree; the latter ones are only used by the unit tests executed when building .#base.kernelconfig.

sespiros

Thanks for the changes! I got confused a bit about the overall structure so I am adding some suggestions in case you want to adopt any:

I would rename the internal package to kconfig. config is ok but while reading code it helps me knowing exactly what config I am referring to (and kernelconfig alias makes me confused about where this package lives, the external or the internal package)
Rename kernelconfig.go to kconfig.go (or even better plain config.go) and make them part of the internal kconfig package not the external kernelconfig one. That would also allow you to drop the alias import everywhere.
since now internal/kconfig/config might be confusing, I would rename the test files to base-config and base-config-nvidia-gpu.
Rename kernel-config-update to cmd/update-base which a) is the standard naming for Go multi-binary layout and also make it clear that this kernelconfig subcommand updates the base kernel configs (and the test files but ok).

So you end up with a cleaner layout

tools/kernelconfig external package. This is the kernelconfig main binary which runs during build time to create the final kernel configuration starting from the embedded base configuration.
the cmd/update-base sub-command used to update the base kernel config from upstream
the internal/kconfig package which contains the shared kernel config manipulation logic

charludo · 2026-03-17T06:15:51Z

I would rename the internal package to kconfig. config is ok but while reading code it helps me knowing exactly what config I am referring to (and kernelconfig alias makes me confused about where this package lives, the external or the internal package)

sounds good, done

Rename kernelconfig.go to kconfig.go (or even better plain config.go) and make them part of the internal kconfig package not the external kernelconfig one. That would also allow you to drop the alias import everywhere.

whoops, yes. Done. Should have been like that from the start, sorry, I moved that file around quite a lot 😄

since now internal/kconfig/config might be confusing, I would rename the test files to base-config and base-config-nvidia-gpu.

opted instead to moving them into a separate internal base package from which their embeddings are exported to the main and kconfig packages. Would have preferred to just do this in the main, but alas, import cycle with kconfig.

Rename kernel-config-update to cmd/update-base which a) is the standard naming for Go multi-binary layout and also make it clear that this kernelconfig subcommand updates the base kernel configs (and the test files but ok).

done

sespiros

Thanks. wrt your comment about unset, iiuc from https://docs.kernel.org/kbuild/kconfig-language.html having a config as # CONFIG_FOO is not set or having it as CONFIG_FOO=n is functionally equivalent so the unset mechanism is probably not needed.

charludo · 2026-03-17T11:15:58Z

Thanks. wrt your comment about unset, iiuc from https://docs.kernel.org/kbuild/kconfig-language.html having a config as # CONFIG_FOO is not set or having it as CONFIG_FOO=n is functionally equivalent so the unset mechanism is probably not needed.

Ah, fair. Well, I guess the mechanism with unset as it is now reduces diffs slightly. Theoretically. 😄

burgerdev · 2026-03-17T11:57:38Z

+  # The source of the main module of this repo. We filter for Go files so that
+  # changes in the other parts of this repo don't trigger a rebuild.


This does not seem to be true.

Ah, whoops. Then it's also wrong where I copied it from 😆
Fixed.

burgerdev · 2026-03-17T12:03:46Z

+		c.lines = append(c.lines, line)
+		key := parseKey(line)
+		if key != "" {
+			c.index[key] = len(c.lines) - 1


Should we care about duplicate entries here, especially contradicting ones?

We probably do not need to care; last entry wins. But, I've added a check that will fail if a duplicate is encountered.

burgerdev · 2026-03-17T12:04:15Z

+	if strings.HasPrefix(line, "# CONFIG_") && strings.HasSuffix(line, " is not set") {
+		return line[2 : len(line)-11]
+	}
+	return ""


Should this fail loudly (or, which lines reach this point)?

Each one of these 5, for example:

# # General setup #

IMO it's fine. The assumption is, lines start with CONFIG_ or # CONFIG_, or are irrelevant.

Added a check anyways for empty/comment lines.

burgerdev · 2026-03-17T12:05:05Z

+	if strings.HasPrefix(line, "# CONFIG_") && strings.HasSuffix(line, " is not set") {
+		return line[2 : len(line)-11]
+	}


Maybe a regex with capture would be better here.

burgerdev · 2026-03-17T12:11:31Z

+		config, err := kconfig.OverrideConfig(upstreamData, cfgInfo.isGPU)
+		if err != nil {
+			log.Fatalf("failed to parse upstream config: %v", err)
+		}
+		if err := os.WriteFile(targetFile, config.Marshal(), 0o644); err != nil {
+			log.Fatalf("failed to write target config: %v", err)
+		}


If the testdata is written with OverrideConfig, does it make sense to test OverrideConfig with the test data? Looks liek this will trivially be true?

This will trivially be true if the testdata is up-to-date. If the test data is out-of date (not running codegen after kata update, manual changes,... building kernelconfig and in turn kata.kernel-uvm will fail. Same mechanism as with the kataconfig.

charludo · 2026-03-17T13:56:58Z

@burgerdev feel free to press merge if you're OK with the state. #2245 is already rebased on this.

charludo requested review from burgerdev, katexochen and sespiros March 12, 2026 14:02

charludo added the no changelog PRs not listed in the release notes label Mar 12, 2026

charludo force-pushed the ch/keep-kernelconfig-in-tree branch from c0b39b3 to bc165ff Compare March 12, 2026 14:05

charludo mentioned this pull request Mar 13, 2026

kata.runtime: 3.26.0 -> 3.27.0 #2245

Merged

sespiros requested changes Mar 16, 2026

View reviewed changes

Comment thread nodeinstaller/internal/kernelconfig/kernel-config-update/main.go Outdated

Comment thread nodeinstaller/internal/kernelconfig/kernel-config-update/main.go Outdated

charludo force-pushed the ch/keep-kernelconfig-in-tree branch from bc165ff to 64979da Compare March 16, 2026 12:30

charludo requested a review from sespiros March 16, 2026 12:30

charludo force-pushed the ch/keep-kernelconfig-in-tree branch from 64979da to f816a01 Compare March 16, 2026 15:07

sespiros requested changes Mar 16, 2026

View reviewed changes

Comment thread packages/scripts.nix Outdated

Comment thread tools/kernelconfig/main.go

charludo force-pushed the ch/keep-kernelconfig-in-tree branch from f7c6220 to 6170df7 Compare March 17, 2026 06:16

charludo requested a review from sespiros March 17, 2026 07:25

charludo force-pushed the ch/keep-kernelconfig-in-tree branch from a1e0fa2 to fa5f990 Compare March 17, 2026 07:42

sespiros approved these changes Mar 17, 2026

View reviewed changes

burgerdev reviewed Mar 17, 2026

View reviewed changes

charludo added 2 commits March 17, 2026 13:33

kata.kernel-uvm: keep kernelconfig in-tree

7299e39

packages.imagepuller-benchmark: remove misleading comment

cf62c5b

charludo force-pushed the ch/keep-kernelconfig-in-tree branch from fa5f990 to cf62c5b Compare March 17, 2026 12:34

charludo requested a review from burgerdev March 17, 2026 12:36

burgerdev approved these changes Mar 17, 2026

View reviewed changes

burgerdev merged commit 3b2b4ca into main Mar 17, 2026
24 of 25 checks passed

burgerdev deleted the ch/keep-kernelconfig-in-tree branch March 17, 2026 14:47

		# The source of the main module of this repo. We filter for Go files so that
		# changes in the other parts of this repo don't trigger a rebuild.

Conversation

charludo commented Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sespiros left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

charludo commented Mar 16, 2026

Uh oh!

sespiros left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

charludo commented Mar 17, 2026

Uh oh!

sespiros left a comment

Choose a reason for hiding this comment

Uh oh!

charludo commented Mar 17, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

charludo commented Mar 17, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

charludo commented Mar 12, 2026 •

edited

Loading