[patina_internal_cpu] remove alloc usage#1423
[patina_internal_cpu] remove alloc usage#1423kuqin12 wants to merge 7 commits intoOpenDevicePartnership:mainfrom
Conversation
✅ QEMU Validation PassedAll QEMU validation jobs completed successfully.
Workflow run: https://github.com/OpenDevicePartnership/patina/actions/runs/23473278105 Boot Time to EFI Shell
Dependencies
This comment was automatically generated by the Patina QEMU PR Validation Post workflow. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
| error::EfiError, | ||
| }; | ||
| #[cfg(feature = "alloc")] | ||
| use patina_paging::PageTable; |
There was a problem hiding this comment.
Design-wise it seems weird to have:
#[cfg(feature = "alloc")]
use patina_paging::PageTable;But no cfg gate in core/patina_internal_cpu/src/interrupts/x64/interrupt_manager.rs:
use patina_paging::PageTable;There was a problem hiding this comment.
My understanding is that this is imported so the PageTable trait is in scope for .dump_page_tables() to resolve in let _ = pt.dump_page_tables(far & !(UEFI_PAGE_MASK as u64), UEFI_PAGE_SIZE as u64); later in the file.
Is that the case? If so, that would no longer work with this behind alloc and that code not.
There was a problem hiding this comment.
This is a good observation. Indeed you are right that original inclusion was needed to resolve the dump_page_tables call. Will remove the change as it is not enforcing the alloc usage in the first place.
As per why this was not caught anywhere, this is because patina has default set to ['alloc']. So as long as any subcrate pulling in patina, it will pick up the default feature and enable alloc globally. I will create a sub-issue and handle that in a separate PR.
| #[derive(Default, Copy, Clone, IntoService)] | ||
| #[service(dyn Cpu)] | ||
| #[derive(Default, Copy, Clone)] | ||
| #[cfg_attr(feature = "alloc", derive(IntoService))] |
There was a problem hiding this comment.
I think this is okay, but it would be a little more direct/clear to have something like a patina_component feature that is dependent on alloc. However, that adds more features/complexity. Feel free to ignore.
There was a problem hiding this comment.
The entire patina_component is guarded behind "alloc" feature as of today. But I agree that we should mention it somewhere.
2 participants
Description
Patina internal CPU hosts the interrupt manager and other fundamental functionalities, which should not work on top of the allocator.
Admittedly, the current crate can support more advanced components that works on top of exception forwarding, but this should not be treated as a required dependency.
How This Was Tested
This was tested on QEMU Q35 and booted to UEFI shell.
Integration Instructions
N/A