From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from EUR05-AM6-obe.outbound.protection.outlook.com (EUR05-AM6-obe.outbound.protection.outlook.com [40.107.22.61]) by mx.groups.io with SMTP id smtpd.web10.5566.1588915917148689081 for ; Thu, 07 May 2020 22:31:57 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@nxp1.onmicrosoft.com header.s=selector2-nxp1-onmicrosoft-com header.b=ILC3XXaO; spf=pass (domain: oss.nxp.com, ip: 40.107.22.61, mailfrom: pankaj.bansal@oss.nxp.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=HjsSIj45A5ztSMHTQGq3WnKi66mjG00CBWu2a8Ps00Ek0VgqHreIZ+xmEhr8fmn0LBq6g5NHCwZAVOCxGum62l5l0avgqbvXUVN3w7OG+99i8ZWIQz8ycE1sdRkImAICt3L2J0weJHmQZpNbICku4k9KY7WqELRUrQ1lTPMamYtYKHy5uX+s/VqffImh1uTl3S1rBBFXPT2EWLzyx1/EJ6LpGBJLX7Jb2hNjFvW0giYPTkBoNp6SUnQ/kkbHPtyJspUf6FPy4CcK/pKwXhX7wesL67nU9jyTfnBRu4FIWMXy4LbW/Qh/lITqlGNe4LFyQVcpOUzIM84z/VDrZyzHRA== 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=4qVVlUCxWhAPSOSX9OxFat1r2GgKGV2FeuQCgoReEMs=; b=XsAL1i2gBIx2CpZnlc+wAZvt2zltma5e//YgUIOj9hpdNlDdiSXxZfpt3ZB7Plf+VD62ICGir0DDJy7wEvJ1mys0ktak2yTS2B8bpuxFrYuiCL4lkKn8ruYRonWy4J5Dm0LGcg1V20LIf0BGzhLZYHRKa+ti0B3I10+jGf2rSkeGReQTdaZRdHtXBrYFjmdHiUc8+YMsMLISCMVz0RWbtB9kT/l38E0k4jHAN75LxOG5tAwnmbufvNrLPMLXyjDYx9WuBolcj6dXvJQqhzlIdX3GWb5cIosYR3QzuZoRSEcwihJH37VejQRPwbTSByoooWjtWqE0tCDJzexRfhwiHA== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=oss.nxp.com; dmarc=pass action=none header.from=oss.nxp.com; dkim=pass header.d=oss.nxp.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=NXP1.onmicrosoft.com; s=selector2-NXP1-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=4qVVlUCxWhAPSOSX9OxFat1r2GgKGV2FeuQCgoReEMs=; b=ILC3XXaOJIu62LzBqdhBATeKVbVwtJj7h9mMzpTK6WdKUZk53x25joHg471p0lr0IgmuPFZGeCTrsU8VeXqbCqG+akxsQCjruobMiS3vndRKcrTeMCgvtN/9K59KlNkbr0/5Ec/XZOpFlwBpDuiAOQ0fwXEPw+77kwmJYMhlZv4= Received: from VI1PR04MB5933.eurprd04.prod.outlook.com (2603:10a6:803:ec::16) by VI1PR04MB5230.eurprd04.prod.outlook.com (2603:10a6:803:5f::15) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2958.27; Fri, 8 May 2020 05:31:53 +0000 Received: from VI1PR04MB5933.eurprd04.prod.outlook.com ([fe80::45c4:8846:5327:9513]) by VI1PR04MB5933.eurprd04.prod.outlook.com ([fe80::45c4:8846:5327:9513%7]) with mapi id 15.20.2958.034; Fri, 8 May 2020 05:31:53 +0000 From: "Pankaj Bansal" To: Leif Lindholm , "Pankaj Bansal (OSS)" CC: Meenakshi Aggarwal , Michael D Kinney , "devel@edk2.groups.io" , Varun Sethi , Samer El-Haj-Mahmoud , Jon Nettleton , Ard Biesheuvel Subject: Re: [PATCH edk2-platforms v4 12/24] Silicon/NXP: Move RAM retrieval from SocLib Thread-Topic: [PATCH edk2-platforms v4 12/24] Silicon/NXP: Move RAM retrieval from SocLib Thread-Index: AQHWJPn7wvyIN1Q98Umen5gG1xBA1Q== Date: Fri, 8 May 2020 05:31:53 +0000 Message-ID: References: <20200501054955.13025-1-pankaj.bansal@oss.nxp.com> <20200501054955.13025-13-pankaj.bansal@oss.nxp.com> <20200506104405.GL21486@vanye> <20200507101535.GQ21486@vanye> In-Reply-To: <20200507101535.GQ21486@vanye> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: nuviainc.com; dkim=none (message not signed) header.d=none;nuviainc.com; dmarc=none action=none header.from=oss.nxp.com; x-ms-exchange-messagesentrepresentingtype: 1 x-originating-ip: [49.36.129.135] x-ms-publictraffictype: Email x-ms-office365-filtering-ht: Tenant x-ms-office365-filtering-correlation-id: f8d06f94-51c9-4617-fc29-08d7f3111dec x-ms-traffictypediagnostic: VI1PR04MB5230:|VI1PR04MB5230: x-ms-exchange-sharedmailbox-routingagent-processed: True x-ms-exchange-transport-forked: True x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:10000; x-forefront-prvs: 039735BC4E x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: 0a2MFmIWpm4FlH7QhamxHh4rCBUPyMb/rEB2P15rQ0NuFl0BfgGR60kx2QQdE/hC4QWEMB7KLHOte6MsW2J+i6jQQf7CmVWvZZZ6tA+UkEQ/OlcZsCAr75G6lmrBfTVG7dJ35aTR/Z8gZItwObNbd5SWvTwV55eKPG55QkyjONvi42UYWaR21MOsEcZyLIoY+X+nk1e0Dabn85S4kMjw6STvhSI7eg11jk4BHRtXZtLoo196LjgS3xAdwhHGByaX8Tmsi/rrUKUJcTS/gnNOSUJetxe9l2nTMTI4EmXYvyOG7DfK73mspuh8DpOvW2ZzNa3NF+EW6F61b4tPh1VDxlpUGUYg4XSQFsQHmBQkfW7IWNpot7rct94+Dtcm6mJ48nx4mxcl6TX6vktS5hhYMprvFC4GLat0VWSFVanPcXvxrKZCq50fkDALg/sZzUFamcCKo9gw5cKqk7VW2VyZkjxFq8qrzbavaVT8zvHfMxRfaGvMAAJeOzWHpQXYq8XwT7i6/rSCq6p6YCmALMe0ieCd6PkwBMp9sdmMdxxB9ojeQMK3eXcdI2LOm+KubWGhnC5KYvqTCUf/rxlNdaGyp87dVTzPgLO6L3bq7028W6c= x-forefront-antispam-report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:VI1PR04MB5933.eurprd04.prod.outlook.com;PTR:;CAT:NONE;SFTY:;SFS:(4636009)(376002)(366004)(346002)(136003)(39860400002)(396003)(33430700001)(71200400001)(2906002)(66946007)(55016002)(6506007)(54906003)(26005)(53546011)(86362001)(5660300002)(966005)(33656002)(33440700001)(478600001)(66476007)(66556008)(4326008)(66446008)(19627235002)(9686003)(8676002)(76116006)(64756008)(7696005)(52536014)(110136005)(83320400001)(83280400001)(83310400001)(83290400001)(186003)(316002)(8936002)(83300400001);DIR:OUT;SFP:1101; x-ms-exchange-antispam-messagedata: ETlHTZLGlCNK35lelEJDXwW69roopWU4JsLokcgkn/PibnWl8rj8/0FsdSTajwA60OCIz/+l97ZWAWPcnXaKJhPvMndjtFyGiicMZNtt9N6C26lsE4NI0IQUcQ4hgQYBGx6NGLhFae1q7RdYM0R7uGHe8+KE2kEQM4TdIMIjHJYhV7opMtjrtZsuFR8/LibHln6Rbe1EaMRVW7o+p1XmQ3mfGDO6V14RiRbi3ZHLZhkKCtImKRBiO5avgMMLwOctYtmN1C4A6X7RgZee9w4s2N+kcrtgTaitGD9qgo79Iz8zJi9v9QGLpnvWV601NrkNzttKnvthBk5ebc+cp481kAQQ5mpBj4EEbZMk1GmFJeTD16Z71QlzCIJT8FJLYPPcr3jsFl/c7kwSyoO5Vm3elKBDakaeOEn2euVQ0GpkUMspjR3oHyF2pHb2FMlyhuspchvj4qyGtW2TzyWNhzCatfUHjQjyB6ytfU0bmzvlNzY= MIME-Version: 1.0 X-OriginatorOrg: oss.nxp.com X-MS-Exchange-CrossTenant-Network-Message-Id: f8d06f94-51c9-4617-fc29-08d7f3111dec X-MS-Exchange-CrossTenant-originalarrivaltime: 08 May 2020 05:31:53.7538 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635 X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: GO6wTiClNi31SEpe1LnlbfM9bjNXGx1VjEHZdDc9lsB624MYyIZg6qh4qrG6b0FoXM9Uoaqm0Tk2zjoE9ML0VQ== X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR04MB5230 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable >=20 > On Thu, May 07, 2020 at 07:28:30 +0000, Pankaj Bansal (OSS) wrote: > > Hi Leif, > > > > > > + ARM_SMC_ARGS ArmSmcArgs; > > > > > > > > -Routine Description: > > > > + ArmSmcArgs.Arg0 =3D SMC_DRAM_BANK_INFO; > > > > + ArmSmcArgs.Arg1 =3D -1; > > > > > > Should this be SMC_UNK? > > > > No. SMC_OK / SMC_UNK is returned values. > > While x0, x1 are arguments. > > I have explained this in the MemoryInitPeiLib.h >=20 > OK, then that -1 needs a separate #define. OK. That I will take care. >=20 > > // This SMC call works in this way: > > // x1 =3D -1 : return x0: SMC_OK, x1: total DDR Ram size > > // x1 >=3D number of DRAM regions to which DDR RAM is mapped : return x= 0: > SMC_UNK > > // 0 <=3D x1 < number of DRAM regions to which DDR RAM is mapped : retu= rn > > // x0: SMC_OK, x1: Base address of DRAM region, > > // x2: Size of DRAM region > > > > > > > > > > > > > + ArmCallSmc (&ArmSmcArgs); > > > > > > > > + if (ArmSmcArgs.Arg0 =3D=3D SMC_OK) { > > > > + return ArmSmcArgs.Arg1; > > > > + } > > > > > > > > .... > > > > > > - { > > > > - Found =3D TRUE; > > > > - break; > > > > - } > > > > - NextHob.Raw =3D GET_NEXT_HOB (NextHob); > > > > + Status =3D GetDramRegionsInfo (DramRegions, ARRAY_SIZE > (DramRegions)); > > > > + ASSERT_EFI_ERROR (Status); > > > > > > Slightly concerned here because we end up with a variable Status that > > > is only *used* in DEBUG builds. That could lead to toolchain warnings= . > > > > I don't think this would cause build warnings in RELEASE builds. I have= tested it. > > Also the Status is frequently being handled in this way in other places= in edk2: > > > > > https://github.com/tianocore/edk2/blob/master/ArmPlatformPkg/PlatformPei/ > PlatformPeim.c#L90 >=20 > The example you point to sets Status, overwrites it (once or twice > depending on conditionals), then returns it. > This patch in its current form sets Status, accesses it only in DEBUG > builds and does not return it. Ok. Then I can return status at the end of this function (MemoryPeim). I still can't bring myself to just ignore the critical error status from a = function. >=20 > > > And I was completely serious last time around, I am OK with the retur= n > > > value being cast away explicitly. What I meant with that is: > > > (VOID)GetDramRegionsInfo (DramRegions, ARRAY_SIZE (DramRegions)); > > > > I agreed with your suggestion to return EFI_BUFFER_TOO_SMALL from > GetDramRegionsInfo, > > But If we discard the return value it means we are OK with some RAM not > being reported to UEFI firmware > > and subsequently to OS. Isn't this a critical error ? >=20 > ASSERTs are only triggered in DEBUG builds, and send the processor > into WFI. >=20 > If it is a critical error (is it critical if you have found some RAM, > but been unable to fully reconcile all of the RAM in the system?), it > should do more than that. >=20 > I am all for properly handling that situation, but this patch has > never done that. Feel free to rework before submitting v5, or leave it > until (if) adding ACPI support and report the condition properly > through BERT. I have referred reference platform lib https://github.com/tianocore/edk2/bl= ob/master/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLibConstr= uctor.c#L83 As per this lib, I have added ASSERTS in all the scenarios, which are criti= cal. I thought ASSERT means we will halt the program execution regardless of DEB= UG or RELEASE build. Can you point me to a reference platform which handles these errors using A= CPI BERT methods ? I see that in DebugLib.h :=20 Note that a reserved macro named MDEPKG_NDEBUG is introduced for the inte= ntion of size reduction when compiler optimization is disabled. If MDEPKG_NDEBU= G is defined, then debug and assert related macros wrapped by it are the NULL = implementations. We have NOT defined MDEPKG_NDEBUG in our platforms for RELEASE or DEBUG bui= lds. Up until we have not implemented the ACPI BERT methods, can we keep it this= way to avoid masking the errors? >=20 > / > Leif