patina_performance: Simplify configuration#1465
patina_performance: Simplify configuration#1465Javagedes wants to merge 3 commits intoOpenDevicePartnership:majorfrom
Conversation
QEMU Validation FailedQEMU validation did not complete successfully or did not shutdown as expected. Workflow run: https://github.com/OpenDevicePartnership/patina/actions/runs/24375068138
|
| Job | Result |
|---|---|
| Gather Incoming PR Metadata | ✅ |
| Run Patina QEMU Validation / Post In-Progress Notification | ✅ |
| Run Patina QEMU Validation / Preflight Checks | ✅ |
| Run Patina QEMU Validation / Get Constants / Get Repository Constants | ✅ |
| Run Patina QEMU Validation / Validate QEMU - Q35 (Linux) | ❌ |
| Run Patina QEMU Validation / Validate QEMU Q35 (Windows) | ❌ |
| Run Patina QEMU Validation / Validate QEMU - SBSA (Linux) | ❌ |
| Run Patina QEMU Validation / Emit PR Metadata | ✅ |
Error Details
qemu-validation-logs-Linux-Q35/q35-linux.log (6 error/warning sections)
error[E0433]: failed to resolve: could not find `config` in `patina_performance`
--> bin/q35_dxe_core.rs:96:40
|
96 | add.config(patina_performance::config::PerfConfig {
| ^^^^^^ could not find `config` in `patina_performance`
error[E0433]: failed to resolve: could not find `performance_config_provider` in `component`
--> bin/q35_dxe_core.rs:114:54
|
114 | add.component(patina_performance::component::performance_config_provider::PerformanceConfigurationProvider);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ could not find `performance_config_provider` in `component`
error[E0423]: expected value, found struct `patina_performance::component::performance::Performance`
--> bin/q35_dxe_core.rs:115:23
|
115 | add.component(patina_performance::component::performance::Performance);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
::: /__w/patina/patina/components/patina_performance/src/component/performance.rs:98:1
|
98 | pub struct Performance {
| ---------------------- `patina_performance::component::performance::Performance` defined here
error[E0603]: module `performance` is private
--> bin/q35_dxe_core.rs:115:54
|
115 | add.component(patina_performance::component::performance::Performance);
| ^^^^^^^^^^^ private module
|
note: the module `performance` is defined here
--> /__w/patina/patina/components/patina_performance/src/component.rs:10:1
|
10 | mod performance;
| ^^^^^^^^^^^^^^^
error: could not compile `qemu_dxe_core` (bin "qemu_q35_dxe_core") due to 4 previous errors
[cargo-make] ERROR - Error while running duckscript: Source: Unknown Line: 113 - Error while executing command, exit code: 101
qemu-validation-logs-Windows-Q35/q35-windows.log (6 error/warning sections)
error[E0433]: failed to resolve: could not find `config` in `patina_performance`
--> bin\q35_dxe_core.rs:96:40
|
96 | add.config(patina_performance::config::PerfConfig {
| ^^^^^^ could not find `config` in `patina_performance`
error[E0433]: failed to resolve: could not find `performance_config_provider` in `component`
--> bin\q35_dxe_core.rs:114:54
|
114 | add.component(patina_performance::component::performance_config_provider::PerformanceConfigurationProvider);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ could not find `performance_config_provider` in `component`
error[E0423]: expected value, found struct `patina_performance::component::performance::Performance`
--> bin\q35_dxe_core.rs:115:23
|
115 | add.component(patina_performance::component::performance::Performance);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
::: D:\a\patina\patina\components\patina_performance\src\component\performance.rs:98:1
|
98 | pub struct Performance {
| ---------------------- `patina_performance::component::performance::Performance` defined here
error[E0603]: module `performance` is private
--> bin\q35_dxe_core.rs:115:54
|
115 | add.component(patina_performance::component::performance::Performance);
| ^^^^^^^^^^^ private module
|
note: the module `performance` is defined here
--> D:\a\patina\patina\components\patina_performance\src\component.rs:10:1
|
10 | mod performance;
| ^^^^^^^^^^^^^^^
error: could not compile `qemu_dxe_core` (bin "qemu_q35_dxe_core") due to 4 previous errors
[cargo-make] ERROR - Error while running duckscript: Source: Unknown Line: 113 - Error while executing command, exit code: 101
qemu-validation-logs-Linux-SBSA/sbsa-linux.log (6 error/warning sections)
error[E0433]: failed to resolve: could not find `config` in `patina_performance`
--> bin/q35_dxe_core.rs:96:40
|
96 | add.config(patina_performance::config::PerfConfig {
| ^^^^^^ could not find `config` in `patina_performance`
error[E0433]: failed to resolve: could not find `performance_config_provider` in `component`
--> bin/q35_dxe_core.rs:114:54
|
114 | add.component(patina_performance::component::performance_config_provider::PerformanceConfigurationProvider);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ could not find `performance_config_provider` in `component`
error[E0423]: expected value, found struct `patina_performance::component::performance::Performance`
--> bin/q35_dxe_core.rs:115:23
|
115 | add.component(patina_performance::component::performance::Performance);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
::: /__w/patina/patina/components/patina_performance/src/component/performance.rs:98:1
|
98 | pub struct Performance {
| ---------------------- `patina_performance::component::performance::Performance` defined here
error[E0603]: module `performance` is private
--> bin/q35_dxe_core.rs:115:54
|
115 | add.component(patina_performance::component::performance::Performance);
| ^^^^^^^^^^^ private module
|
note: the module `performance` is defined here
--> /__w/patina/patina/components/patina_performance/src/component.rs:10:1
|
10 | mod performance;
| ^^^^^^^^^^^^^^^
error: could not compile `qemu_dxe_core` (bin "qemu_q35_dxe_core") due to 4 previous errors
[cargo-make] ERROR - Error while running duckscript: Source: Unknown Line: 113 - Error while executing command, exit code: 101
Dependencies
| Repository | Ref |
|---|---|
| patina | 4f74e51 |
| patina-dxe-core-qemu | dfed98b |
| patina-fw-patcher | b08d6ff |
| patina-qemu firmware | v3.0.0 |
| patina-qemu build script | efe1fb2 |
This comment was automatically generated by the Patina QEMU PR Validation Post workflow.
patina_performance has confusing configuration story. Prior to this change, configuration was done via patina `Config`, A platform could optionally register a secondary component that would read a HOB (if provided) and overwrite any `Config` with the HOB configuration. This commit works to simplify the configuration story by making a couple of changes: 1. Removal of the `Config` object. As stated in documentation throughout patina, `Config` is meant for configuration that is expected to be shared between multiple components. This particular configuration does not need to be shared, so it is moved to be private configuration of the component. 2. Removal of the optional HOB reader driver. Now, a private configuration of the component will allow you to specify if the HOB is able to override local configuration or not. This new story reduces patina_performance setup from three different definitions (Config, performance component, and hob component) to only one, the performance component.
3a36910 to
854c5b7
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
| #[repr(C, packed)] | ||
| struct PerformanceConfig { | ||
| /// Indicates whether the Patina Performance component is enabled. | ||
| pub enable_component: u8, |
There was a problem hiding this comment.
bool is not a supported zerocopy type
There was a problem hiding this comment.
Unfortunately, the current design of the FromHob trait is that parsing must be infallible. I actually think that it would make more sense if it was fallible. But at this point in time the FromHob macro expects zerocopy::FromBytes to be implemented. We could do a manual implementation, but we would have to unwrap the result.
| #[derive(Default)] | ||
| pub struct Performance { | ||
| config: PerformanceConfig, | ||
| ignore_hob: bool, |
There was a problem hiding this comment.
HOBs are already dynamically produced by platform logic, for example, in this case whether or not the platform would want to configure performance. The performance_config_provider was more to keep this component free of HOB logic which it now has anyway.
There was a problem hiding this comment.
Can ignore_hob just be dropped entirely and tell the platform to produce the HOB to override config if they want?
There was a problem hiding this comment.
yes, we could absolutely entirely drop ignore_hob if we wanted, and I'm happy to do that. It would technically result in a regression in configurability, however.
Right now, you it is optional to add the PerformanceConfigurationProvider, which means a platform has the possibility of ignoring the HOB even if it exists. Doing the above suggestion removes that possibility. I'm not against it. But it is a thing.

Description
patina_performance has a confusing configuration story. Prior to this change, configuration was done via patina
Config, A platform could optionally register a secondary component that would read a HOB (if provided) and overwrite anyConfigwith the HOB configuration.This commit works to simplify the configuration story by making a couple of changes:
Configobject. As stated in documentation throughout patina,Configis meant for configuration that is expected to be shared between multiple components. This particular configuration does not need to be shared, so it is moved to be private configuration of the component.This new story reduces patina_performance setup from three different definitions (Config, performance component, and hob component) to only one, the performance component.
How This Was Tested
Reading final configuration of the performance component on Q35 with various configuration settings both in the component and via the HOB
Integration Instructions
Previous
After