Add RHEL support to snp.sh #2

LakshmiSaiHarika · 2023-10-06T23:23:15Z

Changed AMDSEV URL and AMDSEV branch for AMDSEV build with RHEL fixes
rhel_install_dependencies for rhel library package manager dependencies for AMDSEV branch.
requires subscription manager credential for installing RedHat libraries
set_grub_default_snp() using grubby tool for RHEL.
Changing the default kernel in Red Hat Enterprise Linux 8 & 9
https://access.redhat.com/solutions/4326431
Modified save_binary_paths() due to the
>differences in the location of guest kernel file path for ubuntu and rhel
>differences in the boot menu for initial ram disk images(initd.img- for ubuntu and initramfs- for rhel)

ryansavino · 2023-10-10T23:49:00Z

The PR should group like changes together for a required feature. Separate further more distinct parts into separate commit messages.

PR Title: Add RHEL support to snp.sh
Commit 1: RHEL support for setup-host option
Commit 2: ...etc...

Squash the existing two commits. The previous debug changes you made do not need to be part of the tree.

tools/snp.sh

1. Changed AMDSEV URL and AMDSEV branch for AMDSEV build with RHEL fixes 2. rhel_install_dependencies for rhel library package manager dependencies for AMDSEV branch. requires subscription manager credential for installing RedHat libraries 3. set_grub_default_snp() using grubby tool for RHEL 4. Modified save_binary_paths() due to the differences in the location of guest kernel file path for ubuntu and rhel and differences in the boot menu for initial ram disk images(initd.img-<kernel-version> for ubuntu and initramfs-<kernel-version> for rhel) Signed-off-by: Harika <lnittala@amd.com>

larrydewey

Some minor changes to apply. I would also recommend going to check the code at https://www.shellcheck.net/ to make sure we are addressing linting and possible exploit coverage.

larrydewey · 2023-10-17T19:49:34Z

tools/snp.sh

+    ;;
+  rhel)
+    # Can't Initialize CLOUD_INIT_IMAGE_URL for redhat due to redhat subscription requirement
+    echo "Download Red Hat Enterprise Linux 9.2 KVM Guest Image from RedHat Login" 


Change this from static 9.2 to the version in question being installed.

larrydewey · 2023-10-17T19:50:18Z

tools/snp.sh

+install_common_dependencies(){
+  # pip issue on 20.04 - some openssl bug
+  #sudo rm -f "/usr/lib/python3/dist-packages/OpenSSL/crypto.py"
+  pip install sev-snp-measure


Do we need to worry about Ubuntu using pip3 vs pip?

larrydewey · 2023-10-17T19:51:28Z

tools/snp.sh

+  pip install sev-snp-measure
+
+  # Rust is required to build snpguest
+  curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs -sSf | sh -s -- -y


We should not assume that the program is installed. Let's make sure we check before we call it.

larrydewey · 2023-10-17T19:52:35Z

tools/snp.sh

+  sudo dnf install -y git
+  sudo dnf install -y make automake gcc gcc-c++ kernel-devel 


Suggested change

sudo dnf install -y git

sudo dnf install -y make automake gcc gcc-c++ kernel-devel

sudo dnf install -y git make automake gcc gcc-c++ kernel-devel

larrydewey · 2023-10-17T19:53:23Z

tools/snp.sh

+  sudo dnf install -y ninja-build
+  sudo dnf install -y pkg-config
+  sudo dnf install  -y glib2-devel
+  sudo dnf install  -y pixman-devel
+  sudo dnf install -y libslirp-devel
+
+  # ovmf dependencies
+  sudo dnf install -y uuid-devel
+  sudo dnf install -y iasl


Suggested change

sudo dnf install -y ninja-build

sudo dnf install -y pkg-config

sudo dnf install -y glib2-devel

sudo dnf install -y pixman-devel

sudo dnf install -y libslirp-devel

# ovmf dependencies

sudo dnf install -y uuid-devel

sudo dnf install -y iasl

sudo dnf install -y ninja-build pkg-config glib2-devel pixman-devel libslirp-devel

# ovmf dependencies

sudo dnf install -y uuid-devel iasl

larrydewey · 2023-10-17T19:53:58Z

tools/snp.sh

+  sudo apt install -y ninja-build pkg-config
+  sudo apt install -y libglib2.0-dev
+  sudo apt install -y libpixman-1-dev
+  sudo apt install -y libslirp-dev


Suggested change

sudo apt install -y ninja-build pkg-config

sudo apt install -y libglib2.0-dev

sudo apt install -y libpixman-1-dev

sudo apt install -y libslirp-dev

sudo apt install -y ninja-build pkg-config libglib2.0-dev libpixman-1-dev libslirp-dev

LakshmiSaiHarika · 2023-10-23T21:04:52Z

tools/snp.sh

  if ! $UPM; then
-    SETUP_WORKING_DIR="${SETUP_WORKING_DIR}/non-upm"
+    SETUP_WORKING_DIR="${SETUP_WORKING_DIR}/sev-snp-devel"


Small Question

Is this AMDSEV/sev-snp-devel branch actively used now?
Asking this as I need to make changes in the sev-snp-devel branch for RedHat and Fedora environment.
Till now I focused on snp-latest branch

Yes. We need to determine how to handle sev-snp-devel. Do not change the non-upm identifier text. We can discuss this with Mike.

ryansavino · 2023-10-24T21:03:53Z

tools/snp.sh

+  echo "Enter RedHat subscription Manager credentials"
+  read -p "Username: " RHEL_SUBS_MGR_USER
+  read -sp "Password: " RHEL_SUBS_MGR_PASS
+}


Can we add a check to see if the subscription manager status is already active and only prompt if not active? Can you check out this example and test it out?
https://access.redhat.com/discussions/2217891

Also, only prompt for values if not already exported in the variables.

ryansavino · 2023-10-24T21:11:55Z

tools/snp.sh


 # Save binary paths in source file
 cat > "${SETUP_WORKING_DIR}/source-bins" <<EOF
 QEMU_BIN="${SETUP_WORKING_DIR}/AMDSEV/qemu/build/qemu-system-x86_64"
-OVMF_BIN="${SETUP_WORKING_DIR}/AMDSEV/ovmf/Build/AmdSev/DEBUG_GCC5/FV/OVMF.fd"
+OVMF_BIN="${SETUP_WORKING_DIR}/AMDSEV/ovmf/Build/OvmfX64/DEBUG_GCC5/FV/OVMF.fd"


Why was this changed? If you use the standard OVMF package, it does not have support for direct boot.

I changed it as I could not get AmdSev package in ovmf folder.
I was using the package I got after AMDSEV build step from the snp-latest branch.

tools/snp.sh

ryansavino · 2023-10-24T21:27:38Z

tools/snp.sh

+  # dracut built initrd. This dependency is removed for now due to this reason. For now,
+  # initrd is installed with the kernel debian package on the guest, and then scp-ed back to
+  # the host for direct-boot use.
+  sudo apt install -y pkg-config libkmod-dev


Why is this uncommented now?

Sorry.. didn't notice this change earlier
I will modify

ryansavino · 2023-10-24T21:30:47Z

tools/snp.sh

+  sudo dnf install -y make automake gcc gcc-c++ kernel-devel 
+
+  # Enable RedHat Repository for qemu dependencies
+  sudo subscription-manager register --username ${RHEL_SUBS_MGR_USER} --password ${RHEL_SUBS_MGR_PASS} --force


Why register twice? Same comment as before.

ryansavino · 2023-10-24T21:36:53Z

tools/snp.sh

-  source "${HOME}/.cargo/env" 2>/dev/null
-
-  echo "true" > "${dependencies_installed_file}"
+install_dependencies(){


Move the other install_dependencies methods up here. The goal being to keep the ubuntu method closely tied to the code on the original. That way we can see the changes as opposed to seeing a giant delete block on the original and new code on the PR. Keep like code grouped together.

ryansavino · 2023-10-24T21:38:26Z

tools/snp.sh

+    # For UBUNTU, it is /boot/initrd.img-<kernel-version>  
+    # For RHEL, fedora it is /initramfs-<kernel-version>.img 
+# For standardizing, I want to try creating inital ramdisk image manually (apart from intrd which comes from kernel package installaion) using commands like dracut, 
+# but I wanted to confirm before I try


We need to discuss this. The above should be left alone as default value.

ryansavino · 2023-10-24T21:38:57Z

tools/snp.sh

 SNPGUEST_URL="https://github.com/virtee/snpguest.git"
 SNPGUEST_BRANCH="tags/v0.2.2"
 NASM_SOURCE_TAR_URL="https://www.nasm.us/pub/nasm/releasebuilds/2.16.01/nasm-2.16.01.tar.gz"
-CLOUD_INIT_IMAGE_URL="https://cloud-images.ubuntu.com/jammy/current/jammy-server-cloudimg-amd64.img"
+# CLOUD_INIT_IMAGE_URL initialized under set_cloud_init_url_based_on_linux_distribution()


These variables should live here. Create different variables for different distros.

ryansavino · 2023-10-24T21:55:00Z

tools/snp.sh

  if ! $UPM; then
-    SETUP_WORKING_DIR="${SETUP_WORKING_DIR}/non-upm"
+    SETUP_WORKING_DIR="${SETUP_WORKING_DIR}/sev-snp-devel"


Yes. We need to determine how to handle sev-snp-devel. Do not change the non-upm identifier text. We can discuss this with Mike.

ryansavino · 2023-10-24T21:56:31Z

tools/snp.sh

+  sudo dnf install -y rsync
+  sudo dnf install -y ncurses-devel
+
+  # libssl-dev is openssl-devel in RHEL


Why are you calling this out here? I don't see it being installed.

I added rsync and ncurses-devel here as I faced dependencies issue in the amd_sev_snp_build_step.

ryansavino · 2023-10-24T21:57:12Z

tools/snp.sh

+  sudo dnf install -y ncurses-devel
+
+  # libssl-dev is openssl-devel in RHEL
+  # rpm-build -- Scripts and executable programs used to build packages


Comment:

# RPM package building dependencies

Is this a kernel dependency?

Yes, this is a kernel dependency

ryansavino · 2023-10-24T23:03:51Z

tools/snp.sh

+      # Get Host CPU Architecture info( like: x86_64. x86.. so on)
+      # My Assumption: Guest CPU has same cpu architecture as host architecture 
+      # Reason: Guest using CPU type "host" may increase the VM performance
+      local host_arch=$(arch)


You shouldn't need to do this. A regex to determine the only compiled file should work here.

ryansavino · 2023-10-24T23:04:14Z

tools/snp.sh

+
+      # Copy and rename guest snp kernel from bzImage to vmlinuz
+      local bzImage_file=$(realpath ${SETUP_WORKING_DIR}/AMDSEV/linux/guest/arch/$host_arch/boot/bzImage)
+      cp -v $bzImage_file ${SETUP_WORKING_DIR}/AMDSEV/linux/guest/vmlinuz-$guest_kernel_version


If you're doing the cp, might as well do it for all OSes and then you can share more code here.

LakshmiSaiHarika changed the title ~~setup-host option for RHEL~~ WIP: setup-host option for RHEL Oct 9, 2023

LakshmiSaiHarika changed the title ~~WIP: setup-host option for RHEL~~ setup-host option for RHEL Oct 10, 2023

ryansavino requested changes Oct 10, 2023

View reviewed changes

LakshmiSaiHarika force-pushed the rhel-snp branch 2 times, most recently from 4c515b3 to d95cd76 Compare October 16, 2023 20:51

LakshmiSaiHarika changed the title ~~setup-host option for RHEL~~ Add RHEL support to snp.sh Oct 16, 2023

LakshmiSaiHarika force-pushed the rhel-snp branch 2 times, most recently from 89eba48 to 05bf82b Compare October 16, 2023 22:55

LakshmiSaiHarika force-pushed the rhel-snp branch from 05bf82b to 77a5748 Compare October 16, 2023 23:01

larrydewey requested changes Oct 17, 2023

View reviewed changes

LakshmiSaiHarika commented Oct 23, 2023

View reviewed changes

ryansavino reviewed Oct 24, 2023

View reviewed changes

ryansavino requested changes Oct 24, 2023

View reviewed changes

LakshmiSaiHarika marked this pull request as draft December 7, 2023 00:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add RHEL support to snp.sh #2

Add RHEL support to snp.sh #2

LakshmiSaiHarika commented Oct 6, 2023 •

edited

Loading

ryansavino commented Oct 10, 2023

larrydewey left a comment

larrydewey Oct 17, 2023

larrydewey Oct 17, 2023

larrydewey Oct 17, 2023

larrydewey Oct 17, 2023

larrydewey Oct 17, 2023

larrydewey Oct 17, 2023

LakshmiSaiHarika Oct 23, 2023

ryansavino Oct 24, 2023

ryansavino Oct 24, 2023

ryansavino Oct 24, 2023

LakshmiSaiHarika Oct 24, 2023

ryansavino Oct 24, 2023

LakshmiSaiHarika Oct 24, 2023 •

edited

Loading

ryansavino Oct 24, 2023

ryansavino Oct 24, 2023

ryansavino Oct 24, 2023

ryansavino Oct 24, 2023

ryansavino Oct 24, 2023

ryansavino Oct 24, 2023

LakshmiSaiHarika Oct 24, 2023

ryansavino Oct 24, 2023

ryansavino Oct 24, 2023

LakshmiSaiHarika Oct 24, 2023

ryansavino Oct 24, 2023

ryansavino Oct 24, 2023

		sudo dnf install -y git
		sudo dnf install -y make automake gcc gcc-c++ kernel-devel

	sudo dnf install -y git
	sudo dnf install -y make automake gcc gcc-c++ kernel-devel
	sudo dnf install -y git make automake gcc gcc-c++ kernel-devel

Add RHEL support to snp.sh #2

Are you sure you want to change the base?

Add RHEL support to snp.sh #2

Conversation

LakshmiSaiHarika commented Oct 6, 2023 • edited Loading

ryansavino commented Oct 10, 2023

larrydewey 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

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

Choose a reason for hiding this comment

LakshmiSaiHarika Oct 24, 2023 • edited Loading

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

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

Choose a reason for hiding this comment

LakshmiSaiHarika commented Oct 6, 2023 •

edited

Loading

LakshmiSaiHarika Oct 24, 2023 •

edited

Loading