From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from EUR04-VI1-obe.outbound.protection.outlook.com (EUR04-VI1-obe.outbound.protection.outlook.com [40.107.8.73]) by mx.groups.io with SMTP id smtpd.web11.5663.1647008922586715186 for ; Fri, 11 Mar 2022 06:28:43 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@armh.onmicrosoft.com header.s=selector2-armh-onmicrosoft-com header.b=MUa1HP8+; spf=pass (domain: arm.com, ip: 40.107.8.73, mailfrom: sami.mujawar@arm.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=armh.onmicrosoft.com; s=selector2-armh-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=oFYX7kTW1WhAZ6kWJ6nQij8DF1Ap7RfUxmWtapv/3+s=; b=MUa1HP8+qPAzc/pU1+ebIf553GMo4dtSGUkJFkY5pfCW8LCGE/LeDl+BUT/KoD7ljAzoepeiGpPOzW9wLxG7PuXKL3nf5oURVUfbTSHe3GwM0DEFnD4bLldL72tM7i4p3I3vvdK2L6uQyfBDupZUaFpKA5SUm/HZGcDv6BjyHSk= Received: from AS9PR05CA0059.eurprd05.prod.outlook.com (2603:10a6:20b:489::22) by DB6PR08MB2775.eurprd08.prod.outlook.com (2603:10a6:6:17::26) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.5038.17; Fri, 11 Mar 2022 14:28:38 +0000 Received: from VE1EUR03FT031.eop-EUR03.prod.protection.outlook.com (2603:10a6:20b:489:cafe::ab) by AS9PR05CA0059.outlook.office365.com (2603:10a6:20b:489::22) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.5061.22 via Frontend Transport; Fri, 11 Mar 2022 14:28:38 +0000 X-MS-Exchange-Authentication-Results: spf=pass (sender IP is 63.35.35.123) smtp.mailfrom=arm.com; dkim=pass (signature was verified) header.d=armh.onmicrosoft.com;dmarc=pass action=none header.from=arm.com; Received-SPF: Pass (protection.outlook.com: domain of arm.com designates 63.35.35.123 as permitted sender) receiver=protection.outlook.com; client-ip=63.35.35.123; helo=64aa7808-outbound-1.mta.getcheckrecipient.com; Received: from 64aa7808-outbound-1.mta.getcheckrecipient.com (63.35.35.123) by VE1EUR03FT031.mail.protection.outlook.com (10.152.18.69) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.5061.22 via Frontend Transport; Fri, 11 Mar 2022 14:28:37 +0000 Received: ("Tessian outbound 18e50a6f0513:v113"); Fri, 11 Mar 2022 14:28:37 +0000 X-CheckRecipientChecked: true X-CR-MTA-CID: baa8f19117852f48 X-CR-MTA-TID: 64aa7808 Received: from 45a98aa594d2.2 by 64aa7808-outbound-1.mta.getcheckrecipient.com id 3B0E72A7-B177-4381-91B8-D70FB2D00B5D.1; Fri, 11 Mar 2022 14:28:25 +0000 Received: from EUR02-HE1-obe.outbound.protection.outlook.com by 64aa7808-outbound-1.mta.getcheckrecipient.com with ESMTPS id 45a98aa594d2.2 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384); Fri, 11 Mar 2022 14:28:25 +0000 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=kKtvGHnwg97vZR8j7L6qlKp/nm6vFk3fAM9dGSw4oR9u4Pbk0mRzrQWsNjav0Zahx0k1a+3hWHam127Dt2IniLUmdBvodhIyEurc/MNEjs9M1OfQkHqEu6kYo47/OYmnAyglBFK636YiTh8ORPz5BF2jFzPNF4pRjTudDH8vuEHXKQiSVMb4ULzRstDC2ePiLY3d/hBGXhimlT7FNOfCFd2/MJWatiAuBSshWJTKU8XvZelhm5wkF4hlNEC8b7kJhu3cXO46EJJhSryYSnfJBbIfmUekIeyLLry83nIfDiPTQeKc+93wEBus0zdWyoKkht0/p8CSFSMoq+/pt/IiBQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=oFYX7kTW1WhAZ6kWJ6nQij8DF1Ap7RfUxmWtapv/3+s=; b=BjtRoVkf6OAy2bfMIeKKvayEhsI/b5RwogYYwx/+qe2VrE9c/7FBiik3TPlH/oEFmqVb3vndzUl3ejmfvGMyAMIMX/R7f7epQBMFwvi8wTG9JxYs+TCH1IbK9PHFOC2AGNhwq/C4iZeuisFNK+jQro5T4AVM+5RTwYFr27lJxaMAU0dCGAWMLPCfgpA0HqTdZmAxbNbLIOppGyjrkXuOv2pB4jgcpn+u7wZoh7NMauTAde39sPzkVWc7POhG0HXqmJIQmsnXpHaG8MxVU4yHgOi8DGrfsmw4sPTHo1VA3mPARFAPPdrs9BTNIA5LKwOP5/KohAs2nb3Ledd5aJeBpQ== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=arm.com; dmarc=pass action=none header.from=arm.com; dkim=pass header.d=arm.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=armh.onmicrosoft.com; s=selector2-armh-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=oFYX7kTW1WhAZ6kWJ6nQij8DF1Ap7RfUxmWtapv/3+s=; b=MUa1HP8+qPAzc/pU1+ebIf553GMo4dtSGUkJFkY5pfCW8LCGE/LeDl+BUT/KoD7ljAzoepeiGpPOzW9wLxG7PuXKL3nf5oURVUfbTSHe3GwM0DEFnD4bLldL72tM7i4p3I3vvdK2L6uQyfBDupZUaFpKA5SUm/HZGcDv6BjyHSk= Authentication-Results-Original: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=arm.com; Received: from AS8PR08MB6806.eurprd08.prod.outlook.com (2603:10a6:20b:39b::12) by VE1PR08MB5245.eurprd08.prod.outlook.com (2603:10a6:803:10a::23) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.5038.14; Fri, 11 Mar 2022 14:28:20 +0000 Received: from AS8PR08MB6806.eurprd08.prod.outlook.com ([fe80::8929:b37b:cb45:71da]) by AS8PR08MB6806.eurprd08.prod.outlook.com ([fe80::8929:b37b:cb45:71da%3]) with mapi id 15.20.5061.022; Fri, 11 Mar 2022 14:28:20 +0000 Subject: Re: [edk2][PATCH V1 1/1] ArmPkg: Handle warm reboot request correctly To: Pranav Madhu , "devel@edk2.groups.io" Cc: Ard Biesheuvel , nd , Thomas Abraham , Leif Lindholm References: <20220310131037.22334-1-pranav.madhu@arm.com> <95e0263c-099c-c195-45e1-f8bfa5494d91@arm.com> From: "Sami Mujawar" Message-ID: <180d3121-277d-d21f-06cc-ef19dbfe7c91@arm.com> Date: Fri, 11 Mar 2022 14:28:25 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.0.1 In-Reply-To: X-ClientProxiedBy: LO2P123CA0030.GBRP123.PROD.OUTLOOK.COM (2603:10a6:600::18) To AS8PR08MB6806.eurprd08.prod.outlook.com (2603:10a6:20b:39b::12) MIME-Version: 1.0 X-MS-Office365-Filtering-Correlation-Id: e2426fc8-f674-40f6-d345-08da036b6e81 X-MS-TrafficTypeDiagnostic: VE1PR08MB5245:EE_|VE1EUR03FT031:EE_|DB6PR08MB2775:EE_ X-Microsoft-Antispam-PRVS: x-checkrecipientrouted: true NoDisclaimer: true X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam-Untrusted: BCL:0; X-Microsoft-Antispam-Message-Info-Original: HjYS8g8s/FZopJl12xNb0lZB7p6yx8v6G1BKNCN9QqEsk7M8arP4w+U2m+sU270pko3HzvWWe1nDoUAxZgQ+Eu0yDOSW7VJPniLPLmW5syF61bYdKeKsOBj/ypAfqDAP4n88ec+nJoZwaIbz1Co7AY6EcQ6Gb8GiS595BjJsVjQt2zYxOobf8kgHhEb+QoFWOnpJnIx7m1Qfi/OIZBX9WUSy/yE/4rZGLhk2mNJMqJn15BTi3i3r8y4YXDlFYSA91q+y4+300pX7DCP84ccLyX073OrEg2SSiGC3RyATadKsQkEPIL423bwPHnVRH4i+XM1n8mwNKeWFH/GgxoL9RHfJu2FBcGWM8/2jnM6T975pEdXUFl+bpOBG2h5ME6+muZLNmkYZ7WHhRiAYI7KHaY/7ld/hbEuCTOuODvz0yLdMAACGv8BsKKeqhiRTfdQLgJdWUdWmLCVAAKdrHTc237hOiGBweNi9pLuemlDzvxYysN6T/IJJ3qtcrbxNQ9IjFbF4lzvP+dbkhYh/3eMaiRut7uXS9aXt2gVOllNJK57F75CwpZiJMizv0bf1kxegBHMkaa3aopXF3J5d+b61mov4eN0YO49V0+50QXKbptFYuO6EFnWtLiYDwVec72Spa94nIw20H+oWIRYjGLeDRxaL33ojC+y6uJ6BJqyr2uGWY7tMcdd9ireDm8v+qoGSFHMqxo8We41b/yWzBNoMPWWeqK8t5gscDXp02JY46vwSIFTPuiVruLNg28V4j1bWneFrxLhR5giKqoKETuWRgufvh6dT9hff0Ep5SRmBYCa/FpPJhoMLUiMD6AQRZyW5LBu44JTBYz9rX5KNX97+WcpNcFG5+0z1vLXrd2jXDWP7G/jw2ZgtUeUNM06ArVRk X-Forefront-Antispam-Report-Untrusted: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:AS8PR08MB6806.eurprd08.prod.outlook.com;PTR:;CAT:NONE;SFS:(13230001)(4636009)(366004)(86362001)(4326008)(36756003)(44832011)(54906003)(83380400001)(66476007)(66556008)(5660300002)(110136005)(8676002)(30864003)(2906002)(316002)(31686004)(66946007)(8936002)(52116002)(6512007)(186003)(508600001)(53546011)(33964004)(6506007)(966005)(6486002)(26005)(2616005)(38100700002)(38350700002)(31696002)(166002)(43740500002)(45980500001);DIR:OUT;SFP:1101; X-MS-Exchange-Transport-CrossTenantHeadersStamped: VE1PR08MB5245 Original-Authentication-Results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=arm.com; Return-Path: Sami.Mujawar@arm.com X-EOPAttributedMessage: 0 X-MS-Exchange-Transport-CrossTenantHeadersStripped: VE1EUR03FT031.eop-EUR03.prod.protection.outlook.com X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id-Prvs: 24b39149-1aaa-40a1-d309-08da036b63dc X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: Zw4/kiYKMEOE32Epr3qilmj2taXIB74ZmpKoHWdGwG3lhLJD3Esfjdh1vyQ7AKpTE65WpgJTCvvjjcU3MNIw9uA4VuvgYdkxTaZebIKeA1KMA6wgS2Iu6J/tDPlY1RiEwDeFPfooIA34D5Ye0LTvQlvR0zWgKAcoX2fuhZzOYnpe86F9bDXjOodUtlq+M68mWdLtXmn0CNBPR06LLbcUZElImIUpC3c8mmm9lP0wcFhXOqnEGEFh8nCwgKD3vsjA2StRUFSu9p8Rn21Mi8eDRzYzdAqy6stOquhY0Gf35o85DX3No3uS6vj1M66+Kp5c0a6QUTaXkCW3AQ1gAAFKCNz/bicEJ3seurgBSVb0xStUmwfXVJiEt4sQkYsSXMW4g03L17byIhC4eHhujOs+Ls/wziPngfo99hpVJLorawgFiaw4gqoLr6agIBP8xUga7RNsKE7YvozqQ7y0oe2GxRBTO1BFd5duYCzjV8Ow2kS5xWgf3vuqhWcDK9HjsT6iFW3zYxGvccf8+8i3sipdGuo3R6UcOCQ8Aw4o36AH+LIQXo1KEQTahCCDY5SmhUIuq1db7KYQVaXVZLcFF0qAUxmdxVmkOk+qLRrTqVobtczioPz/QJNLKoxGUlIKLK8FUl/lB9CdnnSui/QbuMf7QBZugo0nNLUW2VlCt+dkIiu3REIV1bUAHxNifZyffJOBhoyo931Db9+td17Tv5zSMUQ+uLHyYEBTfnuLyD9c5l8nqPR33sEFf539NjdTfiKEGy2+7iiId/55OMUWvNcB+GQwiYFuRo3dUMdelZvu5373PKmPFc39GIeI6I7M/zSpl2+pBlLrLFqsruiFQmxd4A== X-Forefront-Antispam-Report: CIP:63.35.35.123;CTRY:IE;LANG:en;SCL:1;SRV:;IPV:CAL;SFV:NSPM;H:64aa7808-outbound-1.mta.getcheckrecipient.com;PTR:ec2-63-35-35-123.eu-west-1.compute.amazonaws.com;CAT:NONE;SFS:(13230001)(4636009)(46966006)(36840700001)(40470700004)(82310400004)(81166007)(53546011)(356005)(166002)(110136005)(54906003)(70586007)(31696002)(8936002)(4326008)(33964004)(5660300002)(8676002)(70206006)(508600001)(44832011)(30864003)(6512007)(316002)(6506007)(6486002)(966005)(107886003)(40460700003)(86362001)(336012)(26005)(2616005)(31686004)(186003)(36860700001)(36756003)(83380400001)(47076005)(2906002)(43740500002);DIR:OUT;SFP:1101; X-OriginatorOrg: arm.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Mar 2022 14:28:37.5326 (UTC) X-MS-Exchange-CrossTenant-Network-Message-Id: e2426fc8-f674-40f6-d345-08da036b6e81 X-MS-Exchange-CrossTenant-Id: f34e5979-57d9-4aaa-ad4d-b122a662184d X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=f34e5979-57d9-4aaa-ad4d-b122a662184d;Ip=[63.35.35.123];Helo=[64aa7808-outbound-1.mta.getcheckrecipient.com] X-MS-Exchange-CrossTenant-AuthSource: VE1EUR03FT031.eop-EUR03.prod.protection.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Anonymous X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB6PR08MB2775 Content-Type: multipart/alternative; boundary="------------020E9CC8E7F6FC305EBC75CA" Content-Language: en-GB --------------020E9CC8E7F6FC305EBC75CA Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Hi Pranav, Please see my response inline marked [SAMI]. Regards, Sami Mujawar On 11/03/2022 06:06 AM, Pranav Madhu wrote: > Hi Sami, > > Thanks for your comments. Please find my reply inline. > > Regards, > Pranav > >> -----Original Message----- >> From: Sami Mujawar >> Sent: Thursday, March 10, 2022 9:01 PM >> To: Pranav Madhu ; devel@edk2.groups.io >> Cc: Ard Biesheuvel ; nd >> Subject: Re: [edk2][PATCH V1 1/1] ArmPkg: Handle warm reboot request >> correctly >> >> Hi Pranav, >> >> Thank you for this patch. >> >> Please find my response inline marked [SAMI]. >> >> Regards, >> >> Sami Mujawar >> >> >> On 10/03/2022 01:10 PM, Pranav Madhu wrote: >>> The warm reboot requests are mapped to cold reboot as the power >>> control module was not capable of handling the warm reboot requests in >>> the legacy implementation. The support for warm reboot support is >>> added into the power control module. To support warm reset, update >>> ArmPsciResetSystemLib, and there by invoke the PSCI call with >>> parameters for warm reboot. >>> >>> Signed-off-by: Pranav Madhu >>> --- >>> ArmPkg/Include/IndustryStandard/ArmStdSmc.h | 1 + >>> ArmPkg/Library/ArmPsciResetSystemLib/ArmPsciResetSystemLib.c | 7 >> +++++-- >>> 2 files changed, 6 insertions(+), 2 deletions(-) >>> >>> Link to github branch for this patch - >>> https://github.com/Pranav-Madhu/edk2/tree/topics/warm_reboot >>> >>> diff --git a/ArmPkg/Include/IndustryStandard/ArmStdSmc.h >>> b/ArmPkg/Include/IndustryStandard/ArmStdSmc.h >>> index 655edc21b205..c9059dead6e9 100644 >>> --- a/ArmPkg/Include/IndustryStandard/ArmStdSmc.h >>> +++ b/ArmPkg/Include/IndustryStandard/ArmStdSmc.h >>> @@ -93,6 +93,7 @@ >>> #define ARM_SMC_ID_PSCI_MIGRATE_AARCH32 0x84000005 >>> #define ARM_SMC_ID_PSCI_SYSTEM_OFF 0x84000008 >>> #define ARM_SMC_ID_PSCI_SYSTEM_RESET 0x84000009 >>> +#define ARM_SMC_ID_PSCI_SYSTEM_RESET2_AARCH64 0xc4000012 >>> >>> /* The current PSCI version is: 0.2 */ >>> #define ARM_SMC_PSCI_VERSION_MAJOR 0 diff --git >>> a/ArmPkg/Library/ArmPsciResetSystemLib/ArmPsciResetSystemLib.c >>> b/ArmPkg/Library/ArmPsciResetSystemLib/ArmPsciResetSystemLib.c >>> index 7bcd34849507..27e048ba0f7a 100644 >>> --- a/ArmPkg/Library/ArmPsciResetSystemLib/ArmPsciResetSystemLib.c >>> +++ b/ArmPkg/Library/ArmPsciResetSystemLib/ArmPsciResetSystemLib.c >>> @@ -45,10 +45,13 @@ LibResetSystem ( >>> ARM_SMC_ARGS ArmSmcArgs; >>> >>> switch (ResetType) { >>> + case EfiResetWarm: >>> + ArmSmcArgs.Arg0 = ARM_SMC_ID_PSCI_SYSTEM_RESET2_AARCH64; >>> + ArmSmcArgs.Arg1 = 0; >>> + ArmSmcArgs.Arg2 = 0; >>> + break; >> [SAMI] SYSTEM_RESET2 is an optional feature and if not supported would >> return NOT_SUPPORTED. So, if a platform does not support SYSTEM_RESET2, >> should the code here fall back to SYSTEM_RESET? >> According to the PSCI specification, it is the responsibility of the OS to check >> that SYSTEM_RESET2 is supported before calling SYSTEM_RESET2 (I believe this >> is applicable for the case where UEFI is not used to boot the OS). However, if >> the runtime service ResetSystem() is invoked by the OS requesting a warm >> reset, is it not the firmware's responsibility to ensure that SYSTEM_RESET2 is >> supported? Any thoughts? > Right, from PSCI specification, what I understood is before invoking SYSTEM_RESET2, the OS should query the PSCI capabilities using PSCI_FEATURES for SYSTEM_RESET2. The OS should invoke RESET2 only if PSCI_FEATURES returns 0. From spec, what I understood is it is not the responsibility of firmware. If OS issue RESET2 without querying FEATURES, the only option for firmware is to return NOT_SUPPORTED. [SAMI] There are 2 scenarios: 1. A boot loader other than UEFI is used to boot the OS. In this case the OS shall directly call the PSCI interface to reboot. For this scenario the Linux kernel code performs the required checks and makes the appropriate PSCI call see https://elixir.bootlin.com/linux/v5.16.13/source/drivers/firmware/psci/psci.c#L299 2. UEFI is used to boot the OS. In this case the UEFI Runtime Services ResetSystem() will be invoked by the OS to initiate a reboot. In this scenario the checks performed by the OS to see if SYSTEM_RESET2 is supported are irrelevant. It is the firmware that needs to perform the check. Although the PSCI specification section '5.12.4 Caller responsibilities' mentions 'The calling OS...'; this is an example, and in context with the current usage scenario, it is the responsibility of the firmware (the Caller) to perform these checks. Otherwise this can result in failures on platforms that do not implement the SYSTEM_RESET2 feature. The relevant code in the Linux kernel for this scenario can be seen at: https://elixir.bootlin.com/linux/v5.16.13/source/drivers/firmware/efi/reboot.c#L13 https://elixir.bootlin.com/linux/v5.16.13/source/drivers/firmware/efi/runtime-wrappers.c#L412 I also noticed that there are 2 versions of the ResetSystem library: [A] EfiResetSystemLib based implementation This is ArmPkg\Library\ArmPsciResetSystemLib\ArmPsciResetSystemLib.inf which links with EmbeddedPkg/ResetRuntimeDxe/ResetRuntimeDxe.inf [B] ResetSystemLib based implementation This is ArmPkg\Library\ArmSmcPsciResetSystemLib\ArmSmcPsciResetSystemLib.inf which links with MdeModulePkg/Universal/ResetSystemRuntimeDxe/ResetSystemRuntimeDxe.inf I can see that [A] EfiResetSystemLib is only used by: - Platform/ARM/SgiPkg/SgiPlatform.dsc.inc - Silicon/Ampere/AmpereAltraPkg/AmpereAltraPkg.dsc.inc - Silicon/Phytium/PhytiumCommonPkg/PhytiumCommonPkg.dsc.inc Moreover, the commit b2c55e732888fd721f5235a820b1d1c45209992d states that the ArmSmcPsciResetSystemLib.inf ([B] ResetSystemLib) was introduced with the intention to replace the EfiResetSystemLib based implementation that is deprecated now that we have decided that there is no longer a reason to keep a different ResetSystem() implementation under EmbeddedPkg. Considering this, I don't think this patch should modify ArmPkg\Library\ArmPsciResetSystemLib. SgiPkg (and if possible other platforms) should consider moving to ResetSystemLib based implementation. [/SAMI] >> [/SAMI] >>> case EfiResetPlatformSpecific: >>> // Map the platform specific reset as reboot >>> - case EfiResetWarm: >>> - // Map a warm reset into a cold reset >>> case EfiResetCold: >>> // Send a PSCI 0.2 SYSTEM_RESET command >>> ArmSmcArgs.Arg0 = ARM_SMC_ID_PSCI_SYSTEM_RESET; --------------020E9CC8E7F6FC305EBC75CA Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: 8bit

Hi Pranav,

Please see my response inline marked [SAMI].

Regards,

Sami Mujawar


On 11/03/2022 06:06 AM, Pranav Madhu wrote:
Hi Sami,

Thanks for your comments. Please find my reply inline.

Regards,
Pranav


-----Original Message-----
From: Sami Mujawar <Sami.Mujawar@arm.com>
Sent: Thursday, March 10, 2022 9:01 PM
To: Pranav Madhu <Pranav.Madhu@arm.com>; devel@edk2.groups.io
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; nd <nd@arm.com>
Subject: Re: [edk2][PATCH V1 1/1] ArmPkg: Handle warm reboot request
correctly

Hi Pranav,

Thank you for this patch.

Please find my response inline marked [SAMI].

Regards,

Sami Mujawar


On 10/03/2022 01:10 PM, Pranav Madhu wrote:

The warm reboot requests are mapped to cold reboot as the power
control module was not capable of handling the warm reboot requests in
the legacy implementation. The support for warm reboot support is
added into the power control module. To support warm reset, update
ArmPsciResetSystemLib, and there by invoke the PSCI call with
parameters for warm reboot.

Signed-off-by: Pranav Madhu <pranav.madhu@arm.com>
---
  ArmPkg/Include/IndustryStandard/ArmStdSmc.h                  | 1 +
  ArmPkg/Library/ArmPsciResetSystemLib/ArmPsciResetSystemLib.c | 7

+++++--

  2 files changed, 6 insertions(+), 2 deletions(-)

Link to github branch for this patch -
https://github.com/Pranav-Madhu/edk2/tree/topics/warm_reboot

diff --git a/ArmPkg/Include/IndustryStandard/ArmStdSmc.h
b/ArmPkg/Include/IndustryStandard/ArmStdSmc.h
index 655edc21b205..c9059dead6e9 100644
--- a/ArmPkg/Include/IndustryStandard/ArmStdSmc.h
+++ b/ArmPkg/Include/IndustryStandard/ArmStdSmc.h
@@ -93,6 +93,7 @@
  #define ARM_SMC_ID_PSCI_MIGRATE_AARCH32        0x84000005
  #define ARM_SMC_ID_PSCI_SYSTEM_OFF             0x84000008
  #define ARM_SMC_ID_PSCI_SYSTEM_RESET           0x84000009
+#define ARM_SMC_ID_PSCI_SYSTEM_RESET2_AARCH64  0xc4000012

  /* The current PSCI version is:  0.2 */
  #define ARM_SMC_PSCI_VERSION_MAJOR  0 diff --git
a/ArmPkg/Library/ArmPsciResetSystemLib/ArmPsciResetSystemLib.c
b/ArmPkg/Library/ArmPsciResetSystemLib/ArmPsciResetSystemLib.c
index 7bcd34849507..27e048ba0f7a 100644
--- a/ArmPkg/Library/ArmPsciResetSystemLib/ArmPsciResetSystemLib.c
+++ b/ArmPkg/Library/ArmPsciResetSystemLib/ArmPsciResetSystemLib.c
@@ -45,10 +45,13 @@ LibResetSystem (
    ARM_SMC_ARGS  ArmSmcArgs;

    switch (ResetType) {
+    case EfiResetWarm:
+      ArmSmcArgs.Arg0 = ARM_SMC_ID_PSCI_SYSTEM_RESET2_AARCH64;
+      ArmSmcArgs.Arg1 = 0;
+      ArmSmcArgs.Arg2 = 0;
+      break;

[SAMI] SYSTEM_RESET2 is an optional feature and if not supported would
return NOT_SUPPORTED. So, if a platform does not support SYSTEM_RESET2,
should the code here fall back to SYSTEM_RESET?
According to the PSCI specification, it is the responsibility of the OS to check
that SYSTEM_RESET2 is supported before calling SYSTEM_RESET2 (I believe this
is applicable for the case where UEFI is not used to boot the OS). However, if
the runtime service ResetSystem() is invoked by the OS requesting a warm
reset, is it not the firmware's responsibility to ensure that SYSTEM_RESET2 is
supported? Any thoughts?

Right, from PSCI specification, what I understood is before invoking SYSTEM_RESET2, the OS should query the PSCI capabilities using PSCI_FEATURES for SYSTEM_RESET2. The OS should invoke RESET2 only if PSCI_FEATURES returns 0. From spec, what I understood is it is not the responsibility of firmware. If OS issue RESET2 without querying FEATURES, the only option for firmware is to return NOT_SUPPORTED.

[SAMI] There are 2 scenarios:
               1. A boot loader other than UEFI is used to boot the OS.
                  In this case the OS shall directly call the PSCI interface to reboot.
                  For this scenario the Linux kernel code performs the required checks and makes the appropriate PSCI call see https://elixir.bootlin.com/linux/v5.16.13/source/drivers/firmware/psci/psci.c#L299
                  
               2. UEFI is used to boot the OS.
                   In this case the UEFI Runtime Services ResetSystem() will be invoked by the OS to initiate a reboot.
                   In this scenario the checks performed by the OS to see if SYSTEM_RESET2 is supported are irrelevant. It is the firmware that needs to perform the check. Although
                   the PSCI specification section '5.12.4 Caller responsibilities' mentions 'The calling OS...'; this is an example, and in context with the current usage scenario, it is the
                   responsibility of the firmware (the Caller) to perform these checks. Otherwise this can result in failures on platforms that do not implement the SYSTEM_RESET2 feature.
                   The relevant code in the Linux kernel for this scenario can be seen at:
                    https://elixir.bootlin.com/linux/v5.16.13/source/drivers/firmware/efi/reboot.c#L13
                    https://elixir.bootlin.com/linux/v5.16.13/source/drivers/firmware/efi/runtime-wrappers.c#L412

I also noticed that there are 2 versions of the ResetSystem library:
[A] EfiResetSystemLib based implementation
      This is ArmPkg\Library\ArmPsciResetSystemLib\ArmPsciResetSystemLib.inf which links with EmbeddedPkg/ResetRuntimeDxe/ResetRuntimeDxe.inf
[B] ResetSystemLib based implementation
      This is ArmPkg\Library\ArmSmcPsciResetSystemLib\ArmSmcPsciResetSystemLib.inf which links with MdeModulePkg/Universal/ResetSystemRuntimeDxe/ResetSystemRuntimeDxe.inf

I can see that [A] EfiResetSystemLib is only used by:
- Platform/ARM/SgiPkg/SgiPlatform.dsc.inc
- Silicon/Ampere/AmpereAltraPkg/AmpereAltraPkg.dsc.inc
- Silicon/Phytium/PhytiumCommonPkg/PhytiumCommonPkg.dsc.inc

Moreover, the commit b2c55e732888fd721f5235a820b1d1c45209992d states that the ArmSmcPsciResetSystemLib.inf ([B] ResetSystemLib) was introduced
with the intention to replace the EfiResetSystemLib based implementation that is deprecated now that we have decided that there is
no longer a reason to keep a different ResetSystem() implementation under EmbeddedPkg.

Considering this, I don't think this patch should modify ArmPkg\Library\ArmPsciResetSystemLib.
SgiPkg (and if possible other platforms) should consider moving to ResetSystemLib based implementation.

[/SAMI]

      

        
[/SAMI]


        

          
      case EfiResetPlatformSpecific:
      // Map the platform specific reset as reboot
-    case EfiResetWarm:
-    // Map a warm reset into a cold reset
      case EfiResetCold:
        // Send a PSCI 0.2 SYSTEM_RESET command
        ArmSmcArgs.Arg0 = ARM_SMC_ID_PSCI_SYSTEM_RESET;


        

      

      

    

    

  


--------------020E9CC8E7F6FC305EBC75CA--