From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mx0b-002e3701.pphosted.com (mx0b-002e3701.pphosted.com [148.163.143.35]) by mx.groups.io with SMTP id smtpd.web12.4121.1571153271769257726 for ; Tue, 15 Oct 2019 08:27:52 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: hpe.com, ip: 148.163.143.35, mailfrom: prvs=019184da0c=abner.chang@hpe.com) Received: from pps.filterd (m0134424.ppops.net [127.0.0.1]) by mx0b-002e3701.pphosted.com (8.16.0.42/8.16.0.42) with SMTP id x9FFCC0L028436; Tue, 15 Oct 2019 15:27:50 GMT Received: from g2t2353.austin.hpe.com (g2t2353.austin.hpe.com [15.233.44.26]) by mx0b-002e3701.pphosted.com with ESMTP id 2vnabrpxed-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 15 Oct 2019 15:27:50 +0000 Received: from G1W8106.americas.hpqcorp.net (g1w8106.austin.hp.com [16.193.72.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by g2t2353.austin.hpe.com (Postfix) with ESMTPS id 588D984; Tue, 15 Oct 2019 15:27:49 +0000 (UTC) Received: from G9W8675.americas.hpqcorp.net (16.220.49.22) by G1W8106.americas.hpqcorp.net (16.193.72.61) with Microsoft SMTP Server (TLS) id 15.0.1367.3; Tue, 15 Oct 2019 15:27:45 +0000 Received: from G1W8106.americas.hpqcorp.net (16.193.72.61) by G9W8675.americas.hpqcorp.net (16.220.49.22) with Microsoft SMTP Server (TLS) id 15.0.1367.3; Tue, 15 Oct 2019 15:27:44 +0000 Received: from NAM02-CY1-obe.outbound.protection.outlook.com (15.241.52.13) by G1W8106.americas.hpqcorp.net (16.193.72.61) with Microsoft SMTP Server (TLS) id 15.0.1367.3 via Frontend Transport; Tue, 15 Oct 2019 15:27:44 +0000 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=JJWe6wHt1vHMT4k3wOnp19fFxSp2r0M/AOonOfZbm7Mf1mIDZa5DyoSI4HTl4FQix72o7axpRatAn3rapSe9UAERj2s/yNDkbVXKO7ind29bj8XP0NgN7IxKfdOKiwuPkL6MN3hT2O7QwLLofPV9vyweBg4y9eGhqTShFuOgI4zJWxtNucHJNowbNd9Xm8A9cUValjcQILs4uezv+4ctN9uNvzDRjQN6QjOq8j4Q+GcfmFIPdGoWskFAYN47OAfkZLGtCO4WtD6gjw328QWeB4mPHy93pQbE0qIoLBHuEpWGs+y82HdkBMMvXFRhrBE27ul18Z4C0a0RQqs0qF95tQ== 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-SenderADCheck; bh=vN1nMo6RZ9GfdQU4k837AKfxw5F6666MdQtSONZvcDc=; b=aWKSCMv/kwQzMwof7Tqyy5a/JoY2eIEe0GGEznJhw2mCzr6kXFxd6DKb79Ax1iYcMkcO5OdE54DUZGzFuYX+83eK6+iI7b5n374rkQTJspLG2pwZrMrgFOVpen+6MDDptaMPHe0H9BxqEI1JP/XOIKt0gou8Ks6qnTeKuiBvwmgQpbJoO+N5WK9BHx4eZgdp4D9bBldi3UZOx9iMx8XsGNsc0vx/Od+Iblf+khw8+yvulzIQ245O0nP/NkpuoHoM0n1Lu0X9wNsUYaNkU97zi0HNUflgKXKKvmSBeUk3+3DighDxhdu7569S7y5nAELIsmTo1MeFiSPmCYp12EYrvA== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=hpe.com; dmarc=pass action=none header.from=hpe.com; dkim=pass header.d=hpe.com; arc=none Received: from CS1PR8401MB1192.NAMPRD84.PROD.OUTLOOK.COM (10.169.12.151) by CS1PR8401MB0791.NAMPRD84.PROD.OUTLOOK.COM (10.169.15.151) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2347.16; Tue, 15 Oct 2019 15:27:42 +0000 Received: from CS1PR8401MB1192.NAMPRD84.PROD.OUTLOOK.COM ([fe80::4fb:84b9:76e6:1cde]) by CS1PR8401MB1192.NAMPRD84.PROD.OUTLOOK.COM ([fe80::4fb:84b9:76e6:1cde%8]) with mapi id 15.20.2347.023; Tue, 15 Oct 2019 15:27:42 +0000 From: "Abner Chang" To: "devel@edk2.groups.io" , "leif.lindholm@linaro.org" , "Chen, Gilbert" CC: Palmer Dabbelt Subject: Re: [edk2-devel] [plaforms/devel-riscv-v2 PATCHv2 06/14] RiscV/Universal: Initial version of common RISC-V SEC module Thread-Topic: [edk2-devel] [plaforms/devel-riscv-v2 PATCHv2 06/14] RiscV/Universal: Initial version of common RISC-V SEC module Thread-Index: AQHVbp2TsTIq5l3VNkupkQQyVesQl6dH1gIAgBQmpHA= Date: Tue, 15 Oct 2019 15:27:42 +0000 Message-ID: References: <20190919035131.4700-1-gilbert.chen@hpe.com> <20190919035131.4700-7-gilbert.chen@hpe.com> <20191002194310.GF25504@bivouac.eciton.net> In-Reply-To: <20191002194310.GF25504@bivouac.eciton.net> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [16.242.247.131] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: b079828e-4c81-45d1-9f39-08d7518438f7 x-ms-office365-filtering-ht: Tenant x-ms-traffictypediagnostic: CS1PR8401MB0791: x-ms-exchange-purlcount: 1 x-ms-exchange-transport-forked: True x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:4502; x-forefront-prvs: 01917B1794 x-forefront-antispam-report: SFV:NSPM;SFS:(10019020)(346002)(366004)(376002)(396003)(39860400002)(136003)(199004)(189003)(13464003)(19627235002)(6436002)(4326008)(305945005)(446003)(6246003)(74316002)(966005)(478600001)(229853002)(55016002)(3846002)(9686003)(6116002)(6306002)(7736002)(8936002)(486006)(86362001)(2906002)(81156014)(8676002)(81166006)(11346002)(71190400001)(71200400001)(256004)(14444005)(476003)(102836004)(66476007)(7696005)(76116006)(64756008)(66446008)(30864003)(66946007)(52536014)(99286004)(5660300002)(66556008)(2501003)(316002)(25786009)(6636002)(66066001)(14454004)(33656002)(26005)(6506007)(53546011)(186003)(76176011)(110136005)(559001)(579004)(569006);DIR:OUT;SFP:1102;SCL:1;SRVR:CS1PR8401MB0791;H:CS1PR8401MB1192.NAMPRD84.PROD.OUTLOOK.COM;FPR:;SPF:None;LANG:en;PTR:InfoNoRecords;A:1;MX:1; received-spf: None (protection.outlook.com: hpe.com does not designate permitted sender hosts) x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: rL3Yj48FCGKGQctDTd/X59EZFwGabrpv3z4Q2CIKQ05rsS7Oxr4FrFbO2NclHakTfJjInQQ7pJF4wTxoRFkGhcej5VI5kX1qkRiUqGQ9/miKH0wQCndaDvPit5rVsMAQZ8PfbrhGFLZaIPNBUfKVycufa+6H703UJc0B4uO0jdTsvdvRvnQvEZbpy4vkxarDi6Mh6q6sQ5vw8GypwUMYJgyq/z+VqFSVTOvjGDIQkQnUWlk/Z2KBfL7m7LEGS9esfoH2LFX9f2QwCNAKJ28P6/QceKZf+3eT9QM3RqCi3viKkD7EShwomPGzNk5diI1+elNNL01Gv/0pPErvpRhcDgVPlu/mj/2cRN6BN2WSOo6DgjyUOiDE2OnjJIM4x3ikVyr6rSX5NrJFEvV3/HleLOnkN9otY3fh44hNjt4Hnj7sK5CdqctyOVHJMk7hGJq88AyhywSfgTNKvWBpD4z7ZA== X-MS-Exchange-CrossTenant-Network-Message-Id: b079828e-4c81-45d1-9f39-08d7518438f7 X-MS-Exchange-CrossTenant-originalarrivaltime: 15 Oct 2019 15:27:42.8170 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 105b2061-b669-4b31-92ac-24d304d195dc X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: mHujBR7g0HqFtmTn/gjQzrFBIY9gnVvcQZVMOVznZJXnlr7KaSzReU/xp4nO8pdknU6f44KwkLT2Li0KypqwNw== X-MS-Exchange-Transport-CrossTenantHeadersStamped: CS1PR8401MB0791 X-OriginatorOrg: hpe.com X-Proofpoint-UnRewURL: 0 URL was un-rewritten MIME-Version: 1.0 X-HPE-SCL: -1 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.95,1.0.8 definitions=2019-10-15_05:2019-10-15,2019-10-15 signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 adultscore=0 spamscore=0 impostorscore=0 suspectscore=0 mlxlogscore=999 mlxscore=0 bulkscore=0 priorityscore=1501 lowpriorityscore=0 malwarescore=0 phishscore=0 clxscore=1015 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-1908290000 definitions=main-1910150135 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable > -----Original Message----- > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of > Leif Lindholm > Sent: Thursday, October 3, 2019 3:43 AM > To: devel@edk2.groups.io; Chen, Gilbert > Cc: Palmer Dabbelt > Subject: Re: [edk2-devel] [plaforms/devel-riscv-v2 PATCHv2 06/14] > RiscV/Universal: Initial version of common RISC-V SEC module >=20 > On Thu, Sep 19, 2019 at 11:51:23AM +0800, Gilbert Chen wrote: > > Common RISC-V SEC module for RISC-V platforms. >=20 > If this is common to RISC-V platforms, this really should live in edk2/R= iscVPkg. Edk2/RiscVPlatformPkg is the package of SEC module now. You will see this = in v3 edk2-staging patches. >=20 > > Signed-off-by: Gilbert Chen > > --- > > Platform/RiscV/Universal/Sec/Riscv64/SecEntry.S | 438 > ++++++++++++++++++++ > > Platform/RiscV/Universal/Sec/SecMain.c | 524 > ++++++++++++++++++++++++ > > Platform/RiscV/Universal/Sec/SecMain.h | 57 +++ > > Platform/RiscV/Universal/Sec/SecMain.inf | 75 ++++ > > 4 files changed, 1094 insertions(+) > > create mode 100644 Platform/RiscV/Universal/Sec/Riscv64/SecEntry.S > > create mode 100644 Platform/RiscV/Universal/Sec/SecMain.c > > create mode 100644 Platform/RiscV/Universal/Sec/SecMain.h > > create mode 100644 Platform/RiscV/Universal/Sec/SecMain.inf > > > > diff --git a/Platform/RiscV/Universal/Sec/Riscv64/SecEntry.S > > b/Platform/RiscV/Universal/Sec/Riscv64/SecEntry.S > > new file mode 100644 > > index 00000000..18b54b84 > > --- /dev/null > > +++ b/Platform/RiscV/Universal/Sec/Riscv64/SecEntry.S > > @@ -0,0 +1,438 @@ > > +/* > > + * Copyright (c) 2019 , Hewlett Packard Enterprise Development LP. Al= l > rights reserved. > > + * > > + * SPDX-License-Identifier: BSD-2-Clause > > + * > > + * Copyright (c) 2019 Western Digital Corporation or its affiliates. > > + * > > + * Authors: > > + * Anup Patel >=20 > If Anup is the author, he should be indicated in the commit as the Autho= r. >=20 > If you further modify the patch, you can add a comment above your > signed-off-by: of the form > [Updated coding style issues.] > Signed-off-by: ... >=20 > > + * > > + */ > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#include > > +#include > > + > > +.text > > +.align 3 > > +.global ASM_PFX(_ModuleEntryPoint) > > +ASM_PFX(_ModuleEntryPoint): > > + /* > > + * Jump to warm-boot if this is not the selected core booting, > > + */ > > + csrr a6, CSR_MHARTID > > + li a5, FixedPcdGet32 (PcdBootHartId) > > + bne a6, a5, _wait_for_boot_hart >=20 > We don't actually have a coding style for assembler, but I find it reall= y helps > readability to have the arguments aligned across all lines. Compare (for > example) with OvmfPkg/Sec/X64/SecEntry.nasm. >=20 > > + > > + // light LED on > > + li a5, 0x54002000 > > + li a4, 0xff > > + sw a4, 0x08(a5) > > + li a4, 0x11 > > + sw a4, 0x0c(a5) > > + > > + li ra, 0 > > + call _reset_regs > > + > > + /* Preload HART details > > + * s7 -> HART Count > > + * s8 -> HART Stack Size > > + */ > > + li s7, FixedPcdGet32 (PcdHartCount) li s8, FixedPcdGet32 > > + (PcdOpenSbiStackSize) > > + > > + /* Setup scratch space for all the HARTs*/ > > + li tp, FixedPcdGet32 (PcdScratchRamBase) > > + mul a5, s7, s8 > > + add tp, tp, a5 > > + /* Keep a copy of tp */ > > + add t3, tp, zero > > + /* Counter */ > > + li t2, 1 > > + /* hartid 0 is mandated by ISA */ > > + li t1, 0 > > +_scratch_init: > > + add tp, t3, zero > > + mul a5, s8, t1 > > + sub tp, tp, a5 > > + li a5, SBI_SCRATCH_SIZE > > + sub tp, tp, a5 > > + > > + /* Initialize scratch space */ > > + li a4, FixedPcdGet32 (PcdFwStartAddress) li a5, FixedPcdGet32 > > + (PcdFwEndAddress) sub a5, a5, a4 sd a4, > > + SBI_SCRATCH_FW_START_OFFSET(tp) sd a5, > > + SBI_SCRATCH_FW_SIZE_OFFSET(tp) >=20 > Can you add a blank line here? >=20 > > + /* Note: fw_next_arg1() uses a0, a1, and ra */ call fw_next_arg1 > > + sd a0, SBI_SCRATCH_NEXT_ARG1_OFFSET(tp) >=20 > Can you add a blank line here? >=20 > > + /* Note: fw_next_addr() uses a0, a1, and ra */ call fw_next_addr >=20 > Can you add a blank line here? >=20 > > + sd a0, SBI_SCRATCH_NEXT_ADDR_OFFSET(tp) li a4, PRV_S sd a4, > > + SBI_SCRATCH_NEXT_MODE_OFFSET(tp) la a4, _start_warm sd a4, > > + SBI_SCRATCH_WARMBOOT_ADDR_OFFSET(tp) > > + la a4, platform > > + sd a4, SBI_SCRATCH_PLATFORM_ADDR_OFFSET(tp) > > + la a4, _hartid_to_scratch > > + sd a4, SBI_SCRATCH_HARTID_TO_SCRATCH_OFFSET(tp) > > + sd zero, SBI_SCRATCH_TMP0_OFFSET(tp) >=20 > Can you add a blank line here? >=20 > > +#ifdef FW_OPTIONS > > + li a4, FW_OPTIONS > > + sd a4, SBI_SCRATCH_OPTIONS_OFFSET(tp) #else > > + sd zero, SBI_SCRATCH_OPTIONS_OFFSET(tp) #endif >=20 > Can you add a blank line here? >=20 > > + add t1, t1, t2 > > + blt t1, s7, _scratch_init > > + > > + /* Fill-out temporary memory with 55aa*/ > > + li a4, FixedPcdGet32 (PcdTemporaryRamBase) > > + li a5, FixedPcdGet32 (PcdTemporaryRamSize) > > + add a5, a4, a5 > > +1: > > + li a3, 0x5AA55AA55AA55AA5 > > + sd a3, (a4) > > + add a4, a4, __SIZEOF_POINTER__ > > + blt a4, a5, 1b > > + > > + /* Update boot hart flag */ > > + la a4, _boot_hart_done > > + li a5, 1 > > + sd a5, (a4) > > + > > + /* Wait for boot hart */ > > +_wait_for_boot_hart: > > + la a4, _boot_hart_done > > + ld a5, (a4) > > + /* Reduce the bus traffic so that boot hart may proceed faster */ > > + nop > > + nop > > + nop > > + beqz a5, _wait_for_boot_hart > > + > > +_start_warm: > > + li ra, 0 > > + call _reset_regs > > + > > + /* Disable and clear all interrupts */ csrw CSR_MIE, zero csrw > > + CSR_MIP, zero > > + > > + li s7, FixedPcdGet32 (PcdHartCount) li s8, FixedPcdGet32 > > + (PcdOpenSbiStackSize) > > + > > + /* HART ID should be within expected limit */ csrr s6, CSR_MHARTID > > + bge s6, s7, _start_hang > > + > > + /* find the scratch space for this hart */ li tp, FixedPcdGet32 > > + (PcdScratchRamBase) mul a5, s7, s8 add tp, tp, a5 mul a5, s8, s6 > > + sub tp, tp, a5 li a5, SBI_SCRATCH_SIZE sub tp, tp, a5 > > + > > + /* update the mscratch */ > > + csrw CSR_MSCRATCH, tp > > + > > + /*make room for Hart specific Firmware Context*/ li a5, > > + FIRMWARE_CONTEXT_HART_SPECIFIC_SIZE > > + sub tp, tp, a5 > > + > > + /* Setup stack */ > > + add sp, tp, zero > > + > > + /* Setup stack for the Hart executing EFI to top of temporary ram*/ > > + csrr a6, CSR_MHARTID li a5, FixedPcdGet32 (PcdBootHartId) bne a6, > > + a5, 1f > > + > > + li a4, FixedPcdGet32(PcdTemporaryRamBase) > > + li a5, FixedPcdGet32(PcdTemporaryRamSize) > > + add sp, a4, a5 > > + 1: > > + > > + /* Setup trap handler */ > > + la a4, _trap_handler > > + csrw CSR_MTVEC, a4 >=20 > Can you add a blank line here? >=20 > > + /* Make sure that mtvec is updated */ > > + 1: > > + csrr a5, CSR_MTVEC > > + bne a4, a5, 1b > > + > > + /* Call library constructors before jup to SEC core */ call > > + ProcessLibraryConstructorList > > + > > + /* jump to SEC Core C */ > > + csrr a0, CSR_MHARTID > > + csrr a1, CSR_MSCRATCH > > + call SecCoreStartUpWithStack > > + > > + /* We do not expect to reach here hence just hang */ j _start_hang > > + > > + .align 3 > > + .section .data, "aw" > > +_boot_hart_done: > > + RISCV_PTR 0 > > + > > + .align 3 > > + .section .entry, "ax", %progbits > > + .globl _hartid_to_scratch > > +_hartid_to_scratch: > > + add sp, sp, -(3 * __SIZEOF_POINTER__) > > + sd s0, (sp) > > + sd s1, (__SIZEOF_POINTER__)(sp) > > + sd s2, (__SIZEOF_POINTER__ * 2)(sp) >=20 > Can you add a blank line here? >=20 > > + /* > > + * a0 -> HART ID (passed by caller) > > + * s0 -> HART Stack Size > > + * s1 -> HART Stack End > > + * s2 -> Temporary > > + */ > > + la s2, platform > > +#if __riscv_xlen =3D=3D 64 > > + lwu s0, SBI_PLATFORM_HART_STACK_SIZE_OFFSET(s2) > > + lwu s2, SBI_PLATFORM_HART_COUNT_OFFSET(s2) > > +#else > > + lw s0, SBI_PLATFORM_HART_STACK_SIZE_OFFSET(s2) > > + lw s2, SBI_PLATFORM_HART_COUNT_OFFSET(s2) > > +#endif > > + mul s2, s2, s0 > > + li s1, FixedPcdGet32 (PcdScratchRamBase) > > + add s1, s1, s2 > > + mul s2, s0, a0 > > + sub s1, s1, s2 > > + li s2, SBI_SCRATCH_SIZE > > + sub a0, s1, s2 > > + ld s0, (sp) > > + ld s1, (__SIZEOF_POINTER__)(sp) > > + ld s2, (__SIZEOF_POINTER__ * 2)(sp) > > + add sp, sp, (3 * __SIZEOF_POINTER__) > > + ret > > + > > + .align 3 > > + .section .entry, "ax", %progbits > > + .globl _start_hang > > +_start_hang: > > + wfi > > + j _start_hang > > + > > + .align 3 > > + .section .entry, "ax", %progbits > > + .globl _trap_handler > > +_trap_handler: > > + /* Swap TP and MSCRATCH */ > > + csrrw tp, CSR_MSCRATCH, tp > > + > > + /* Save T0 in scratch space */ > > + sd t0, SBI_SCRATCH_TMP0_OFFSET(tp) > > + > > + /* Check which mode we came from */ csrr t0, CSR_MSTATUS srl t0, > > + t0, MSTATUS_MPP_SHIFT and t0, t0, PRV_M xori t0, t0, PRV_M beq > > + t0, zero, _trap_handler_m_mode > > + > > + /* We came from S-mode or U-mode */ > > +_trap_handler_s_mode: > > + /* Set T0 to original SP */ > > + add t0, sp, zero > > + > > + /* Setup exception stack */ > > + add sp, tp, -(SBI_TRAP_REGS_SIZE) > > + > > + /* Jump to code common for all modes */ j _trap_handler_all_mode > > + > > + /* We came from M-mode */ > > +_trap_handler_m_mode: > > + /* Set T0 to original SP */ > > + add t0, sp, zero > > + > > + /* Re-use current SP as exception stack */ add sp, sp, > > + -(SBI_TRAP_REGS_SIZE) > > + > > +_trap_handler_all_mode: > > + /* Save original SP (from T0) on stack */ > > + sd t0, SBI_TRAP_REGS_OFFSET(sp)(sp) > > + > > + /* Restore T0 from scratch space */ ld t0, > > + SBI_SCRATCH_TMP0_OFFSET(tp) > > + > > + /* Save T0 on stack */ > > + sd t0, SBI_TRAP_REGS_OFFSET(t0)(sp) > > + > > + /* Swap TP and MSCRATCH */ > > + csrrw tp, CSR_MSCRATCH, tp > > + > > + /* Save MEPC and MSTATUS CSRs */ > > + csrr t0, CSR_MEPC > > + sd t0, SBI_TRAP_REGS_OFFSET(mepc)(sp) csrr t0, CSR_MSTATUS sd t0, > > + SBI_TRAP_REGS_OFFSET(mstatus)(sp) > > + > > + /* Save all general regisers except SP and T0 */ sd zero, > > + SBI_TRAP_REGS_OFFSET(zero)(sp) sd ra, SBI_TRAP_REGS_OFFSET(ra)(sp) > > + sd gp, SBI_TRAP_REGS_OFFSET(gp)(sp) sd tp, > > + SBI_TRAP_REGS_OFFSET(tp)(sp) sd t1, SBI_TRAP_REGS_OFFSET(t1)(sp) > > + sd t2, SBI_TRAP_REGS_OFFSET(t2)(sp) sd s0, > > + SBI_TRAP_REGS_OFFSET(s0)(sp) sd s1, SBI_TRAP_REGS_OFFSET(s1)(sp) > > + sd a0, SBI_TRAP_REGS_OFFSET(a0)(sp) sd a1, > > + SBI_TRAP_REGS_OFFSET(a1)(sp) sd a2, SBI_TRAP_REGS_OFFSET(a2)(sp) > > + sd a3, SBI_TRAP_REGS_OFFSET(a3)(sp) sd a4, > > + SBI_TRAP_REGS_OFFSET(a4)(sp) sd a5, SBI_TRAP_REGS_OFFSET(a5)(sp) > > + sd a6, SBI_TRAP_REGS_OFFSET(a6)(sp) sd a7, > > + SBI_TRAP_REGS_OFFSET(a7)(sp) sd s2, SBI_TRAP_REGS_OFFSET(s2)(sp) > > + sd s3, SBI_TRAP_REGS_OFFSET(s3)(sp) sd s4, > > + SBI_TRAP_REGS_OFFSET(s4)(sp) sd s5, SBI_TRAP_REGS_OFFSET(s5)(sp) > > + sd s6, SBI_TRAP_REGS_OFFSET(s6)(sp) sd s7, > > + SBI_TRAP_REGS_OFFSET(s7)(sp) sd s8, SBI_TRAP_REGS_OFFSET(s8)(sp) > > + sd s9, SBI_TRAP_REGS_OFFSET(s9)(sp) sd s10, > > + SBI_TRAP_REGS_OFFSET(s10)(sp) sd s11, > SBI_TRAP_REGS_OFFSET(s11)(sp) > > + sd t3, SBI_TRAP_REGS_OFFSET(t3)(sp) sd t4, > > + SBI_TRAP_REGS_OFFSET(t4)(sp) sd t5, SBI_TRAP_REGS_OFFSET(t5)(sp) > > + sd t6, SBI_TRAP_REGS_OFFSET(t6)(sp) > > + > > + /* Call C routine */ > > + add a0, sp, zero > > + csrr a1, CSR_MSCRATCH > > + call sbi_trap_handler > > + > > + /* Restore all general regisers except SP and T0 */ ld ra, > > + SBI_TRAP_REGS_OFFSET(ra)(sp) ld gp, SBI_TRAP_REGS_OFFSET(gp)(sp) > > + ld tp, SBI_TRAP_REGS_OFFSET(tp)(sp) ld t1, > > + SBI_TRAP_REGS_OFFSET(t1)(sp) ld t2, SBI_TRAP_REGS_OFFSET(t2)(sp) > > + ld s0, SBI_TRAP_REGS_OFFSET(s0)(sp) ld s1, > > + SBI_TRAP_REGS_OFFSET(s1)(sp) ld a0, SBI_TRAP_REGS_OFFSET(a0)(sp) > > + ld a1, SBI_TRAP_REGS_OFFSET(a1)(sp) ld a2, > > + SBI_TRAP_REGS_OFFSET(a2)(sp) ld a3, SBI_TRAP_REGS_OFFSET(a3)(sp) > > + ld a4, SBI_TRAP_REGS_OFFSET(a4)(sp) ld a5, > > + SBI_TRAP_REGS_OFFSET(a5)(sp) ld a6, SBI_TRAP_REGS_OFFSET(a6)(sp) > > + ld a7, SBI_TRAP_REGS_OFFSET(a7)(sp) ld s2, > > + SBI_TRAP_REGS_OFFSET(s2)(sp) ld s3, SBI_TRAP_REGS_OFFSET(s3)(sp) > > + ld s4, SBI_TRAP_REGS_OFFSET(s4)(sp) ld s5, > > + SBI_TRAP_REGS_OFFSET(s5)(sp) ld s6, SBI_TRAP_REGS_OFFSET(s6)(sp) > > + ld s7, SBI_TRAP_REGS_OFFSET(s7)(sp) ld s8, > > + SBI_TRAP_REGS_OFFSET(s8)(sp) ld s9, SBI_TRAP_REGS_OFFSET(s9)(sp) > > + ld s10, SBI_TRAP_REGS_OFFSET(s10)(sp) ld s11, > > + SBI_TRAP_REGS_OFFSET(s11)(sp) ld t3, SBI_TRAP_REGS_OFFSET(t3)(sp) > > + ld t4, SBI_TRAP_REGS_OFFSET(t4)(sp) ld t5, > > + SBI_TRAP_REGS_OFFSET(t5)(sp) ld t6, SBI_TRAP_REGS_OFFSET(t6)(sp) > > + > > + /* Restore MEPC and MSTATUS CSRs */ ld t0, > > + SBI_TRAP_REGS_OFFSET(mepc)(sp) csrw CSR_MEPC, t0 ld t0, > > + SBI_TRAP_REGS_OFFSET(mstatus)(sp) csrw CSR_MSTATUS, t0 > > + > > + /* Restore T0 */ > > + ld t0, SBI_TRAP_REGS_OFFSET(t0)(sp) > > + > > + /* Restore SP */ > > + ld sp, SBI_TRAP_REGS_OFFSET(sp)(sp) > > + > > + mret > > + > > + .align 3 > > + .section .entry, "ax", %progbits > > + .globl _reset_regs > > +_reset_regs: > > + > > + /* flush the instruction cache */ > > + fence.i > > + /* Reset all registers except ra, a0,a1 */ li sp, 0 li gp, 0 li > > + tp, 0 li t0, 0 li t1, 0 li t2, 0 li s0, 0 li s1, 0 li a2, 0 > > + li a3, 0 li a4, 0 li a5, 0 li a6, 0 li a7, 0 li s2, 0 li s3, 0 > > + li s4, 0 li s5, 0 li s6, 0 li s7, 0 li s8, 0 li s9, 0 li s10, > > + 0 li s11, 0 li t3, 0 li t4, 0 li t5, 0 li t6, 0 csrw > > + CSR_MSCRATCH, 0 ret > > + > > + .align 3 > > + .section .entry, "ax", %progbits > > + .global fw_prev_arg1 > > +fw_prev_arg1: > > + /* We return previous arg1 in 'a0' */ > > + add a0, zero, zero > > + ret > > + > > + .align 3 > > + .section .entry, "ax", %progbits > > + .global fw_next_arg1 > > +fw_next_arg1: > > + /* We return next arg1 in 'a0' */ > > + li a0, FixedPcdGet32(PcdRiscVPeiFvBase) > > + ret > > + > > + .align 3 > > + .section .entry, "ax", %progbits > > + .global fw_next_addr > > +fw_next_addr: > > + /* We return next address in 'a0' */ > > + la a0, _jump_addr > > + ld a0, (a0) > > + ret > > + > > + .align 3 > > + .section .entry, "ax", %progbits > > +_jump_addr: > > + RISCV_PTR SecCoreStartUpWithStack > > diff --git a/Platform/RiscV/Universal/Sec/SecMain.c > > b/Platform/RiscV/Universal/Sec/SecMain.c > > new file mode 100644 > > index 00000000..40b351ca > > --- /dev/null > > +++ b/Platform/RiscV/Universal/Sec/SecMain.c > > @@ -0,0 +1,524 @@ > > +/** @file > > + RISC-V SEC phase module. > > + > > + Copyright (c) 2019, Hewlett Packard Enterprise Development LP. All > > + rights reserved.
> > + > > + SPDX-License-Identifier: BSD-2-Clause-Patent > > + > > +**/ > > + > > +#include "SecMain.h" > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include >=20 > Please sort includes alphabetically. >=20 > > +#include > > + > > +int HartsIn =3D 0; >=20 > UINTN? >=20 > > + > > +EFI_PEI_TEMPORARY_RAM_SUPPORT_PPI mTemporaryRamSupportPpi =3D > { > > + TemporaryRamMigration > > +}; > > + > > +EFI_PEI_TEMPORARY_RAM_DONE_PPI mTemporaryRamDonePpi =3D { > > + TemporaryRamDone > > +}; > > + > > +EFI_PEI_PPI_DESCRIPTOR mPrivateDispatchTable[] =3D { > > + { > > + EFI_PEI_PPI_DESCRIPTOR_PPI, > > + &gEfiTemporaryRamSupportPpiGuid, > > + &mTemporaryRamSupportPpi > > + }, > > + { > > + (EFI_PEI_PPI_DESCRIPTOR_PPI | > EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST), > > + &gEfiTemporaryRamDonePpiGuid, > > + &mTemporaryRamDonePpi > > + }, > > +}; >=20 > Can the above three variables all be made STATIC? >=20 > > + > > +/** > > + Locates a section within a series of sections > > + with the specified section type. > > + > > + The Instance parameter indicates which instance of the section > > + type to return. (0 is first instance, 1 is second...) > > + > > + @param[in] Sections The sections to search > > + @param[in] SizeOfSections Total size of all sections > > + @param[in] SectionType The section type to locate > > + @param[in] Instance The section instance number > > + @param[out] FoundSection The FFS section if found > > + > > + @retval EFI_SUCCESS The file and section was found > > + @retval EFI_NOT_FOUND The file and section was not found > > + @retval EFI_VOLUME_CORRUPTED The firmware volume was corrupted > > + > > +**/ > > +EFI_STATUS > > +FindFfsSectionInstance ( >=20 > This is a note, not something that needs to be addressed for setting up = the - > devel branch: but this function is identical to the one in > /OvmfPkg/Sec/SecMain.c and the one in > Platform/Intel/SimicsOpenBoardPkg/SecCore/SecMain.c > We should address this. >=20 > > + IN VOID *Sections, > > + IN UINTN SizeOfSections, > > + IN EFI_SECTION_TYPE SectionType, > > + IN UINTN Instance, > > + OUT EFI_COMMON_SECTION_HEADER **FoundSection > > + ) > > +{ > > + EFI_PHYSICAL_ADDRESS CurrentAddress; > > + UINT32 Size; > > + EFI_PHYSICAL_ADDRESS EndOfSections; > > + EFI_COMMON_SECTION_HEADER *Section; > > + EFI_PHYSICAL_ADDRESS EndOfSection; > > + > > + // > > + // Loop through the FFS file sections within the PEI Core FFS file > > + // EndOfSection =3D (EFI_PHYSICAL_ADDRESS)(UINTN) Sections; > > + EndOfSections =3D EndOfSection + SizeOfSections; for (;;) { > > + if (EndOfSection =3D=3D EndOfSections) { > > + break; > > + } > > + CurrentAddress =3D (EndOfSection + 3) & ~(3ULL); > > + if (CurrentAddress >=3D EndOfSections) { > > + return EFI_VOLUME_CORRUPTED; > > + } > > + > > + Section =3D (EFI_COMMON_SECTION_HEADER*)(UINTN) CurrentAddress; > > + > > + Size =3D SECTION_SIZE (Section); > > + if (Size < sizeof (*Section)) { > > + return EFI_VOLUME_CORRUPTED; > > + } > > + > > + EndOfSection =3D CurrentAddress + Size; > > + if (EndOfSection > EndOfSections) { > > + return EFI_VOLUME_CORRUPTED; > > + } > > + > > + // > > + // Look for the requested section type > > + // > > + if (Section->Type =3D=3D SectionType) { > > + if (Instance =3D=3D 0) { > > + *FoundSection =3D Section; > > + return EFI_SUCCESS; > > + } else { > > + Instance--; > > + } > > + } > > + } > > + > > + return EFI_NOT_FOUND; > > +} > > + > > +/** > > + Locates a section within a series of sections > > + with the specified section type. > > + > > + @param[in] Sections The sections to search > > + @param[in] SizeOfSections Total size of all sections > > + @param[in] SectionType The section type to locate > > + @param[out] FoundSection The FFS section if found > > + > > + @retval EFI_SUCCESS The file and section was found > > + @retval EFI_NOT_FOUND The file and section was not found > > + @retval EFI_VOLUME_CORRUPTED The firmware volume was corrupted > > + > > +**/ > > +EFI_STATUS > > +FindFfsSectionInSections ( >=20 > The same applies to this one. >=20 > > + IN VOID *Sections, > > + IN UINTN SizeOfSections, > > + IN EFI_SECTION_TYPE SectionType, > > + OUT EFI_COMMON_SECTION_HEADER **FoundSection > > + ) > > +{ > > + return FindFfsSectionInstance ( > > + Sections, > > + SizeOfSections, > > + SectionType, > > + 0, > > + FoundSection > > + ); > > +} > > + > > +/** > > + Locates a FFS file with the specified file type and a section > > + within that file with the specified section type. > > + > > + @param[in] Fv The firmware volume to search > > + @param[in] FileType The file type to locate > > + @param[in] SectionType The section type to locate > > + @param[out] FoundSection The FFS section if found > > + > > + @retval EFI_SUCCESS The file and section was found > > + @retval EFI_NOT_FOUND The file and section was not found > > + @retval EFI_VOLUME_CORRUPTED The firmware volume was corrupted > > + > > +**/ > > +EFI_STATUS > > +FindFfsFileAndSection ( >=20 > And this one. >=20 > > + IN EFI_FIRMWARE_VOLUME_HEADER *Fv, > > + IN EFI_FV_FILETYPE FileType, > > + IN EFI_SECTION_TYPE SectionType, > > + OUT EFI_COMMON_SECTION_HEADER **FoundSection > > + ) > > +{ > > + EFI_STATUS Status; > > + EFI_PHYSICAL_ADDRESS CurrentAddress; > > + EFI_PHYSICAL_ADDRESS EndOfFirmwareVolume; > > + EFI_FFS_FILE_HEADER *File; > > + UINT32 Size; > > + EFI_PHYSICAL_ADDRESS EndOfFile; > > + > > + if (Fv->Signature !=3D EFI_FVH_SIGNATURE) { > > + DEBUG ((DEBUG_ERROR, "FV at %p does not have FV header > signature\n", Fv)); > > + return EFI_VOLUME_CORRUPTED; > > + } > > + > > + CurrentAddress =3D (EFI_PHYSICAL_ADDRESS)(UINTN) Fv; > > + EndOfFirmwareVolume =3D CurrentAddress + Fv->FvLength; > > + > > + // > > + // Loop through the FFS files in the Boot Firmware Volume // for > > + (EndOfFile =3D CurrentAddress + Fv->HeaderLength; ; ) { > > + > > + CurrentAddress =3D (EndOfFile + 7) & ~(7ULL); > > + if (CurrentAddress > EndOfFirmwareVolume) { > > + return EFI_VOLUME_CORRUPTED; > > + } > > + > > + File =3D (EFI_FFS_FILE_HEADER*)(UINTN) CurrentAddress; > > + Size =3D *(UINT32*) File->Size & 0xffffff; > > + if (Size < (sizeof (*File) + sizeof (EFI_COMMON_SECTION_HEADER)))= { > > + return EFI_VOLUME_CORRUPTED; > > + } > > + > > + EndOfFile =3D CurrentAddress + Size; > > + if (EndOfFile > EndOfFirmwareVolume) { > > + return EFI_VOLUME_CORRUPTED; > > + } > > + > > + // > > + // Look for the request file type > > + // > > + if (File->Type !=3D FileType) { > > + continue; > > + } > > + > > + Status =3D FindFfsSectionInSections ( > > + (VOID*) (File + 1), > > + (UINTN) EndOfFile - (UINTN) (File + 1), > > + SectionType, > > + FoundSection > > + ); > > + if (!EFI_ERROR (Status) || (Status =3D=3D EFI_VOLUME_CORRUPTED)) = { > > + return Status; > > + } > > + } > > +} > > + > > +/** > > + Locates the PEI Core entry point address > > + > > + @param[in] Fv The firmware volume to search > > + @param[out] PeiCoreEntryPoint The entry point of the PEI Core > > + image > > + > > + @retval EFI_SUCCESS The file and section was found > > + @retval EFI_NOT_FOUND The file and section was not found > > + @retval EFI_VOLUME_CORRUPTED The firmware volume was corrupted > > + > > +**/ > > +EFI_STATUS > > +FindPeiCoreImageBaseInFv ( >=20 > And this. >=20 > > + IN EFI_FIRMWARE_VOLUME_HEADER *Fv, > > + OUT EFI_PHYSICAL_ADDRESS *PeiCoreImageBase > > + ) > > +{ > > + EFI_STATUS Status; > > + EFI_COMMON_SECTION_HEADER *Section; > > + > > + Status =3D FindFfsFileAndSection ( > > + Fv, > > + EFI_FV_FILETYPE_PEI_CORE, > > + EFI_SECTION_PE32, > > + &Section > > + ); > > + if (EFI_ERROR (Status)) { > > + Status =3D FindFfsFileAndSection ( > > + Fv, > > + EFI_FV_FILETYPE_PEI_CORE, > > + EFI_SECTION_TE, > > + &Section > > + ); > > + if (EFI_ERROR (Status)) { > > + DEBUG ((DEBUG_ERROR, "Unable to find PEI Core image\n")); > > + return Status; > > + } > > + } > > + DEBUG ((DEBUG_ERROR, "PeiCoreImageBase found\n")); > > + *PeiCoreImageBase =3D (EFI_PHYSICAL_ADDRESS)(UINTN)(Section + 1); > > + return EFI_SUCCESS; > > +} > > + > > +/** > > + Locates the PEI Core entry point address > > + > > + @param[in,out] Fv The firmware volume to search > > + @param[out] PeiCoreEntryPoint The entry point of the PEI Core = image > > + > > + @retval EFI_SUCCESS The file and section was found > > + @retval EFI_NOT_FOUND The file and section was not found > > + @retval EFI_VOLUME_CORRUPTED The firmware volume was corrupted > > + > > +**/ > > +VOID > > +FindPeiCoreImageBase ( > > + IN OUT EFI_FIRMWARE_VOLUME_HEADER **BootFv, > > + OUT EFI_PHYSICAL_ADDRESS *PeiCoreImageBase > > + ) > > +{ > > + *PeiCoreImageBase =3D 0; > > + > > + DEBUG ((DEBUG_INFO, "FindPeiCoreImageBaseInFv\n")); >=20 > This one is the same with S3 support ripped out, and the above line adde= d. > Can you delete the message, or change it to something more human friendl= y? > I personally would interpret that as something that was printed _from_ t= hat > function (and in that situation should be using %a __FUNCTION__). >=20 > > + FindPeiCoreImageBaseInFv (*BootFv, PeiCoreImageBase); } > > + > > +/* > > + Find and return Pei Core entry point. > > + > > + It also find SEC and PEI Core file debug inforamtion. It will > > + report them if remote debug is enabled. > > + > > +**/ > > +VOID > > +FindAndReportEntryPoints ( > > + IN EFI_FIRMWARE_VOLUME_HEADER **BootFirmwareVolumePtr, > > + OUT EFI_PEI_CORE_ENTRY_POINT *PeiCoreEntryPoint > > + ) > > +{ > > + EFI_STATUS Status; > > + EFI_PHYSICAL_ADDRESS PeiCoreImageBase; > > + > > + DEBUG ((DEBUG_INFO, "FindAndReportEntryPoints\n")); >=20 > Use %a __FUNCTION__. >=20 > > + > > + FindPeiCoreImageBase (BootFirmwareVolumePtr, &PeiCoreImageBase); > > + // // Find PEI Core entry point // Status =3D > > + PeCoffLoaderGetEntryPoint ((VOID *) (UINTN) PeiCoreImageBase, > > + (VOID**) PeiCoreEntryPoint); if (EFI_ERROR(Status)) { > > + *PeiCoreEntryPoint =3D 0; > > + } > > + DEBUG ((DEBUG_INFO, "PeCoffLoaderGetEntryPoint success: %x\n", > > + *PeiCoreEntryPoint)); > > + > > + return; > > +} > > +/* > > + Print out the content of firmware context. > > + > > +**/ > > +VOID > > +DebutPrintFirmwareContext ( >=20 > Debut -> Debug? >=20 > > + EFI_RISCV_OPENSBI_FIRMWARE_CONTEXT *FirmwareContext > > + ) > > +{ > > + DEBUG ((DEBUG_INFO, "[OpenSBI]: OpenSBI Firmware Context at > > +0x%x\n", FirmwareContext)); >=20 > Please drop the [OpenSBI] prefix. >=20 > > + DEBUG ((DEBUG_INFO, " PEI Service at 0x%x\n\n", > FirmwareContext->PeiServiceTable)); > > +} > > + > > +EFI_STATUS > > +EFIAPI > > +TemporaryRamMigration ( > > + IN CONST EFI_PEI_SERVICES **PeiServices, > > + IN EFI_PHYSICAL_ADDRESS TemporaryMemoryBase, > > + IN EFI_PHYSICAL_ADDRESS PermanentMemoryBase, > > + IN UINTN CopySize > > + ) > > +{ > > + VOID *OldHeap; > > + VOID *NewHeap; > > + VOID *OldStack; > > + VOID *NewStack; > > + struct sbi_platform *ThisSbiPlatform; > > + > > + DEBUG ((DEBUG_INFO, > > + "TemporaryRamMigration(0x%Lx, 0x%Lx, 0x%Lx)\n", >=20 > %a > __FUNCTION__ >=20 > > + TemporaryMemoryBase, > > + PermanentMemoryBase, > > + (UINT64)CopySize > > + )); > > + > > + OldHeap =3D (VOID*)(UINTN)TemporaryMemoryBase; > > + NewHeap =3D (VOID*)((UINTN)PermanentMemoryBase + (CopySize >> 1)); > > + > > + OldStack =3D (VOID*)((UINTN)TemporaryMemoryBase + (CopySize >> 1)); > > + NewStack =3D (VOID*)(UINTN)PermanentMemoryBase; > > + > > + CopyMem (NewHeap, OldHeap, CopySize >> 1); // Migrate Heap > > + CopyMem (NewStack, OldStack, CopySize >> 1); // Migrate Stack > > + > > + // > > + // Reset firmware context pointer > > + // > > + ThisSbiPlatform =3D (struct sbi_platform > > + *)sbi_platform_ptr(sbi_scratch_thishart_ptr()); > > + ThisSbiPlatform->firmware_context +=3D (unsigned > > + long)((UINTN)NewStack - (UINTN)OldStack); >=20 > "unsigned long" is not a type that exists in UEFI. > Please use UINTN. Applies throughout. >=20 > > + // > > + // Relocate PEI Service ** > > + // > > + ((EFI_RISCV_OPENSBI_FIRMWARE_CONTEXT > > + *)ThisSbiPlatform->firmware_context)->PeiServiceTable +=3D (unsigned > > + long)((UINTN)NewStack - (UINTN)OldStack); >=20 > The above line is begging for a temporary pointer for the firmware_conte= xt. >=20 > > + DEBUG ((DEBUG_INFO, "[OpenSBI]: OpenSBI Firmware Context is > > + relocated to 0x%x\n", ThisSbiPlatform->firmware_context)); >=20 > Please >=20 > > + DebutPrintFirmwareContext > ((EFI_RISCV_OPENSBI_FIRMWARE_CONTEXT > > + *)ThisSbiPlatform->firmware_context); > > + > > + register uintptr_t a0 asm ("a0") =3D (uintptr_t)((UINTN)NewStack - > > + (UINTN)OldStack); asm volatile ("add sp, sp, a0"::"r"(a0):); >=20 > Urgh. > Is there any way this could be broken out into a separate assembler func= tion > instead? If not, we still need to get rid of the uintptr_t:s. >=20 > > + return EFI_SUCCESS; > > +} > > + > > +EFI_STATUS EFIAPI TemporaryRamDone (VOID) { > > + DEBUG ((DEBUG_INFO, "2nd time PEI core, temporary ram done.\n")); > > + return EFI_SUCCESS; > > +} > > + > > +#if 1 >=20 > Just delete the #if 1? >=20 > > +#define GPIO_CTRL_ADDR 0x54002000 > > +#define GPIO_OUTPUT_VAL 0x0C > > +static volatile UINT32 * const gpio =3D (void *)GPIO_CTRL_ADDR; > > +#define REG32(p, i) ((p)[(i)>>2]) > > +#endif >=20 > And the #endif >=20 > Moreover, please move the ADDRESS/VALUE macros to SecMain.h, and use > IoLib instead of defining your own variables and macros. >=20 > > + > > +static VOID EFIAPI PeiCore(VOID) >=20 > STATIC > Also, please follow coding style for function definition, this is suppos= ed to be > over multiple lines. >=20 > > +{ > > + EFI_SEC_PEI_HAND_OFF SecCoreData; > > + EFI_PEI_CORE_ENTRY_POINT PeiCoreEntryPoint; > > + EFI_FIRMWARE_VOLUME_HEADER *BootFv =3D > (EFI_FIRMWARE_VOLUME_HEADER > > +*)FixedPcdGet32(PcdRiscVPeiFvBase); > > + EFI_RISCV_OPENSBI_FIRMWARE_CONTEXT FirmwareContext; > > + struct sbi_platform *ThisSbiPlatform; > > + UINT32 HartId; > > + > > + REG32(gpio, GPIO_OUTPUT_VAL) =3D 0x88; FindAndReportEntryPoints > > + (&BootFv, &PeiCoreEntryPoint); > > + > > + SecCoreData.DataSize =3D sizeof(EFI_SEC_PEI_HAND_OFF)= ; > > + SecCoreData.BootFirmwareVolumeBase =3D BootFv; > > + SecCoreData.BootFirmwareVolumeSize =3D (UINTN) BootFv->FvLength; > > + SecCoreData.TemporaryRamBase =3D (VOID*)(UINT64) > FixedPcdGet32(PcdTemporaryRamBase); > > + SecCoreData.TemporaryRamSize =3D (UINTN) > FixedPcdGet32(PcdTemporaryRamSize); > > + SecCoreData.PeiTemporaryRamBase =3D > SecCoreData.TemporaryRamBase; > > + SecCoreData.PeiTemporaryRamSize =3D SecCoreData.TemporaryRamSize > >> 1; > > + SecCoreData.StackBase =3D (UINT8 > *)SecCoreData.TemporaryRamBase + (SecCoreData.TemporaryRamSize >> > 1); >=20 > Please don't use UINT8 * to get away from pointer arithmetic. It disguis= es > what function is actually being performed. > (VOID *)((UINTN) ...) is a much preferred form where that level of > description is required. >=20 > > + SecCoreData.StackSize =3D SecCoreData.TemporaryRamSize= >> 1; > > + > > + // > > + // Print out scratch address of each hart // DEBUG ((DEBUG_INFO, > > + "[OpenSBI]: OpenSBI scratch address for each hart:\n")); >=20 > Please drop [OpenSBI] tag. >=20 > > + for (HartId =3D 0; HartId < FixedPcdGet32 (PcdHartCount); HartId ++= ) { > > + DEBUG ((DEBUG_INFO, " Hart %d: 0x%x\n", HartId, > sbi_hart_id_to_scratch(sbi_scratch_thishart_ptr(), HartId))); >=20 > Please wrap long line. >=20 > > + } > > + > > + // > > + // Set up OpepSBI firmware context poitner on boot hart OpenSbi > > + scratch. Firmware context residents in stack and will be >=20 > Please wrap long line. >=20 > > + // switched to memory when temporary ram migration. > > + // > > + ZeroMem ((VOID *)&FirmwareContext, sizeof > > + (EFI_RISCV_OPENSBI_FIRMWARE_CONTEXT)); > > + ThisSbiPlatform =3D (struct sbi_platform > > + *)sbi_platform_ptr(sbi_scratch_thishart_ptr()); > > + if (ThisSbiPlatform->opensbi_version > OPENSBI_VERSION) { > > + DEBUG ((DEBUG_ERROR, "[OpenSBI]: OpenSBI platform table version > 0x%x is newer than OpenSBI version 0x%x.\n" > > + "There maybe be some backward compatable > > + issues.\n", >=20 > Please drop [OpenSBI] tag. Please convert the two lines to two separate > DEBUG prints. >=20 > > + ThisSbiPlatform->opensbi_version, > > + OPENSBI_VERSION > > + )); > > + ASSERT(FALSE); > > + } > > + DEBUG ((DEBUG_INFO, "[OpenSBI]: OpenSBI platform table at address: > > + 0x%x\nFirmware Context is located at 0x%x\n", >=20 > Please drop [OpenSBI] tag. >=20 > > + ThisSbiPlatform, > > + &FirmwareContext > > + )); > > + ThisSbiPlatform->firmware_context =3D (unsigned > > + long)&FirmwareContext; >=20 > UINTN (probably best to do a global search and replace on this). >=20 > > + // > > + // Set firmware context Hart-specific pointer // for (HartId =3D = 0; > > + HartId < FixedPcdGet32 (PcdHartCount); HartId ++) { > > + FirmwareContext.HartSpecific [HartId] =3D \ > > + (EFI_RISCV_FIRMWARE_CONTEXT_HART_SPECIFIC *)((UINT8 > > + *)sbi_hart_id_to_scratch(sbi_scratch_thishart_ptr(), HartId) - > > + FIRMWARE_CONTEXT_HART_SPECIFIC_SIZE); >=20 > Please don't use (UINT8 *) to get away from pointer arithmetic. > UINTN should work fine in this context. >=20 > Screaming out for a temporary variable. >=20 > > + DEBUG ((DEBUG_INFO, "[OpenSBI]: OpenSBI Hart %d Firmware > Context > > + Hart-specific at address: 0x%x\n", >=20 > Please drop [OpenSBI] tag. >=20 > > + HartId, > > + FirmwareContext.HartSpecific [HartId] > > + )); > > + } > > + > > + // > > + // Transfer the control to the PEI core > > + // > > + (*PeiCoreEntryPoint) (&SecCoreData, (EFI_PEI_PPI_DESCRIPTOR > > +*)&mPrivateDispatchTable); } > > +/** > > + This function initilizes hart specific information and SBI. > > + For the boot hart, it boots system through PEI core and initial SBI= in the > DXE IPL. > > + For others, it goes to initial SBI and halt. > > + > > + the lay out of memory region for each hart is as below delineates, > > + > > + _ = ____ > > + |----Scratch ends | = | > > + | | sizeof (sbi_scratch= ) | > > + | _| = | > > + |----Scratch buffer start s <----- *scratch = | > > + |----Firmware Context Hart-specific ends _ = | > > + | | = | > > + | | FIRMWARE_CONTEXT_HA= RT_SPECIFIC_SIZE > | > > + | | = | PcdOpenSbiStackSize > > + | _| = | > > + |----Firmware Context Hart-specific starts <----- > **HartFirmwareContext | > > + |----Hart stack top _ = | > > + | | = | > > + | | = | > > + | | Stack = | > > + | | = | > > + | _| = ____| > > + |----Hart stack bottom > > + > > +**/ > > +VOID EFIAPI SecCoreStartUpWithStack(unsigned long hartid, struct > > +sbi_scratch *scratch) >=20 > Split up over multiple lines. >=20 > > +{ > > + EFI_RISCV_FIRMWARE_CONTEXT_HART_SPECIFIC > *HartFirmwareContext; > > + > > + // > > + // Setup EFI_RISCV_FIRMWARE_CONTEXT_HART_SPECIFIC for each hart. > > + // > > + HartFirmwareContext =3D > (EFI_RISCV_FIRMWARE_CONTEXT_HART_SPECIFIC > > + *)((UINT8 *)scratch - FIRMWARE_CONTEXT_HART_SPECIFIC_SIZE); >=20 > Please don't use UINT8 * to get away from pointer arithmetic. > In this context, UINTN should work just fine. >=20 > > + HartFirmwareContext->IsaExtensionSupported =3D RiscVReadMisa (); > > + HartFirmwareContext->MachineVendorId.Value64_L =3D > RiscVReadMVendorId > > + (); HartFirmwareContext->MachineVendorId.Value64_H =3D 0; > > + HartFirmwareContext->MachineArchId.Value64_L =3D RiscVReadMArchId > (); > > + HartFirmwareContext->MachineArchId.Value64_H =3D 0; > > + HartFirmwareContext->MachineImplId.Value64_L =3D RiscVReadMImplId ()= ; > > + HartFirmwareContext->MachineImplId.Value64_H =3D 0; > > + > > +#if 0 >=20 > Please don't leave unused code in. > If you want to keep the printouts for extreme debugging sessions, change > the loglevel to DEBUG_VERBOSE. >=20 > > + while (HartsIn !=3D hartid); > > + DEBUG ((DEBUG_INFO, "[OpenSBI]: Initial Firmware Context > > + Hart-specific for HART ID:%d\n", hartid)); >=20 > Please drop [OpenSBI] tag. >=20 > > + DEBUG ((DEBUG_INFO, " Scratch at address: 0x%x\n", scratc= h)); > > + DEBUG ((DEBUG_INFO, " Firmware Context Hart-specific at a= ddress: > 0x%x\n", HartFirmwareContext)); > > + DEBUG ((DEBUG_INFO, " stack pointer at address: 0x%x\n", > stack_point)); > > + DEBUG ((DEBUG_INFO, " MISA: 0x%x\n", > HartFirmwareContext->IsaExtensionSupported)); > > + DEBUG ((DEBUG_INFO, " MVENDORID: 0x%x\n", > HartFirmwareContext->MachineVendorId.Value64_L)); > > + DEBUG ((DEBUG_INFO, " MARCHID: 0x%x\n", > HartFirmwareContext->MachineArchId.Value64_L)); > > + DEBUG ((DEBUG_INFO, " MIMPID: 0x%x\n\n", > HartFirmwareContext->MachineImplId.Value64_L)); > > + HartsIn ++; > > + for (;;); > > +#endif > > + > > + if (hartid =3D=3D FixedPcdGet32(PcdBootHartId)) { > > + PeiCore(); > > + } > > + sbi_init(scratch); > > +} > > diff --git a/Platform/RiscV/Universal/Sec/SecMain.h > > b/Platform/RiscV/Universal/Sec/SecMain.h > > new file mode 100644 > > index 00000000..e7565f5e > > --- /dev/null > > +++ b/Platform/RiscV/Universal/Sec/SecMain.h > > @@ -0,0 +1,57 @@ > > +/** @file > > + RISC-V SEC phase module definitions.. > > + > > + Copyright (c) 2019, Hewlett Packard Enterprise Development LP. All > > + rights reserved.
> > + > > + SPDX-License-Identifier: BSD-2-Clause-Patent > > + > > +**/ > > + > > +#ifndef _SECMAIN_H_ > > +#define _SECMAIN_H_ >=20 > Plese drop leading _. >=20 > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include #include > > + > > +#include > > +#include > > +#include > > +#include >=20 > Please drop all include statements not required by this file, and add th= em > instead in the files that do use them. >=20 > > + > > +VOID > > +SecMachineModeTrapHandler ( > > + IN VOID > > + ); > > + > > +VOID > > +EFIAPI > > +SecStartupPhase2 ( > > + IN VOID *Context > > + ); > > + > > +EFI_STATUS > > +EFIAPI > > +TemporaryRamMigration ( > > + IN CONST EFI_PEI_SERVICES **PeiServices, > > + IN EFI_PHYSICAL_ADDRESS TemporaryMemoryBase, > > + IN EFI_PHYSICAL_ADDRESS PermanentMemoryBase, > > + IN UINTN CopySize > > + ); > > + > > +EFI_STATUS > > +EFIAPI > > +TemporaryRamDone ( > > + VOID > > + ); > > + > > +#endif // _SECMAIN_H_ > > diff --git a/Platform/RiscV/Universal/Sec/SecMain.inf > > b/Platform/RiscV/Universal/Sec/SecMain.inf > > new file mode 100644 > > index 00000000..c408fc8d > > --- /dev/null > > +++ b/Platform/RiscV/Universal/Sec/SecMain.inf > > @@ -0,0 +1,75 @@ > > +## @file > > +# RISC-V SEC module. > > +# > > +# Copyright (c) 2019, Hewlett Packard Enterprise Development LP. All > > +rights reserved.
# # SPDX-License-Identifier: > > +BSD-2-Clause-Patent # ## > > + > > +[Defines] > > + INF_VERSION =3D 0x00010005 >=20 > Can this be updated to a recent specification version. >=20 > > + BASE_NAME =3D SecMain > > + FILE_GUID =3D df1ccef6-f301-4a63-9661-fc6030dc= c880 > > + MODULE_TYPE =3D SEC > > + VERSION_STRING =3D 1.0 > > + ENTRY_POINT =3D SecMain > > + > > +# > > +# The following information is for reference only and not required by= the > build tools. > > +# > > +# VALID_ARCHITECTURES =3D RISCV64 EBC >=20 > Pretty sure EBC is not valid here. >=20 > > +# > > + > > +[Sources] > > + SecMain.c > > + > > +[Sources.RISCV64] > > + Riscv64/SecEntry.s > > + > > +[Packages] > > + MdePkg/MdePkg.dec > > + MdeModulePkg/MdeModulePkg.dec > > + RiscVPkg/RiscVPkg.dec > > + Platform/RiscV/RiscVPlatformPkg.dec > > + > > +[LibraryClasses] > > + BaseLib > > + DebugLib > > + BaseMemoryLib > > + PcdLib > > + DebugAgentLib > > + IoLib > > + PeCoffLib > > + PeCoffGetEntryPointLib > > + PeCoffExtraActionLib > > + ExtractGuidedSectionLib > > + RiscVCpuLib > > + PrintLib > > + SerialPortLib > > + RiscVOpensbiLib > > + OpenSbiPlatformLib # This is required library which > > + # provides platform level opensbi > > + # functions. >=20 > I don't doubt it, but I can tell it's required from the fact that it is = listed. And > the name itself says all of the rest. The comment is superfluous and can= be > deleted. >=20 > > + > > +[Ppis] > > + gEfiTemporaryRamSupportPpiGuid # PPI ALWAYS_PRODUCED > > + gEfiTemporaryRamDonePpiGuid # PPI ALWAYS_PRODUCED > > + > > +[Guids] > > + gUefiRiscVMachineContextGuid > > + > > +[FixedPcd] > > + gUefiRiscVPlatformPkgTokenSpaceGuid.PcdRiscVPeiFvBase > > + gUefiRiscVPlatformPkgTokenSpaceGuid.PcdRiscVPeiFvSize > > + > > +[Pcd] > > + gUefiRiscVPlatformPkgTokenSpaceGuid.PcdFwStartAddress > > + gUefiRiscVPlatformPkgTokenSpaceGuid.PcdFwEndAddress > > + gUefiRiscVPlatformPkgTokenSpaceGuid.PcdHartCount > > + gUefiRiscVPlatformPkgTokenSpaceGuid.PcdBootHartId > > + gUefiRiscVPlatformPkgTokenSpaceGuid.PcdScratchRamBase > > + gUefiRiscVPlatformPkgTokenSpaceGuid.PcdScratchRamSize > > + gUefiRiscVPlatformPkgTokenSpaceGuid.PcdOpenSbiStackSize > > + gUefiRiscVPlatformPkgTokenSpaceGuid.PcdTemporaryRamBase > > + gUefiRiscVPlatformPkgTokenSpaceGuid.PcdTemporaryRamSize >=20 > Please sort the above alphabetically ( > apart from where it makes sense not to - for example PcdFwStartAddress > before PcdFwEndAddress makes more sense than the other way around). >=20 > / > Leif >=20 > > -- > > 2.12.0.windows.1 > > > > > > > > >=20 >=20