| Message ID | 1460108711-12122-7-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | Accepted |
| Commit | 93f9a23f8771e718106d1c024b8d0a466f8ddcda |
| Headers | show |
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
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
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