Windows offline activation with ACPI SLIC table #10774

victortoso · 2023-11-22T22:00:03Z

What this PR does / why we need it:
It allows setting the SLIC ACPI table which is useful for offline Windows activation.

The user needs to have the data following Microsoft specification. This PR does not provide a way for users to generate this binary data.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

This is a working PR with a proposal to provide this feature. We can test this without Windows VMs as the ACPI SLIC table can be verified in a Linux Guest.

Checklist

This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.

Design: A design document was considered and is present (link) or not required
PR: The PR description is expressive enough and will help future contributors
Code: Write code that humans can understand and Keep it simple
Refactor: You have left the code cleaner than you found it (Boy Scout Rule)
Upgrade: Impact of this change on upgrade flows was considered and addressed if required
Testing: New code requires new unit tests. New features and bug fixes require at least on e2e test
Documentation: A user-guide update was considered and is present (link) or not required. You want a user-guide update if it's a user facing feature / API change.
Community: Announcement to kubevirt-dev was considered

Release note:

Windows offline activation with ACPI SLIC table

victortoso · 2023-11-22T22:43:19Z

pull-kubevirt-generate — Job failed.

./hack/verify-generate.sh
./hack/check-for-binaries.sh
< tests/testdata/firmware-acpi-slic-table.bin:1
Binary files are present in git repostory.
make: *** [Makefile:56: generate-verify] Error 1

This is a 36 bytes file... it should be fine for test purposes but I can move it to const array if binary files are no-no.

alicefr · 2023-11-23T07:37:41Z

/cc

alicefr · 2023-11-23T07:46:12Z

tests/vmi_configuration_test.go

+				const volumeSlicSecretName = "volume-slic-secret"
+				const secretWithSlicName = "secret-with-slic-data"
+				const hexData = "534c49432400000001494352415348204d450000000000008804000071656d7500000000"


super nit: you can use a single const block

Hi Alice, thanks for the quick review. I'll make it a const block in the next iteration. I'll probably move the binary data here as well and use that to compare (instead of hexData).

alicefr · 2023-11-23T07:46:50Z

@victortoso the code looks already great!
/lgtm

vasiliy-ul

@victortoso, the code looks good overall. Just a thought from my side: as I see now there is a need to manually setup the volume with volume source pointing to the proper secret. Wouldn't it be more convenient for the end-user if we had just SlicNameRef in the VMI spec referencing the secret (not to the volume name), and the volume could be rendered automatically if vmi.Spec.Domain.Firmware.ACPI.SlicNameRef is set?

vasiliy-ul · 2023-11-23T08:19:54Z

staging/src/kubevirt.io/api/core/v1/schema.go

+}
+
+type ACPI struct {
+	// SliceNameRef should match the volume name of a secret object. The data in the secret should


Suggested change

// SliceNameRef should match the volume name of a secret object. The data in the secret should

// SlicNameRef should match the volume name of a secret object. The data in the secret should

Hi @vasiliy-ul , thanks for the quick review.

I thought about referencing the Secret in the SlicNameRef as well but I decided to do the easier path first.

I don't have a strong opinion in this so I can change it. Let's just double check if others agree. @alicefr, @xpivarc what do you think?

IMO, pointing to the volume name is fine. If for any reasons we decide to support another type of volume for this, then this mechanism still remains true. Also, kubernetes references the secrets by volume name inside the pod spec. So, I don't think it is too unusual for the user to reference the volume name instead of the secret name.

Just to note: changing later the reference from volume to secret will be a breaking API change. I am not pushing with this, but IMHO it is better to come up with a long-term decision when the API is introduced. Altering it after the release may become problematic.

I agree that deciding now is better. For the moment, I'm keeping the current code behavior to reference the volume name.

The SLIC table is used for generic activation of Windows guests. https://learn.microsoft.com/en-us/previous-versions/windows/hardware/design/dn653305(v=vs.85) Signed-off-by: Victor Toso <victortoso@redhat.com>

It is specific and big enough logic block. - Return early if firmware is not set - Use local 'firmware' variable Signed-off-by: Victor Toso <victortoso@redhat.com>

Signed-off-by: Victor Toso <victortoso@redhat.com>

Result of make generate Signed-off-by: Victor Toso <victortoso@redhat.com>

Verifies the steps: - Creating a Secret with binary SLIC table - Passing its name to new ACPI api - Creates Guest with ACPI SLIC table Signed-off-by: Victor Toso <victortoso@redhat.com>

victortoso · 2023-11-23T21:44:20Z

Rebased
Fixed the nits, thanks Alice and Vasiliy for the reviews.
Removed the embed 36 bytes binary to workaround the check-for-binaries.sh. The slic table is now a []byte in the test.

victortoso · 2023-11-24T05:19:24Z

/retest

alicefr · 2023-11-30T08:01:30Z

/test-required
/approve
/hold
Putting on hold to let others reviewing it
@vasiliy-ul what do you think about this PR? IMO, this is ready.

kubevirt-bot · 2023-11-30T08:01:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alicefr

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [alicefr]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

vasiliy-ul

@vasiliy-ul what do you think about this PR? IMO, this is ready.

The PR looks good from my perspective. Regarding the issue I raised in #10774 (review), I do not see it as a blocker and do not insist on changing the implementation.

Still leaving it up to @victortoso to unhold :)

vasiliy-ul · 2023-11-30T09:30:53Z

/lgtm

vasiliy-ul · 2023-11-30T09:50:59Z

Removed the embed 36 bytes binary to workaround the #10774 (comment).

BTW, I guess you could have used base64 to store the table in a text file (and silence the checker) and use embed. Then decode it in the test... Just a thought.

victortoso · 2023-11-30T14:37:15Z

Still leaving it up to @victortoso to unhold :)

Thanks @vasiliy-ul and @alicefr for the quick reviews. Let's unhold it then. I'm not sure I can do it, but does not hurt to try:

/unhold

BTW, I guess you could have used base64 to store the table in a text file (and silence the checker) and use embed. Then decode it in the test... Just a thought.

That's neat. I'll keep it in mind for the next time.

victortoso · 2023-11-30T14:37:41Z

/retest

victortoso · 2023-11-30T19:21:14Z

/test pull-kubevirt-check-tests-for-flakes

victortoso · 2023-11-30T20:18:56Z

19:49:20: 19:49:20: 2023/11/30 19:49:20 Image pull was unsuccessful: reading image "quay.io/kubevirt/alpine-ext-kernel-boot-demo@sha256:a2ddb2f568bf3814e594a14bc793d5a655a61d5983f3561d60d02afa7bbc56b4": GET https://quay.io/v2/: unsupported status code 502; body:

/test pull-kubevirt-check-tests-for-flakes

kubevirt-commenter-bot · 2023-12-01T05:23:24Z

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

kubevirt-bot · 2023-12-01T09:36:19Z

@victortoso: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubevirt-check-tests-for-flakes	`6a04bfb`	link	false	`/test pull-kubevirt-check-tests-for-flakes`
pull-kubevirt-e2e-k8s-1.26-sig-compute	`6a04bfb`	link	unknown	`/test pull-kubevirt-e2e-k8s-1.26-sig-compute`

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

victortoso · 2023-12-01T10:31:49Z

/retest

xpivarc · 2023-12-07T13:34:40Z

staging/src/kubevirt.io/api/core/v1/schema.go

+	// SlicNameRef should match the volume name of a secret object. The data in the secret should
+	// be a binary blob that follows the ACPI SLIC standard, see:
+	// https://learn.microsoft.com/en-us/previous-versions/windows/hardware/design/dn653305(v=vs.85)
+	SlicNameRef string `json:"slicNameRef,omitempty"`


Do we need to restrict this to x86? @alicefr @victortoso

I don't think so. I'm double checking and I'll let you know.

Hi @xpivarc, there is no explicit restriction. It should work for Windows on arm, albeit this was not tested.

kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Nov 22, 2023

kubevirt-bot requested review from kbidarkar and vasiliy-ul November 22, 2023 22:00

kubevirt-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API labels Nov 22, 2023

victortoso force-pushed the windows-offline-activation branch from 400cbf2 to 1b0be18 Compare November 22, 2023 22:05

kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 22, 2023

victortoso force-pushed the windows-offline-activation branch from 1b0be18 to 1979262 Compare November 22, 2023 22:25

kubevirt-bot requested a review from alicefr November 23, 2023 07:37

alicefr reviewed Nov 23, 2023

View reviewed changes

kubevirt-bot assigned alicefr Nov 23, 2023

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 23, 2023

vasiliy-ul reviewed Nov 23, 2023

View reviewed changes

victortoso added 5 commits November 23, 2023 22:37

api: add ACPI SLIC table

a9f29e1

The SLIC table is used for generic activation of Windows guests. https://learn.microsoft.com/en-us/previous-versions/windows/hardware/design/dn653305(v=vs.85) Signed-off-by: Victor Toso <victortoso@redhat.com>

virtwrap: converter: factor out v1.Firmware handling

110c7d2

It is specific and big enough logic block. - Return early if firmware is not set - Use local 'firmware' variable Signed-off-by: Victor Toso <victortoso@redhat.com>

virtwrap: handle ACPI SLIC table

77ed9a6

Signed-off-by: Victor Toso <victortoso@redhat.com>

generate: ACPI SLIC table

03f5169

Result of make generate Signed-off-by: Victor Toso <victortoso@redhat.com>

tests: ACPI SLIC table test on Alpine

6a04bfb

Verifies the steps: - Creating a Secret with binary SLIC table - Passing its name to new ACPI api - Creates Guest with ACPI SLIC table Signed-off-by: Victor Toso <victortoso@redhat.com>

victortoso force-pushed the windows-offline-activation branch from 1979262 to 6a04bfb Compare November 23, 2023 21:38

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 23, 2023

kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 30, 2023

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 30, 2023

vasiliy-ul reviewed Nov 30, 2023

View reviewed changes

kubevirt-bot assigned vasiliy-ul Nov 30, 2023

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 30, 2023

kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 30, 2023

kubevirt-bot merged commit c4fc4ab into kubevirt:main Dec 1, 2023
35 of 37 checks passed

xpivarc reviewed Dec 7, 2023

View reviewed changes

chenrui333 mentioned this pull request Mar 6, 2024

virtctl 1.2.0 Homebrew/homebrew-core#165266

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Windows offline activation with ACPI SLIC table #10774

Windows offline activation with ACPI SLIC table #10774

victortoso commented Nov 22, 2023

victortoso commented Nov 22, 2023 •

edited

alicefr commented Nov 23, 2023

alicefr Nov 23, 2023

victortoso Nov 23, 2023

alicefr commented Nov 23, 2023

vasiliy-ul left a comment

vasiliy-ul Nov 23, 2023

victortoso Nov 23, 2023

alicefr Nov 23, 2023

vasiliy-ul Nov 23, 2023

victortoso Nov 23, 2023

victortoso commented Nov 23, 2023

victortoso commented Nov 24, 2023

alicefr commented Nov 30, 2023

kubevirt-bot commented Nov 30, 2023

vasiliy-ul left a comment

vasiliy-ul commented Nov 30, 2023

vasiliy-ul commented Nov 30, 2023

victortoso commented Nov 30, 2023

victortoso commented Nov 30, 2023

victortoso commented Nov 30, 2023

victortoso commented Nov 30, 2023

kubevirt-commenter-bot commented Dec 1, 2023

kubevirt-bot commented Dec 1, 2023 •

edited

victortoso commented Dec 1, 2023

xpivarc Dec 7, 2023

victortoso Dec 11, 2023

victortoso Dec 13, 2023 •

edited

	// SliceNameRef should match the volume name of a secret object. The data in the secret should
	// SlicNameRef should match the volume name of a secret object. The data in the secret should

Windows offline activation with ACPI SLIC table #10774

Windows offline activation with ACPI SLIC table #10774

Conversation

victortoso commented Nov 22, 2023

victortoso commented Nov 22, 2023 • edited

alicefr commented Nov 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alicefr commented Nov 23, 2023

vasiliy-ul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victortoso commented Nov 23, 2023

victortoso commented Nov 24, 2023

alicefr commented Nov 30, 2023

kubevirt-bot commented Nov 30, 2023

vasiliy-ul left a comment

Choose a reason for hiding this comment

vasiliy-ul commented Nov 30, 2023

vasiliy-ul commented Nov 30, 2023

victortoso commented Nov 30, 2023

victortoso commented Nov 30, 2023

victortoso commented Nov 30, 2023

victortoso commented Nov 30, 2023

kubevirt-commenter-bot commented Dec 1, 2023

kubevirt-bot commented Dec 1, 2023 • edited

victortoso commented Dec 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victortoso Dec 13, 2023 • edited

Choose a reason for hiding this comment

victortoso commented Nov 22, 2023 •

edited

kubevirt-bot commented Dec 1, 2023 •

edited

victortoso Dec 13, 2023 •

edited