Skip to content

DRAFT-DRAFT: Cpu profiles #1: CPUID feature restriction#9

Closed
olivereanderson wants to merge 24 commits into
gardenlinux-v46from
oanderson/cpu-profiles
Closed

DRAFT-DRAFT: Cpu profiles #1: CPUID feature restriction#9
olivereanderson wants to merge 24 commits into
gardenlinux-v46from
oanderson/cpu-profiles

Conversation

@olivereanderson

@olivereanderson olivereanderson commented Jul 31, 2025

Copy link
Copy Markdown

This is the first part of a (presumably) three part effort to introduce CPU profiles, which are largely analogous to QEMU's CPU Models.

In short the motivation for this feature is to enable live migrations between hosts that share a reasonably large set of features (identified with a CPU profile). Note that this MR is only concerned with the restriction of CPUID entries for virtual CPUs, but MSR (model specific register) configuration is also necessary for migration compatibility. The latter will be addressed in part 2. While the final part three will just be about updating the existing end user documentation.

Some arguments for the current approach, from a user's perspective, can be found in the following under specified RFC.

Steps to undraft

  • Should it be a hard error to attempt to create a VM on a host that does not support all the features declared for a given CPU profile?
  • Write multiple integration tests using QEMU.
  • Confirm my suspicion that it is enough to perform the restriction in arch::generate_common_cpuid (in other words that it does not need to be done elsewhere in the code base as well).
  • Figure out if we should structure some of the code differently (what goes in which module).
  • Figure out if we are happy with the current macro usage. The macro introduced here can either be improved, or we may prefer defining the feature flag sets in terms of the "official" lowercase cpu id feature names. The alternative way I had in mind is demonstrated here. There are also very valid reasons not to use a macro at all here.
  • Fix git state and the final commit message.

@olivereanderson olivereanderson self-assigned this Jul 31, 2025
@olivereanderson olivereanderson changed the title Cpu profiles #1: CPUID feature restriction DRAFT-DRAFT: Cpu profiles #1: CPUID feature restriction Jul 31, 2025
Comment thread arch/src/x86_64/mod.rs Outdated
Comment thread arch/src/x86_64/mod.rs Outdated
Comment thread arch/src/x86_64/mod.rs Outdated
@olivereanderson

Copy link
Copy Markdown
Author

I see this is quite a bit more involved than I initially thought, but I am making good progress. Will update the MR in the coming days.

dependabot Bot and others added 23 commits August 11, 2025 15:14
Bumps [crate-ci/typos](https://github.com/crate-ci/typos) from 1.33.1 to 1.34.0.
- [Release notes](https://github.com/crate-ci/typos/releases)
- [Changelog](https://github.com/crate-ci/typos/blob/master/CHANGELOG.md)
- [Commits](crate-ci/typos@v1.33.1...v1.34.0)

---
updated-dependencies:
- dependency-name: crate-ci/typos
  dependency-version: 1.34.0
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
When invoking the script chown shows a warning.

    chown: warning: '.' should be ':': ‘1000.1000’

From `info coreutils 'chown invocation'`.

   Some older scripts may still use ‘.’ in place of the ‘:’ separator.
POSIX 1003.1-2001 (*note Standards conformance::) does not require
support for that, but for backward compatibility GNU ‘chown’ supports
‘.’ so long as no ambiguity results, although it issues a warning and
support may be removed in future versions.  New scripts should avoid the
use of ‘.’ because it is not portable, and because it has undesirable
results if the entire OWNER‘.’GROUP happens to identify a user whose
name contains ‘.’.

Signed-off-by: Wei Liu <liuwe@microsoft.com>
@olivereanderson olivereanderson changed the base branch from main to gardenlinux August 11, 2025 13:14
Comment thread docs/macvtap-bridge.md

# Ensure that we can access this device
sudo chown "$UID.$UID" "$tapdevice"
sudo chown "$UID:$UID" "$tapdevice"

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

No idea how this change got here. I have not touched that.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

seems like your branch has the wrong base; why not drop the first two commits and force-push?

- uses: actions/checkout@v4
# Executes "typos ."
- uses: crate-ci/typos@v1.33.1
- uses: crate-ci/typos@v1.34.0

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Also not my doing.

Should fix this before merging.

Comment on lines +467 to +487
pub struct CpuIdFeatureFlags {
pub edx_1: CpuIdEntryRegister<1, 0, { CpuidReg::EDX as u8 }>,
pub ecx_1: CpuIdEntryRegister<1, 0, { CpuidReg::ECX as u8 }>,
pub ebx_7_0: CpuIdEntryRegister<7, 0, { CpuidReg::EBX as u8 }>,
pub ecx_7_0: CpuIdEntryRegister<7, 0, { CpuidReg::ECX as u8 }>,
pub edx_7_0: CpuIdEntryRegister<7, 0, { CpuidReg::EDX as u8 }>,
pub eax_7_1: CpuIdEntryRegister<7, 1, { CpuidReg::EAX as u8 }>,
pub ecx_7_1: CpuIdEntryRegister<7, 1, { CpuidReg::ECX as u8 }>,
pub edx_7_1: CpuIdEntryRegister<7, 1, { CpuidReg::EDX as u8 }>,
pub edx_7_2: CpuIdEntryRegister<7, 2, { CpuidReg::EDX as u8 }>,
pub eax_0dh_1: CpuIdEntryRegister<0xd, 1, { CpuidReg::EAX as u8 }>,
pub ecx_14h: CpuIdEntryRegister<0x14, 0, { CpuidReg::ECX as u8 }>,
pub ecx_24h_1: CpuIdEntryRegister<0x24, 1, { CpuidReg::ECX as u8 }>,
pub edx_8000_0001h: CpuIdEntryRegister<0x8000_0001, 0, { CpuidReg::EDX as u8 }>,
pub ecx_8000_0001h: CpuIdEntryRegister<0x8000_0001, 0, { CpuidReg::ECX as u8 }>,
pub edx_8000_0007h: CpuIdEntryRegister<0x8000_0007, 0, { CpuidReg::EDX as u8 }>,
pub ebx_8000_0008h: CpuIdEntryRegister<0x8000_0008, 0, { CpuidReg::EBX as u8 }>,
/// FEAT_SVM in QEMU
pub edx_8000_000ah: CpuIdEntryRegister<0x8000_000A, 0, { CpuidReg::EDX as u8 }>,
pub eax_8000_0021h: CpuIdEntryRegister<0x8000_0021, 0, { CpuidReg::EAX as u8 }>,
}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

After talking to @snue I think we can avoid this struct and just inline it's restrict method where it is needed. This way we can avoid naming all the fields in this struct (which is kind of duplicate work as the names are reflected in the const generic parameters) and instead just call restrict on each of the relevant CpuIdEntryRegister types directly.

Comment thread arch/src/x86_64/cpu_profiles.rs Outdated
pub(super) struct CascadeLakeServerV1CpuIdFeatures {
edx_1: CpuIdFeatureFlags<1, 0, { CpuidReg::EDX as u8 }>,
ecx_1: CpuIdFeatureFlags<1, 0, { CpuidReg::ECX as u8 }>,
edx_8000_0001h: CpuIdFeatureFlags<0x8000_0001, 0, { CpuidReg::EDX as u8 }>,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't know x86 cpuid well: why does this value look so different to the others?

Comment thread arch/src/x86_64/mod.rs Outdated
Comment thread arch/src/lib.rs Outdated
#[default]
Host,
#[cfg(target_arch = "x86_64")]
CascadeLakeServerV1,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

what is V1, and what is V2 then?

Comment thread arch/src/lib.rs
Comment thread arch/src/x86_64/cpu_profile_feature_flags.rs Outdated
Comment thread arch/src/x86_64/mod.rs Outdated
Comment thread arch/src/x86_64/mod.rs Outdated
Comment thread arch/src/lib.rs Outdated
Comment thread vmm/src/vm.rs
Comment thread arch/src/x86_64/mod.rs Outdated
@phip1611 phip1611 deleted the oanderson/cpu-profiles branch November 28, 2025 08:13
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