From 9c6782db737d40fbf2169b8f60d9749e9a474b19 Mon Sep 17 00:00:00 2001 From: Denys Fedoryshchenko Date: Fri, 10 Jul 2026 02:59:02 +0300 Subject: [PATCH] fix(sel): correct event data selector bit order IPMI encodes the format and availability selectors for Event Data 2 and Event Data 3 in Event Data 1. The decoder assigned those selector fields in reverse: it read bits [5:4] for Event Data 2 and bits [7:6] for Event Data 3. This issue was noticed while decoding Lenovo SEL entries. Those records exposed standard System Firmware Progress extension values that disappeared during EventData parsing, leading to the selector-bit mismatch. IPMI v2.0 revision 1.1, section 29.7, Table 29-6 defines the opposite layout for both threshold and discrete events: bits [7:6] describe Event Data 2, while bits [5:4] describe Event Data 3. Swap the shifts used by EventData::parse and update the API documentation to match the normative table. The reversal loses valid sensor-specific extension data whenever the two selectors differ. A standard System Firmware Progress record with event bytes C2 13 FF demonstrates the problem. Per Table 29-6, C2 means that Event Data 2 carries a sensor-specific extension and Event Data 3 is unspecified. Per Table 42-3, sensor type 0Fh, offset 02h, extension 13h means Starting operating system boot process. The old decoder instead treated Event Data 2 as unspecified, interpreted FF as Event Data 3, and consequently discarded 13h. The existing 59 42 50 test did not expose the reversal because both selector fields in 59h are 01b. Add asymmetric regression tests for C2 13 FF and for the inverse Data-3-only selector layout 32 FF 23. These independently pin each selector to its correct bit field and prevent a future swap from passing accidentally. Normative reference: Intelligent Platform Management Interface Specification, Second Generation, v2.0 revision 1.1, section 29.7 Table 29-6 and section 42 Table 42-3: https://www.intel.com/content/dam/www/public/us/en/documents/product-briefs/ipmi-second-gen-interface-spec-v2-rev1-1.pdf Concrete firmware-progress encoding examples: Oracle documents Event Data 2 value 13h for Starting operating system boot process: https://docs.oracle.com/cd/E19121-01/sf.v20z/817-5249-17/chapter2.html NVIDIA shows an ipmitool SEL record with Event Data c213ff decoded as System boot initiated: https://docs.nvidia.com/networking/display/bluefieldbsp453/logging fix(doc): remove misleading power supply test heading Remove a stale Rustdoc comment that describes the generic EventData test module as a Power Supply sensor-specific decoder. The comment was introduced with the original event-data implementation in 8439308, directly above #[cfg(test)] mod tests. Git history shows that no Power Supply decoder was removed from or moved away from this location; the documented item has always been the test module. Keeping the heading causes generated documentation and source readers to infer that a Sensor Type 08h decoder exists here when the module only contains generic EventData parsing tests. Remove the inaccurate heading separately from the selector-bit behavior fix. Reference: IPMI 2.0 revision 1.1, Table 42-3 defines Sensor Type 08h as Power Supply, but this module does not implement a dedicated decoder for it: https://www.intel.com/content/dam/www/public/us/en/documents/product-briefs/ipmi-second-gen-interface-spec-v2-rev1-1.pdf --- ipmi-rs-core/src/storage/sdr/event_data.rs | 47 +++++++++++++++++----- 1 file changed, 38 insertions(+), 9 deletions(-) diff --git a/ipmi-rs-core/src/storage/sdr/event_data.rs b/ipmi-rs-core/src/storage/sdr/event_data.rs index afef852..8f2011d 100644 --- a/ipmi-rs-core/src/storage/sdr/event_data.rs +++ b/ipmi-rs-core/src/storage/sdr/event_data.rs @@ -1,6 +1,7 @@ //! Event Data Field Decoding //! -//! Reference: IPMI 2.0 Specification, Section 29.7 "Event Data Field Formats" +//! Reference: IPMI 2.0 Specification, Section 29.7 "Event Data Field Formats", +//! particularly Table 29-6 "Event Request Message Event Data Field Contents" //! //! This module provides decoding for the 3-byte event data field in SEL entries. @@ -8,7 +9,7 @@ use core::fmt; use nonmax::NonMaxU8; -/// Event data byte 2 availability/format (from event_data\[0\] bits \[5:4\]) +/// Event data byte 2 availability/format (from event_data\[0\] bits \[7:6\]) #[derive(Debug, Clone, Copy, PartialEq)] pub enum EventData2Type { /// Unspecified (00b) @@ -21,7 +22,7 @@ pub enum EventData2Type { SensorSpecific(NonMaxU8), } -/// Event data byte 3 availability/format (from event_data\[0\] bits \[7:6\]) +/// Event data byte 3 availability/format (from event_data\[0\] bits \[5:4\]) #[derive(Debug, Clone, Copy, PartialEq)] pub enum EventData3Type { /// Unspecified (00b) @@ -98,7 +99,7 @@ impl fmt::Display for EventData3Type { /// Decoded event data from a SEL entry. /// -/// Reference: IPMI 2.0 Specification, Section 29.7 +/// Reference: IPMI 2.0 Specification, Section 29.7, Table 29-6 #[derive(Debug, Clone, PartialEq)] pub struct EventData { /// Event offset (bits \[3:0\] of byte 1) @@ -113,8 +114,8 @@ impl EventData { /// Parse event data from the 3-byte event data field. pub fn parse(data: &[u8; 3]) -> Self { let offset = data[0] & 0x0F; - let data2_type = EventData2Type::parse((data[0] >> 4) & 0x03, data[1]); - let data3_type = EventData3Type::parse((data[0] >> 6) & 0x03, data[2]); + let data2_type = EventData2Type::parse((data[0] >> 6) & 0x03, data[1]); + let data3_type = EventData3Type::parse((data[0] >> 4) & 0x03, data[2]); Self { offset, @@ -139,9 +140,6 @@ impl fmt::Display for EventData { } } -/// Decode Power Supply sensor-specific event data. -/// -/// Reference: IPMI 2.0 Specification, Table 42-3, Sensor Type 08h #[cfg(test)] mod tests { use super::*; @@ -172,4 +170,35 @@ mod tests { let data = EventData::parse(&[0x00, 0xFF, 0xFF]); assert_eq!(data.to_string(), ""); } + + #[test] + fn test_data2_sensor_specific_selector_uses_bits_7_6() { + // IPMI 2.0, Table 42-3: System Firmware Progress (sensor type 0Fh), + // offset 02h, extension 13h means "Starting operating system boot + // process". Per Table 29-6, C2h selects a sensor-specific extension + // in Event Data 2 (bits 7:6 = 11b) and leaves Event Data 3 unspecified + // (bits 5:4 = 00b). + let data = EventData::parse(&[0xC2, 0x13, 0xFF]); + + assert_eq!(data.offset, 0x02); + assert_eq!( + data.data2_type, + EventData2Type::SensorSpecific(NonMaxU8::new(0x13).unwrap()) + ); + assert_eq!(data.data3_type, EventData3Type::Unspecified); + } + + #[test] + fn test_data3_sensor_specific_selector_uses_bits_5_4() { + // Use unequal selector fields to ensure Event Data 3 is selected only + // by bits 5:4, independently of Event Data 2's bits 7:6. + let data = EventData::parse(&[0x32, 0xFF, 0x23]); + + assert_eq!(data.offset, 0x02); + assert_eq!(data.data2_type, EventData2Type::Unspecified); + assert_eq!( + data.data3_type, + EventData3Type::SensorSpecific(NonMaxU8::new(0x23).unwrap()) + ); + } }