Skip to content

Commit 332a2a1

Browse files
jackschefer-msftjackschefer-msft
authored
pci: update PciConfigSpace to pass partial-DWORDs through all devices (#3856)
This change builds on #3751 and #3830 to push partial-DWORD config space accesses to all devices in the `PciConfigSpace` trait. Updated devices fall into a couple categories: - Bus type devices (vpci, legacy pci, pcie) now no longer do read-modify-write on partial accesses, they just pass the corresponding byte-enabled value downstream through the trait - Endpoint devices that delegate all config space accesses to shared `pci_core::cfg_space_emu` helpers continue to do so, passing the byte-enabled values now - Endpoint devices with custom config space emulation are updated on a case by case basis.
1 parent b3f87b0 commit 332a2a1

44 files changed

Lines changed: 1429 additions & 799 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎Cargo.lock‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -624,7 +624,6 @@ dependencies = [
624624
"parking_lot",
625625
"range_map_vec",
626626
"tracing",
627-
"zerocopy",
628627
]
629628

630629
[[package]]

‎openvmm/openvmm_core/src/worker/dispatch.rs‎

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,9 @@ use crate::worker::rom::RomBuilder;
2323
use acpi::dsdt;
2424
use anyhow::Context;
2525
use cfg_if::cfg_if;
26+
use chipset_device::io::IoResult;
27+
use chipset_device::pci::ByteEnabledDwordRead;
28+
use chipset_device::pci::ByteEnabledDwordWrite;
2629
use chipset_device_resources::IRQ_LINE_SET;
2730
use chipset_resources::LEGACY_CHIPSET_PCI_BUS_NAME;
2831
use chipset_resources::cmos_rtc_time_source::SystemTimeClockHandle;
@@ -4150,11 +4153,7 @@ struct WeakMutexPciBusDevice(
41504153
);
41514154

41524155
impl pci_bus::GenericPciBusDevice for WeakMutexPciBusDevice {
4153-
fn pci_cfg_read(
4154-
&mut self,
4155-
offset: u16,
4156-
value: &mut u32,
4157-
) -> Option<chipset_device::io::IoResult> {
4156+
fn pci_cfg_read(&mut self, offset: u16, value: ByteEnabledDwordRead<'_>) -> Option<IoResult> {
41584157
Some(
41594158
self.0
41604159
.upgrade()?
@@ -4164,7 +4163,7 @@ impl pci_bus::GenericPciBusDevice for WeakMutexPciBusDevice {
41644163
)
41654164
}
41664165

4167-
fn pci_cfg_write(&mut self, offset: u16, value: u32) -> Option<chipset_device::io::IoResult> {
4166+
fn pci_cfg_write(&mut self, offset: u16, value: ByteEnabledDwordWrite) -> Option<IoResult> {
41684167
Some(
41694168
self.0
41704169
.upgrade()?
@@ -4180,8 +4179,8 @@ impl pci_bus::GenericPciBusDevice for WeakMutexPciBusDevice {
41804179
target_bus: u8,
41814180
function: u8,
41824181
offset: u16,
4183-
value: &mut u32,
4184-
) -> Option<chipset_device::io::IoResult> {
4182+
value: ByteEnabledDwordRead<'_>,
4183+
) -> Option<IoResult> {
41854184
Some(
41864185
self.0
41874186
.upgrade()?
@@ -4197,8 +4196,8 @@ impl pci_bus::GenericPciBusDevice for WeakMutexPciBusDevice {
41974196
target_bus: u8,
41984197
function: u8,
41994198
offset: u16,
4200-
value: u32,
4201-
) -> Option<chipset_device::io::IoResult> {
4199+
value: ByteEnabledDwordWrite,
4200+
) -> Option<IoResult> {
42024201
Some(
42034202
self.0
42044203
.upgrade()?

‎vm/chipset_device/src/pci.rs‎

Lines changed: 31 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -8,10 +8,11 @@ use crate::io::IoError;
88
use crate::io::IoResult;
99
use inspect::Inspect;
1010
use inspect::InspectMut;
11+
use mesh::MeshPayload;
1112
use zerocopy::IntoBytes;
1213

1314
/// Byte enables for the four lanes of a PCI configuration DWORD.
14-
#[derive(Debug, Clone, Copy, PartialEq, Eq, Inspect)]
15+
#[derive(Debug, Clone, Copy, PartialEq, Eq, Inspect, MeshPayload)]
1516
pub struct PciConfigByteEnable(u8);
1617

1718
impl PciConfigByteEnable {
@@ -135,11 +136,23 @@ impl ByteEnabledDwordWrite {
135136
Self::new(temp, byte_enable)
136137
}
137138

139+
/// Retrieve a mutable slice of the enabled byte lanes of the DWORD.
140+
pub fn as_valid_byte_slice(&self) -> &[u8] {
141+
let (byte_offset, len) = self.byte_enable.to_byte_offset_len();
142+
let byte_offset = byte_offset as usize;
143+
&self.value.as_bytes()[byte_offset..byte_offset + len]
144+
}
145+
138146
/// Returns true if all byte lanes are enabled.
139147
pub const fn is_full(self) -> bool {
140148
self.byte_enable.is_full()
141149
}
142150

151+
/// Retrieve the underlying byte enable.
152+
pub const fn byte_enable(&self) -> PciConfigByteEnable {
153+
self.byte_enable
154+
}
155+
143156
/// Get the mask of valid bytes.
144157
pub const fn valid_mask(self) -> u32 {
145158
self.byte_enable.mask()
@@ -208,10 +221,20 @@ impl<'a> ByteEnabledDwordRead<'a> {
208221

209222
/// Fill the intercept buffer with the enabled byte lanes of the DWORD.
210223
pub fn fill_intercept_buffer(self, buffer: &mut [u8]) {
224+
let src = self.into_valid_byte_slice();
225+
buffer.copy_from_slice(src);
226+
}
227+
228+
/// Retrieve a mutable slice of the enabled byte lanes of the DWORD.
229+
pub fn into_valid_byte_slice(self) -> &'a mut [u8] {
211230
let (byte_offset, len) = self.byte_enable.to_byte_offset_len();
212231
let byte_offset = byte_offset as usize;
213-
let src = self.value.as_bytes()[byte_offset..byte_offset + len].as_ref();
214-
buffer.copy_from_slice(src);
232+
&mut self.value.as_mut_bytes()[byte_offset..byte_offset + len]
233+
}
234+
235+
/// Get the mask of valid bytes.
236+
pub const fn valid_mask(&self) -> u32 {
237+
self.byte_enable.mask()
215238
}
216239

217240
/// Update the value of the DWORD, honoring byte enables.
@@ -316,9 +339,9 @@ impl PciConfigAddress {
316339
/// Implemented by devices which have a PCI config space.
317340
pub trait PciConfigSpace: ChipsetDevice {
318341
/// Dispatch a PCI config space read to the device with the given address.
319-
fn pci_cfg_read(&mut self, offset: u16, value: &mut u32) -> IoResult;
342+
fn pci_cfg_read(&mut self, offset: u16, value: ByteEnabledDwordRead<'_>) -> IoResult;
320343
/// Dispatch a PCI config space write to the device with the given address.
321-
fn pci_cfg_write(&mut self, offset: u16, value: u32) -> IoResult;
344+
fn pci_cfg_write(&mut self, offset: u16, value: ByteEnabledDwordWrite) -> IoResult;
322345

323346
/// Handle a PCI configuration space read with full routing context.
324347
///
@@ -356,12 +379,12 @@ pub trait PciConfigSpace: ChipsetDevice {
356379
target_bus: u8,
357380
function: u8,
358381
offset: u16,
359-
value: &mut u32,
382+
mut value: ByteEnabledDwordRead<'_>,
360383
) -> IoResult {
361384
if secondary_bus == target_bus && function == 0 {
362385
self.pci_cfg_read(offset, value)
363386
} else {
364-
*value = !0;
387+
value.set(!0);
365388
IoResult::Ok
366389
}
367390
}
@@ -401,7 +424,7 @@ pub trait PciConfigSpace: ChipsetDevice {
401424
target_bus: u8,
402425
function: u8,
403426
offset: u16,
404-
value: u32,
427+
value: ByteEnabledDwordWrite,
405428
) -> IoResult {
406429
if secondary_bus == target_bus && function == 0 {
407430
self.pci_cfg_write(offset, value)

‎vm/chipset_device_fuzz/Cargo.toml‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@ range_map_vec.workspace = true
1616

1717
arbitrary = { workspace = true, features = ["derive"] }
1818
parking_lot.workspace = true
19-
zerocopy.workspace = true
2019
tracing.workspace = true
2120

2221
[lints]

‎vm/chipset_device_fuzz/src/lib.rs‎

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,8 @@ use chipset_device::io::IoResult;
2020
use chipset_device::io::deferred::DeferredToken;
2121
use chipset_device::mmio::ControlMmioIntercept;
2222
use chipset_device::mmio::RegisterMmioIntercept;
23+
use chipset_device::pci::ByteEnabledDwordRead;
24+
use chipset_device::pci::ByteEnabledDwordWrite;
2325
use chipset_device::pio::ControlPortIoIntercept;
2426
use chipset_device::pio::RegisterPortIoIntercept;
2527
use closeable_mutex::CloseableMutex;
@@ -32,7 +34,6 @@ use std::sync::Weak;
3234
use std::task::Context;
3335
use std::task::Poll;
3436
use std::task::Waker;
35-
use zerocopy::FromBytes;
3637

3738
type InterceptRanges<U> =
3839
Arc<RwLock<RangeMap<U, (Box<str>, Weak<CloseableMutex<dyn ChipsetDevice>>)>>>;
@@ -171,18 +172,22 @@ impl FuzzChipset {
171172
fn pci_read(&self, bdf: (u8, u8, u8), offset: u16, data: &mut [u8]) -> Option<()> {
172173
let dev = self.pci_devices.get(&bdf)?.upgrade().unwrap();
173174
let mut locked_dev = dev.lock();
175+
let mut value = 0;
174176
let result = locked_dev
175177
.supports_pci()
176178
.expect("objects on the pci bus support pci")
177-
.pci_cfg_read(offset, u32::mut_from_bytes(data).unwrap());
179+
.pci_cfg_read(
180+
offset,
181+
ByteEnabledDwordRead::with_all_bytes_enabled(&mut value),
182+
);
178183
// Convert to a non-deferred result
179184
let result = match result {
180185
IoResult::Ok => Ok(()),
181186
IoResult::Err(e) => Err(e),
182187
IoResult::Defer(t) => self.defer_read_now_or_never(&mut *locked_dev, t, data),
183188
};
184189
match result {
185-
Ok(()) => {}
190+
Ok(()) => data.copy_from_slice(&value.to_ne_bytes()[..data.len()]),
186191
Err(_) => {
187192
data.fill(0);
188193
}
@@ -197,7 +202,7 @@ impl FuzzChipset {
197202
let result = locked_dev
198203
.supports_pci()
199204
.expect("objects on the pci bus support pci")
200-
.pci_cfg_write(offset, value);
205+
.pci_cfg_write(offset, ByteEnabledDwordWrite::with_all_bytes_enabled(value));
201206
match result {
202207
IoResult::Ok => {}
203208
IoResult::Err(_) => {}
@@ -348,7 +353,7 @@ impl FuzzChipset {
348353
let attached_bdfs = self.pci_devices.keys().collect::<Vec<_>>();
349354
let bdf = **u.choose(&attached_bdfs)?;
350355

351-
let offset = u.int_in_range(0..=4096)?; // pci-e max cfg space size
356+
let offset = u.int_in_range(0..=1023)? * 4; // pci-e max cfg space size
352357

353358
if matches!(action_kind, ChipsetActionKind::PciRead) {
354359
ChipsetAction::PciRead { bdf, offset }

‎vm/devices/chipset_legacy/src/i440bx_host_pci_bridge/mod.rs‎

Lines changed: 15 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,8 @@ pub use chipset_resources::i440bx_host_pci_bridge::GpaState;
1111
use chipset_device::ChipsetDevice;
1212
use chipset_device::io::IoError;
1313
use chipset_device::io::IoResult;
14+
use chipset_device::pci::ByteEnabledDwordRead;
15+
use chipset_device::pci::ByteEnabledDwordWrite;
1416
use chipset_device::pci::PciConfigSpace;
1517
use inspect::Inspect;
1618
use inspect::InspectMut;
@@ -157,13 +159,13 @@ impl ChipsetDevice for HostPciBridge {
157159
}
158160

159161
impl PciConfigSpace for HostPciBridge {
160-
fn pci_cfg_read(&mut self, offset: u16, value: &mut u32) -> IoResult {
161-
*value = match ConfigSpace(offset) {
162+
fn pci_cfg_read(&mut self, offset: u16, mut value: ByteEnabledDwordRead<'_>) -> IoResult {
163+
value.set(match ConfigSpace(offset) {
162164
// for bug-for-bug compat with the hyper-v implementation: return
163165
// hardcoded status register instead of letting the config space
164166
// emulator take care of it
165167
_ if offset == pci_core::spec::cfg_space::HeaderType00::STATUS_COMMAND.0 => 0x02000006,
166-
_ if offset < 0x40 => return self.cfg_space.read_u32(offset, value),
168+
_ if offset < 0x40 => return self.cfg_space.read_byte_enabled(offset, value),
167169
ConfigSpace::PAM1 => self.state.pam_reg1,
168170
ConfigSpace::PAM2 => self.state.pam_reg2,
169171
ConfigSpace::DRB_1 => self.state.host_pci_dram1,
@@ -198,29 +200,31 @@ impl PciConfigSpace for HostPciBridge {
198200
tracing::debug!(?offset, "unimplemented config space read");
199201
return IoResult::Err(IoError::InvalidRegister);
200202
}
201-
};
203+
});
202204

203205
IoResult::Ok
204206
}
205207

206-
fn pci_cfg_write(&mut self, offset: u16, value: u32) -> IoResult {
208+
fn pci_cfg_write(&mut self, offset: u16, value: ByteEnabledDwordWrite) -> IoResult {
207209
match ConfigSpace(offset) {
208-
_ if offset < 0x40 => return self.cfg_space.write_u32(offset, value),
209-
ConfigSpace::DRB_1 => self.state.host_pci_dram1 = value,
210-
ConfigSpace::DRB_2 => self.state.host_pci_dram2 = value,
210+
_ if offset < 0x40 => return self.cfg_space.write_byte_enabled(offset, value),
211+
ConfigSpace::DRB_1 => value.merge_into(&mut self.state.host_pci_dram1),
212+
ConfigSpace::DRB_2 => value.merge_into(&mut self.state.host_pci_dram2),
211213
ConfigSpace::PAM1 => {
214+
let value = value.merge(self.state.pam_reg1);
212215
self.adjust_bios_override_ranges(value, self.state.pam_reg2, false);
213216
}
214217
ConfigSpace::PAM2 => {
218+
let value = value.merge(self.state.pam_reg2);
215219
self.adjust_bios_override_ranges(self.state.pam_reg1, value, false);
216220
}
217-
ConfigSpace::BSPAD_1 => self.state.bios_scratch1 = value,
218-
ConfigSpace::BSPAD_2 => self.state.bios_scratch2 = value,
221+
ConfigSpace::BSPAD_1 => value.merge_into(&mut self.state.bios_scratch1),
222+
ConfigSpace::BSPAD_2 => value.merge_into(&mut self.state.bios_scratch2),
219223
ConfigSpace::SMRAM => {
220224
// Configuration registers 70-71 are reserved. Only 72-73 (the top 16
221225
// bits of this four-byte range) are defined. We'll therefore shift
222226
// off the bottom portion.
223-
let mut new_smm_word = (value >> 16) as u16;
227+
let mut new_smm_word = value.merge_high(self.state.smm_config_word);
224228

225229
// If the register is "locked" (i.e. bit 4 has been set), then
226230
// all of the other bits become read-only.

‎vm/devices/chipset_legacy/src/piix4_pci_isa_bridge/mod.rs‎

Lines changed: 19 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,8 @@ pub mod resolver;
88
use chipset_device::ChipsetDevice;
99
use chipset_device::io::IoError;
1010
use chipset_device::io::IoResult;
11+
use chipset_device::pci::ByteEnabledDwordRead;
12+
use chipset_device::pci::ByteEnabledDwordWrite;
1113
use chipset_device::pci::PciConfigSpace;
1214
use chipset_device::pio::PortIoIntercept;
1315
use inspect::Inspect;
@@ -222,13 +224,13 @@ impl PortIoIntercept for PciIsaBridge {
222224
}
223225

224226
impl PciConfigSpace for PciIsaBridge {
225-
fn pci_cfg_read(&mut self, offset: u16, value: &mut u32) -> IoResult {
226-
*value = match ConfigSpace(offset) {
227+
fn pci_cfg_read(&mut self, offset: u16, mut value: ByteEnabledDwordRead<'_>) -> IoResult {
228+
value.set(match ConfigSpace(offset) {
227229
// for bug-for-bug compat with the hyper-v implementation: return
228230
// hardcoded status register instead of letting the config space
229231
// emulator take care of it
230232
_ if offset == pci_core::spec::cfg_space::HeaderType00::STATUS_COMMAND.0 => 0x02000007,
231-
_ if offset < 0x40 => return self.cfg_space.read_u32(offset, value),
233+
_ if offset < 0x40 => return self.cfg_space.read_byte_enabled(offset, value),
232234
// these magic values mainly consist of default values pulled from
233235
// the PIIX4 documentation
234236
ConfigSpace::TOP => 0x00000200,
@@ -252,22 +254,24 @@ impl PciConfigSpace for PciIsaBridge {
252254
tracing::debug!(?offset, "unimplemented config space read");
253255
return IoResult::Err(IoError::InvalidRegister);
254256
}
255-
};
257+
});
256258

257259
IoResult::Ok
258260
}
259261

260-
fn pci_cfg_write(&mut self, offset: u16, value: u32) -> IoResult {
262+
fn pci_cfg_write(&mut self, offset: u16, value: ByteEnabledDwordWrite) -> IoResult {
261263
match ConfigSpace(offset) {
262-
_ if offset < 0x40 => return self.cfg_space.write_u32(offset, value),
264+
_ if offset < 0x40 => return self.cfg_space.write_byte_enabled(offset, value),
263265
ConfigSpace::PIRQ => {
266+
let value = value.merge(self.state.pci_irq_routing);
264267
if self.state.pci_irq_routing != value {
265268
tracelimit::info_ratelimited!(new_pci_irq_routing = ?value, "custom PCI IRQ routing is not implemented!");
266269
}
267270

268271
self.state.pci_irq_routing = value;
269272
}
270273
ConfigSpace::SER_IRQ => {
274+
let value = value.extract();
271275
if !(value == 0x0000000D0 || value == 0x000000010) {
272276
tracelimit::warn_ratelimited!(
273277
?value,
@@ -279,17 +283,21 @@ impl PciConfigSpace for PciIsaBridge {
279283
// Make sure ISA/DMA 512-640K Region Forwarding Enable is never cleared.
280284
// This controls whether addresses 512K to 640K are forwarded to RAM
281285
// (which we always want to do).
286+
let value = value.extract();
282287
if (value & 0x00000200) == 0 {
283288
tracing::debug!("ISA/DMA 512-640K Region Forwarding Enable was cleared!");
284289
}
285290
}
286-
ConfigSpace::SMI => self.state.smi_control = value & 0x00FF001F,
287-
ConfigSpace::SEE => self.state.system_event = value,
288-
ConfigSpace::FTM => self.state.smi_request = value,
289-
ConfigSpace::CTL_TMR => self.state.clock_scale = value,
291+
ConfigSpace::SMI => {
292+
self.state.smi_control = value.merge(self.state.smi_control) & 0x00FF001F;
293+
}
294+
ConfigSpace::SEE => value.merge_into(&mut self.state.system_event),
295+
ConfigSpace::FTM => value.merge_into(&mut self.state.smi_request),
296+
ConfigSpace::CTL_TMR => value.merge_into(&mut self.state.clock_scale),
290297
ConfigSpace::RTC_CONFIG => {
291298
// For now, the code assumes the default value. We don't support
292299
// disabling extended CMOS (upper 128 bytes).
300+
let value = value.extract();
293301
if (value & 0x04000000) == 0 {
294302
tracing::debug!("Trying to disable extended CMOS - not supported")
295303
}
@@ -301,6 +309,7 @@ impl PciConfigSpace for PciIsaBridge {
301309
ConfigSpace::APIC_BASE => {
302310
// If any of bits 0..5 have changed, then we need to change the base of
303311
// the IoApic.
312+
let value = value.merge(self.state.apic_base);
304313
if (value & 0x3F) != (self.state.apic_base & 0x3F) {
305314
// DEVNOTE: this shouldn't actually be all that difficult to
306315
// implement, but until there is definitive proof that there

0 commit comments

Comments
 (0)