From 53beb2990e18e6405be8a53a63a924e10f7a5efa Mon Sep 17 00:00:00 2001 From: Sergio Lopez Date: Mon, 13 Sep 2021 13:38:20 +0200 Subject: [PATCH 1/3] vring: Add "get_call" method Add a "get_call" method to obtain the current value for the call EventFd. Signed-off-by: Sergio Lopez --- src/vring.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/vring.rs b/src/vring.rs index 3761e7e..39aecee 100644 --- a/src/vring.rs +++ b/src/vring.rs @@ -251,6 +251,11 @@ impl VringState { self.call = file.map(|f| unsafe { EventFd::from_raw_fd(f.into_raw_fd()) }); } + /// Get the `EventFd` for call. + pub fn get_call(&self) -> &Option { + &self.call + } + /// Set `EventFd` for err. fn set_err(&mut self, file: Option) { // SAFETY: see comment in set_kick() From c0bcc33119c6c46f9f026da399697f2ca2f3e719 Mon Sep 17 00:00:00 2001 From: Sergio Lopez Date: Mon, 13 Sep 2021 13:39:10 +0200 Subject: [PATCH 2/3] Register listeners only with both call and kick The vhost-user protocol doesn't impose an order for the SET_VRING_KICK and SET_VRING_CALL messages, which implies it's valid to emit the first before the latter. With the current code, this means that if the VMM sends SET_VRING_KICK before SET_VRING_CALL, and the guest has already placed a request into the vring, we may miss signaling the guest as that request may be processed before we have an EventFd for "call". To fix this, delay listener registration until we have an EventFd for both call and kick, using "VringState::Queue.ready" as an indicator that the vring has not been initialized yet. Signed-off-by: Sergio Lopez --- src/handler.rs | 58 +++++++++++++++++++++++++++++++++++--------------- 1 file changed, 41 insertions(+), 17 deletions(-) diff --git a/src/handler.rs b/src/handler.rs index 672785b..bfaad88 100644 --- a/src/handler.rs +++ b/src/handler.rs @@ -175,6 +175,38 @@ where Err(VhostUserHandlerError::MissingMemoryMapping) } + + fn vring_needs_init(&self, vring: &V) -> bool { + let vring_state = vring.get_ref(); + + // If the vring wasn't initialized and we already have an EventFd for + // both VRING_KICK and VRING_CALL, initialize it now. + !vring_state.get_queue().ready + && vring_state.get_call().is_some() + && vring_state.get_kick().is_some() + } + + fn initialize_vring(&self, vring: &V, index: u8) -> VhostUserResult<()> { + assert!(vring.get_ref().get_call().is_some()); + assert!(vring.get_ref().get_kick().is_some()); + + if let Some(fd) = vring.get_ref().get_kick() { + for (thread_index, queues_mask) in self.queues_per_thread.iter().enumerate() { + let shifted_queues_mask = queues_mask >> index; + if shifted_queues_mask & 1u64 == 1u64 { + let evt_idx = queues_mask.count_ones() - shifted_queues_mask.count_ones(); + self.handlers[thread_index] + .register_event(fd.as_raw_fd(), epoll::Events::EPOLLIN, u64::from(evt_idx)) + .map_err(VhostUserError::ReqHandlerError)?; + break; + } + } + } + + self.vrings[index as usize].set_queue_ready(true); + + Ok(()) + } } impl VhostUserSlaveReqHandlerMut for VhostUserHandler @@ -361,6 +393,9 @@ where } } + self.vrings[index as usize].set_kick(None); + self.vrings[index as usize].set_call(None); + let next_avail = self.vrings[index as usize].queue_next_avail(); Ok(VhostUserVringState::new(index, u32::from(next_avail))) @@ -377,23 +412,8 @@ where // such as that proposed by Rust RFC #3128. self.vrings[index as usize].set_kick(file); - // Quote from vhost-user specification: - // Client must start ring upon receiving a kick (that is, detecting - // that file descriptor is readable) on the descriptor specified by - // VHOST_USER_SET_VRING_KICK, and stop ring upon receiving - // VHOST_USER_GET_VRING_BASE. - self.vrings[index as usize].set_queue_ready(true); - if let Some(fd) = self.vrings[index as usize].get_ref().get_kick() { - for (thread_index, queues_mask) in self.queues_per_thread.iter().enumerate() { - let shifted_queues_mask = queues_mask >> index; - if shifted_queues_mask & 1u64 == 1u64 { - let evt_idx = queues_mask.count_ones() - shifted_queues_mask.count_ones(); - self.handlers[thread_index] - .register_event(fd.as_raw_fd(), epoll::Events::EPOLLIN, u64::from(evt_idx)) - .map_err(VhostUserError::ReqHandlerError)?; - break; - } - } + if self.vring_needs_init(&self.vrings[index as usize]) { + self.initialize_vring(&self.vrings[index as usize], index)?; } Ok(()) @@ -406,6 +426,10 @@ where self.vrings[index as usize].set_call(file); + if self.vring_needs_init(&self.vrings[index as usize]) { + self.initialize_vring(&self.vrings[index as usize], index)?; + } + Ok(()) } From 4ed57c232de5271923cfc093a8e9709cf069be76 Mon Sep 17 00:00:00 2001 From: Sergio Lopez Date: Mon, 13 Sep 2021 13:45:03 +0200 Subject: [PATCH 3/3] Reset the Queue on GET_VRING_BASE According to the vhost-user specs, we should start the vring upon receiving the first kick, and stop it when we receive GET_VRING_BASE. Strictly speaking, we should reset the underlying Queue on the first kick, but it's actually easier to simply do that in GET_VRING_BASE and be ready in case the guest re-initializes the vring. Signed-off-by: Sergio Lopez --- src/handler.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/handler.rs b/src/handler.rs index bfaad88..5cba893 100644 --- a/src/handler.rs +++ b/src/handler.rs @@ -396,6 +396,14 @@ where self.vrings[index as usize].set_kick(None); self.vrings[index as usize].set_call(None); + // Strictly speaking, we should do this upon receiving the first kick, + // but it's actually easier to just do it here so we're ready in case + // the vring gets re-initialized by the guest. + self.vrings[index as usize] + .get_mut() + .get_queue_mut() + .reset(); + let next_avail = self.vrings[index as usize].queue_next_avail(); Ok(VhostUserVringState::new(index, u32::from(next_avail)))