[patina::pi::hob] Adjust hob misc design by yangrongwei · Pull Request #1752 · OpenDevicePartnership/patina

yangrongwei · 2026-08-21T01:58:30Z

Description

Refactor the Hob::Misc design.

Introduce hob::GenericHob to hold the HOB generic header for Hob::Misc.

Per the PI spec, HOB is an open design, and all HOBs share a common header. Binding to a generic header is more intuitive than using a heterogeneous integer representation.

Bug Fix:
Introduce hob::EndOfHobList for unit tests, this is the correct struct for end-of-hob per PI Spec.
Correct related unit tests to use hob::EndOfHobList

Code Enhancement:
Add align(8) for hob::HobHeader. This is optional for consumers, but following PI Spec is not a bad choice.

How This Was Tested

Pass cargo make test

Integration Instructions

N/A

patina-automation · 2026-08-21T01:58:41Z

⌛ QEMU Validation Pending

QEMU validation is pending on successful CI completion.

Note: Any previous results are available in this comment's edit history.

This comment was automatically generated by the Patina QEMU PR Validation workflow.

yangrongwei · 2026-08-21T02:04:06Z

Original bug, #1748

joschock · 2026-08-21T19:15:53Z

                Hob::ResourceDescriptorV2(hob) => *hob = Box::leak(Box::new(ResourceDescriptorV2::clone(hob))),
-                Hob::Misc(_) => (), // Data is owned in Misc (nothing to move),
+                Hob::EndOfHobList(_) => {
+                    unreachable!("EndOfHobList Shoul not in list.")


This unreachable!() is a runtime panic; a comment indicating why this can't be reached would be nice. If it can be reached based on invalid input (e.g. a weirdly formatted hoblist or something), consider changing it to a debug_assert!() to avoid runtime panic based on input we don't control.

I don't think unreachable!() is appropriate here. The code is reachable given a malformed HOB list. It falls more into input validation than a logically unreachable case that macro is designed for. Regardless of anything else, I think that should be avoided. debug_assert!() is reasonable to catch the error in debug builds.

I used unreachable!() because I think the relocate_hobs() fn itself is questionable. I intend to remove the fn.

Q1

For release build, should we relocate these HOBs as well?

Q2

BTW, for the only consumer of relocate_hobs() in patina_dxe_core/src/lib.rs, I think relocate_hobs() is unnecessary.

Given that we redeclare the hob_list on relocated_hob_list.

patina/patina_dxe_core/src/lib.rs

Lines 452 to 455 in e15d0a4

// we have to relocate HOBs after memory services are initialized as we are going to allocate memory and

// the initial free memory may not be enough to contain the HOB list. We need to relocate the HOBs because

// the initial HOB list is not in mapped memory as passed from pre-DXE.

hob_list.relocate_hobs();

patina/patina_dxe_core/src/lib.rs

Lines 447 to 448 in e15d0a4

// Leak a DXE allocated PI HOB list so it is available throughout the DXE phase.

let relocated_hob_list = Box::leak(pi_hob_slice.to_vec().into_boxed_slice()).as_mut_ptr().cast::<c_void>();

Minor

Why should we copy the HOB from pre-DXE physical_hob_list to relocated_hob_list?

We have three copys now

The original physical_hob_list

The dxe managed relocated_hob_list

The rust used (partial) hob_list

We need both copies of the HOB list. The hob list as passed in to Patina does not live in allocated memory. It must be relocated before all memory is discovered because it is in free memory. We have one copy of the full HOB list for C drivers to consume in the PI spec format. However, this is not a great format for Rust. So, we use a native Rust format that is good for Rust code to consume and provides a safer abstraction.

As mentioned, we need both. I think this is also getting into scope creep.

makubacki · 2026-08-21T22:12:59Z

                Hob::ResourceDescriptorV2(hob) => *hob = Box::leak(Box::new(ResourceDescriptorV2::clone(hob))),
-                Hob::Misc(_) => (), // Data is owned in Misc (nothing to move),
+                Hob::EndOfHobList(_) => {
+                    unreachable!("EndOfHobList Shoul not in list.")


I don't think unreachable!() is appropriate here. The code is reachable given a malformed HOB list. It falls more into input validation than a logically unreachable case that macro is designed for. Regardless of anything else, I think that should be avoided. debug_assert!() is reasonable to catch the error in debug builds.

makubacki · 2026-08-21T22:32:02Z

+    fn test_gen_memory_pool() {
+        let buffer = gen_memory_pool();
+        assert_eq!(buffer.len(), 0x18);
+        let header = unsafe { &*(buffer.as_ptr() as *const hob::GenericHob) };


Please add a safety comment before this unsafe block. Also, I think you should use read_unaligned() here gen_memory_pool() is just an array on the stack which is not guaranteed to by 8-byte aligned.

Will follow

Javagedes · 2026-08-31T21:16:17Z

+    /// - MEMORY_POOL
+    /// - UNUSED
+    /// - LOAD_PEIM_UNUSED
+    Misc(&'a GenericHob),


There may have been conversation around this, so let me know if so, but If I'm understanding correctly, the Misc hob is incredibly close to the GuidHob. Due to this, I'm not a fan of the fact that we just give the user the header, and it is up to the caller to unsafe-ly extract any bytes after the header if the length exceeds the length of a header. I also don't think we should be using GenericHob. In my opinion it should just be a HobHeader.

I personally think it makes more sense for this enum variable to be below, where our code takes on the burden of extracting the bytes after the header:

Misc(&'a Hobheader, &'a [u8])

Javagedes · 2026-08-31T21:17:38Z

                }
                END_OF_HOB_LIST => return None,
-                hob_type => Hob::Misc(hob_type),
+                MEMORY_POOL | LOAD_PEIM_UNUSED | UNUSED => {


Here we would do something similar to GUID_EXTENSION and call the slice::from_raw_parts(data_ptr, data_len for the user.

Javagedes · 2026-08-31T21:19:28Z

+///
+#[repr(C)]
+#[derive(Debug)]
+pub struct GenericHob {


I don't see a need for GenericHob. It feels like we should just use the HobHeader directly - via the usage I described above.

Adjust hob misc design

32235e0

github-actions Bot added impact:non-functional Does not have a functional impact impact:testing Affects testing labels Aug 21, 2026

yangrongwei mentioned this pull request Aug 21, 2026

[Bug]: Question for patina::pi::hob::Hob #1748

Open

1 task

os-d requested review from Javagedes, joschock, makubacki and vineelko August 21, 2026 17:32

os-d linked an issue Aug 21, 2026 that may be closed by this pull request

[Bug]: Question for patina::pi::hob::Hob #1748

Open

1 task

os-d self-requested a review August 21, 2026 17:34

os-d reviewed Aug 21, 2026

View reviewed changes

Comment thread sdk/patina/src/pi/hob/hob_list.rs Outdated

Comment thread sdk/patina/src/pi/hob/hob_list.rs Outdated

Comment thread sdk/patina/src/pi/hob/hob_list.rs Outdated

Comment thread sdk/patina/src/pi/hob.rs Outdated

joschock reviewed Aug 21, 2026

View reviewed changes

Comment thread sdk/patina/src/pi/hob/hob_list.rs Outdated

joschock reviewed Aug 21, 2026

View reviewed changes

Comment thread sdk/patina/src/pi/hob.rs Outdated

makubacki reviewed Aug 21, 2026

View reviewed changes

apply review changes

817de26

yangrongwei requested review from joschock, makubacki and os-d August 27, 2026 01:28

yangrongwei added 2 commits August 28, 2026 23:14

Add doc + test for PI HOB list iter

0b3c23c

clear comment

b76d03f

github-actions Bot added the type:documentation Improvements or additions to documentation label Aug 28, 2026

Javagedes reviewed Aug 31, 2026

View reviewed changes

yangrongwei added 2 commits September 2, 2026 20:44

fix unit test for GenericHob, add unit test for GuidHob

8b658d4

correct comment

d795a5b

	// we have to relocate HOBs after memory services are initialized as we are going to allocate memory and
	// the initial free memory may not be enough to contain the HOB list. We need to relocate the HOBs because
	// the initial HOB list is not in mapped memory as passed from pre-DXE.
	hob_list.relocate_hobs();

	// Leak a DXE allocated PI HOB list so it is available throughout the DXE phase.
	let relocated_hob_list = Box::leak(pi_hob_slice.to_vec().into_boxed_slice()).as_mut_ptr().cast::<c_void>();

Uh oh!

Conversation

yangrongwei commented Aug 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How This Was Tested

Integration Instructions

Uh oh!

patina-automation Bot commented Aug 21, 2026

⌛ QEMU Validation Pending

Uh oh!

yangrongwei commented Aug 21, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Q1

Q2

Minor

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Javagedes Aug 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Javagedes Aug 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

yangrongwei commented Aug 21, 2026 •

edited

Loading

Javagedes Aug 31, 2026 •

edited

Loading

Javagedes Aug 31, 2026 •

edited

Loading