Skip to content

Commit 95fc34b

Browse files
zulinx86zulinx86
committed
refactor(vmm): Bit-range manipulation with RangeInclusive
Since CPUID notation uses the upper bound inclusive, use RangeInclusive instead of Range. Also, make set_range() and get_range() more readable and reliable by always validating that `y` fits within the given range and by asserting `end` is less than 32. Signed-off-by: Takahiro Itazuri <[email protected]>
1 parent f915b23 commit 95fc34b

File tree

3 files changed

+61
-73
lines changed

3 files changed

+61
-73
lines changed

‎src/vmm/src/cpu_config/x86_64/cpuid/amd/normalize.rs‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -229,15 +229,15 @@ impl super::AmdCpuid {
229229
// Fn8000_0008_ECX[NC]. A value of zero indicates that legacy methods must be
230230
// used to determine the maximum number of logical processors, as indicated by
231231
// CPUID Fn8000_0008_ECX[NC].
232-
set_range(&mut leaf_80000008.result.ecx, 12..16, THREAD_ID_MAX_SIZE).unwrap();
232+
set_range(&mut leaf_80000008.result.ecx, 12..=15, THREAD_ID_MAX_SIZE).unwrap();
233233

234234
// CPUID Fn8000_0008_ECX[7:0] (Field Name: NC)
235235
// Number of physical threads - 1. The number of threads in the processor is NT+1
236236
// (e.g., if NT = 0, then there is one thread). See “Legacy Method” on page 633.
237237
let sub = cpu_count
238238
.checked_sub(1)
239239
.ok_or(FeatureEntryError::NumberOfPhysicalThreadsOverflow)?;
240-
set_range(&mut leaf_80000008.result.ecx, 0..8, u32::from(sub))
240+
set_range(&mut leaf_80000008.result.ecx, 0..=7, u32::from(sub))
241241
.map_err(FeatureEntryError::NumberOfPhysicalThreads)?;
242242

243243
Ok(())
@@ -263,7 +263,7 @@ impl super::AmdCpuid {
263263
// 011b Level 3
264264
// 111b-100b Reserved.
265265
// ```
266-
let cache_level = get_range(subleaf.result.eax, 5..8);
266+
let cache_level = get_range(subleaf.result.eax, 5..=7);
267267

268268
// CPUID Fn8000_001D_EAX_x[25:14] (Field Name: NumSharingCache)
269269
// Specifies the number of logical processors sharing the cache enumerated by N,
@@ -283,7 +283,7 @@ impl super::AmdCpuid {
283283
1 | 2 => {
284284
// SAFETY: We know `cpus_per_core > 0` therefore this is always safe.
285285
let sub = u32::from(cpus_per_core.checked_sub(1).unwrap());
286-
set_range(&mut subleaf.result.eax, 14..26, sub)
286+
set_range(&mut subleaf.result.eax, 14..=25, sub)
287287
.map_err(|err| ExtendedCacheTopologyError::NumSharingCache(i, err))?;
288288
}
289289
// L3 Cache
@@ -292,7 +292,7 @@ impl super::AmdCpuid {
292292
let sub = cpu_count
293293
.checked_sub(1)
294294
.ok_or(ExtendedCacheTopologyError::NumSharingCacheOverflow(i))?;
295-
set_range(&mut subleaf.result.eax, 14..26, u32::from(sub))
295+
set_range(&mut subleaf.result.eax, 14..=25, u32::from(sub))
296296
.map_err(|err| ExtendedCacheTopologyError::NumSharingCache(i, err))?;
297297
}
298298
_ => (),
@@ -331,13 +331,13 @@ impl super::AmdCpuid {
331331

332332
// CPUID Fn8000_001E_EAX[31:0] (Field Name: ExtendedApicId)
333333
// Extended APIC ID. If MSR0000_001B[ApicEn] = 0, this field is reserved.
334-
set_range(&mut leaf_8000001e.result.eax, 0..32, u32::from(cpu_index))
334+
set_range(&mut leaf_8000001e.result.eax, 0..=31, u32::from(cpu_index))
335335
.map_err(ExtendedApicIdError::ExtendedApicId)?;
336336

337337
// CPUID Fn8000_001E_EBX[7:0] (Field Name: ComputeUnitId)
338338
// Compute unit ID. Identifies a Compute Unit, which may be one or more physical cores that
339339
// each implement one or more logical processors.
340-
set_range(&mut leaf_8000001e.result.ebx, 0..8, core_id)
340+
set_range(&mut leaf_8000001e.result.ebx, 0..=7, core_id)
341341
.map_err(ExtendedApicIdError::ComputeUnitId)?;
342342

343343
// CPUID Fn8000_001E_EBX[15:8] (Field Name: ThreadsPerComputeUnit)
@@ -354,7 +354,7 @@ impl super::AmdCpuid {
354354
//
355355
// SAFETY: We know `cpus_per_core > 0` therefore this is always safe.
356356
let sub = u32::from(cpus_per_core.checked_sub(1).unwrap());
357-
set_range(&mut leaf_8000001e.result.ebx, 8..16, sub)
357+
set_range(&mut leaf_8000001e.result.ebx, 8..=15, sub)
358358
.map_err(ExtendedApicIdError::ThreadPerComputeUnit)?;
359359

360360
// CPUID Fn8000_001E_ECX[10:8] (Field Name: NodesPerProcessor)
@@ -364,14 +364,14 @@ impl super::AmdCpuid {
364364
//
365365
// SAFETY: We know the value always fits within the range and thus is always safe.
366366
// Set nodes per processor.
367-
set_range(&mut leaf_8000001e.result.ecx, 8..11, NODES_PER_PROCESSOR).unwrap();
367+
set_range(&mut leaf_8000001e.result.ecx, 8..=10, NODES_PER_PROCESSOR).unwrap();
368368

369369
// CPUID Fn8000_001E_ECX[7:0] (Field Name: NodeId)
370370
// Specifies the ID of the node containing the current logical processor. NodeId
371371
// values are unique across the system.
372372
//
373373
// Put all the cpus in the same node.
374-
set_range(&mut leaf_8000001e.result.ecx, 0..8, 0).unwrap();
374+
set_range(&mut leaf_8000001e.result.ecx, 0..=7, 0).unwrap();
375375

376376
Ok(())
377377
}

‎src/vmm/src/cpu_config/x86_64/cpuid/intel/normalize.rs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ impl super::IntelCpuid {
9999

100100
// CPUID.04H:EAX[7:5]
101101
// Cache Level (Starts at 1)
102-
let cache_level = get_range(subleaf.result.eax, 5..8);
102+
let cache_level = get_range(subleaf.result.eax, 5..=7);
103103

104104
// CPUID.04H:EAX[25:14]
105105
// Maximum number of addressable IDs for logical processors sharing this cache.
@@ -116,7 +116,7 @@ impl super::IntelCpuid {
116116
// The L1 & L2 cache is shared by at most 2 hyperthreads
117117
1 | 2 => {
118118
let sub = u32::from(cpus_per_core.checked_sub(1).unwrap());
119-
set_range(&mut subleaf.result.eax, 14..26, sub)
119+
set_range(&mut subleaf.result.eax, 14..=25, sub)
120120
.map_err(DeterministicCacheError::MaxCpusPerCore)?;
121121
}
122122
// L3 Cache
@@ -127,7 +127,7 @@ impl super::IntelCpuid {
127127
.checked_sub(1)
128128
.ok_or(DeterministicCacheError::MaxCpusPerCoreUnderflow)?,
129129
);
130-
set_range(&mut subleaf.result.eax, 14..26, sub)
130+
set_range(&mut subleaf.result.eax, 14..=25, sub)
131131
.map_err(DeterministicCacheError::MaxCpusPerCore)?;
132132
}
133133
_ => (),
@@ -150,7 +150,7 @@ impl super::IntelCpuid {
150150
let sub = u32::from(cores)
151151
.checked_sub(1)
152152
.ok_or(DeterministicCacheError::MaxCorePerPackageUnderflow)?;
153-
set_range(&mut subleaf.result.eax, 26..32, sub)
153+
set_range(&mut subleaf.result.eax, 26..=31, sub)
154154
.map_err(DeterministicCacheError::MaxCorePerPackage)?;
155155
} else {
156156
break;

‎src/vmm/src/cpu_config/x86_64/cpuid/normalize.rs‎

Lines changed: 47 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -95,66 +95,54 @@ pub fn set_bit(x: &mut u32, bit: u8, y: bool) {
9595
}
9696

9797
/// Sets a given range to a given value.
98-
#[allow(clippy::arithmetic_side_effects)]
9998
pub fn set_range(
10099
x: &mut u32,
101-
range: std::ops::Range<u8>,
100+
range: std::ops::RangeInclusive<u8>,
102101
y: u32,
103102
) -> Result<(), CheckedAssignError> {
104-
debug_assert!(range.end >= range.start);
105-
match range.end - range.start {
106-
z @ 0..=31 => {
107-
if y >= 2u32.pow(u32::from(z)) {
108-
Err(CheckedAssignError)
109-
} else {
110-
let shift = y << range.start;
111-
*x = shift | (*x & !mask(range));
112-
Ok(())
113-
}
114-
}
115-
32 => {
116-
let shift = y << range.start;
117-
*x = shift | (*x & !mask(range));
118-
Ok(())
119-
}
120-
33.. => Err(CheckedAssignError),
103+
let start = *range.start();
104+
let end = *range.end();
105+
106+
debug_assert!(end >= start);
107+
debug_assert!(end < 32);
108+
109+
// Ensure `y` fits within the number of bits in the specified range.
110+
// Note that
111+
// - 1 <= `num_bits` <= 32 from the above assertion
112+
// - if `num_bits` equals to 32, `y` always fits within it since `y` is `u32`.
113+
let num_bits = end - start + 1;
114+
if num_bits < 32 && y >= (1u32 << num_bits) {
115+
return Err(CheckedAssignError);
121116
}
117+
118+
let mask = get_mask(range);
119+
*x = (*x & !mask) | (y << start);
120+
121+
Ok(())
122122
}
123+
123124
/// Gets a given range within a given value.
124-
#[allow(clippy::arithmetic_side_effects)]
125-
pub fn get_range(x: u32, range: std::ops::Range<u8>) -> u32 {
126-
debug_assert!(range.end >= range.start);
127-
(x & mask(range.clone())) >> range.start
125+
pub fn get_range(x: u32, range: std::ops::RangeInclusive<u8>) -> u32 {
126+
let start = *range.start();
127+
let end = *range.end();
128+
129+
debug_assert!(end >= start);
130+
debug_assert!(end < 32);
131+
132+
let mask = get_mask(range);
133+
(x & mask) >> start
128134
}
129135

130136
/// Returns a mask where the given range is ones.
131-
#[allow(
132-
clippy::as_conversions,
133-
clippy::arithmetic_side_effects,
134-
clippy::cast_possible_truncation
135-
)]
136-
const fn mask(range: std::ops::Range<u8>) -> u32 {
137-
/// Returns a value where in the binary representation all bits to the right of the x'th bit
138-
/// from the left are 1.
139-
#[allow(clippy::unreachable)]
140-
const fn shift(x: u8) -> u32 {
141-
if x == 0 {
142-
0
143-
} else if x < u32::BITS as u8 {
144-
(1 << x) - 1
145-
} else if x == u32::BITS as u8 {
146-
u32::MAX
147-
} else {
148-
unreachable!()
149-
}
137+
const fn get_mask(range: std::ops::RangeInclusive<u8>) -> u32 {
138+
let num_bits = *range.end() - *range.start() + 1;
139+
let shift = *range.start();
140+
141+
if num_bits == 32 {
142+
u32::MAX
143+
} else {
144+
((1u32 << num_bits) - 1) << shift
150145
}
151-
152-
debug_assert!(range.end >= range.start);
153-
debug_assert!(range.end <= u32::BITS as u8);
154-
155-
let front = shift(range.start);
156-
let back = shift(range.end);
157-
!front & back
158146
}
159147

160148
// We use this 2nd implementation so we can conveniently define functions only used within
@@ -228,7 +216,7 @@ impl super::Cpuid {
228216

229217
// CPUID.01H:EBX[15:08]
230218
// CLFLUSH line size (Value * 8 = cache line size in bytes; used also by CLFLUSHOPT).
231-
set_range(&mut leaf_1.result.ebx, 8..16, 8).map_err(FeatureInformationError::Clflush)?;
219+
set_range(&mut leaf_1.result.ebx, 8..=15, 8).map_err(FeatureInformationError::Clflush)?;
232220

233221
// CPUID.01H:EBX[23:16]
234222
// Maximum number of addressable IDs for logical processors in this physical package.
@@ -240,15 +228,15 @@ impl super::Cpuid {
240228
get_max_cpus_per_package(cpu_count)
241229
.map_err(FeatureInformationError::GetMaxCpusPerPackage)?,
242230
);
243-
set_range(&mut leaf_1.result.ebx, 16..24, max_cpus_per_package)
231+
set_range(&mut leaf_1.result.ebx, 16..=23, max_cpus_per_package)
244232
.map_err(FeatureInformationError::SetMaxCpusPerPackage)?;
245233

246234
// CPUID.01H:EBX[31:24]
247235
// Initial APIC ID.
248236
//
249237
// The 8-bit initial APIC ID in EBX[31:24] is replaced by the 32-bit x2APIC ID, available
250238
// in Leaf 0BH and Leaf 1FH.
251-
set_range(&mut leaf_1.result.ebx, 24..32, u32::from(cpu_index))
239+
set_range(&mut leaf_1.result.ebx, 24..=31, u32::from(cpu_index))
252240
.map_err(FeatureInformationError::InitialApicId)?;
253241

254242
// CPUID.01H:ECX[15] (Mnemonic: PDCM)
@@ -345,18 +333,18 @@ impl super::Cpuid {
345333
0 => {
346334
// To get the next level APIC ID, shift right with at most 1 because we have
347335
// maximum 2 logical procerssors per core that can be represented by 1 bit.
348-
set_range(&mut subleaf.result.eax, 0..5, u32::from(cpu_bits))
336+
set_range(&mut subleaf.result.eax, 0..=4, u32::from(cpu_bits))
349337
.map_err(|err| ExtendedTopologyError::RightShiftBits(index, err))?;
350338

351339
// When cpu_count == 1 or HT is disabled, there is 1 logical core at this
352340
// domain; otherwise there are 2
353-
set_range(&mut subleaf.result.ebx, 0..16, u32::from(cpus_per_core))
341+
set_range(&mut subleaf.result.ebx, 0..=15, u32::from(cpus_per_core))
354342
.map_err(|err| ExtendedTopologyError::NumLogicalProcs(index, err))?;
355343

356344
// Skip setting 0 to ECX[7:0] since it's already reset to 0.
357345

358346
// Set the domain type identification value for logical processor,
359-
set_range(&mut subleaf.result.ecx, 8..16, 1)
347+
set_range(&mut subleaf.result.ecx, 8..=15, 1)
360348
.map_err(|err| ExtendedTopologyError::DomainType(index, err))?;
361349
}
362350
// Core domain
@@ -368,19 +356,19 @@ impl super::Cpuid {
368356
// 2^N is greater than or equal to the maximum number of vCPUs.
369357
set_range(
370358
&mut subleaf.result.eax,
371-
0..5,
359+
0..=4,
372360
MAX_SUPPORTED_VCPUS.next_power_of_two().ilog2(),
373361
)
374362
.map_err(|err| ExtendedTopologyError::RightShiftBits(index, err))?;
375-
set_range(&mut subleaf.result.ebx, 0..16, u32::from(cpu_count))
363+
set_range(&mut subleaf.result.ebx, 0..=15, u32::from(cpu_count))
376364
.map_err(|err| ExtendedTopologyError::NumLogicalProcs(index, err))?;
377365

378366
// Setting the input ECX value (i.e. `index`)
379-
set_range(&mut subleaf.result.ecx, 0..8, index)
367+
set_range(&mut subleaf.result.ecx, 0..=7, index)
380368
.map_err(|err| ExtendedTopologyError::InputEcx(index, err))?;
381369

382370
// Set the domain type identification value for core.
383-
set_range(&mut subleaf.result.ecx, 8..16, 2)
371+
set_range(&mut subleaf.result.ecx, 8..=15, 2)
384372
.map_err(|err| ExtendedTopologyError::DomainType(index, err))?;
385373
}
386374
_ => {

0 commit comments

Comments
 (0)