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 flags to install latest kernel/tag during SNP host setup #29

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

LakshmiSaiHarika
Copy link
Contributor

@LakshmiSaiHarika LakshmiSaiHarika commented Jan 14, 2025

User can install SNP kernel from the upstream kernel using this PR's new additional command options as follows:

  1. User can install upstream SNP kernel from the master branch.
    ./snp.sh setup-host --upsteam-kernel

  2. User can install specific SNP kernel tagged version from the kernel upstream repository
    ./snp.sh setup-host --upstream-kernel-tag <specific kernel tag>

@LakshmiSaiHarika LakshmiSaiHarika changed the title Install SNP upstream kernel during SNP host setup 1) Install SNP upstream kernel during SNP host setup Jan 14, 2025
@LakshmiSaiHarika LakshmiSaiHarika force-pushed the build-install-upstream-snp-kernel branch from d742431 to dab6bb0 Compare January 15, 2025 21:11
docs/snp.md Outdated
@@ -62,6 +62,14 @@ Setup the host by building SNP patched versions of qemu, ovmf and the linux kern
The `--non-upm` option can be specified with the above command if a non-upm version
of the kernel is desired.

The `--upstr-kernel` option can be specified with the above command if an upstream kernel version from the master branch is desired.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

--upstream-kernel

@LakshmiSaiHarika LakshmiSaiHarika changed the title 1) Install SNP upstream kernel during SNP host setup Add flags to install latest kernel/tag during SNP host setup Jan 17, 2025
tools/snp.sh Outdated
@@ -1319,6 +1347,17 @@ main() {
shift; shift
;;

-u-k|--upstr-kernel)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

--upstream-kernel

tools/snp.sh Outdated
shift;
;;

-k-t|--kernel-tag)
Copy link
Contributor Author

@LakshmiSaiHarika LakshmiSaiHarika Jan 17, 2025

Choose a reason for hiding this comment

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

--upstream-kernel-tag

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add check if kernel tag version exists

docs/snp.md Outdated
@@ -62,6 +62,14 @@ Setup the host by building SNP patched versions of qemu, ovmf and the linux kern
The `--non-upm` option can be specified with the above command if a non-upm version
of the kernel is desired.

The `--upstr-kernel` option can be specified with the above command if an upstream kernel version from the master branch is desired.

The `--kernel-tag` option can be specified with the above command if a specific snp kernel tag version from the upstream kernel is desired as follows:
Copy link
Contributor Author

@LakshmiSaiHarika LakshmiSaiHarika Jan 17, 2025

Choose a reason for hiding this comment

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

The --upstream-kernel-tag option can be specified with the above command to install specific tag version from the upstream kernel:

tools/snp.sh Outdated
@@ -94,6 +95,11 @@ GENERATED_INITRD_BIN="${SETUP_WORKING_DIR}/initrd.img"
AMDSEV_URL="https://github.com/confidential-containers/amdese-amdsev.git"
AMDSEV_DEFAULT_BRANCH="amd-snp"
AMDSEV_NON_UPM_BRANCH="amd-snp-202306070000"
UPSTREAM_KERNEL_HOST_GIT_URL="https://github.com/torvalds/linux.git"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

UPSTREAM_KERNEL_GIT_URL

tools/snp.sh Outdated
@@ -94,6 +95,11 @@ GENERATED_INITRD_BIN="${SETUP_WORKING_DIR}/initrd.img"
AMDSEV_URL="https://github.com/confidential-containers/amdese-amdsev.git"
AMDSEV_DEFAULT_BRANCH="amd-snp"
AMDSEV_NON_UPM_BRANCH="amd-snp-202306070000"
UPSTREAM_KERNEL_HOST_GIT_URL="https://github.com/torvalds/linux.git"
UPSTREAM_KERNEL_HOST_DEFAULT_BRANCH="master"
UPSTREAM_KERNEL_GUEST_GIT_URL="https://github.com/torvalds/linux.git"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

remove this line

tools/snp.sh Outdated
@@ -94,6 +95,11 @@ GENERATED_INITRD_BIN="${SETUP_WORKING_DIR}/initrd.img"
AMDSEV_URL="https://github.com/confidential-containers/amdese-amdsev.git"
AMDSEV_DEFAULT_BRANCH="amd-snp"
AMDSEV_NON_UPM_BRANCH="amd-snp-202306070000"
UPSTREAM_KERNEL_HOST_GIT_URL="https://github.com/torvalds/linux.git"
UPSTREAM_KERNEL_HOST_DEFAULT_BRANCH="master"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

UPSTREAM_KERNEL_BRANCH

tools/snp.sh Outdated
UPSTREAM_KERNEL_HOST_GIT_URL="https://github.com/torvalds/linux.git"
UPSTREAM_KERNEL_HOST_DEFAULT_BRANCH="master"
UPSTREAM_KERNEL_GUEST_GIT_URL="https://github.com/torvalds/linux.git"
UPSTREAM_KERNEL_GUEST_DEFAULT_BRANCH="master"
Copy link
Contributor Author

@LakshmiSaiHarika LakshmiSaiHarika Jan 17, 2025

Choose a reason for hiding this comment

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

remove these lines

tools/snp.sh Outdated
UPSTREAM_KERNEL_HOST_DEFAULT_BRANCH="master"
UPSTREAM_KERNEL_GUEST_GIT_URL="https://github.com/torvalds/linux.git"
UPSTREAM_KERNEL_GUEST_DEFAULT_BRANCH="master"
KERNEL_HOST_GUEST_BRANCH_TAG=""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

UPSTREAM_KERNEL_TAG

tools/snp.sh Outdated
@@ -116,6 +122,8 @@ usage() {
>&2 echo " stop-guests Stop all SNP guests started by this script"
>&2 echo " where OPTIONS are:"
>&2 echo " -n|--non-upm Build AMDSEV non UPM kernel (sev-snp-devel)"
>&2 echo " -u-k|--upstr-kernel Build upstream kernel with the master as default branch"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Build upstream kernel from the master branch

tools/snp.sh Outdated
@@ -116,6 +122,8 @@ usage() {
>&2 echo " stop-guests Stop all SNP guests started by this script"
>&2 echo " where OPTIONS are:"
>&2 echo " -n|--non-upm Build AMDSEV non UPM kernel (sev-snp-devel)"
>&2 echo " -u-k|--upstr-kernel Build upstream kernel with the master as default branch"
>&2 echo " -k-t|--kernel-tag Build upstream kernel with the desired kernel version"
Copy link
Contributor Author

@LakshmiSaiHarika LakshmiSaiHarika Jan 17, 2025

Choose a reason for hiding this comment

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

Build upstream kernel from the desired kernel tag version

@@ -816,6 +824,23 @@ set_acl_for_sev_device() {
echo "${setfacl_command}" | sudo tee -a "${rc_local_file}" >/dev/null
}

set_snp_kernel_upstream() {
local amsev_stable_commits_file="${SETUP_WORKING_DIR}/AMDSEV/stable-commits"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

error out if kernel_tag does not exist

tools/snp.sh Outdated
local amsev_stable_commits_file="${SETUP_WORKING_DIR}/AMDSEV/stable-commits"

# Set kernel source URL to the upstream
sed -i -e "s|^\(KERNEL_HOST_GIT_URL=\).*$|\1\"${UPSTREAM_KERNEL_HOST_GIT_URL}\"|g" "${amsev_stable_commits_file}"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

sed -i -e "s|^(KERNEL_HOST_GIT_URL=).*$|\1"${UPSTREAM_KERNEL_URL}"|g" "${amsev_stable_commits_file}"

tools/snp.sh Outdated
sed -i -e "s|^\(KERNEL_GUEST_GIT_URL=\).*$|\1\"${UPSTREAM_KERNEL_GUEST_GIT_URL}\"|g" "${amsev_stable_commits_file}"

# Set SNP kernel default branch to the kernel tagged input version if specified or the upstream default branch
if [[ ! -z "${KERNEL_HOST_GUEST_BRANCH_TAG}" ]]; then
Copy link
Contributor Author

Choose a reason for hiding this comment

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

[[ -n "${KERNEL_HOST_GUEST_BRANCH_TAG}" ]]

tools/snp.sh Outdated
@@ -839,6 +864,9 @@ build_and_install_amdsev() {
# Delete the ovmf/ directory prior to the build step for ovmf re-initialization
[ ! -d "ovmf" ] || rm -rf "ovmf"

# Set kernel source to the upstream kernel
[ ${SET_KERNEL_UPSTREAM} = "true" ] && set_snp_kernel_upstream
Copy link
Contributor Author

@LakshmiSaiHarika LakshmiSaiHarika Jan 17, 2025

Choose a reason for hiding this comment

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

! ${SET_KERNEL_UPSTREAM} || set_snp_kernel_upstream

Copy link
Contributor Author

@LakshmiSaiHarika LakshmiSaiHarika left a comment

Choose a reason for hiding this comment

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

Feedback session #1 from Ryan, Larry and Diego

@LakshmiSaiHarika LakshmiSaiHarika force-pushed the build-install-upstream-snp-kernel branch 2 times, most recently from 130c354 to cfeb872 Compare January 20, 2025 21:16
@LakshmiSaiHarika
Copy link
Contributor Author

@DGonzalezVillal , @larrydewey and @ryansavino, thanks for the previous feedback session, I modified this PR as per the suggestions. Please re-review this PR and let me know if any further changes are required.

docs/snp.md Outdated
The `--upstream-kernel-tag` option can be specified with the above command to install specific tag version from the upstream kernel:

```
./snp.sh setup-host --upstream-kernel --upstream-kernel-tag <specific kernel tag>
Copy link
Contributor

Choose a reason for hiding this comment

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

Do the flags need to be used together?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using the flag --upstream-kernel-tag only gives the same result as the combination of these flags as the fields get updated with the upstream-kernel-tag version

Using --upstream-kernel only installs upstream kernel from master branch
Using --upstream-kernel-tag only installs upstream kernel from the tag version

@LakshmiSaiHarika LakshmiSaiHarika force-pushed the build-install-upstream-snp-kernel branch from cfeb872 to 5aa2acc Compare January 21, 2025 22:54
@@ -816,6 +822,34 @@ set_acl_for_sev_device() {
echo "${setfacl_command}" | sudo tee -a "${rc_local_file}" >/dev/null
}

# Checks if the kernel tag exists in the kernel upstream using the kernel tag URL HTTP status code
verify_upstream_kernel_tag() {
local kernel_tag_url="https://github.com/torvalds/linux/tree/${UPSTREAM_KERNEL_TAG}"

Choose a reason for hiding this comment

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

# Get the HTTP status code for the kernel tag URL
local kernel_tag_http_status_code=$(curl -s --head "${kernel_tag_url}" | head -n 1 | grep "HTTP" | awk '{print $2}')

tools/snp.sh Outdated

# Kernel tag doesn't exist if the HTTP status code is other than 200
if [ ${kernel_tag_http_status_code} -ne 200 ]; then
>&2 echo -e "ERROR: ${UPSTREAM_KERNEL_TAG} doesn't exist in the upstream kernel"

Choose a reason for hiding this comment

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

  • upstream repository instead of kernel?


set_snp_kernel_upstream() {
local amsev_stable_commits_file="${SETUP_WORKING_DIR}/AMDSEV/stable-commits"

Choose a reason for hiding this comment

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

# Validate that the stable-commits file exists
if [[ ! -f "${amsev_stable_commits_file}" ]]; then
>&2 echo -e "ERROR: The file '${amsev_stable_commits_file}' does not exist."
return 1
fi
Probably a good idea to check if file exists before performing sed operations

Copy link
Contributor Author

@LakshmiSaiHarika LakshmiSaiHarika Jan 24, 2025

Choose a reason for hiding this comment

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

I think the file exists normally as stable-commits exists in AMDSEV repository which comes when we setup the SNP host via command line.

Do we still require this check?

sed -i -e "s|^\(KERNEL_HOST_BRANCH=\).*$|\1\"${UPSTREAM_KERNEL_BRANCH}\"|g" "${amsev_stable_commits_file}"
sed -i -e "s|^\(KERNEL_GUEST_BRANCH=\).*$|\1\"${UPSTREAM_KERNEL_BRANCH}\"|g" "${amsev_stable_commits_file}"
fi
}

Choose a reason for hiding this comment

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

echo "Successfully updated SNP kernel upstream branches in '${amsev_stable_commits_file}'."

Copy link

@AdithyaKrishnan AdithyaKrishnan left a comment

Choose a reason for hiding this comment

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

Left some minor changes. Incorporate them if they feel useful to you.

Sets the SNP host kernel and SNP guest kernel to the linux upstream

User can setup host with the upstream kernel pointing to the master branch(as default) as follows :
	./snp.sh setup-host --upstream-kernel

User can setup host with the desired SNP kernel tag as follows:
	./snp.sh setup-host --upstream-kernel  --upstream-kernel-tag  <user-specific kernel tag>

Signed-off-by: Harika Nittala <[email protected]>
Added additional  kernel options (--upstream-kernel, --upstream-kernel-tag) usage in the docs

Signed-off-by: Harika Nittala <[email protected]>
@LakshmiSaiHarika LakshmiSaiHarika force-pushed the build-install-upstream-snp-kernel branch from 5aa2acc to cd78a30 Compare January 27, 2025 23:52
Copy link

@AdithyaKrishnan AdithyaKrishnan left a comment

Choose a reason for hiding this comment

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

LGTM!
Thanks @LakshmiSaiHarika

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.

3 participants