From 731698919cae9f8c88bcea8a786c5030d146c4a6 Mon Sep 17 00:00:00 2001 From: Philipp Schuster Date: Fri, 11 Apr 2025 08:49:58 +0200 Subject: [PATCH 1/6] uefi: SNP transmit: document parameters --- uefi/src/proto/network/snp.rs | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/uefi/src/proto/network/snp.rs b/uefi/src/proto/network/snp.rs index 208477d59..ec90f0caf 100644 --- a/uefi/src/proto/network/snp.rs +++ b/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 + /// + /// for examples. Typically, this is `0x0800` (IPv4) or `0x0806` (ARP). pub fn transmit( &self, header_size: usize, buffer: &[u8], src_addr: Option, - dest_addr: Option, + dst_addr: Option, protocol: Option, ) -> 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()), ) } From d655bb81c0ebd18f7479d8f171154ccfc1273bf5 Mon Sep 17 00:00:00 2001 From: Philipp Schuster Date: Tue, 15 Apr 2025 09:02:29 +0200 Subject: [PATCH 2/6] integration-test: SNP: improve setup and teardown routine --- uefi-test-runner/src/proto/network/snp.rs | 40 ++++++++++------------- uefi/src/proto/network/snp.rs | 2 +- 2 files changed, 19 insertions(+), 23 deletions(-) diff --git a/uefi-test-runner/src/proto/network/snp.rs b/uefi-test-runner/src/proto/network/snp.rs index a0c2ccba4..d5ecdd085 100644 --- a/uefi-test-runner/src/proto/network/snp.rs +++ b/uefi-test-runner/src/proto/network/snp.rs @@ -1,6 +1,6 @@ // SPDX-License-Identifier: MIT OR Apache-2.0 -use uefi::proto::network::snp::{InterruptStatus, ReceiveFlags, SimpleNetwork}; +use uefi::proto::network::snp::{InterruptStatus, NetworkState, ReceiveFlags, SimpleNetwork}; use uefi::proto::network::MacAddress; use uefi::{boot, Status}; @@ -10,29 +10,30 @@ pub fn test() { let handles = boot::find_handles::().unwrap_or_default(); for handle in handles { - let simple_network = boot::open_protocol_exclusive::(handle); - if simple_network.is_err() { + let Ok(simple_network) = boot::open_protocol_exclusive::(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())); + 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 +53,6 @@ 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; - } - let payload = b"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\ \x45\x00\ \x00\x21\ @@ -133,5 +127,7 @@ pub fn test() { } } } + + simple_network.shutdown().unwrap(); } } diff --git a/uefi/src/proto/network/snp.rs b/uefi/src/proto/network/snp.rs index ec90f0caf..2d9eb7c75 100644 --- a/uefi/src/proto/network/snp.rs +++ b/uefi/src/proto/network/snp.rs @@ -267,7 +267,7 @@ impl SimpleNetwork { unsafe { &*(ptr::from_ref(&self.0.wait_for_packet).cast::()) } } - /// 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 } From d80efd5e146c6f6795ebd8102edc9f0129f00f1a Mon Sep 17 00:00:00 2001 From: Philipp Schuster Date: Tue, 15 Apr 2025 09:15:07 +0200 Subject: [PATCH 3/6] integration-test: SNP: improve clarity Interestingly, the interrupt status never shows that we can receive a packet. Buggy OVMF firmware? --- uefi-test-runner/src/proto/network/snp.rs | 56 +++++++++++++++++------ 1 file changed, 43 insertions(+), 13 deletions(-) diff --git a/uefi-test-runner/src/proto/network/snp.rs b/uefi-test-runner/src/proto/network/snp.rs index d5ecdd085..c9965ef04 100644 --- a/uefi-test-runner/src/proto/network/snp.rs +++ b/uefi-test-runner/src/proto/network/snp.rs @@ -1,16 +1,47 @@ // SPDX-License-Identifier: MIT OR Apache-2.0 +use core::ops::DerefMut; 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 { + 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::().unwrap_or_default(); for handle in handles { - let Ok(simple_network) = boot::open_protocol_exclusive::(handle) else { + let Ok(mut simple_network) = boot::open_protocol_exclusive::(handle) else { continue; }; @@ -53,6 +84,12 @@ pub fn test() { ) .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\ @@ -69,7 +106,6 @@ pub fn test() { \xa9\xe4\ \x04\x01\x02\x03\x04"; - let dest_addr = MacAddress([0xffu8; 32]); assert!(!simple_network .get_interrupt_status() .unwrap() @@ -81,8 +117,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"); @@ -97,16 +133,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 From 398184de43fd4989cc50b65da4a0817c697659a9 Mon Sep 17 00:00:00 2001 From: Philipp Schuster Date: Tue, 15 Apr 2025 09:20:56 +0200 Subject: [PATCH 4/6] integration-test: SNP: print device path --- uefi-test-runner/src/proto/network/snp.rs | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/uefi-test-runner/src/proto/network/snp.rs b/uefi-test-runner/src/proto/network/snp.rs index c9965ef04..c7fa291d6 100644 --- a/uefi-test-runner/src/proto/network/snp.rs +++ b/uefi-test-runner/src/proto/network/snp.rs @@ -1,6 +1,8 @@ // SPDX-License-Identifier: MIT OR Apache-2.0 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}; @@ -39,11 +41,21 @@ pub fn test() { info!("Testing the simple network protocol"); let handles = boot::find_handles::().unwrap_or_default(); - for handle in handles { let Ok(mut simple_network) = boot::open_protocol_exclusive::(handle) else { continue; }; + // Print device path + { + let simple_network_dvp = boot::open_protocol_exclusive::(handle) + .expect("Should have device path"); + log::info!( + "Network interface: {}", + simple_network_dvp + .to_string(DisplayOnly(true), AllowShortcuts(true)) + .unwrap() + ); + } assert_eq!( simple_network.mode().state, From 521eb1bf0322e6cd9de7c068c7e27f7fc0ad74a3 Mon Sep 17 00:00:00 2001 From: Philipp Schuster Date: Tue, 15 Apr 2025 10:59:34 +0200 Subject: [PATCH 5/6] integration-test: add comment for test order constraint of SNP --- ovmf-firmware-debugcon.log | 0 uefi-test-runner/src/proto/network/mod.rs | 12 ++++++++++-- uefi-test-runner/src/proto/network/pxe.rs | 5 ----- 3 files changed, 10 insertions(+), 7 deletions(-) create mode 100644 ovmf-firmware-debugcon.log diff --git a/ovmf-firmware-debugcon.log b/ovmf-firmware-debugcon.log new file mode 100644 index 000000000..e69de29bb diff --git a/uefi-test-runner/src/proto/network/mod.rs b/uefi-test-runner/src/proto/network/mod.rs index 0e21db626..ff38ca813 100644 --- a/uefi-test-runner/src/proto/network/mod.rs +++ b/uefi-test-runner/src/proto/network/mod.rs @@ -4,10 +4,18 @@ pub fn test() { info!("Testing Network protocols"); http::test(); - 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(); + } } mod http; +#[cfg(feature = "pxe")] mod pxe; +#[cfg(feature = "pxe")] mod snp; diff --git a/uefi-test-runner/src/proto/network/pxe.rs b/uefi-test-runner/src/proto/network/pxe.rs index 9f29733f7..257af86ad 100644 --- a/uefi-test-runner/src/proto/network/pxe.rs +++ b/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::().expect("failed to get PXE base code handles"); From 2c881ba052ed81fadb30732321f9a56efd95d5d6 Mon Sep 17 00:00:00 2001 From: Philipp Schuster Date: Tue, 15 Apr 2025 09:20:23 +0200 Subject: [PATCH 6/6] xtask: explicitly set a MAC address This helps to identify the network interface in integration tests. So far, we also only use a single network device. In case there are more, the next device should have a similar MAC address, such as the last component being incremented by one. --- xtask/src/qemu.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/xtask/src/qemu.rs b/xtask/src/qemu.rs index 90c2081ca..b25c3431e 100644 --- a/xtask/src/qemu.rs +++ b/xtask/src/qemu.rs @@ -524,7 +524,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 {