uefi-test-runner: SNP: improve clarity

Originally, the idea was to make this test idempotent from the PXE test,
but it turned out that test has surprisingly many sharp edges. A huge amount
of time went into debugging certain problems.

While this commit doesn't change the fundamentals of the test, it helps to
ease future debugging and to form the mental model of what is going on in this
test.

PS: Interestingly, the interrupt status never shows that we
can receive a packet. Probably not implemented by OVMF?

Author: phip1611

uefi-test-runner/src/proto/network/mod.rs

@@ -5,6 +5,9 @@ pub fn test() {
 
     http::test();
     pxe::test();
+    // Currently, we are in the unfortunate situation that the SNP test
+    // depends on the PXE test, as it assigns an IPv4 address to the
+    // interface via DHCP.
     snp::test();
 }
 

uefi-test-runner/src/proto/network/snp.rs

@@ -1,67 +1,143 @@
 // SPDX-License-Identifier: MIT OR Apache-2.0
 
+use core::ops::DerefMut;
 use core::time::Duration;
-
 use uefi::proto::network::MacAddress;
-use uefi::proto::network::snp::{InterruptStatus, ReceiveFlags, SimpleNetwork};
+use uefi::proto::network::snp::{InterruptStatus, NetworkState, ReceiveFlags, SimpleNetwork};
 use uefi::{Status, boot};
 
+const ETHERNET_PROTOCOL_IPV4: u16 = 0x0800;
+/// The MAC address configured for the interface.
+const EXPECTED_MAC: [u8; 6] = [0x52, 0x54, 0, 0, 0, 0x1];
+
+/// Receives the next IPv4 packet and prints corresponding metadata.
+///
+/// Returns the length of the response.
+fn receive(simple_network: &mut SimpleNetwork, buffer: &mut [u8]) -> uefi::Result<usize> {
+    // Wait for a bit to ensure that the previous packet has been processed.
+    boot::stall(Duration::from_millis(500));
+
+    let mut recv_src_mac = MacAddress([0; 32]);
+    let mut recv_dst_mac = MacAddress([0; 32]);
+    let mut recv_ethernet_protocol = 0;
+
+    let res = simple_network.receive(
+        buffer,
+        None,
+        Some(&mut recv_src_mac),
+        Some(&mut recv_dst_mac),
+        Some(&mut recv_ethernet_protocol),
+    );
+
+    // To simplify debugging when receive an unexpected packet, we print the
+    // necessary info. This is especially useful if an unexpected IPv4 or ARP
+    // packet is received, which can easily happen when fiddling around with
+    // this test.
+    res.inspect(|_| {
+        debug!("Received:");
+        debug!("  src_mac       =  {:x?}", recv_src_mac);
+        debug!("  dst_mac       =  {:x?}", recv_dst_mac);
+        debug!("  ethernet_proto=0x{:x?}", recv_ethernet_protocol);
+
+        // Assert the ethernet frame was sent to the expected interface.
+        {
+            // UEFI reports proper DST MAC
+            assert_eq!(recv_dst_mac.0[0..6], EXPECTED_MAC);
+        }
+
+        // Ensure that we do not accidentally get an ARP packet, which we
+        // do not expect in this test.
+        assert_eq!(recv_ethernet_protocol, ETHERNET_PROTOCOL_IPV4)
+    })
+}
+
+/// This test sends a simple UDP/IP packet to the `EchoService` (created by
+/// `cargo xtask run`) and receives its response.
 pub fn test() {
+    // Skip the test if the `pxe` feature is not enabled.
+    if cfg!(not(feature = "pxe")) {
+        return;
+    }
+
     info!("Testing the simple network protocol");
 
     let handles = boot::find_handles::<SimpleNetwork>().unwrap_or_default();
 
+    // The handle to our specific network device, as the test requires also a
+    // specific environment. We do not test all possible handles.
+    let mut simple_network = None;
+
+    // We iterate over all handles until we found the right network device.
     for handle in handles {
-        let simple_network = boot::open_protocol_exclusive::<SimpleNetwork>(handle);
-        if simple_network.is_err() {
+        let Ok(handle) = boot::open_protocol_exclusive::<SimpleNetwork>(handle) else {
             continue;
-        }
-        let simple_network = simple_network.unwrap();
-
-        // Check shutdown
-        let res = simple_network.shutdown();
-        assert!(res == Ok(()) || res == Err(Status::NOT_STARTED.into()));
-
-        // Check stop
-        let res = simple_network.stop();
-        assert!(res == Ok(()) || res == Err(Status::NOT_STARTED.into()));
-
-        // Check start
-        simple_network
-            .start()
-            .expect("Failed to start Simple Network");
-
-        // Check initialize
-        simple_network
-            .initialize(0, 0)
-            .expect("Failed to initialize Simple Network");
-
-        // edk2 virtio-net driver does not support statistics, so
-        // allow UNSUPPORTED (same for collect_statistics below).
-        let res = simple_network.reset_statistics();
-        assert!(res == Ok(()) || res == Err(Status::UNSUPPORTED.into()));
-
-        // Reading the interrupt status clears it
-        simple_network.get_interrupt_status().unwrap();
-
-        // Set receive filters
-        simple_network
-            .receive_filters(
-                ReceiveFlags::UNICAST | ReceiveFlags::BROADCAST,
-                ReceiveFlags::empty(),
-                false,
-                None,
-            )
-            .expect("Failed to set receive filters");
-
-        // Check media
-        if !bool::from(simple_network.mode().media_present_supported)
-            || !bool::from(simple_network.mode().media_present)
+        };
+
+        // Check media is present
+        if !bool::from(handle.mode().media_present_supported)
+            || !bool::from(handle.mode().media_present)
         {
             continue;
         }
 
-        let payload = b"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\
+        let has_mac = handle.mode().current_address.0[0..6] == EXPECTED_MAC
+            && handle.mode().permanent_address.0[0..6] == EXPECTED_MAC;
+        if !has_mac {
+            continue;
+        }
+
+        simple_network.replace(handle);
+    }
+
+    let mut simple_network = simple_network.unwrap_or_else(|| panic!(
+        "Failed to find SNP handle for network device with MAC address {:x}:{:x}:{:x}:{:x}:{:x}:{:x}",
+        EXPECTED_MAC[0],
+        EXPECTED_MAC[1],
+        EXPECTED_MAC[2],
+        EXPECTED_MAC[3],
+        EXPECTED_MAC[4],
+        EXPECTED_MAC[5]
+    ));
+
+    assert_eq!(
+        simple_network.mode().state,
+        NetworkState::STOPPED,
+        "Should be in stopped state"
+    );
+
+    simple_network
+        .start()
+        .expect("Failed to start Simple Network");
+
+    simple_network
+        .initialize(0, 0)
+        .expect("Failed to initialize Simple Network");
+
+    // edk2 virtio-net driver does not support statistics, so
+    // allow UNSUPPORTED (same for collect_statistics below).
+    let res = simple_network.reset_statistics();
+    assert!(res == Ok(()) || res == Err(Status::UNSUPPORTED.into()));
+
+    // Reading the interrupt status clears it
+    simple_network.get_interrupt_status().unwrap();
+
+    // Set receive filters
+    simple_network
+        .receive_filters(
+            ReceiveFlags::UNICAST | ReceiveFlags::BROADCAST,
+            ReceiveFlags::empty(),
+            false,
+            None,
+        )
+        .expect("Failed to set receive filters");
+
+    // EthernetFrame(IPv4Packet(UDPPacket(Payload))).
+    // The ethernet frame header will be filled by `transmit()`.
+    // The UDP packet contains the byte sequence `4, 4, 3, 2, 1`.
+    //
+    // The packet is sent to the `EchoService` created by
+    // `cargo xtask run`. It runs on UDP port 21572.
+    let payload = b"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\
             \x45\x00\
             \x00\x21\
             \x00\x01\
@@ -77,65 +153,59 @@ pub fn test() {
             \xa9\xe4\
             \x04\x01\x02\x03\x04";
 
-        let dest_addr = MacAddress([0xffu8; 32]);
-        assert!(
-            !simple_network
-                .get_interrupt_status()
-                .unwrap()
-                .contains(InterruptStatus::TRANSMIT)
-        );
-
-        // Send the frame
-        simple_network
-            .transmit(
-                simple_network.mode().media_header_size as usize,
-                payload,
-                None,
-                Some(dest_addr),
-                Some(0x0800),
-            )
-            .expect("Failed to transmit frame");
-
-        info!("Waiting for the transmit");
-        while !simple_network
+    assert!(
+        !simple_network
             .get_interrupt_status()
             .unwrap()
             .contains(InterruptStatus::TRANSMIT)
-        {}
-
-        // Attempt to receive a frame
-        let mut buffer = [0u8; 1500];
-
-        info!("Waiting for the reception");
-        if simple_network.receive(&mut buffer, None, None, None, None)
-            == Err(Status::NOT_READY.into())
-        {
-            boot::stall(Duration::from_secs(1));
-
-            simple_network
-                .receive(&mut buffer, None, None, None, None)
-                .unwrap();
+    );
+
+    // Send the frame
+    simple_network
+        .transmit(
+            simple_network.mode().media_header_size as usize,
+            payload,
+            None,
+            Some(simple_network.mode().broadcast_address),
+            Some(ETHERNET_PROTOCOL_IPV4),
+        )
+        .expect("Failed to transmit frame");
+
+    info!("Waiting for the transmit");
+    while !simple_network
+        .get_interrupt_status()
+        .unwrap()
+        .contains(InterruptStatus::TRANSMIT)
+    {}
+
+    // Attempt to receive a frame
+    let mut buffer = [0u8; 1500];
+
+    info!("Waiting for the reception");
+    let n = receive(simple_network.deref_mut(), &mut buffer).unwrap();
+    debug!("Reply has {n} bytes");
+
+    // Check payload in UDP packet that was reversed by our EchoService.
+    assert_eq!(buffer[42..47], [4, 4, 3, 2, 1]);
+
+    // Get stats
+    let res = simple_network.collect_statistics();
+    match res {
+        Ok(stats) => {
+            info!("Stats: {:?}", stats);
+
+            // One frame should have been transmitted and one received
+            assert_eq!(stats.tx_total_frames().unwrap(), 1);
+            assert_eq!(stats.rx_total_frames().unwrap(), 1);
         }
-
-        assert_eq!(buffer[42..47], [4, 4, 3, 2, 1]);
-
-        // Get stats
-        let res = simple_network.collect_statistics();
-        match res {
-            Ok(stats) => {
-                info!("Stats: {:?}", stats);
-
-                // One frame should have been transmitted and one received
-                assert_eq!(stats.tx_total_frames().unwrap(), 1);
-                assert_eq!(stats.rx_total_frames().unwrap(), 1);
-            }
-            Err(e) => {
-                if e == Status::UNSUPPORTED.into() {
-                    info!("Stats: unsupported.");
-                } else {
-                    panic!("{e}");
-                }
+        Err(e) => {
+            if e == Status::UNSUPPORTED.into() {
+                info!("Stats: unsupported.");
+            } else {
+                panic!("{e}");
             }
         }
     }
+
+    simple_network.shutdown().unwrap();
 }

uefi/src/proto/network/snp.rs

@@ -269,7 +269,7 @@ impl SimpleNetwork {
         unsafe { &*(ptr::from_ref(&self.0.wait_for_packet).cast::<Event>()) }
     }
 
-    /// Returns a reference to the Simple Network mode.
+    /// Returns a reference to the [`NetworkMode`].
     #[must_use]
     pub fn mode(&self) -> &NetworkMode {
         unsafe { &*self.0.mode }