Added additional SNP checks on the host and guest via verification of SNP bit status from instruction set #12
Conversation
LakshmiSaiHarika
commented
Jun 26, 2024
LakshmiSaiHarika
commented
- Added check on the host for all host SEV features in CPU via cpuid 0x8000001f instruction set
- Added additional check on the guest to see if guest sev, sev-es, and snp bits are active from MSR 0xc0010131 (MSR_AMD64_SEV)
There was a problem hiding this comment.
Will you please run these through shellcheck? There are a number of changes which I would like to see applied for this PR before we approve it.
|
Hi @larrydewey, I made few changes and did shellcheck... Please let me know if any further changes are required. |
LakshmiSaiHarika
force-pushed
the
add-snp-check-via-instruction-set
branch
from
4ba7408 to
4ff4dcd
Compare
|
Hi @larrydewey I addressed the changes as per the conversation above, please let me know if any additional changes are required |
docs/snp.md
Outdated
| Read the dedicated host cpuid Fn8000_001F[EAX] instruction set to verify if the SNP is on and supported on the host: | ||
| ``` | ||
| ./snp.sh check-host-snp-cpuid | ||
| ``` | ||
|
|
||
|
|
There was a problem hiding this comment.
Do we want this to be it's own individual command? I see that it is being called when people are setting up the host and launching the guest. Is there additional functionality to be it's own individual command?
tools/snp.sh
Outdated
| local host_cpuid_eax | ||
| host_cpuid_eax=$(cpuid -1 -r -l 0x8000001f | grep -oE "eax=[[:alnum:]]+ " | cut -c5- | tr -d '[:space:]') | ||
|
|
There was a problem hiding this comment.
I had created a function that does this for you called get_cpuid if you want to use that instead.
Line 933 in 4419036
tools/snp.sh
Outdated
| declare -A actual_sev_snp_bit_status=( | ||
| [SME]=$((${host_cpuid_eax} & 1)) | ||
| [SEV]=$((${host_cpuid_eax} & 2)) | ||
| [SEV-ES]=$((${host_cpuid_eax} & 8)) | ||
| [SNP]=$((${host_cpuid_eax} & 16)) | ||
| ) | ||
|
|
||
| # Map all the expected host sev/snp bit values in a single associative array | ||
| declare -A expected_sev_snp_bit_value=( | ||
| [SME]=1 | ||
| [SEV]=2 | ||
| [SEV-ES]=8 | ||
| [SNP]=16 | ||
| ) |
There was a problem hiding this comment.
You could just grab the specific bit for each of the features and that way you don't have to create two different arrays.
tools/snp.sh
Outdated
| declare -A actual_sev_snp_bit_status=( | ||
| [SEV]=$((${guest_msr_read} & 1)) | ||
| [SEV-ES]=$((${guest_msr_read} & 2)) | ||
| [SNP]=$((${guest_msr_read} & 4)) | ||
| ) | ||
|
|
||
| declare -A expected_sev_snp_bit_value=( | ||
| [SEV]=1 | ||
| [SEV-ES]=2 | ||
| [SNP]=4 | ||
| ) |
There was a problem hiding this comment.
Same thing as above. You can just get the specific bit for each feature and that way you don't need 2 different arrays.
| @@ -1228,6 +1325,7 @@ main() { | |||
|
|
|||
| setup_and_launch_guest | |||
| wait_and_retry_command verify_snp_guest | |||
| wait_and_retry_command verify_platform_snp_bit_status guest | |||
There was a problem hiding this comment.
This should maybe replace verify_snp_guest. They are essentially doing the same thing, the only difference is that the original function is checking dmesg, while the bit_status is looking at the guest MSR to verify enablement. No need to run both of them if the MSR check is working correctly.
…able Read the dedicated host cpuid 0x8000001f instruction set to check if SNP is on and supported Signed-off-by: Harika <[email protected]> Add shift operator for SNP host check Signed-off-by: Harika Nittala <[email protected]>
This check reads the dedicated guest MSR(MSR_AMD64_SEV, MSR 0xc0010131) to determine if SNP is on and supported. Signed-off-by: Harika Nittala <[email protected]>
LakshmiSaiHarika
force-pushed
the
add-snp-check-via-instruction-set
branch
from
4ff4dcd to
699d8b1
Compare
| [SEV-ES]=$(( (${host_cpuid_eax} >> 3) & 1)) | ||
| [SNP]=$(( (${host_cpuid_eax} >> 4) & 1)) | ||
| ) | ||
|
|
There was a problem hiding this comment.
Additionally to the CPUID (which checks if the feature is supported by the CPU) there is also an MSR that you can use to check if SME and SNP are enabled.
MSR 0xC0010010 bit 23 checks SME
MSR 0xC0010010 bit 24 checks SNP
| # Install guest rdmsr package dependencies to insert & insert guest msr module | ||
| ssh_guest_command "sudo DEBIAN_FRONTEND=noninteractive sudo apt install -y msr-tools > /dev/null 2>&1" > /dev/null 2>&1 | ||
| ssh_guest_command "sudo modprobe msr" > /dev/null 2>&1 |
There was a problem hiding this comment.
Will this only work on Debian?
| guest_msr_read=$(ssh_guest_command "sudo rdmsr -p 0 0xc0010131") | ||
| guest_msr_read=$(echo "${guest_msr_read}" | tr -d '\r' | bc) |
There was a problem hiding this comment.
In one of my tools that I built out to do this, I had an issue with padding. If I had a bit number that looked like 0000001, my msr read would return 1 not the full number. So when I tried to reach a bit that was a 0 I'd get a an issue trying to reach a digit that was not there. Have you checked for this issue? Try running this command in a guest with no SEV enabled and see if the fail case doesn't cause an error.
