On 04/08/16 11:44, Ard Biesheuvel wrote:
> Instead of relying on VirtFdtDxe to detect the PSCI method, move our
> EfiResetSystemLib to the FDT client protocol to interrogate the device
> tree directly.
>
> Since this library is only consumed by EmbeddedPkg/ResetRuntimeDxe, and
> considering that the PCD is no longer set, and even removed completely in a
> subsequent patch, this conversion is guaranteed to be safe.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <[email protected]>
> ---
> ArmVirtPkg/Library/ArmVirtPsciResetSystemLib/ArmVirtPsciResetSystemLib.c |
> 31 ++++++++++++++++++--
> ArmVirtPkg/Library/ArmVirtPsciResetSystemLib/ArmVirtPsciResetSystemLib.inf |
> 9 ++++--
> 2 files changed, 34 insertions(+), 6 deletions(-)
>
> diff --git
> a/ArmVirtPkg/Library/ArmVirtPsciResetSystemLib/ArmVirtPsciResetSystemLib.c
> b/ArmVirtPkg/Library/ArmVirtPsciResetSystemLib/ArmVirtPsciResetSystemLib.c
> index 88332f56fd9c..189519ccccda 100644
> --- a/ArmVirtPkg/Library/ArmVirtPsciResetSystemLib/ArmVirtPsciResetSystemLib.c
> +++ b/ArmVirtPkg/Library/ArmVirtPsciResetSystemLib/ArmVirtPsciResetSystemLib.c
> @@ -26,19 +26,44 @@
> #include <Library/EfiResetSystemLib.h>
> #include <Library/ArmSmcLib.h>
> #include <Library/ArmHvcLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
Should be added to [LibraryClasses] too.
>
> #include <IndustryStandard/ArmStdSmc.h>
>
> +#include <Protocol/FdtClient.h>
> +
> STATIC UINT32 mArmPsciMethod;
>
> RETURN_STATUS
> -EFIAPI
I think you shouldn't remove EFIAPI. (It wasn't intentional, right?)
> ArmPsciResetSystemLibConstructor (
> VOID
> )
> {
> - mArmPsciMethod = PcdGet32 (PcdArmPsciMethod);
> - return RETURN_SUCCESS;
> + EFI_STATUS Status;
> + FDT_CLIENT_PROTOCOL *FdtClient;
> + CONST VOID *Prop;
> +
> + Status = gBS->LocateProtocol (&gFdtClientProtocolGuid, NULL, (VOID
> **)&FdtClient);
Line is overlong.
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
Could be replaced with ASSERT_EFI_ERROR(), due to the DepEx.
> +
> + Status = FdtClient->FindCompatibleNodeProperty (FdtClient, "arm,psci-0.2",
> + "method", &Prop, NULL);
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> +
> + if (AsciiStrnCmp (Prop, "hvc", 3) == 0) {
> + mArmPsciMethod = 1;
> + } else if (AsciiStrnCmp (Prop, "smc", 3) == 0) {
> + mArmPsciMethod = 2;
> + } else {
> + DEBUG ((EFI_D_ERROR, "%a: Unknown PSCI method \"%a\"\n",
> __FUNCTION__,
> + Prop));
> + return EFI_NOT_FOUND;
indentation is too deep
> + }
> + return EFI_SUCCESS;
> }
>
Hm, I think this refactoring will slightly change behavior. In the
original code, if the node or the property is not found, or the property
didn't have one of the expected prefixes, then the PCD was left at zero.
In which case this library initialized fine (with mArmPsciMethod=0),
only LibResetSystem() would return EFI_UNSUPPORTED.
In comparison, the above cases will now trigger an assertion failure --
that's what happens when a library constructor fails.
I'm not sure if the PSCI method is a hard requirement for the DTB.
If it is, then:
- this logic change is fine, and
- with the above warts fixed:
Reviewed-by: Laszlo Ersek <[email protected]>
- If the PSCI method is optional in the DTB (i.e., it makes sense to
run the guest without it), then you might want to rework the return
statuses in the constructor.
Personally I do think that the reset capability is a hard requirement,
so the logic change should be fine.
Thanks
Laszlo
> /**
> diff --git
> a/ArmVirtPkg/Library/ArmVirtPsciResetSystemLib/ArmVirtPsciResetSystemLib.inf
> b/ArmVirtPkg/Library/ArmVirtPsciResetSystemLib/ArmVirtPsciResetSystemLib.inf
> index 86d6104ca258..875591766d04 100644
> ---
> a/ArmVirtPkg/Library/ArmVirtPsciResetSystemLib/ArmVirtPsciResetSystemLib.inf
> +++
> b/ArmVirtPkg/Library/ArmVirtPsciResetSystemLib/ArmVirtPsciResetSystemLib.inf
> @@ -20,7 +20,7 @@ [Defines]
> FILE_GUID = c81d76ed-66fa-44a3-ac4a-f163120187a9
> MODULE_TYPE = BASE
> VERSION_STRING = 1.0
> - LIBRARY_CLASS = EfiResetSystemLib
> + LIBRARY_CLASS = EfiResetSystemLib|DXE_DRIVER
> DXE_RUNTIME_DRIVER
> CONSTRUCTOR = ArmPsciResetSystemLibConstructor
>
> [Sources]
> @@ -38,5 +38,8 @@ [LibraryClasses]
> ArmSmcLib
> ArmHvcLib
>
> -[Pcd]
> - gArmVirtTokenSpaceGuid.PcdArmPsciMethod
> +[Protocols]
> + gFdtClientProtocolGuid ## CONSUMES
> +
> +[Depex]
> + gFdtClientProtocolGuid
>
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
