[edk2,v2,06/24] ArmVirtPkg/ArmVirtPsciResetSystemLib: move to FDT client protocol

Message ID 1460108711-12122-7-git-send-email-ard.biesheuvel@linaro.org
State Accepted
Commit 93f9a23f8771e718106d1c024b8d0a466f8ddcda
Headers show

Commit Message

Ard Biesheuvel April 8, 2016, 9:44 a.m.
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 <ard.biesheuvel@linaro.org>

---
 ArmVirtPkg/Library/ArmVirtPsciResetSystemLib/ArmVirtPsciResetSystemLib.c   | 31 ++++++++++++++++++--
 ArmVirtPkg/Library/ArmVirtPsciResetSystemLib/ArmVirtPsciResetSystemLib.inf |  9 ++++--
 2 files changed, 34 insertions(+), 6 deletions(-)

-- 
2.5.0

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Laszlo Ersek April 8, 2016, 2:23 p.m. | #1
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 <ard.biesheuvel@linaro.org>

> ---

>  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 <lersek@redhat.com>


- 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
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patch

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>
 
 #include <IndustryStandard/ArmStdSmc.h>
 
+#include <Protocol/FdtClient.h>
+
 STATIC UINT32 mArmPsciMethod;
 
 RETURN_STATUS
-EFIAPI
 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);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  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;
+  }
+  return EFI_SUCCESS;
 }
 
 /**
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