From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f68.google.com (mail-wr1-f68.google.com [209.85.221.68]) by mx.groups.io with SMTP id smtpd.web11.5893.1578477600936913535 for ; Wed, 08 Jan 2020 02:00:01 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=sz61PSfM; spf=pass (domain: linaro.org, ip: 209.85.221.68, mailfrom: ard.biesheuvel@linaro.org) Received: by mail-wr1-f68.google.com with SMTP id q10so2624475wrm.11 for ; Wed, 08 Jan 2020 02:00:00 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to; bh=oEkQbpqg3Hq+dQIRsNNVmttay7teewf8rD13vEVbowA=; b=sz61PSfMs/g2BqClcX/7yxwLk+ouhSMmnoj7LfMtRXA/P2K5Bp0GX4irlqCqyxDIwu jyFXmBhekNTviObCdLWLFdQtqYSEcnxESNlpOLvOfNnBP0ptMSpO1Xcxq0NnVnKXcf1z kCFHwt2N8ATBlXjhq1GyXPLm+5yMuGoYt5xr6VW2g7eArxsliW3kQNax38EmvCHjINdS BmOeVrrNX9PMeTAWe6QCCxPa+KCdoquWraWJKPlW43pkfeyaWF7haB5OHYyqNCuwd+wm 0lOM4OiOJA19pDoJmMLL4RbwXVJKzP2vJSl2mMltgv4ZblFz62LMjeS9+eAimNuEv2NK IeJg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to; bh=oEkQbpqg3Hq+dQIRsNNVmttay7teewf8rD13vEVbowA=; b=JETKKMjuREDm+BgslV27JuNVSNh6WqlsUUo9H3pN8zTNFtZvRWQs6p7+kRmFSqXCBh NmT6RgwP2U1VI/HBWylne9rN/6G3xsNl24z7TpelrCUyl9nDILuEN1npcBYKHNjEDsht MQjQBs0bGVpsjdD1kfOBWCvx/DrjOZo0ZlJ9zbTfqEhMsDvIAIuM0wADqDpTwW3ZGw/C 4h51F7KHSrqPRz5PuE4WhMGvO5244jquzyRi3JkoNFT/+xFw1elyMDc7jE0OUANZwqb4 ZND0ly17MKiRgfuS7eBvO82EUSKhWiXUepZvLICJY8N3y/7nCaelfuhMhV3++f+FHPhf u0JQ== X-Gm-Message-State: APjAAAXp9VXYKuAq04uh4GgwjKLD+kquFtEx5azi7gJFfl00NGQE63Ab Khn8sykjtM3+OWoNAHe+Ibr9jPKJSrtZEQu4fM2epx5HAQQ= X-Google-Smtp-Source: APXvYqxb5KXUipf+EraR36kOLdwoDia/c6LcqrGZVcAPAY/vLNc0ZKVOcXeycAb+6U/+98GuUGPjYKYWWZZdzdqxrhc= X-Received: by 2002:a5d:43c7:: with SMTP id v7mr3370686wrr.32.1578477598939; Wed, 08 Jan 2020 01:59:58 -0800 (PST) MIME-Version: 1.0 References: <20200107094800.4488-1-ard.biesheuvel@linaro.org> <20200107094800.4488-4-ard.biesheuvel@linaro.org> <913896f6-bd27-3942-1f79-20e347b48573@redhat.com> <6fb4250c-b675-3562-2099-66ef9554752d@redhat.com> In-Reply-To: <6fb4250c-b675-3562-2099-66ef9554752d@redhat.com> From: "Ard Biesheuvel" Date: Wed, 8 Jan 2020 10:59:47 +0100 Message-ID: Subject: Re: [edk2-devel] [PATCH 3/4] ArmVirtPkg/PlatformPeiLib: implement Reset2 PPI based on PSCI To: edk2-devel-groups-io , Laszlo Ersek Content-Type: text/plain; charset="UTF-8" On Tue, 7 Jan 2020 at 19:47, Laszlo Ersek wrote: > > 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. > Yeah, I'm fine with all of that. Thanks for reminding me why it is safe to refer to the DT at the base of memory throughout the PEI phase.