From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:400c:c09::241; helo=mail-wm0-x241.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wm0-x241.google.com (mail-wm0-x241.google.com [IPv6:2a00:1450:400c:c09::241]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id A58C42115BE04 for ; Thu, 7 Jun 2018 07:54:14 -0700 (PDT) Received: by mail-wm0-x241.google.com with SMTP id o13-v6so18554992wmf.4 for ; Thu, 07 Jun 2018 07:54:14 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=9m1x0VpHP4Vtf4x4NBUMc4KpJl5aEHfUhpwk9rFJ13w=; b=iMIL7AeneObwhYlHLycqzLRf8ZjqW4G048HVfx+2LAxxT4s0rz6fqhglzN2Sj5//y8 JtvcDNI16VWXDm1TCnibgb6O66TJxded8vhbNH+vAdZChNES6kD/bwkKHd5KoLpYDsQx H+D5137BJ4wAX5GyC+kHgaIy0eBw7JI7qm2NA= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=9m1x0VpHP4Vtf4x4NBUMc4KpJl5aEHfUhpwk9rFJ13w=; b=f6EbgXN1i+qfdlhvwVLZR5bluHjI9u5SWggKpw7OlqF6mpzSf1e7NISLzIzCnujHfu biKiOk9q9OYPQOgcCLB9yXy7zB/maUqDqpRhDWFA3juCQLwt/T4Z7qxT52MeJ0qlGOGe Ozq5FvWJfer2EDCzv9rpUJysjKIaMfpFQB3UhXgYYWs1lM//nzQLtMkUPUAwex+4e0bO 7mf/qKimj6rm/89BcXmqB3aqCwvP0Tj6WXPddhfCe3jxOqkgU+TW6hIRHjL7CfoRtV+3 WDXfFmtglpcjmZdJSIeITsQoBHP0ai+/kskNULaW/vqRKiaQTPcDT9vQE8Va2y7l00ZX FQGw== X-Gm-Message-State: APt69E3NwiueItTLi/9jU069eHnGH4Z5rHgVXpgLwAonVcywx3qM6keZ 68i1uf0HYZG/7sOjSeWJ4Mi1nA== X-Google-Smtp-Source: ADUXVKJg/vRsxhuVPO4i0G5Z5qqhGLFWGaSNR6o4FhpUmLB1z/XpQHnCujPeDDmVtb0AreSlU30vxw== X-Received: by 2002:a1c:30d7:: with SMTP id w206-v6mr1742265wmw.153.1528383252501; Thu, 07 Jun 2018 07:54:12 -0700 (PDT) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id h65-v6sm2347890wmf.7.2018.06.07.07.54.10 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 07 Jun 2018 07:54:11 -0700 (PDT) Date: Thu, 7 Jun 2018 15:54:09 +0100 From: Leif Lindholm To: Ard Biesheuvel Cc: Laszlo Ersek , "edk2-devel@lists.01.org" Message-ID: <20180607145409.wdtanukhuzx72rb7@bivouac.eciton.net> References: <20180606123701.4275-1-ard.biesheuvel@linaro.org> MIME-Version: 1.0 In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [PATCH] ArmPkg/ArmSmcPsciResetSystemLib: implement fallback for warm reboot X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 07 Jun 2018 14:54:15 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Thu, Jun 07, 2018 at 08:57:04AM +0200, Ard Biesheuvel wrote: > On 6 June 2018 at 15:29, Laszlo Ersek wrote: > > On 06/06/18 14:37, Ard Biesheuvel wrote: > >> Implement ResetSystemLib's EnterS3WithImmediateWake() routine using > >> a jump back to the PEI entry point with interrupts and MMU+caches > >> disabled. This is only possible at boot time, when we are sure that > >> the current CPU is the only one up and running. Also, it depends on > >> the platform whether the PEI code is preserved in memory (it may be > >> copied to DRAM rather than execute in place), so also add a feature > >> PCD to selectively enable this feature. > >> > >> Contributed-under: TianoCore Contribution Agreement 1.1 > >> Signed-off-by: Ard Biesheuvel > >> --- > >> ArmPkg/ArmPkg.dec | 4 ++++ > >> ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.c | 21 ++++++++++++++++++-- > >> ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.inf | 9 +++++++++ > >> 3 files changed, 32 insertions(+), 2 deletions(-) > >> > >> diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec > >> index debe066b6f7b..3aa229fe2ec9 100644 > >> --- a/ArmPkg/ArmPkg.dec > >> +++ b/ArmPkg/ArmPkg.dec > >> @@ -85,6 +85,10 @@ [PcdsFeatureFlag.common] > >> # Define if the GICv3 controller should use the GICv2 legacy > >> gArmTokenSpaceGuid.PcdArmGicV3WithV2Legacy|FALSE|BOOLEAN|0x00000042 > >> > >> + # Whether to implement warm reboot for capsule update using a jump back to the > >> + # PEI entry point with caches and interrupts disabled. > >> + gArmTokenSpaceGuid.PcdArmReenterPeiForCapsuleWarmReboot|FALSE|BOOLEAN|0x0000001F > >> + > >> [PcdsFeatureFlag.ARM] > >> # Whether to map normal memory as non-shareable. FALSE is the safe choice, but > >> # TRUE may be appropriate to fix performance problems if you don't care about > >> diff --git a/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.c b/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.c > >> index d6d26bce5009..cb75e32771c2 100644 > >> --- a/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.c > >> +++ b/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.c > >> @@ -15,10 +15,13 @@ > >> > >> #include > >> > >> +#include > >> +#include > >> #include > >> #include > >> #include > >> -#include > >> +#include > >> +#include > >> > >> #include > >> > >> @@ -89,7 +92,21 @@ EnterS3WithImmediateWake ( > >> VOID > >> ) > >> { > >> - // Not implemented > >> + VOID (*Reset)(VOID); > >> + > >> + if (FeaturePcdGet (PcdArmReenterPeiForCapsuleWarmReboot) && > >> + !EfiAtRuntime ()) { > >> + // > >> + // At boot time, we are the only core running, so we can implement the > >> + // immediate wake (which is used by capsule update) by disabling the MMU > >> + // and interrupts, and jumping to the PEI entry point. > >> + // > >> + Reset = (VOID *)(UINTN)FixedPcdGet64 (PcdFvBaseAddress); > > > > ISO C99 6.3.2.3 Pointers > > > > 1 A pointer to void may be converted to or from a pointer to any > > incomplete or object type. [...] > > > > [...] > > > > 5 An integer may be converted to any pointer type. Except as > > previously specified, the result is implementation-defined, might > > not be correctly aligned, might not point to an entity of the > > referenced type, and might be a trap representation. [...] > > > > [...] > > > > 8 A pointer to a function of one type may be converted to a pointer to > > a function of another type and back again; [...] > > > > The point is, converting pointer-to-void to pointer-to-function is > > undefined, and I expect at least clang will yell at us for it. However, > > converting an integer to pointer-to-function is implementation-defined; > > i.e., the C language implementation must support it to an extent, and > > must document how it works. (The "previously specified" part is about > > null pointer constants, in paragraph 3, which I'm not quoting now.) > > > > Thus, I suggest either > > > > typedef VOID (*RESET_FUNCTION_PTR) (VOID); > > RESET_FUNCTION_PTR Reset; > > > > Reset = (RESET_FUNCTION_PTR)(UINTN)FixedPcdGet64 (PcdFvBaseAddress); > > > > or else (for kicks and giggles regarding the syntax), just > > > > Reset = (VOID (*)(VOID))(UINTN)FixedPcdGet64 (PcdFvBaseAddress); > > Thanks Laszlo > > I will go for option #2 unless Leif has any objections to that? I think my preference would be the first simply becauase it feels more TianoCore. But I'm relaly happy with either option. Regards, Leif