Platform/ARM: VExpressPkg: Implement Platform Configuration menu #856
Conversation
samimujawar
left a comment
samimujawar
left a comment
There was a problem hiding this comment.
@sarah-walker-arm Thank you for enabling this feature.
After trying the patch I think two changes are needed before merge:
- The Boot CPU should never be de-selectable.
- The UI should inform users that a system reset is required for the new settings to take effect, and optionally facilitate the reset on approval from the user.
Other comments are minor.
Platform/ARM/VExpressPkg/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManager.c
Outdated
Show resolved
Hide resolved
Platform/ARM/VExpressPkg/Include/Guid/ArmVExpressPlatformConfig.h
Outdated
Show resolved
Hide resolved
| &mPrivateData->ConfigAccess, | ||
| NULL | ||
| ); | ||
| ASSERT_EFI_ERROR (Status); |
There was a problem hiding this comment.
I think it would be good to handle the error rather than just asserting as the ASSERTs would vanish in release builds.
Same at other places in this patch.
Platform/ARM/VExpressPkg/Drivers/ArmVExpressPlatformConfigDxe/ArmVExpressPlatformConfig.c
Show resolved
Hide resolved
Platform/ARM/VExpressPkg/Drivers/ArmVExpressPlatformConfigDxe/PlatformConfigVfr.vfr
Outdated
Show resolved
Hide resolved
sarah-walker-arm
force-pushed
the
4042_vexpress_platformconfig
branch
from
6a3c816 to
87e639b
Compare
Platform/ARM/VExpressPkg/Drivers/ArmVExpressPlatformConfigDxe/PlatformConfigVfr.vfr
Outdated
Show resolved
Hide resolved
Platform/ARM/VExpressPkg/Drivers/ArmVExpressPlatformConfigDxe/PlatformConfigVfr.vfr
Outdated
Show resolved
Hide resolved
sarah-walker-arm
force-pushed
the
4042_vexpress_platformconfig
branch
from
87e639b to
cd64894
Compare
| // | ||
| Status = gBS->LocateProtocol (&gEfiHiiConfigRoutingProtocolGuid, NULL, (VOID **)&HiiConfigRouting); | ||
| if (EFI_ERROR (Status)) { | ||
| return Status; |
There was a problem hiding this comment.
Should mPrivateData be freed when errorcase?
| // | ||
| ConfigRequestHdr = HiiConstructConfigHdr (&gArmVExpressPlatformConfigGuid, VariableName, DriverHandle); | ||
| if (ConfigRequestHdr == NULL) { | ||
| ASSERT (ConfigRequestHdr != NULL); |
There was a problem hiding this comment.
Is this assert required?
| ActionFlag = HiiSetToDefaults (ConfigRequestHdr, EFI_HII_DEFAULT_CLASS_STANDARD); | ||
| if (!ActionFlag) { | ||
| ArmVExpressPlatformConfigUnload (ImageHandle); | ||
| return EFI_INVALID_PARAMETER; |
There was a problem hiding this comment.
I think ConfigRequestHdr should be freed with FreePool() at error case since it isn't freed in
ArmVExpressPlatformConfigUnload() or Am I missing something?
|
@sarah-walker-arm looks like there are merge conflicts when rebasing with latest edk2-platforms codebase. Can you resolve the conflicts and push the changes, please? |
Implement ArmVExpressPlatformConfigDxe driver, to allow VExpress platform configuration via UEFI menus. Currently CPU enable/disable is supported. ACPI tables are updated to reflect the chosen configuration. Signed-off-by: Sarah Walker <[email protected]>
sarah-walker-arm
force-pushed
the
4042_vexpress_platformconfig
branch
from
cd64894 to
ec8dba5
Compare
3 participants
Implement ArmVExpressPlatformConfigDxe driver, to allow VExpress platform configuration via UEFI menus. Currently CPU enable/disable is supported. ACPI tables are updated to reflect the chosen configuration.