SNP Integration Test: Improve Clarity

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

@@ -3,9 +3,17 @@
 pub fn test() {
     info!("Testing Network protocols");
 
-    pxe::test();
-    snp::test();
+    #[cfg(feature = "pxe")]
+    {
+        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.
+        snp::test();
+    }
 }
 
+#[cfg(feature = "pxe")]
 mod pxe;
+#[cfg(feature = "pxe")]
 mod snp;

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

@@ -5,11 +5,6 @@ use uefi::proto::network::IpAddress;
 use uefi::{boot, CStr8};
 
 pub fn test() {
-    // Skip the test if the `pxe` feature is not enabled.
-    if cfg!(not(feature = "pxe")) {
-        return;
-    }
-
     info!("Testing The PXE base code protocol");
 
     let handles = boot::find_handles::<BaseCode>().expect("failed to get PXE base code handles");

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

@@ -1,38 +1,82 @@
 // SPDX-License-Identifier: MIT OR Apache-2.0
 
-use uefi::proto::network::snp::{InterruptStatus, ReceiveFlags, SimpleNetwork};
+use core::ops::DerefMut;
+use uefi::proto::device_path::text::{AllowShortcuts, DisplayOnly};
+use uefi::proto::device_path::DevicePath;
+use uefi::proto::network::snp::{InterruptStatus, NetworkState, ReceiveFlags, SimpleNetwork};
 use uefi::proto::network::MacAddress;
 use uefi::{boot, Status};
 
+const ETHERNET_PROTOCOL_IPV4: u16 = 0x0800;
+
+/// Receives the next IPv4 packet and prints corresponding metadata.
+fn receive(simple_network: &mut SimpleNetwork, buffer: &mut [u8]) -> uefi::Result<usize> {
+    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),
+    );
+
+    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);
+
+        // 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 message.
 pub fn test() {
     info!("Testing the simple network protocol");
 
     let handles = boot::find_handles::<SimpleNetwork>().unwrap_or_default();
-
     for handle in handles {
-        let simple_network = boot::open_protocol_exclusive::<SimpleNetwork>(handle);
-        if simple_network.is_err() {
+        let Ok(mut simple_network) = boot::open_protocol_exclusive::<SimpleNetwork>(handle) else {
             continue;
+        };
+        // Print device path
+        {
+            let simple_network_dvp = boot::open_protocol_exclusive::<DevicePath>(handle)
+                .expect("Should have device path");
+            log::info!(
+                "Network interface: {}",
+                simple_network_dvp
+                    .to_string(DisplayOnly(true), AllowShortcuts(true))
+                    .unwrap()
+            );
         }
-        let simple_network = simple_network.unwrap();
 
-        // Check shutdown
-        let res = simple_network.shutdown();
-        assert!(res == Ok(()) || res == Err(Status::NOT_STARTED.into()));
+        assert_eq!(
+            simple_network.mode().state,
+            NetworkState::STOPPED,
+            "Should be in stopped state"
+        );
 
-        // Check stop
-        let res = simple_network.stop();
-        assert!(res == Ok(()) || res == Err(Status::NOT_STARTED.into()));
+        // Check media
+        if !bool::from(simple_network.mode().media_present_supported)
+            || !bool::from(simple_network.mode().media_present)
+        {
+            continue;
+        }
 
-        // Check start
         simple_network
             .start()
-            .expect("Failed to start Simple Network");
+            .expect("Network should not be started yet");
 
-        // Check initialize
         simple_network
             .initialize(0, 0)
-            .expect("Failed to initialize Simple Network");
+            .expect("Network should not be initialized yet");
 
         // edk2 virtio-net driver does not support statistics, so
         // allow UNSUPPORTED (same for collect_statistics below).
@@ -52,13 +96,12 @@ pub fn test() {
             )
             .expect("Failed to set receive filters");
 
-        // Check media
-        if !bool::from(simple_network.mode().media_present_supported)
-            || !bool::from(simple_network.mode().media_present)
-        {
-            continue;
-        }
-
+        // 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\
@@ -75,7 +118,6 @@ pub fn test() {
             \xa9\xe4\
             \x04\x01\x02\x03\x04";
 
-        let dest_addr = MacAddress([0xffu8; 32]);
         assert!(!simple_network
             .get_interrupt_status()
             .unwrap()
@@ -87,8 +129,8 @@ pub fn test() {
                 simple_network.mode().media_header_size as usize,
                 payload,
                 None,
-                Some(dest_addr),
-                Some(0x0800),
+                Some(simple_network.mode().broadcast_address),
+                Some(ETHERNET_PROTOCOL_IPV4),
             )
             .expect("Failed to transmit frame");
 
@@ -103,16 +145,10 @@ pub fn test() {
         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(1_000_000);
-
-            simple_network
-                .receive(&mut buffer, None, None, None, None)
-                .unwrap();
-        }
+        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
@@ -133,5 +169,7 @@ pub fn test() {
                 }
             }
         }
+
+        simple_network.shutdown().unwrap();
     }
 }

uefi/src/proto/network/snp.rs

@@ -186,13 +186,36 @@ impl SimpleNetwork {
         status.to_result_with_val(|| NonNull::new(tx_buf.cast()))
     }
 
-    /// Place a packet in the transmit queue of a network interface.
+    /// Place a packet in the transmit queue of the network interface.
+    ///
+    /// The packet structure depends on the type of network interface, but
+    /// effectively this is always a (wired) ethernet interface. In these cases,
+    /// this function transmits ethernet frames.
+    ///
+    /// The header of the packet can be filled by the function with the given
+    /// parameters, but the buffer must already reserve the space for the
+    /// header.
+    ///
+    /// # Arguments
+    /// - `header_size`: The size in bytes of the media header to be filled by
+    ///   the `transmit()` function. If this is `0`, the (ethernet frame) header
+    ///   will not be filled by the function and taken as-is from the buffer.
+    ///   If it is nonzero, then it must be equal to `media_header_size` of
+    ///   the corresponding [`NetworkMode`] and the `dst_addr` and `protocol`
+    ///   parameters must not be `None`.
+    /// - `buffer`: The buffer containing the whole network packet with all
+    ///   its payload including the header for the medium.
+    /// - `src_addr`: The optional source address.
+    /// - `dst_addr`: The optional destination address.
+    /// - `protocol`: Ether Type as of RFC 3232. See
+    ///   <https://www.iana.org/assignments/ieee-802-numbers/ieee-802-numbers.xhtml#ieee-802-numbers-1>
+    ///   for examples. Typically, this is `0x0800` (IPv4) or `0x0806` (ARP).
     pub fn transmit(
         &self,
         header_size: usize,
         buffer: &[u8],
         src_addr: Option<MacAddress>,
-        dest_addr: Option<MacAddress>,
+        dst_addr: Option<MacAddress>,
         protocol: Option<u16>,
     ) -> Result {
         unsafe {
@@ -202,7 +225,7 @@ impl SimpleNetwork {
                 buffer.len(),
                 buffer.as_ptr().cast(),
                 src_addr.as_ref().map(ptr::from_ref).unwrap_or(ptr::null()),
-                dest_addr.as_ref().map(ptr::from_ref).unwrap_or(ptr::null()),
+                dst_addr.as_ref().map(ptr::from_ref).unwrap_or(ptr::null()),
                 protocol.as_ref().map(ptr::from_ref).unwrap_or(ptr::null()),
             )
         }
@@ -244,7 +267,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 }

xtask/src/qemu.rs

@@ -531,7 +531,7 @@ pub fn run_qemu(arch: UefiArch, opt: &QemuOpt) -> Result<()> {
             "-netdev",
             "user,id=net0,net=192.168.17.0/24,tftp=uefi-test-runner/tftp/,bootfile=fake-boot-file",
             "-device",
-            "virtio-net-pci,netdev=net0",
+            "virtio-net-pci,netdev=net0,mac=52:54:00:00:00:01",
         ]);
         Some(net::EchoService::start())
     } else {