windows: move vm_evt_wrtube/rdtube out of Config

Config should contain only configuration data (e.g. flags, paths, etc.),
not objects used at runtime like tubes and events. The Windows code
currently passes a few Tubes from the broker to child processes via
Config, which this patch begins moving to a separate BrokerTubes struct
that will be serialized and sent over the bootstrap tube from the broker
after the Config struct.

This helps clean up the cross-platform Config struct, as well as fixing
the `crosvm run` command (which does not use the broker) on Windows;
previously the vm_evt_wrtube and vm_evt_rdtube were missing in this
case.

BUG=None
TEST=tools/presubmit --all
TEST=Boot x86-64 Linux kernel with crosvm run on Windows

Change-Id: Ida00ec3948f09735fcdc333b3b5f217dca9fdbb9
Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/4108782
Reviewed-by: Vikram Auradkar <auradkar@google.com>
Commit-Queue: Daniel Verkamp <dverkamp@chromium.org>

src/crosvm/config.rs

@@ -26,10 +26,6 @@ use arch::Pstore;
 use arch::VcpuAffinity;
 use base::debug;
 use base::pagesize;
-#[cfg(windows)]
-use base::RecvTube;
-#[cfg(windows)]
-use base::SendTube;
 use cros_async::ExecutorKind;
 use devices::serial_device::SerialHardware;
 use devices::serial_device::SerialParameters;
@@ -1253,10 +1249,6 @@ pub struct Config {
     pub virtio_snds: Vec<SndParameters>,
     pub virtio_switches: Vec<PathBuf>,
     pub virtio_trackpad: Vec<TouchDeviceOption>,
-    #[cfg(windows)]
-    pub vm_evt_rdtube: Option<RecvTube>,
-    #[cfg(windows)]
-    pub vm_evt_wrtube: Option<SendTube>,
     #[cfg(all(feature = "vtpm", target_arch = "x86_64"))]
     pub vtpm_proxy: bool,
     pub vvu_proxy: Vec<VvuOption>,
@@ -1469,10 +1461,6 @@ impl Default for Config {
             virtio_snds: Vec::new(),
             virtio_switches: Vec::new(),
             virtio_trackpad: Vec::new(),
-            #[cfg(windows)]
-            vm_evt_rdtube: None,
-            #[cfg(windows)]
-            vm_evt_wrtube: None,
             #[cfg(all(feature = "vtpm", target_arch = "x86_64"))]
             vtpm_proxy: false,
             vvu_proxy: Vec::new(),

src/crosvm/sys/windows/broker.rs

@@ -43,7 +43,9 @@ use base::EventToken;
 use base::FramingMode;
 use base::RawDescriptor;
 use base::ReadNotifier;
+use base::RecvTube;
 use base::SafeDescriptor;
+use base::SendTube;
 #[cfg(feature = "gpu")]
 use base::StreamChannel;
 use base::Timer;
@@ -81,6 +83,8 @@ use metrics::MetricEventType;
 use net_util::slirp::sys::windows::SlirpStartupConfig;
 #[cfg(feature = "slirp")]
 use net_util::slirp::sys::windows::SLIRP_BUFFER_SIZE;
+use serde::Deserialize;
+use serde::Serialize;
 use tube_transporter::TubeToken;
 use tube_transporter::TubeTransferData;
 use tube_transporter::TubeTransporter;
@@ -94,6 +98,13 @@ use crate::Config;
 
 const KILL_CHILD_EXIT_CODE: u32 = 1;
 
+/// Tubes created by the broker and sent to child processes via the bootstrap tube.
+#[derive(Serialize, Deserialize)]
+pub struct BrokerTubes {
+    pub vm_evt_wrtube: SendTube,
+    pub vm_evt_rdtube: RecvTube,
+}
+
 /// This struct represents a configured "disk" device as returned by the platform's API. There will
 /// be two instances of it for each disk device, with the Tubes connected appropriately. The broker
 /// will send one of these to the main process, and the other to the vhost user disk backend.
@@ -566,11 +577,16 @@ fn run_internal(mut cfg: Config) -> Result<()> {
 
     let (vm_evt_wrtube, vm_evt_rdtube) =
         Tube::directional_pair().context("failed to create vm event tube")?;
-    cfg.vm_evt_wrtube = Some(vm_evt_wrtube);
-    cfg.vm_evt_rdtube = Some(vm_evt_rdtube);
 
     #[cfg(feature = "gpu")]
-    let gpu_cfg = platform_create_gpu(&cfg, &mut main_child, &mut exit_events)?;
+    let gpu_cfg = platform_create_gpu(
+        &cfg,
+        &mut main_child,
+        &mut exit_events,
+        vm_evt_wrtube
+            .try_clone()
+            .exit_context(Exit::CloneEvent, "failed to clone event")?,
+    )?;
 
     #[cfg(feature = "gpu")]
     let _gpu_child = if cfg.vhost_user_gpu.is_empty() {
@@ -613,6 +629,12 @@ fn run_internal(mut cfg: Config) -> Result<()> {
     main_child.bootstrap_tube.send(&exit_event).unwrap();
     exit_events.push(exit_event);
 
+    let broker_tubes = BrokerTubes {
+        vm_evt_wrtube,
+        vm_evt_rdtube,
+    };
+    main_child.bootstrap_tube.send(&broker_tubes).unwrap();
+
     // Setup our own metrics agent
     {
         let broker_metrics = metrics_tube_pair(&mut metric_tubes)?;
@@ -1394,6 +1416,7 @@ fn platform_create_gpu(
     cfg: &Config,
     #[allow(unused_variables)] main_child: &mut ChildProcess,
     exit_events: &mut Vec<Event>,
+    exit_evt_wrtube: SendTube,
 ) -> Result<(GpuBackendConfig, GpuVmmConfig)> {
     let exit_event = Event::new().exit_context(Exit::CreateEvent, "failed to create exit event")?;
     exit_events.push(
@@ -1430,13 +1453,6 @@ fn platform_create_gpu(
     event_devices.push(EventDevice::keyboard(event_device_pipe));
     input_event_keyboard_pipes.push(virtio_input_pipe);
 
-    let exit_evt_wrtube = cfg
-        .vm_evt_wrtube
-        .as_ref()
-        .expect("vm_evt_wrtube should be set")
-        .try_clone()
-        .exit_context(Exit::CloneEvent, "failed to clone event")?;
-
     let (backend_config_product, vmm_config_product) =
         get_gpu_product_configs(cfg, main_child.alias_pid)?;
 

src/sys/windows.rs

@@ -189,6 +189,7 @@ use crate::crosvm::config::TouchDeviceOption;
 use crate::crosvm::sys::config::HypervisorKind;
 #[cfg(any(feature = "gvm", feature = "whpx"))]
 use crate::crosvm::sys::config::IrqChipKind;
+use crate::crosvm::sys::windows::broker::BrokerTubes;
 #[cfg(feature = "stats")]
 use crate::crosvm::sys::windows::stats::StatisticsCollector;
 pub(crate) use crate::sys::windows::product::get_gpu_product_configs;
@@ -1550,23 +1551,25 @@ pub fn run_config_for_broker(raw_tube_transporter: RawDescriptor) -> Result<Exit
     #[cfg(feature = "crash-report")]
     crash_report::set_crash_tube_map(crash_tube_map);
 
-    run_config_inner(cfg)
+    let BrokerTubes {
+        vm_evt_wrtube,
+        vm_evt_rdtube,
+    } = bootstrap_tube
+        .recv::<BrokerTubes>()
+        .exit_context(Exit::TubeFailure, "failed to read bootstrap tube")?;
+
+    run_config_inner(cfg, vm_evt_wrtube, vm_evt_rdtube)
 }
 
-pub fn run_config(mut cfg: Config) -> Result<ExitState> {
+pub fn run_config(cfg: Config) -> Result<ExitState> {
     let _raise_timer_resolution = enable_high_res_timers()
         .exit_context(Exit::EnableHighResTimer, "failed to enable high res timer")?;
 
-    assert_eq!(cfg.vm_evt_wrtube.is_some(), cfg.vm_evt_rdtube.is_some());
-    if cfg.vm_evt_wrtube.is_none() {
-        // There is no broker when using run_config(), so the vm_evt tubes need to be created.
-        let (vm_evt_wrtube, vm_evt_rdtube) =
-            Tube::directional_pair().context("failed to create vm event tube")?;
-        cfg.vm_evt_wrtube = Some(vm_evt_wrtube);
-        cfg.vm_evt_rdtube = Some(vm_evt_rdtube);
-    }
+    // There is no broker when using run_config(), so the vm_evt tubes need to be created.
+    let (vm_evt_wrtube, vm_evt_rdtube) =
+        Tube::directional_pair().context("failed to create vm event tube")?;
 
-    run_config_inner(cfg)
+    run_config_inner(cfg, vm_evt_wrtube, vm_evt_rdtube)
 }
 
 fn create_guest_memory(
@@ -1581,7 +1584,11 @@ fn create_guest_memory(
         .exit_context(Exit::CreateGuestMemory, "failed to create guest memory")
 }
 
-fn run_config_inner(cfg: Config) -> Result<ExitState> {
+fn run_config_inner(
+    cfg: Config,
+    vm_evt_wrtube: SendTube,
+    vm_evt_rdtube: RecvTube,
+) -> Result<ExitState> {
     product::setup_common_metric_invariants(&cfg);
 
     #[cfg(feature = "perfetto")]
@@ -1622,6 +1629,8 @@ fn run_config_inner(cfg: Config) -> Result<ExitState> {
                 vm,
                 WindowsIrqChip::Userspace(irq_chip).as_mut(),
                 Some(ioapic_host_tube),
+                vm_evt_wrtube,
+                vm_evt_rdtube,
             )
         }
         #[cfg(feature = "whpx")]
@@ -1684,6 +1693,8 @@ fn run_config_inner(cfg: Config) -> Result<ExitState> {
                 vm,
                 irq_chip.as_mut(),
                 Some(ioapic_host_tube),
+                vm_evt_wrtube,
+                vm_evt_rdtube,
             )
         }
         #[cfg(feature = "gvm")]
@@ -1709,7 +1720,15 @@ fn run_config_inner(cfg: Config) -> Result<ExitState> {
                     )?)
                 }
             };
-            run_vm::<GvmVcpu, GvmVm>(cfg, components, vm, irq_chip.as_mut(), ioapic_host_tube)
+            run_vm::<GvmVcpu, GvmVm>(
+                cfg,
+                components,
+                vm,
+                irq_chip.as_mut(),
+                ioapic_host_tube,
+                vm_evt_wrtube,
+                vm_evt_rdtube,
+            )
         }
     }
 }
@@ -1721,6 +1740,8 @@ fn run_vm<Vcpu, V>(
     mut vm: V,
     irq_chip: &mut dyn IrqChipArch,
     ioapic_host_tube: Option<Tube>,
+    vm_evt_wrtube: SendTube,
+    vm_evt_rdtube: RecvTube,
 ) -> Result<ExitState>
 where
     Vcpu: VcpuArch + 'static,
@@ -1775,15 +1796,6 @@ where
     let gralloc =
         RutabagaGralloc::new().exit_context(Exit::CreateGralloc, "failed to create gralloc")?;
 
-    let (vm_evt_wrtube, vm_evt_rdtube) = (
-        cfg.vm_evt_wrtube
-            .take()
-            .expect("vm_evt_wrtube should be set"),
-        cfg.vm_evt_rdtube
-            .take()
-            .expect("vm_evt_rdtube should be set"),
-    );
-
     let pstore_size = components.pstore.as_ref().map(|pstore| pstore.size as u64);
     let mut sys_allocator = SystemAllocator::new(
         Arch::get_system_allocator_config(&vm),