From b9865082346e2069ebe629e7f73e86bfa5a2b54e Mon Sep 17 00:00:00 2001 From: Takahiro Itazuri Date: Wed, 5 Jul 2023 10:14:19 +0000 Subject: [PATCH 1/4] fix(vmm): Set IA32_ARCH_CAPABILITIES.RRSBA to 1 with T2S We updated the fingerprint files in PR #3813, since Intel microcode release (microcode-20230512) changed to set IA32_ARCH_CAPABILITIES.RRSBA (bit 19) to 1 on Intel CascadeLake CPU. The mitigation itself is already in place which is eIBRS. Since the kernel enables eIBRS by default using SPECTRE_V2_EIBRS mode regardless of the IA32_ARCH_CAPABILITIES.RRSBA bit, hosts and guests should not get impacted by this change. However, it has a role to inform softwares whether the part has the RRSBA behavior. The T2S template has set it to 0 explicitly before, but this commit changes to set it to 1 so that guest kernels and applications can know that the processor has the RRSBA behavior. The reason why it sets the bit to 1 instead of passing through it from the host is that it aims to provide the ability to securely migrate snapshots between Intel Skylake and Intel CascadeLake. Signed-off-by: Takahiro Itazuri --- CHANGELOG.md | 7 +++++++ src/cpuid/src/template/intel/t2s.rs | 3 ++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 30d810e84d1..d05334cb205 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,12 @@ # Changelog +## [Unreleased] + +### Fixes + +- Fixed the T2S CPU template to set the RRSBA bit of the IA32_ARCH_CAPABILITIES + MSR to 1 in accordance with an Intel microcode update. + ## [1.3.3] ### Fixed diff --git a/src/cpuid/src/template/intel/t2s.rs b/src/cpuid/src/template/intel/t2s.rs index 20e845741a9..167374a21f7 100644 --- a/src/cpuid/src/template/intel/t2s.rs +++ b/src/cpuid/src/template/intel/t2s.rs @@ -35,7 +35,8 @@ pub fn update_msr_entries(msr_entries: &mut Vec) { | ArchCapaMSRFlags::SKIP_L1DFL_VMENTRY | ArchCapaMSRFlags::IF_PSCHANGE_MC_NO | ArchCapaMSRFlags::MISC_PACKAGE_CTRLS - | ArchCapaMSRFlags::ENERGY_FILTERING_CTL; + | ArchCapaMSRFlags::ENERGY_FILTERING_CTL + | ArchCapaMSRFlags::RRSBA; msr_entries.push(kvm_msr_entry { index: MSR_IA32_ARCH_CAPABILITIES, data: capabilities.bits(), From 73b88213c47389771eafb41bdfc99dafc4093177 Mon Sep 17 00:00:00 2001 From: Takahiro Itazuri Date: Wed, 5 Jul 2023 13:14:21 +0000 Subject: [PATCH 2/4] fix(vmm): Pass through IA32_ARCH_CAPABILITIES.{RSBA,RRSBA} with T2CL We updated the fingerprint files in PR #3813, since Intel microcode release (microcode-20230512) changed to set IA32_ARCH_CAPABILITIES.RRSBA (bit 19) to 1 on Intel CascadeLake CPU. The mitigation itself is already in place which is eIBRS. Since the kernel enables eIBRS by default using SPECTRE_V2_EIBRS mode regardless of the IA32_ARCH_CAPABILITIES.RRSBA bit, hosts and guests should not get impacted by this change. However, it has a role to inform softwares whether the part has the RRSBA behavior. The T2CL template has set the RRSBA bit to 0 explicitly before, but this commit changes to pass through the bit from the host so that guest kernels and applications can know that the processor has the RRSBA behavior. The reason why it passes through the bit from the host opposed to the T2S template is that the T2CL template is not designed to allow snapshot migration between different CPU models. In addition to the RRSBA bit, this comit also changes to pass through the RSBA bit, as it is safer to let guest know these informative bits of the host CPU than to overwrite them with templates. Signed-off-by: Takahiro Itazuri --- CHANGELOG.md | 3 ++ src/arch/src/x86_64/msr.rs | 30 ++++++++++++++++ src/cpuid/src/template/intel/t2cl.rs | 34 ++++++++++++++++--- src/vmm/src/vstate/vcpu/x86_64.rs | 8 +++-- .../integration_tests/build/test_coverage.py | 2 +- .../functional/test_feat_parity.py | 2 ++ 6 files changed, 72 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d05334cb205..11db03c400e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,9 @@ - Fixed the T2S CPU template to set the RRSBA bit of the IA32_ARCH_CAPABILITIES MSR to 1 in accordance with an Intel microcode update. +- Fixed the T2CL CPU template to pass through the RSBA and RRSBA bits of the + IA32_ARCH_CAPABILITIES MSR from the host in accordance with an Intel microcode + update. ## [1.3.3] diff --git a/src/arch/src/x86_64/msr.rs b/src/arch/src/x86_64/msr.rs index 23204b21c49..9772d5ba720 100644 --- a/src/arch/src/x86_64/msr.rs +++ b/src/arch/src/x86_64/msr.rs @@ -331,6 +331,36 @@ pub fn supported_guest_msrs(kvm_fd: &Kvm) -> Result { Ok(msr_list) } +/// Error type for [`get_msrs`]. +#[derive(Debug, thiserror::Error)] +pub enum GetMsrError { + /// Failed to create `Msrs`. + #[error("Could not create `Msrs`")] + Fam(utils::fam::Error), + /// Getting a MSR resulted in an error. + #[error("Getting a MSR resulted in an error: {0}")] + Get(#[from] kvm_ioctls::Error), +} + +/// Get a value for the given MSR index. +/// +/// # Arguments +/// +/// * `vcpu_fd` - Structure for the vCPU that holds the vCPU's fd. +/// * `index` - MSR index to query +pub fn get_msr(vcpu_fd: &VcpuFd, index: u32) -> std::result::Result { + let mut msrs = Msrs::from_entries(&[kvm_msr_entry { + index, + ..Default::default() + }]) + .map_err(GetMsrError::Fam)?; + + vcpu_fd.get_msrs(&mut msrs).map_err(GetMsrError::Get)?; + + // The access to the 0-th item is safe, since the size of `msrs` is 1. + Ok(msrs.as_slice()[0].data) +} + #[cfg(test)] mod tests { use kvm_ioctls::Kvm; diff --git a/src/cpuid/src/template/intel/t2cl.rs b/src/cpuid/src/template/intel/t2cl.rs index 0ae7ab86eda..c36a7f1ff26 100644 --- a/src/cpuid/src/template/intel/t2cl.rs +++ b/src/cpuid/src/template/intel/t2cl.rs @@ -73,16 +73,20 @@ pub fn set_cpuid_entries(kvm_cpuid: &mut CpuId, vm_spec: &VmSpec) -> Result<(), } /// Add the MSR entries speciffic to this T2CL template. -pub fn update_msr_entries(msr_entries: &mut Vec) { - let capabilities = ArchCapaMSRFlags::RDCL_NO +pub fn update_msr_entries(msr_entries: &mut Vec, default_arch_cap: u64) { + let arch_cap = ArchCapaMSRFlags::RDCL_NO | ArchCapaMSRFlags::IBRS_ALL | ArchCapaMSRFlags::SKIP_L1DFL_VMENTRY | ArchCapaMSRFlags::MDS_NO | ArchCapaMSRFlags::IF_PSCHANGE_MC_NO | ArchCapaMSRFlags::TSX_CTRL; + + // Pass through RSBA and RRSBA bits if they are set. + let rsba_rrbsa = default_arch_cap & (ArchCapaMSRFlags::RSBA | ArchCapaMSRFlags::RRSBA).bits(); + msr_entries.push(kvm_msr_entry { index: MSR_IA32_ARCH_CAPABILITIES, - data: capabilities.bits(), + data: arch_cap.bits() | rsba_rrbsa, ..Default::default() }); } @@ -220,8 +224,10 @@ mod tests { #[test] fn test_update_msr_entries() { + // Case 1: The default IA32_ARCH_CAPABILITIES MSR does not enumerate RSBA and RRSBA. let mut msrs = Vec::::new(); - update_msr_entries(&mut msrs); + let default_arch_cap = 0; + update_msr_entries(&mut msrs, default_arch_cap); let arch_cap = msrs[0]; assert_eq!(arch_cap.index, MSR_IA32_ARCH_CAPABILITIES); @@ -235,5 +241,25 @@ mod tests { | ArchCapaMSRFlags::TSX_CTRL) .bits() ); + + // Case 2: The default IA32_ARCH_CAPABILITIES MSR enumerates both RSBA and RRSBA. + let mut msrs = Vec::::new(); + let default_arch_cap = (ArchCapaMSRFlags::RSBA | ArchCapaMSRFlags::RRSBA).bits(); + update_msr_entries(&mut msrs, default_arch_cap); + let arch_cap = msrs[0]; + + assert_eq!(arch_cap.index, MSR_IA32_ARCH_CAPABILITIES); + assert_eq!( + arch_cap.data, + (ArchCapaMSRFlags::RDCL_NO + | ArchCapaMSRFlags::IBRS_ALL + | ArchCapaMSRFlags::RSBA + | ArchCapaMSRFlags::SKIP_L1DFL_VMENTRY + | ArchCapaMSRFlags::MDS_NO + | ArchCapaMSRFlags::IF_PSCHANGE_MC_NO + | ArchCapaMSRFlags::TSX_CTRL + | ArchCapaMSRFlags::RRSBA) + .bits() + ); } } diff --git a/src/vmm/src/vstate/vcpu/x86_64.rs b/src/vmm/src/vstate/vcpu/x86_64.rs index f578f6e089c..0bd02f57e22 100644 --- a/src/vmm/src/vstate/vcpu/x86_64.rs +++ b/src/vmm/src/vstate/vcpu/x86_64.rs @@ -10,7 +10,7 @@ use std::fmt::{Display, Formatter}; use std::{fmt, result}; use arch::x86_64::interrupts; -use arch::x86_64::msr::SetMSRsError; +use arch::x86_64::msr::{GetMsrError, SetMSRsError, MSR_IA32_ARCH_CAPABILITIES}; use arch::x86_64::regs::{SetupFpuError, SetupRegistersError, SetupSpecialRegistersError}; use cpuid::{c3, filter_cpuid, msrs_to_save_by_cpuid, t2, t2a, t2cl, t2s, VmSpec}; use kvm_bindings::{ @@ -191,6 +191,8 @@ pub enum KvmVcpuConfigureError { VmSpec(cpuid::Error), #[error("Failed to filter CPUID: {0}")] FilterCpuid(cpuid::Error), + #[error("Failed to get a MSR: {0}")] + GetMsr(#[from] GetMsrError), #[error("Failed to set CPUID entries: {0}")] SetCpuidEntries(cpuid::Error), #[error("Failed to set CPUID: {0}")] @@ -317,7 +319,9 @@ impl KvmVcpu { } CpuFeaturesTemplate::T2CL => { self.msr_list.extend(t2cl::msr_entries_to_save()); - t2cl::update_msr_entries(&mut msr_boot_entries); + let default_arch_cap = + arch::x86_64::msr::get_msr(&self.fd, MSR_IA32_ARCH_CAPABILITIES)?; + t2cl::update_msr_entries(&mut msr_boot_entries, default_arch_cap); } _ => (), } diff --git a/tests/integration_tests/build/test_coverage.py b/tests/integration_tests/build/test_coverage.py index 1a9aae6a7d8..ddee25fd1f0 100644 --- a/tests/integration_tests/build/test_coverage.py +++ b/tests/integration_tests/build/test_coverage.py @@ -17,7 +17,7 @@ # Checkout the cpuid crate. In the future other # differences may appear. if utils.is_io_uring_supported(): - COVERAGE_DICT = {"Intel": 82.98, "AMD": 82.14, "ARM": 82.43} + COVERAGE_DICT = {"Intel": 82.95, "AMD": 82.14, "ARM": 82.43} else: COVERAGE_DICT = {"Intel": 80.13, "AMD": 79.28, "ARM": 79.34} diff --git a/tests/integration_tests/functional/test_feat_parity.py b/tests/integration_tests/functional/test_feat_parity.py index 7fe00632e29..85fd887c5c9 100644 --- a/tests/integration_tests/functional/test_feat_parity.py +++ b/tests/integration_tests/functional/test_feat_parity.py @@ -281,6 +281,8 @@ def test_feat_parity_msr_arch_cap(vm): (1 << 6) | # IF_PSCHANGE_MC_NO (1 << 7) # TSX_CTRL ) + if global_props.cpu_model == "Intel(R) Xeon(R) Platinum 8259CL CPU @ 2.50GHz": + expected |= (1 << 19) # RRSBA # fmt: on assert actual == expected, f"{actual=:#x} != {expected=:#x}" elif cpu_template == "T2A": From 955049d655949b54b7c436bac45c57804e88711b Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Fri, 2 Jun 2023 09:43:01 +0100 Subject: [PATCH 3/4] chore: adjust buildkite timeout m6a.metal tests have been timing out due to too many different combinations of firecracker versions being tested in the snapshot tests. Signed-off-by: Patrick Roy --- .buildkite/pipeline_pr.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.buildkite/pipeline_pr.py b/.buildkite/pipeline_pr.py index 75dbf9e7e6e..3750ea9338a 100755 --- a/.buildkite/pipeline_pr.py +++ b/.buildkite/pipeline_pr.py @@ -33,7 +33,7 @@ def get_changed_files(branch): "platforms": DEFAULT_PLATFORMS, # buildkite step parameters "priority": DEFAULT_PRIORITY, - "timeout_in_minutes": 30, + "timeout_in_minutes": 45, } build_grp = group( From 578bba8dc6414322aa11158a0274534b664aafed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Barb=C3=A1chano?= Date: Tue, 9 May 2023 13:27:10 +0200 Subject: [PATCH 4/4] test: delete test_versionized_serialization_benchmark.py MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Removing as it is a microbenchmark and not representative or a real workload. If there is a performance regression on this piece, the snapshot/restore should be able to detect it. Signed-off-by: Pablo Barbáchano --- .../test_versioned_serialization_benchmark.py | 121 ------------------ 1 file changed, 121 deletions(-) delete mode 100644 tests/integration_tests/performance/test_versioned_serialization_benchmark.py diff --git a/tests/integration_tests/performance/test_versioned_serialization_benchmark.py b/tests/integration_tests/performance/test_versioned_serialization_benchmark.py deleted file mode 100644 index fb2723ea079..00000000000 --- a/tests/integration_tests/performance/test_versioned_serialization_benchmark.py +++ /dev/null @@ -1,121 +0,0 @@ -# Copyright 2020 Amazon.com, Inc. or its affiliates. All Rights Reserved. -# SPDX-License-Identifier: Apache-2.0 -"""A test that ensures that all unit tests pass at integration time.""" - -import json -import logging -import os -import platform -import shutil -from pathlib import Path - -import pytest - -from framework import utils -from framework.defs import FC_WORKSPACE_DIR -from host_tools import proc - -BENCHMARK_DIRECTORY = "{}/src/vmm".format(FC_WORKSPACE_DIR) -DEFAULT_BUILD_TARGET = "{}-unknown-linux-musl".format(platform.machine()) - -PROC_MODEL = proc.proc_type() - -NSEC_IN_MSEC = 1000000 - -BASELINES = { - "Intel": { - "serialize": { - "no-crc": {"target": 0.205, "delta": 0.050}, # milliseconds # milliseconds - "crc": {"target": 0.244, "delta": 0.44}, # milliseconds # milliseconds - }, - "deserialize": { - "no-crc": {"target": 0.056, "delta": 0.02}, # milliseconds # milliseconds - "crc": {"target": 0.075, "delta": 0.030}, # milliseconds # milliseconds - }, - }, - "AMD": { - "serialize": { - "no-crc": {"target": 0.084, "delta": 0.05}, # milliseconds # milliseconds - "crc": {"target": 0.108, "delta": 0.025}, # milliseconds # milliseconds - }, - "deserialize": { - "no-crc": {"target": 0.030, "delta": 0.02}, # milliseconds # milliseconds - "crc": {"target": 0.052, "delta": 0.04}, # milliseconds # milliseconds - }, - }, - "ARM": { - "serialize": { - "no-crc": {"target": 0.050, "delta": 0.03}, # milliseconds # milliseconds - "crc": {"target": 0.075, "delta": 0.025}, # milliseconds # milliseconds - }, - "deserialize": { - "no-crc": {"target": 0.060, "delta": 0.02}, # milliseconds # milliseconds - "crc": {"target": 0.065, "delta": 0.02}, # milliseconds # milliseconds - }, - }, -} - - -def _check_statistics(directory, mean): - proc_model = [item for item in BASELINES if item in PROC_MODEL] - assert len(proc_model) == 1, "Could not get processor model!" - - if "deserialize" in directory.lower(): - bench = "deserialize" - else: - bench = "serialize" - - if "crc" in directory.lower(): - attribute = "crc" - else: - attribute = "no-crc" - - measure = BASELINES[proc_model[0]][bench][attribute] - target, delta = measure["target"], measure["delta"] - assert target == pytest.approx( - mean, abs=delta, rel=1e-6 - ), f"Benchmark result {directory} has changed!" - - return f"{target - delta} <= result <= {target + delta}" - - -def test_serialization_benchmark(monkeypatch, record_property): - """ - Benchmark test for MicrovmState serialization/deserialization. - - @type: performance - """ - logger = logging.getLogger("serialization_benchmark") - - # Move into the benchmark directory - monkeypatch.chdir(BENCHMARK_DIRECTORY) - - # Run benchmark test - cmd = "cargo bench --target {}".format(DEFAULT_BUILD_TARGET) - result = utils.run_cmd_sync(cmd) - assert result.returncode == 0 - - # Parse each Criterion benchmark from the result folder and - # check the results against a baseline - results_dir = Path(FC_WORKSPACE_DIR) / "build/vmm_benchmark" - for directory in os.listdir(results_dir): - # Ignore the 'report' directory as it is of no use to us - if directory == "report": - continue - - logger.info("Benchmark: %s", directory) - - # Retrieve the 'estimates.json' file content - json_file = results_dir / directory / "base/estimates.json" - estimates = json.loads(json_file.read_text()) - - # Save the Mean measurement(nanoseconds) and transform it(milliseconds) - mean = estimates["mean"]["point_estimate"] / NSEC_IN_MSEC - logger.info("Mean: %f", mean) - - criteria = _check_statistics(directory, mean) - record_property(f"{directory}_ms", mean) - record_property(f"{directory}_criteria", criteria) - - # Cleanup the Target directory - shutil.rmtree(results_dir)