From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.61]) by mx.groups.io with SMTP id smtpd.web11.8306.1578415823015020342 for ; Tue, 07 Jan 2020 08:50:23 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=iYpTHjRL; spf=pass (domain: redhat.com, ip: 205.139.110.61, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1578415822; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=m6ghVLKcdpMHA11DPbd0zqMDji+9d7T9N9VEuyi13xw=; b=iYpTHjRL05QQtKqqXJjS73bdqYZH6H5LV/nVeddKJ4t3+mILE5mnjK7/qk8odXOVglZuGJ buhCW6wFMKuBFSslay9XOfDQb5pMKCkdhBAJOAvejeIPWiE51RgHUe2xWo8SMuzjb5EAUZ cON2QBC9vt0mee29u5WMsQ3oHs6fajE= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-266-CB6zcXjoMeygXDEUwj4VHA-1; Tue, 07 Jan 2020 11:50:20 -0500 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 596C5801E7B; Tue, 7 Jan 2020 16:50:19 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-126.ams2.redhat.com [10.36.117.126]) by smtp.corp.redhat.com (Postfix) with ESMTP id 849D3271B5; Tue, 7 Jan 2020 16:50:18 +0000 (UTC) Subject: Re: [PATCH 3/4] ArmVirtPkg/PlatformPeiLib: implement Reset2 PPI based on PSCI To: Ard Biesheuvel , devel@edk2.groups.io References: <20200107094800.4488-1-ard.biesheuvel@linaro.org> <20200107094800.4488-4-ard.biesheuvel@linaro.org> From: "Laszlo Ersek" Message-ID: <913896f6-bd27-3942-1f79-20e347b48573@redhat.com> Date: Tue, 7 Jan 2020 17:50:17 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20200107094800.4488-4-ard.biesheuvel@linaro.org> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-MC-Unique: CB6zcXjoMeygXDEUwj4VHA-1 X-Mimecast-Spam-Score: 0 Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit On 01/07/20 10:47, Ard Biesheuvel wrote: > Extend the existing DT traversal routine in PlatformPeiLib with > discovery of the PSCI method, and expose an implementation of the > Reset2 PPI based on the method found. > > This satisfies a dependency of Tcg2Pei, which needs to reset the > platform in some cases. Since there are no other uses for system > reset in PEI on ArmVirtQemu, simply expose the PPI directly rather > than using the generic ResetSystemPei and the associated plumbing. > > Signed-off-by: Ard Biesheuvel > --- > ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.inf | 3 + > ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c | 123 ++++++++++++++++++++ > 2 files changed, 126 insertions(+) Tcg2Pei uses ResetCold() from ResetSystemLib. ArmVirtPkg's existent ResetSystemLib instance (ArmVirtPsciResetSystemLib/ArmVirtPsciResetSystemLib.inf) is only suitable for DXE_DRIVER and DXE_RUNTIME_DRIVER instances. It uses our FDT Client protocol for looking up (I think) more or less the same things that you parse here. As a PEI phase replacement, this patch produces gEfiPeiReset2PpiGuid, and then in patch#4, we resolve ResetSystemLib, for PEIMs, to the MdeModulePkg/Library/PeiResetSystemLib/PeiResetSystemLib.inf instance, which depends on the PPI installed here. I'm not too happy about installing the gEfiPeiReset2PpiGuid from PlatformPeiLib. As replacement, it's not ResetSystemPei what I'd recommend (which depends on a PEI-phase ResetSystemLib instance anyway, which we don't have, in the first place). (1) Instead, I'd recommend implementing a PEI-phase ResetSystemLib for ArmVirtQemu. (Under ArmVirtPkg/Library/ArmVirtPsciResetSystemLib -- new INF file.) Would that be a large burden? I think we'd need a helper function in that lib instance, for returning HVC versus SMC (from the FDT), and then we'd have to call the proper interface for the actual reset. Not fast, but resets don't need to be fast I think. BTW I think the following modules are never meant to be used together: MdeModulePkg/Library/PeiResetSystemLib/PeiResetSystemLib.inf MdeModulePkg/Universal/ResetSystemPei/ResetSystemPei.inf because they seem to depend mutually on each other's abstract interface (PPI vs. lib class). So I think a given platform should include *at most one* of these, on top of the "other" facility that it already exposes. In ArmVirtQemu's case, I'd suggest implementing the lib class for PEI, and then we can include "ResetSystemPei" whenever the need arises. Thanks, Laszlo > > diff --git a/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.inf b/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.inf > index c41ee22c9767..72ed2413a768 100644 > --- a/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.inf > +++ b/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.inf > @@ -28,6 +28,8 @@ [Packages] > SecurityPkg/SecurityPkg.dec > > [LibraryClasses] > + ArmSmcLib > + ArmHvcLib > DebugLib > HobLib > FdtLib > @@ -44,6 +46,7 @@ [Pcd] > gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress ## SOMETIMES_PRODUCES > > [Ppis] > + gEfiPeiReset2PpiGuid ## SOMETIMES_PRODUCES > gOvmfTpmDiscoveredPpiGuid ## SOMETIMES_PRODUCES > > [Guids] > diff --git a/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c b/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c > index 249e45c04624..7af351eda003 100644 > --- a/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c > +++ b/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c > @@ -9,6 +9,8 @@ > > #include > > +#include > +#include > #include > #include > #include > @@ -16,15 +18,113 @@ > #include > #include > > +#include > + > #include > #include > > +#include > + > STATIC CONST EFI_PEI_PPI_DESCRIPTOR mTpm2DiscoveredPpi = { > EFI_PEI_PPI_DESCRIPTOR_PPI | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST, > &gOvmfTpmDiscoveredPpiGuid, > NULL > }; > > +/** > + The ResetSystem function resets the entire platform. > + > + @param[in] ResetType The type of reset to perform. > + @param[in] ResetStatus The status code for the reset. > + @param[in] DataSize The size, in bytes, of ResetData. > + @param[in] ResetData For a ResetType of EfiResetCold, EfiResetWarm, or > + EfiResetShutdown the data buffer starts with a > + Null-terminated string, optionally followed by > + additional binary data. The string is a description > + that the caller may use to further indicate the > + reason for the system reset. > +**/ > +STATIC > +VOID > +EFIAPI > +ResetSystemHvc ( > + IN EFI_RESET_TYPE ResetType, > + IN EFI_STATUS ResetStatus, > + IN UINTN DataSize, > + IN VOID *ResetData OPTIONAL > + ) > +{ > + ARM_HVC_ARGS ArmHvcArgs; > + > + switch (ResetType) { > + case EfiResetWarm: > + case EfiResetCold: > + case EfiResetPlatformSpecific: > + // Send a PSCI 0.2 SYSTEM_RESET command > + ArmHvcArgs.Arg0 = ARM_SMC_ID_PSCI_SYSTEM_RESET; > + break; > + > + case EfiResetShutdown: > + // Send a PSCI 0.2 SYSTEM_OFF command > + ArmHvcArgs.Arg0 = ARM_SMC_ID_PSCI_SYSTEM_OFF; > + break; > + > + default: > + ASSERT (FALSE); > + return; > + } > + ArmCallHvc (&ArmHvcArgs); > +} > + > +STATIC > +VOID > +EFIAPI > +ResetSystemSmc ( > + IN EFI_RESET_TYPE ResetType, > + IN EFI_STATUS ResetStatus, > + IN UINTN DataSize, > + IN VOID *ResetData OPTIONAL > + ) > +{ > + ARM_SMC_ARGS ArmSmcArgs; > + > + switch (ResetType) { > + case EfiResetWarm: > + case EfiResetCold: > + case EfiResetPlatformSpecific: > + // Send a PSCI 0.2 SYSTEM_RESET command > + ArmSmcArgs.Arg0 = ARM_SMC_ID_PSCI_SYSTEM_RESET; > + break; > + > + case EfiResetShutdown: > + // Send a PSCI 0.2 SYSTEM_OFF command > + ArmSmcArgs.Arg0 = ARM_SMC_ID_PSCI_SYSTEM_OFF; > + break; > + > + default: > + ASSERT (FALSE); > + return; > + } > + ArmCallSmc (&ArmSmcArgs); > +} > + > +STATIC CONST EFI_PEI_RESET2_PPI mPpiReset[] = { > + { ResetSystemHvc }, > + { ResetSystemSmc }, > +}; > + > +STATIC CONST EFI_PEI_PPI_DESCRIPTOR mPlatformHvcResetPpi = { > + EFI_PEI_PPI_DESCRIPTOR_PPI | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST, > + &gEfiPeiReset2PpiGuid, > + (EFI_PEI_RESET2_PPI *)&mPpiReset[0] > +}; > + > +STATIC CONST EFI_PEI_PPI_DESCRIPTOR mPlatformSmcResetPpi = { > + EFI_PEI_PPI_DESCRIPTOR_PPI | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST, > + &gEfiPeiReset2PpiGuid, > + (EFI_PEI_RESET2_PPI *)&mPpiReset[1] > +}; > + > EFI_STATUS > EFIAPI > PlatformPeim ( > @@ -47,6 +147,7 @@ PlatformPeim ( > INT32 StatusLen; > CONST UINT64 *RegProp; > CONST UINT32 *RangesProp; > + CONST VOID *MethodProp; > UINT64 UartBase; > UINT64 TpmBase; > EFI_STATUS Status; > @@ -155,6 +256,28 @@ PlatformPeim ( > Status = PeiServicesInstallPpi (&mTpm2DiscoveredPpi); > ASSERT_EFI_ERROR (Status); > break; > + } else if (AsciiStrCmp (CompItem, "arm,psci-0.2") == 0) { > + MethodProp = fdt_getprop (Base, Node, "method", &Len); > + if (MethodProp == NULL) { > + DEBUG ((DEBUG_ERROR, "%a: Missing PSCI method property\n", > + __FUNCTION__)); > + break; > + } > + > + if (AsciiStrnCmp (MethodProp, "hvc", 3) == 0) { > + Status = PeiServicesInstallPpi (&mPlatformHvcResetPpi); > + ASSERT_EFI_ERROR (Status); > + } else if (AsciiStrnCmp (MethodProp, "smc", 3) == 0) { > + Status = PeiServicesInstallPpi (&mPlatformSmcResetPpi); > + ASSERT_EFI_ERROR (Status); > + } else { > + DEBUG ((DEBUG_ERROR, "%a: Unknown PSCI method \"%a\"\n", __FUNCTION__, > + MethodProp)); > + break; > + } > + DEBUG ((DEBUG_INFO, "%a: Detected PSCI method \"%a\"\n", __FUNCTION__, > + MethodProp)); > + break; > } > } > } >