Skip to content

uefi: SNP transmit: document parameters #1664

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 26 additions & 3 deletions uefi/src/proto/network/snp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there some citation we can point to for this paragraph? In particular, is true that it's always wired? (I wouldn't be terribly surprised if some enterprising motherboard manufacturers supported wifi netboot, for example.)

Copy link
Member Author

@phip1611 phip1611 May 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my understanding, WiFis packet structure makes effective use incompatible with the SNP interface.

Therefore, we have EFI_WIRELESS_MAC_CONNECTION_PROTOCOL. Would you prefer it if I rephrase it as "in most cases, this refers to an Ethernet interface"?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I didn't know that a wifi interface would be incompatible. "in most cases, this refers to an Ethernet interface" sounds good to me

///
/// 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>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: For nicer rendering, how about changing to a named link like See [IANA IEEE 802 Numbers][ethertype] for examples.

/// 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 {
Expand All @@ -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()),
)
}
Expand Down