test-runner: Drop the second serial device

The screenshot test sends a command to the host via the serial device,
then waits for a reply before continuing. We used a second serial device
just for this request and reply so that we could open the protocol in
exclusive mode without breaking logging for tests after that.

It turns out there's a simpler way to do this: we can use the same
serial device as the one used for logs, and after we're finished use
`connect_controller` to restore the connection between the serial device
and the console output.

Author: nicholasbishop

uefi-test-runner/src/main.rs

@@ -11,6 +11,7 @@ extern crate alloc;
 use alloc::string::ToString;
 use uefi::prelude::*;
 use uefi::proto::console::serial::Serial;
+use uefi::Result;
 use uefi_services::{print, println};
 
 mod boot;
@@ -75,46 +76,71 @@ fn check_revision(rev: uefi::table::Revision) {
     );
 }
 
+/// Send the `request` string to the host via the `serial` device, then
+/// wait up to 10 seconds to receive a reply. Returns an error if the
+/// reply is not `"OK\n"`.
+fn send_request_helper(serial: &mut Serial, request: &str) -> Result {
+    // Set a 10 second timeout for the read and write operations.
+    let mut io_mode = *serial.io_mode();
+    io_mode.timeout = 10_000_000;
+    serial.set_attributes(&io_mode)?;
+
+    // Send a screenshot request to the host.
+    serial.write(request.as_bytes()).discard_errdata()?;
+
+    // Wait for the host's acknowledgement before moving forward.
+    let mut reply = [0; 3];
+    serial.read(&mut reply[..]).discard_errdata()?;
+
+    if reply == *b"OK\n" {
+        Ok(())
+    } else {
+        Err(Status::ABORTED.into())
+    }
+}
+
 /// Ask the test runner to check the current screen output against a reference.
 fn check_screenshot(bt: &BootServices, name: &str) {
-    let serial_handles = bt
-        .find_handles::<Serial>()
-        .expect("Failed to get serial handles");
-
-    // Use the second serial device handle. Opening a serial device
-    // in exclusive mode breaks the connection between stdout and
-    // the serial device, and we don't want that to happen to the
-    // first serial device since it's used for log transport.
-    let serial_handle = *serial_handles
-        .get(1)
-        .expect("Second serial device is missing");
-
+    let request = format!("SCREENSHOT: {name}\n");
+
+    let serial_handle = bt
+        .get_handle_for_protocol::<Serial>()
+        .expect("Failed to get serial handle");
+
+    // Open the serial protocol in exclusive mode.
+    //
+    // EDK2's [console splitter driver] periodically tries to sample
+    // from console devices to see if keys are being pressed, which will
+    // overwrite the timeout set below and potentially swallow the reply
+    // from the host. Opening in exclusive mode stops the driver from
+    // using this device. However, it also prevents logs from from going
+    // to the serial device, so we have to restore the connection at the
+    // end with `connect_controller`.
+    //
+    // [console splitter driver]: https://github.com/tianocore/edk2/blob/HEAD/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
     let mut serial = bt
         .open_protocol_exclusive::<Serial>(serial_handle)
         .expect("Could not open serial protocol");
 
-    // Set a large timeout to avoid problems with CI
-    let mut io_mode = *serial.io_mode();
-    io_mode.timeout = 10_000_000;
-    serial
-        .set_attributes(&io_mode)
-        .expect("Failed to configure serial port timeout");
-
-    // Send a screenshot request to the host
-    serial
-        .write(b"SCREENSHOT: ")
-        .expect("Failed to send request");
-    let name_bytes = name.as_bytes();
-    serial.write(name_bytes).expect("Failed to send request");
-    serial.write(b"\n").expect("Failed to send request");
-
-    // Wait for the host's acknowledgement before moving forward
-    let mut reply = [0; 3];
-    serial
-        .read(&mut reply[..])
-        .expect("Failed to read host reply");
-
-    assert_eq!(&reply[..], b"OK\n", "Unexpected screenshot request reply");
+    // Send the request, but don't check the result yet so that first
+    // we can reconnect the console output for the logger.
+    let res = send_request_helper(&mut serial, &request);
+
+    // Release the serial device and reconnect all controllers to the
+    // serial handle. This is necessary to restore the connection
+    // between the console output device used for logging and the serial
+    // device, which was broken when we opened the protocol in exclusive
+    // mode above.
+    drop(serial);
+    let _ = bt.connect_controller(serial_handle, None, None, true);
+
+    if let Err(err) = res {
+        panic!(
+            "request failed: \"{}\": {:?}",
+            request.trim_end(),
+            err.status()
+        );
+    }
 }
 
 fn shutdown(image: uefi::Handle, mut st: SystemTable<Boot>) -> ! {

xtask/src/qemu.rs

@@ -7,11 +7,11 @@ use crate::{net, platform};
 use anyhow::{bail, Context, Result};
 use regex::bytes::Regex;
 use serde_json::{json, Value};
+use std::env;
 use std::ffi::OsString;
 use std::io::{BufRead, BufReader, Read, Write};
 use std::path::{Path, PathBuf};
 use std::process::{Child, Command, Stdio};
-use std::{env, thread};
 use tempfile::TempDir;
 
 #[derive(Clone, Copy, Debug)]
@@ -281,35 +281,22 @@ impl Io {
     }
 }
 
-fn echo_filtered_stdout(mut child_io: Io) {
+fn process_qemu_io(mut monitor_io: Io, mut serial_io: Io, tmp_dir: &Path) -> Result<()> {
     // This regex is used to detect and strip ANSI escape codes. These
     // escapes are added by the console output protocol when writing to
     // the serial device.
     let ansi_escape = Regex::new(r"(\x9b|\x1b\[)[0-?]*[ -/]*[@-~]").expect("invalid regex");
 
-    while let Ok(line) = child_io.read_line() {
-        let line = line.trim();
-        let stripped = ansi_escape.replace_all(line.as_bytes(), &b""[..]);
-        let stripped = String::from_utf8(stripped.into()).expect("line is not utf8");
-
-        // Print out the processed QEMU output for logging & inspection.
-        println!("{stripped}");
-    }
-}
-
-fn process_qemu_io(mut monitor_io: Io, mut serial_io: Io, tmp_dir: &Path) -> Result<()> {
     // Execute the QEMU monitor handshake, doing basic sanity checks.
     assert!(monitor_io.read_line()?.starts_with(r#"{"QMP":"#));
     monitor_io.write_json(json!({"execute": "qmp_capabilities"}))?;
     assert_eq!(monitor_io.read_json()?, json!({"return": {}}));
 
     while let Ok(line) = serial_io.read_line() {
-        // Strip whitespace from the end. No need to strip ANSI escape
-        // codes like in the stdout, because those escape codes are
-        // inserted by the console output protocol, whereas the
-        // "SCREENSHOT" line we are interested in is written via the
-        // serial protocol.
+        // Strip whitespace and ANSI escape codes.
         let line = line.trim_end();
+        let line = ansi_escape.replace_all(line.as_bytes(), &b""[..]);
+        let line = String::from_utf8(line.into()).expect("line is not utf8");
 
         // If the app requests a screenshot, take it.
         if let Some(reference_name) = line.strip_prefix("SCREENSHOT: ") {
@@ -343,6 +330,8 @@ fn process_qemu_io(mut monitor_io: Io, mut serial_io: Io, tmp_dir: &Path) -> Res
                 expected == actual,
                 "screenshot does not match reference image"
             )
+        } else {
+            println!("{line}");
         }
     }
 
@@ -510,16 +499,10 @@ pub fn run_qemu(arch: UefiArch, opt: &QemuOpt) -> Result<()> {
     cmd.arg(drive_arg);
 
     let qemu_monitor_pipe = Pipe::new(tmp_dir, "qemu-monitor")?;
-    let serial_pipe = Pipe::new(tmp_dir, "serial")?;
-
-    // Open two serial devices. The first one is connected to the host's
-    // stdout, and serves to just transport logs. The second one is
-    // connected to a pipe, and used to receive the SCREENSHOT command
-    // and send the response. That second will also receive logs up
-    // until the test runner opens the handle in exclusive mode, but we
-    // can just read and ignore those lines.
+
+    // Open a serial device connected to stdio. This is used for
+    // printing logs and to receive and reply to commands.
     cmd.args(["-serial", "stdio"]);
-    cmd.args(["-serial", serial_pipe.qemu_arg()]);
 
     // Map the QEMU monitor to a pair of named pipes
     cmd.args(["-qmp", qemu_monitor_pipe.qemu_arg()]);
@@ -543,21 +526,16 @@ pub fn run_qemu(arch: UefiArch, opt: &QemuOpt) -> Result<()> {
     let mut child = ChildWrapper(cmd.spawn().context("failed to launch qemu")?);
 
     let monitor_io = qemu_monitor_pipe.open_io()?;
-    let serial_io = serial_pipe.open_io()?;
     let child_io = Io::new(
         child.0.stdout.take().unwrap(),
         child.0.stdin.take().unwrap(),
     );
 
-    // Start a thread to process stdout from the child.
-    let stdout_thread = thread::spawn(|| echo_filtered_stdout(child_io));
-
     // Capture the result to check it, but first wait for the child to
     // exit.
-    let res = process_qemu_io(monitor_io, serial_io, tmp_dir);
+    let res = process_qemu_io(monitor_io, child_io, tmp_dir);
     let status = child.0.wait()?;
 
-    stdout_thread.join().expect("stdout thread panicked");
     if let Some(echo_service) = echo_service {
         echo_service.stop();
     }