vmbus_server: revert to creating interrupts before channel open (#1850)

This change reverts the changes made by #649. While the model used by
that change was better, it was discovered that some channels, notably
some IC channels, begin communicating as soon as the device is notified
of the open. This leads to the possibility of the guest attempting to
send an interrupt before the synic event port was created, so the
interrupt would be lost.

This change therefore moves the creation of the guest-to-host event port
back to when the open request is received.

Note that this is not a straight-forward revert of the original commit,
as the code had diverged too much, particularly because the synic
handling for vmbus_relay was moved into vmbus_client. This is therefore
essentially a new change that reverts to the same functionality as
before.

Author: SvenGroot

vm/devices/net/netvsp/src/test.rs

@@ -57,7 +57,6 @@ use vmbus_channel::bus::OfferInput;
 use vmbus_channel::bus::OfferResources;
 use vmbus_channel::bus::OpenData;
 use vmbus_channel::bus::OpenRequest;
-use vmbus_channel::bus::OpenResult;
 use vmbus_channel::bus::ParentBus;
 use vmbus_channel::channel::ChannelHandle;
 use vmbus_channel::channel::VmbusDevice;
@@ -87,7 +86,7 @@ use zerocopy::KnownLayout;
 const VMNIC_CHANNEL_TYPE_GUID: Guid = guid::guid!("f8615163-df3e-46c5-913f-f2d2f965ed0e");
 
 enum ChannelResponse {
-    Open(Option<OpenResult>),
+    Open(bool),
     Close,
     Gpadl(bool),
     // TeardownGpadl(GpadlId),
@@ -576,18 +575,19 @@ impl TestNicDevice {
             .await
             .expect("open successful");
 
-        let ChannelResponse::Open(Some(result)) = open_response else {
+        let ChannelResponse::Open(true) = open_response else {
             panic!("Unexpected return value");
         };
 
         let mem = self.mock_vmbus.memory.clone();
+        let guest_to_host_interrupt = self.offer_input.event.clone();
         TestNicChannel::new(
             self,
             &mem,
             gpadl_map,
             ring_gpadl_id,
             host_to_guest_event,
-            result.guest_to_host_interrupt,
+            guest_to_host_interrupt,
         )
     }
 
@@ -631,16 +631,17 @@ impl TestNicDevice {
             .await
             .expect("open successful");
 
-        let ChannelResponse::Open(Some(result)) = open_response else {
+        let ChannelResponse::Open(true) = open_response else {
             panic!("Unexpected return value");
         };
 
+        let guest_to_host_interrupt = self.offer_input.event.clone();
         TestNicSubchannel::new(
             &self.mock_vmbus.memory,
             gpadl_map,
             ring_gpadl_id,
             host_to_guest_event,
-            result.guest_to_host_interrupt,
+            guest_to_host_interrupt,
         )
     }
 
@@ -690,7 +691,7 @@ impl TestNicDevice {
         next_avail_guest_page: usize,
         next_avail_gpadl_id: u32,
         host_to_guest_interrupt: Interrupt,
-    ) -> anyhow::Result<Option<Interrupt>> {
+    ) -> anyhow::Result<()> {
         // Restore the previous memory settings
         assert_eq!(self.next_avail_gpadl_id, 1);
         self.next_avail_gpadl_id = next_avail_gpadl_id;
@@ -709,7 +710,6 @@ impl TestNicDevice {
             })
             .collect::<Vec<(GpadlId, MultiPagedRangeBuf<Vec<u64>>)>>();
 
-        let mut guest_to_host_interrupt = None;
         mesh::CancelContext::new()
             .with_timeout(Duration::from_millis(1000))
             .until_cancelled(async {
@@ -732,8 +732,7 @@ impl TestNicDevice {
                                             accepted: true,
                                         }
                                     }).collect::<Vec<vmbus_channel::bus::RestoredGpadl>>();
-                                    rpc.handle_sync(|open| {
-                                        guest_to_host_interrupt = open.map(|open| open.guest_to_host_interrupt);
+                                    rpc.handle_sync(|_open| {
                                         Ok(vmbus_channel::bus::RestoreResult {
                                             open_request: Some(OpenRequest {
                                                 open_data: OpenData {
@@ -761,7 +760,7 @@ impl TestNicDevice {
             .await
             .unwrap()?;
 
-        Ok(guest_to_host_interrupt)
+        Ok(())
     }
 }
 
@@ -1275,6 +1274,7 @@ impl<'a> TestNicChannel<'a> {
         buffer: SavedStateBlob,
     ) -> anyhow::Result<TestNicChannel<'_>> {
         let mem = self.nic.mock_vmbus.memory.clone();
+        let guest_to_host_interrupt = nic.offer_input.event.clone();
         let host_to_guest_interrupt = {
             let event = self.host_to_guest_event.clone();
             Interrupt::from_fn(move || event.signal())
@@ -1285,17 +1285,15 @@ impl<'a> TestNicChannel<'a> {
         let next_avail_guest_page = self.nic.next_avail_guest_page;
         let next_avail_gpadl_id = self.nic.next_avail_gpadl_id;
 
-        let guest_to_host_interrupt = nic
-            .restore(
-                buffer,
-                gpadl_map.clone(),
-                channel_id,
-                next_avail_guest_page,
-                next_avail_gpadl_id,
-                host_to_guest_interrupt,
-            )
-            .await?
-            .expect("should be open");
+        nic.restore(
+            buffer,
+            gpadl_map.clone(),
+            channel_id,
+            next_avail_guest_page,
+            next_avail_gpadl_id,
+            host_to_guest_interrupt,
+        )
+        .await?;
 
         Ok(TestNicChannel::new(
             nic,

vm/devices/vmbus/vmbus_channel/src/bus.rs

@@ -23,6 +23,8 @@ use vmcore::interrupt::Interrupt;
 pub struct OfferInput {
     /// Parameters describing the offer.
     pub params: OfferParams,
+    /// The event to signal when the guest needs attention.
+    pub event: Interrupt,
     /// A mesh channel to send channel-related requests to.
     pub request_send: mesh::Sender<ChannelRequest>,
     /// A mesh channel to receive channel-related requests to.
@@ -79,7 +81,7 @@ impl OfferResources {
 #[derive(Debug, MeshPayload)]
 pub enum ChannelRequest {
     /// Open the channel.
-    Open(Rpc<OpenRequest, Option<OpenResult>>),
+    Open(Rpc<OpenRequest, bool>),
     /// Close the channel.
     ///
     /// Although there is no response from the host, this is still modeled as an
@@ -94,14 +96,6 @@ pub enum ChannelRequest {
     Modify(Rpc<ModifyRequest, i32>),
 }
 
-/// The successful result of an open request.
-#[derive(Debug, MeshPayload)]
-pub struct OpenResult {
-    /// The interrupt object vmbus should signal when the guest signals the
-    /// host.
-    pub guest_to_host_interrupt: Interrupt,
-}
-
 /// GPADL information from the guest.
 #[derive(Debug, MeshPayload)]
 pub struct GpadlRequest {
@@ -128,8 +122,8 @@ pub enum ModifyRequest {
 pub enum ChannelServerRequest {
     /// A request to restore the channel.
     ///
-    /// The input parameter provides the open result if the channel was saved open.
-    Restore(FailableRpc<Option<OpenResult>, RestoreResult>),
+    /// The input parameter indicates if the channel was saved open.
+    Restore(FailableRpc<bool, RestoreResult>),
     /// A request to revoke the channel.
     ///
     /// A channel can also be revoked by dropping it. This request is only necessary if you need to
@@ -176,8 +170,7 @@ pub trait ParentBus: Send + Sync {
     /// time.
     fn clone_bus(&self) -> Box<dyn ParentBus>;
 
-    /// Returns whether [`OpenResult::guest_to_host_interrupt`] needs to be
-    /// backed by an OS event.
+    /// Returns whether [`OfferInput::event`] needs to be backed by an OS event.
     ///
     /// TODO: Remove this and just return the appropriate notify type directly
     /// once subchannel creation and enable are separated.

vm/devices/vmbus/vmbus_channel/src/channel.rs

@@ -10,7 +10,6 @@ use crate::bus::OfferInput;
 use crate::bus::OfferParams;
 use crate::bus::OfferResources;
 use crate::bus::OpenRequest;
-use crate::bus::OpenResult;
 use crate::bus::ParentBus;
 use crate::gpadl::GpadlMap;
 use crate::gpadl::GpadlMapView;
@@ -334,6 +333,7 @@ async fn offer_generic(
 
     let request = OfferInput {
         params: offer,
+        event: events[0].clone().interrupt(),
         request_send,
         server_request_recv,
     };
@@ -592,22 +592,21 @@ impl Device {
         channel: &mut dyn VmbusDevice,
         channel_idx: usize,
         open_request: OpenRequest,
-    ) -> Option<OpenResult> {
+    ) -> bool {
         assert!(!self.open[channel_idx]);
         // N.B. Any asynchronous GPADL requests will block while in
         //      open(). This should be fine for all known devices.
-        let opened = if let Err(error) = channel.open(channel_idx as u16, &open_request).await {
-            tracelimit::error_ratelimited!(
-                error = error.as_ref() as &dyn std::error::Error,
-                "failed to open channel"
-            );
-            None
-        } else {
-            Some(OpenResult {
-                guest_to_host_interrupt: self.events[channel_idx].clone().interrupt(),
+        let opened = channel
+            .open(channel_idx as u16, &open_request)
+            .await
+            .inspect_err(|error| {
+                tracelimit::error_ratelimited!(
+                    error = error.as_ref() as &dyn std::error::Error,
+                    "failed to open channel"
+                );
             })
-        };
-        self.open[channel_idx] = opened.is_some();
+            .is_ok();
+        self.open[channel_idx] = opened;
         opened
     }
 
@@ -764,6 +763,7 @@ impl Device {
                     subchannel_index: subchannel_idx as u16,
                     ..offer.clone()
                 },
+                event: self.events[subchannel_idx].clone().interrupt(),
                 request_send,
                 server_request_recv,
             };
@@ -800,12 +800,9 @@ impl Device {
             .map_err(ChannelRestoreError::EnablingSubchannels)?;
 
         let mut results = Vec::with_capacity(states.len());
-        for (channel_idx, (open, event)) in states.iter().copied().zip(&self.events).enumerate() {
-            let open_result = open.then(|| OpenResult {
-                guest_to_host_interrupt: event.clone().interrupt(),
-            });
+        for (channel_idx, open) in states.iter().copied().enumerate() {
             let result = self.server_requests[channel_idx]
-                .call_failable(ChannelServerRequest::Restore, open_result)
+                .call_failable(ChannelServerRequest::Restore, open)
                 .await
                 .map_err(|err| ChannelRestoreError::RestoreError(err.into()))?;
 

vm/devices/vmbus/vmbus_channel/src/offer.rs

@@ -12,7 +12,6 @@ use crate::bus::OfferInput;
 use crate::bus::OfferParams;
 use crate::bus::OfferResources;
 use crate::bus::OpenRequest;
-use crate::bus::OpenResult;
 use crate::bus::ParentBus;
 use crate::gpadl::GpadlMap;
 use crate::gpadl::GpadlMapView;
@@ -71,6 +70,7 @@ impl Offer {
         let result = bus
             .add_child(OfferInput {
                 params: offer_params,
+                event: Interrupt::from_event(event.clone()),
                 request_send,
                 server_request_recv,
             })
@@ -181,9 +181,7 @@ impl Offer {
             channel,
             gpadl_map: self.gpadl_map.clone(),
         };
-        message.response.respond(Some(OpenResult {
-            guest_to_host_interrupt: self.event.clone().interrupt(),
-        }));
+        message.response.respond(true);
         Ok(resources)
     }
 
@@ -222,18 +220,18 @@ struct OpenMessage {
     response: OpenResponse,
 }
 
-struct OpenResponse(Option<Rpc<(), Option<OpenResult>>>);
+struct OpenResponse(Option<Rpc<(), bool>>);
 
 impl OpenResponse {
-    fn respond(mut self, result: Option<OpenResult>) {
-        self.0.take().unwrap().complete(result)
+    fn respond(mut self, open: bool) {
+        self.0.take().unwrap().complete(open)
     }
 }
 
 impl Drop for OpenResponse {
     fn drop(&mut self) {
         if let Some(rpc) = self.0.take() {
-            rpc.complete(None);
+            rpc.complete(false);
         }
     }
 }

vm/devices/vmbus/vmbus_client/src/driver.rs

@@ -88,6 +88,7 @@ impl<D: SpawnDriver> ChannelWorker<D> {
         &mut self,
         input: OpenParams,
         close_send: mesh::OneshotSender<()>,
+        guest_to_host: Interrupt,
         host_to_guest: pal_event::Event,
         revoked: Arc<AtomicBool>,
     ) -> anyhow::Result<RawAsyncChannel<MemoryBlockRingMem>> {
@@ -109,8 +110,7 @@ impl<D: SpawnDriver> ChannelWorker<D> {
 
         self.is_gpadl_created = true;
 
-        let open = self
-            .request_send
+        self.request_send
             .call_failable(
                 ChannelRequest::Open,
                 OpenRequest {
@@ -143,7 +143,7 @@ impl<D: SpawnDriver> ChannelWorker<D> {
         };
 
         let signal = ClientSignaller {
-            guest_to_host: open.guest_to_host_signal,
+            guest_to_host,
             host_to_guest: Notify::from_event(host_to_guest).pollable(&self.driver)?,
             revoked: revoked.clone(),
             _close: close_send,
@@ -196,7 +196,13 @@ impl<D: SpawnDriver> ChannelWorker<D> {
         let (close_send, close_recv) = mesh::oneshot();
         let host_to_guest = pal_event::Event::new();
         match worker
-            .open(input, close_send, host_to_guest.clone(), revoked.clone())
+            .open(
+                input,
+                close_send,
+                offer_info.guest_to_host_interrupt,
+                host_to_guest.clone(),
+                revoked.clone(),
+            )
             .await
         {
             Ok(channel) => {

vm/devices/vmbus/vmbus_client/src/filter.rs

@@ -240,6 +240,7 @@ impl Inspect for Filters {
 
 impl FilterWorker {
     async fn run(&mut self, req: mesh::Receiver<FilterRequest>, offers: mesh::Receiver<OfferInfo>) {
+        #[allow(clippy::large_enum_variant)]
         enum Event {
             Request(FilterRequest),
             Done,