From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [205.139.110.120]) by mx.groups.io with SMTP id smtpd.web10.10172.1578422827196465879 for ; Tue, 07 Jan 2020 10:47:07 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=Np6sXI6W; spf=pass (domain: redhat.com, ip: 205.139.110.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1578422826; 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=tKJLiPzFGCG8ZAhHzORdqRROkmrAPfRzXbrKVWn+ksE=; b=Np6sXI6WK2SBhBlQjSZRWJoun+nVuoohyvH+da0UWBOvww58oaLymK7XJt07ExTmEHz72g KL3vkJVwcdMOSympdNhTXf57Np/q0TqKQtgd4vNSMFC4x0RbyTxAwiuB/7H2f1o2OmgSA0 zskrLb+5vT8Te7WL13NfTw5FDE92ZlQ= 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-258-NkQnTflDOcSb-zNTfTzrqw-1; Tue, 07 Jan 2020 13:47:04 -0500 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id B60B694EE3; Tue, 7 Jan 2020 18:47:03 +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 EEC5A7C357; Tue, 7 Jan 2020 18:47:02 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 3/4] ArmVirtPkg/PlatformPeiLib: implement Reset2 PPI based on PSCI To: Ard Biesheuvel , edk2-devel-groups-io References: <20200107094800.4488-1-ard.biesheuvel@linaro.org> <20200107094800.4488-4-ard.biesheuvel@linaro.org> <913896f6-bd27-3942-1f79-20e347b48573@redhat.com> From: "Laszlo Ersek" Message-ID: <6fb4250c-b675-3562-2099-66ef9554752d@redhat.com> Date: Tue, 7 Jan 2020 19:47:02 +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: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-MC-Unique: NkQnTflDOcSb-zNTfTzrqw-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 17:55, Ard Biesheuvel wrote: > On Tue, 7 Jan 2020 at 17:50, Laszlo Ersek wrote: >> >> 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. >> > > That is what I started out with. The problem is that I am not 100% > convinced that it is safe to dereference the initial FDT base address > at arbitrary times during PEI, Great point; this is one of those things that I had to think about for many minutes before posting my email. I think it's safe. For two reasons: (i) all of the PEIMs, and the PEI_CORE, in ArmVirtQemu, use the ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.inf instance for writing to the serial port. This library instance re-parses the initial DTB at every DEBUG call, in effect, across all the PEIMs. (See SerialPortWrite() --> SerialPortGetBaseAddress() --> PcdGet64(PcdDeviceTreeInitialBaseAddress)). In other words, we're already doing this, at the moment. (ii) In ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLibConstructor.c we have: // // We need to make sure that the machine we are running on has at least // 128 MB of memory configured, and is currently executing this binary from // NOR flash. This prevents a device tree image in DRAM from getting // clobbered when our caller installs permanent PEI RAM, before we have a // chance of marking its location as reserved or copy it to a freshly // allocated block in the permanent PEI RAM in the platform PEIM. // ASSERT (NewSize >= SIZE_128MB); ASSERT ( (((UINT64)PcdGet64 (PcdFdBaseAddress) + (UINT64)PcdGet32 (PcdFdSize)) <= NewBase) || ((UINT64)PcdGet64 (PcdFdBaseAddress) >= (NewBase + NewSize))); To elaborate on this: initially we use the temporary SEC/PEI heap+stack; later on we use the permanent PEI RAM. (ii.1) The temp SEC/PEI heap+stack is set up in ArmPlatformPkg/PrePeiCore/MainUniCore.c and it is based on PcdCPUCoresStackBase. The value of PcdCPUCoresStackBase is fixed, in ArmVirtQemu.dsc: gArmPlatformTokenSpaceGuid.PcdCPUCoresStackBase|0x4007c000 whereas the initial DTB is at the base of DRAM: gArmTokenSpaceGuid.PcdSystemMemoryBase|0x40000000 gArmVirtTokenSpaceGuid.PcdDeviceTreeInitialBaseAddress|0x40000000 so if the initial DTB fits in 0x7C000 bytes (496 KiB), we're good, as far as the temporary SEC/PEI heap+stack is concerned. (ii.2) The permanent PEI RAM is 64MB in size: # Size of the region used by UEFI in permanent memory (Reserved 64MB) gArmPlatformTokenSpaceGuid.PcdSystemMemoryUefiRegionSize|0x04000000 Because of the *two* ASSERT()s in "QemuVirtMemInfoPeiLibConstructor.c" that I quoted above, we know that the lowest DRAM node is at least 128MB in size, and also that it does not overlap with the flash device. Consequently, the the InitializeMemory() function in ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c will take the following branch: ------ } else { // Check the Firmware does not overlapped with the system memory ASSERT ((FdBase < SystemMemoryBase) || (FdBase >= SystemMemoryTop)); ASSERT ((FdTop <= SystemMemoryBase) || (FdTop > SystemMemoryTop)); UefiMemoryBase = SystemMemoryTop - FixedPcdGet32 (PcdSystemMemoryUefiRegionSize); } Status = PeiServicesInstallPeiMemory (UefiMemoryBase, FixedPcdGet32 (PcdSystemMemoryUefiRegionSize)); ------ and therefore UefiMemoryBase == (PcdSystemMemoryBase + PcdSystemMemorySize) - PcdSystemMemoryUefiRegionSize == 1GiB + PcdSystemMemorySize - 64MiB Given that PcdSystemMemorySize >= 128 MiB, we get UefiMemoryBase >= 1GiB + 64 MiB Which means that the permanent PEI RAM too is safely distinct from the initial DTB (which is at the base of DRAM: at 1 GiB). In summary: it's safe, and it's so by design. We did this intentionally. Originally in commit a36d531f5d56 ("ArmPlatformPkg/ArmVirtualizationPkg: add ArmVirtualizationPlatformLib library", 2014-09-18): commit a36d531f5d565e6cb5496ea53824e36487a227dd Author: Michael Casadevall Date: Thu Sep 18 18:05:03 2014 +0000 ArmPlatformPkg/ArmVirtualizationPkg: add ArmVirtualizationPlatformLib library This is an implementation of ArmPlatformLib that discovers the size of system DRAM from a device tree blob located at the address passed in gArmTokenSpaceGuid.PcdDeviceTreeBaseAddress, which should equal the value in gArmTokenSpaceGuid.PcdSystemMemoryBase. As the device tree blob is passed in system DRAM, this library can only be used if sufficient DRAM is available (>= 128 MB) and if not using shadowed NOR. The reason for this is that it makes it easier to guarantee that such a device tree blob at base of DRAM will not be clobbered before we get a chance to preserve it. Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Michael Casadevall Acked-by: Laszlo Ersek Signed-off-by: Ard Biesheuvel Signed-off-By: Olivier Martin And then we moved it to its present spot in the series that contains commit 048651260b66 ("ArmVirtPkg: create QemuVirtMemInfoLib version for ArmVirtQemu", 2017-11-23). > and so it would be better to consume > the FDT from the GUIDed HOB. That, however, creates another ordering > issue, and so we should install a PPI that signals the availability of > the FDT GUIDed HOB. > > At this point, I decided it would be better to just produce the PPI in > the PlatformPeiLib, but I agree it is not the cleanest approach. > >> 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. >> > > The idea is that other PEIMs use the library, which is backed by the > PEIM, so that any hooks and notifications that occur at reset time can > be dispatched correctly. If every PEIM that needs to reset the system > calls into a library directly, this no longer works. > Good point -- now I realize my exclusivity argument above is wrong. I now recall why. The following is a quite common pattern in edk2: - there is a lib class offering some service, at the abstract level - there is a PEIM or DXE driver that exposes the same service as a PPI or protocol, respectively - this PEIM or DXE driver internally calls the lib API - there are two lib instances: one instance does the real thing, and the other instance calls out to the PPI or protocol - all PEIMs / DXE drivers *except* the one PEIM or DXE driver mentioned above get the "shim" lib instance - the one PEIM / DXE driver that exposes the PPI / protocol gets the "real deal" lib instance, via module-level lib class resolution override in the DSC file. We can do the same here, I think: - resolve ResetSystemLib, generally for PEIMs, to "MdeModulePkg/Library/PeiResetSystemLib/PeiResetSystemLib.inf", - include "MdeModulePkg/Universal/ResetSystemPei/ResetSystemPei.inf" in the firmware binary, - resolve ResetSystemLib, specifically for "ResetSystemPei.inf", to our new (FDT-parsing) lib instance. How does it sound to you? Yes, it's more fluff, but it's clean, and native to edk2. Thanks, Laszlo