Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 26 additions & 13 deletions vm/devices/pci/vpci/src/device.rs
Original file line number Diff line number Diff line change
Expand Up @@ -525,6 +525,7 @@ enum ProtocolState {

struct ReadyState {
send_device: bool,
send_completion: Option<u64>,
vpci_version: protocol::ProtocolVersion,
}

Expand Down Expand Up @@ -566,6 +567,7 @@ impl<T: RingMem> VpciChannelState<T> {
self.state = ProtocolState::Ready(ReadyState {
vpci_version: version,
send_device: false,
send_completion: None,
});
}
} else {
Expand Down Expand Up @@ -639,6 +641,10 @@ impl ReadyState {
self.send_child_device(conn, dev).await?;
self.send_device = false;
}
if let Some(transaction_id) = self.send_completion {
conn.send_completion(Some(transaction_id), &protocol::Status::SUCCESS, &[])?;
Comment on lines +644 to +645
Copy link
Preview

Copilot AI Aug 29, 2025

Choose a reason for hiding this comment

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

The completion is being sent with Some(transaction_id) but the original code used just transaction_id. This changes the function signature expectation and may cause a type mismatch or incorrect protocol behavior.

Suggested change
if let Some(transaction_id) = self.send_completion {
conn.send_completion(Some(transaction_id), &protocol::Status::SUCCESS, &[])?;
conn.send_completion(transaction_id, &protocol::Status::SUCCESS, &[])?;

Copilot uses AI. Check for mistakes.

self.send_completion = None;
}

// Don't pull a packets off the ring until there is space for its completion.
conn.wait_for_completion_space().await?;
Expand Down Expand Up @@ -689,8 +695,9 @@ impl ReadyState {
PacketData::FdoD0Entry { mmio_start } => {
tracing::trace!(?mmio_start, ?dev.instance_id, "FDO D0 entry");
dev.config_space.map(mmio_start);
conn.send_completion(transaction_id, &protocol::Status::SUCCESS, &[])?;
self.send_device = true;
// Send the completion after the device has been sent.
self.send_completion = transaction_id;
}
PacketData::FdoD0Exit => {
tracing::trace!(?dev.instance_id, "FDO D0 exit");
Expand Down Expand Up @@ -1058,11 +1065,16 @@ impl VpciConfigSpace {
}

fn map(&mut self, addr: u64) {
tracing::debug!(addr, "mapping config space");
self.offset.0.store(addr, Ordering::Relaxed);
self.control_mmio.map(addr);
}

fn unmap(&mut self) {
tracing::debug!(
addr = self.offset.0.load(Ordering::Relaxed),
"unmapping config space"
);
// Note that there may be some current accessors that this will not
// flush out synchronously. The MMIO implementation in bus.rs must be
// careful to ignore reads/writes that are not to an expected address.
Expand All @@ -1078,7 +1090,7 @@ impl VpciConfigSpace {
/// PCI Config space offset structure
#[derive(Debug, Clone, Inspect)]
#[inspect(transparent)]
pub struct VpciConfigSpaceOffset(Arc<AtomicU64>);
pub struct VpciConfigSpaceOffset(#[inspect(hex)] Arc<AtomicU64>);

impl VpciConfigSpaceOffset {
const INVALID: u64 = !0;
Expand Down Expand Up @@ -1399,7 +1411,7 @@ mod tests {
}
}

async fn power_on(&mut self, base_address: u64) {
async fn initiate_power_on(&mut self, base_address: u64) -> u64 {
let power_on = protocol::FdoD0Entry {
message_type: protocol::MessageType::FDO_D0_ENTRY,
padding: 0,
Expand All @@ -1409,15 +1421,7 @@ mod tests {
self.write_packet(Some(transaction_id), &power_on)
.await
.unwrap();

let mut pkt_info = ReadPacketInfo::None;
let status: protocol::Status = self.read_packet(&mut pkt_info).await.unwrap();
if let ReadPacketInfo::Completion(id) = pkt_info {
assert_eq!(id, transaction_id);
assert_eq!(status, protocol::Status::SUCCESS);
} else {
panic!("Unexpected D0 (power on) reply");
}
transaction_id
}

fn verify_device_relations2(&self, message: &Relations2) {
Expand All @@ -1440,7 +1444,7 @@ mod tests {

async fn start_device(&mut self, base_address: u64) {
self.negotiate_version().await;
self.power_on(base_address).await;
let transaction_id = self.initiate_power_on(base_address).await;
let mut pkt_info = ReadPacketInfo::None;
let relations: Relations2 = self.read_packet(&mut pkt_info).await.unwrap();
if let ReadPacketInfo::NewTransaction = pkt_info {
Expand All @@ -1452,6 +1456,15 @@ mod tests {
} else {
panic!("Expecting QueryBusRelations2 message in response to version.");
}

let mut pkt_info = ReadPacketInfo::None;
let status: protocol::Status = self.read_packet(&mut pkt_info).await.unwrap();
if let ReadPacketInfo::Completion(id) = pkt_info {
assert_eq!(id, transaction_id);
assert_eq!(status, protocol::Status::SUCCESS);
} else {
panic!("Unexpected D0 (power on) reply");
}
}

// returns MSI address and data
Expand Down
Loading