openvmm/pcie: Unify type 0 and type 1 save restore state into SavedState to make it backward compatible #2603
Conversation
… backward compatible
There was a problem hiding this comment.
Pull request overview
This PR unifies the save/restore state structures for PCIe Type 0 and Type 1 configuration space emulators to restore backward compatibility that was broken in a previous change. The unified SavedState struct now contains both common fields (indices 1-5) and Type 1-specific fields (indices 6-15), allowing old save states (with only common fields) to be restored by defaulting Type 1 fields to 0.
Key changes:
- Unified
SavedStatestruct replaces separateSavedType0StateandSavedType1Statestructures - Type 0 emulators save Type 1 fields as zeros and ignore them during restore
- Type 1 emulators now directly serialize all fields in the unified format, with proper BAR padding
| // Pad base_addresses to 6 elements for common header (Type 1 uses 2 BARs) | ||
| let mut full_base_addresses = [0u32; 6]; | ||
| for (i, &addr) in base_addresses.iter().enumerate().take(2) { | ||
| full_base_addresses[i] = addr; | ||
| } | ||
| self.common | ||
| .set_base_addresses(&[full_base_addresses[0], full_base_addresses[1]]); |
There was a problem hiding this comment.
The logic for extracting only the first 2 BARs from base_addresses is unnecessarily complex. The take(2) already limits the iteration to the first 2 elements, making the manual array initialization and indexing redundant. This could be simplified to directly copy the first 2 elements.
| // Pad base_addresses to 6 elements for common header (Type 1 uses 2 BARs) | |
| let mut full_base_addresses = [0u32; 6]; | |
| for (i, &addr) in base_addresses.iter().enumerate().take(2) { | |
| full_base_addresses[i] = addr; | |
| } | |
| self.common | |
| .set_base_addresses(&[full_base_addresses[0], full_base_addresses[1]]); | |
| // Use only the first two BARs (Type 1 uses 2 BARs), zero-filling if missing. | |
| let mut bar_addrs = [0u32; 2]; | |
| for (dst, src) in bar_addrs.iter_mut().zip(base_addresses.iter()) { | |
| *dst = *src; | |
| } | |
| self.common.set_base_addresses(&bar_addrs); |
| emu.write_u32(0x3C, 0x0040_0000).unwrap(); // Set latency_timer | ||
|
|
||
| // Save the state | ||
| let saved_state = common_emu.save().expect("save should succeed"); | ||
| let saved_state = emu.save().expect("save should succeed"); | ||
|
|
||
| // Reset the emulator | ||
| common_emu.reset(); | ||
| let result = common_emu.read_u32(0x04, &mut test_val); | ||
| assert_eq!(result, CommonHeaderResult::Handled); | ||
| emu.reset(); | ||
|
|
||
| // Verify state is reset | ||
| emu.read_u32(0x04, &mut test_val).unwrap(); | ||
| assert_eq!(test_val & 0x0007, 0x0000); // Should be reset | ||
|
|
||
| // Restore the state | ||
| common_emu | ||
| .restore(saved_state) | ||
| .expect("restore should succeed"); | ||
| let result = common_emu.read_u32(0x04, &mut test_val); | ||
| assert_eq!(result, CommonHeaderResult::Handled); | ||
| assert_eq!(test_val & 0x0007, 0x0007); // Should be restored | ||
| emu.restore(saved_state).expect("restore should succeed"); | ||
|
|
||
| // Test Type 1 common header emulator save/restore | ||
| let hardware_ids = HardwareIds { | ||
| vendor_id: 0x3333, | ||
| device_id: 0x4444, | ||
| revision_id: 1, | ||
| prog_if: ProgrammingInterface::NONE, | ||
| sub_class: Subclass::BRIDGE_PCI_TO_PCI, | ||
| base_class: ClassCode::BRIDGE, | ||
| type0_sub_vendor_id: 0, | ||
| type0_sub_system_id: 0, | ||
| }; | ||
| // Verify state is restored | ||
| emu.read_u32(0x04, &mut test_val).unwrap(); | ||
| assert_eq!(test_val & 0x0007, 0x0007); // Should be restored | ||
| } |
There was a problem hiding this comment.
The test writes to the latency_timer field via register 0x3C at line 2098 but never verifies that this value is properly restored after the restore operation. Consider adding a verification step after line 2115 to read back register 0x3C and confirm the latency_timer value was correctly restored.
|
@tjones60 Can we combine this PR with your work so that we can enable a gen 1 servicing test as soon as this fixes it? |
2 participants
Previous change in the PCIe type0/type1 config space emulator broken backward compatibility of the save and restore. This change updates the save and restore implementation to be backward compatible.