From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=104.47.0.43; helo=eur01-he1-obe.outbound.protection.outlook.com; envelope-from=meenakshi.aggarwal@nxp.com; receiver=edk2-devel@lists.01.org Received: from EUR01-HE1-obe.outbound.protection.outlook.com (mail-he1eur01on0043.outbound.protection.outlook.com [104.47.0.43]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 69BFF2243692F for ; Fri, 23 Feb 2018 01:41:41 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nxp.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version; bh=SQ+1N9iCPzOcTlgnB5TDGD4ZHzarbYS2qzHyZZNuDdU=; b=XNa/pGnXFS2QmTr3uBZDZ167652LX54JbacA5ctKQiJqz3SwgIguTD0iWe26pcI9afTcUui4q71Ma5mQNLA319mpzoz4KOte7mdZITLS5BIq9KvVaL6txJbpAv9rosqW77S8Zy8dUl6IZyWBDQiS+IddtRNXH/Bvnpkq+jM/Gbk= Received: from DB5PR04MB0998.eurprd04.prod.outlook.com (10.161.199.12) by DB5PR04MB1462.eurprd04.prod.outlook.com (10.162.221.144) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P256) id 15.20.506.18; Fri, 23 Feb 2018 09:47:39 +0000 Received: from DB5PR04MB0998.eurprd04.prod.outlook.com ([fe80::5b4:dfb7:891f:32ce]) by DB5PR04MB0998.eurprd04.prod.outlook.com ([fe80::5b4:dfb7:891f:32ce%13]) with mapi id 15.20.0506.023; Fri, 23 Feb 2018 09:47:38 +0000 From: Meenakshi Aggarwal To: Laszlo Ersek , Leif Lindholm CC: "michael.d.kinney@intel.com" , "edk2-devel@lists.01.org" , "ard.biesheuvel@linaro.org" , Udit Kumar , Pankaj Bansal Thread-Topic: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add support for Big Endian Mmio APIs Thread-Index: AQHTpwN6NYlxP1QRkk672pzWdh56TqOvCB6AgAAFmACAADAhAIAA4+6AgAA3Z4CAACKogIABOfYAgAALe4CAAAN74A== Date: Fri, 23 Feb 2018 09:47:38 +0000 Message-ID: References: <1518771035-6733-1-git-send-email-meenakshi.aggarwal@nxp.com> <1518771035-6733-2-git-send-email-meenakshi.aggarwal@nxp.com> <20180221154601.nkbp2xmy3zb2xolm@bivouac.eciton.net> <20180221185818.arwfhombntutnt23@bivouac.eciton.net> <20180222115223.xtfpc7du22drfkju@bivouac.eciton.net> <2a1fa56f-98db-a1c1-d973-7e84cc7dc1fa@redhat.com> <946bd4f7-ed57-018c-00ca-cee154fcb2f0@redhat.com> In-Reply-To: <946bd4f7-ed57-018c-00ca-cee154fcb2f0@redhat.com> Accept-Language: en-GB, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: spf=none (sender IP is ) smtp.mailfrom=meenakshi.aggarwal@nxp.com; x-originating-ip: [180.188.246.227] x-ms-publictraffictype: Email x-microsoft-exchange-diagnostics: 1; DB5PR04MB1462; 7:zMppK/mGOZSeeF0Zhgd4Oxv3s76QEdZAuESGbmfe4VKRn/xyRDo2qQNGBpUzUsn15ZBTW3+kEcnI95DEmAQ0ZF4g4bXQmNrS47gw2JXAE7id6YzQgxYxOod7Z8x1ScEtfHDOHBxjLGs799L4CPcBqCO7X4I0vKuFYShte6ARgcb08mTBYHwwIbqYryfIMKv5Tx1dETaOCNc8uubI0vjw0SV0aS+oWkgDdwXU0Rfp9dyRNiiWZ0khS0hZ5CB/gXzm x-ms-exchange-antispam-srfa-diagnostics: SSOS;SSOR; x-forefront-antispam-report: SFV:SKI; SCL:-1; SFV:NSPM; SFS:(10009020)(39380400002)(346002)(366004)(39860400002)(376002)(396003)(13464003)(52314003)(189003)(199004)(53754006)(81166006)(6116002)(26005)(3846002)(59450400001)(6506007)(966005)(53546011)(8676002)(14454004)(93886005)(186003)(8936002)(99286004)(3660700001)(45080400002)(3280700002)(74316002)(7736002)(305945005)(106356001)(478600001)(81156014)(2906002)(76176011)(86362001)(7696005)(102836004)(5250100002)(6246003)(2950100002)(229853002)(6306002)(9686003)(316002)(4326008)(2900100001)(53936002)(5660300001)(25786009)(54906003)(97736004)(55016002)(6436002)(66066001)(68736007)(110136005)(33656002)(105586002); DIR:OUT; SFP:1101; SCL:1; SRVR:DB5PR04MB1462; H:DB5PR04MB0998.eurprd04.prod.outlook.com; FPR:; SPF:None; PTR:InfoNoRecords; MX:1; A:1; LANG:en; x-ms-office365-filtering-ht: Tenant x-ms-office365-filtering-correlation-id: 33d8279a-0eb3-4a63-d8fb-08d57aa279cd x-microsoft-antispam: UriScan:; BCL:0; PCL:0; RULEID:(7020095)(4652020)(4534165)(4627221)(201703031133081)(201702281549075)(48565401081)(5600026)(4604075)(3008032)(2017052603307)(7153060)(7193020); SRVR:DB5PR04MB1462; x-ms-traffictypediagnostic: DB5PR04MB1462: x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:(189930954265078)(185117386973197)(162533806227266)(45079756050767)(228905959029699); x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(8211001083)(6040501)(2401047)(5005006)(8121501046)(3231183)(944501161)(52105095)(3002001)(93006095)(93001095)(10201501046)(6055026)(6041288)(20161123562045)(20161123558120)(201703131423095)(201702281528075)(20161123555045)(201703061421075)(201703061406153)(20161123560045)(20161123564045)(6072148)(201708071742011); SRVR:DB5PR04MB1462; BCL:0; PCL:0; RULEID:; SRVR:DB5PR04MB1462; x-forefront-prvs: 0592A9FDE6 received-spf: None (protection.outlook.com: nxp.com does not designate permitted sender hosts) x-microsoft-antispam-message-info: Z9JcXCCDQGKtISq6YKrO+ZhsZC2cONyDx6iK3u+K3Nz7yXmrP+cr7gYMvMlqxX9/QugRFlamDI1N+5KKFjabZfEruyRIqH53IBLx8SI4NMyn0Wor+NbAHcZTkU7dqrwX87+MPil07QnWmGRlLOoBpvn3lgsnk4BlpUpL8wEQs4o= spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM MIME-Version: 1.0 X-OriginatorOrg: nxp.com X-MS-Exchange-CrossTenant-Network-Message-Id: 33d8279a-0eb3-4a63-d8fb-08d57aa279cd X-MS-Exchange-CrossTenant-originalarrivaltime: 23 Feb 2018 09:47:38.7898 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635 X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB5PR04MB1462 Subject: Re: [PATCH edk2-platforms 01/39] Silicon/NXP: Add support for Big Endian Mmio APIs X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 23 Feb 2018 09:41:43 -0000 Content-Language: en-US Content-Type: text/plain; charset="Windows-1252" Content-Transfer-Encoding: quoted-printable > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of > Laszlo Ersek > Sent: Friday, February 23, 2018 2:51 PM > To: Pankaj Bansal ; Leif Lindholm > > Cc: michael.d.kinney@intel.com; edk2-devel@lists.01.org; > ard.biesheuvel@linaro.org > Subject: Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add support > for Big Endian Mmio APIs >=20 > On 02/23/18 09:40, Pankaj Bansal wrote: > > Hi All > > > >> -----Original Message----- > >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of > >> Laszlo Ersek > >> Sent: Thursday, February 22, 2018 7:26 PM > >> To: Leif Lindholm > >> Cc: michael.d.kinney@intel.com; edk2-devel@lists.01.org; > >> ard.biesheuvel@linaro.org > >> Subject: Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add > support > >> for Big Endian Mmio APIs > >> > >> On 02/22/18 12:52, Leif Lindholm wrote: > >>> On Thu, Feb 22, 2018 at 09:34:05AM +0100, Laszlo Ersek wrote: > >> > >>>>> But that brings back the complication as to how we have a driver > >>>>> that needs an LE IO library to write output, and a BE IO library to > >>>>> manipulate the hardware. > >>>> > >>>> Can you please explain the "write output" use case more precisely? > >>>> > >>>> My thinking would be this: > >>>> > >>>> - Use the IoLib class directly for "writing output" in little endian > >>>> byte order (which is still unclear to me sorry). > >>> > >>> If the IoLib class is mapped to a an instance that byte-swaps (hereto > >>> referred to as BeIoLib if IoLibSwap is unsuitable), would we not then > >>> end up mapping the non-swapping, currently implemented in > >>> BaseLibIoIntrinsic, variant as BeIoLib? Or if not, do we end up > >>> needing to duplicated all IoLib implementation .infs to provide an > >>> IoLib and a BeIoLib for each? > >>> > >>> It's at that point I burst an aneurysm. > >>> Am I overthinking/underthinking this? > >> > >> We need two library classes, one for talking to LE devices and another= to > BE > >> devices. These should be usable in a given module at the same time, as > Ard > >> says. > >> > >> Both library classes need to work on both LE and BE CPUs (working from > your > >> suggestion that UEFI might grow BE CPU support at some point). > >> Whether that is implemented by dumb, separate library instances > (yielding in > >> total 2*2=3D4 library instances), or by smart, CPU-endianness-agnostic > library > >> instances (in total, 2), is a different question. > >> > >> Note that such "smarts" could be less than trivial to implement: > >> - check CPU endianness in each library API? > >> - or check in the lib constructor only, and flip some function pointer= s? > >> - use a dynamic PCD for caching CPU endianness? > >> - use a HOB for the same? > >> - use a lib global variable (for caching only on the module level)? > >> > >> I think the solution that saves the most on the *source* code size is: > >> - introduce the BeIoLib class > >> - duplicate the MMIO functions from BaseIoLibIntrinsic to the one > >> BeIoLib instance that we introduce > >> - modify the MMIO functions in *both* lib instances (original LE, and > >> new BE), like this: > >> > >> - If the CPU architecture is known to be bound to a single endiannes= s, > >> then hardcode the appropriate operation. This can be done with > >> preprocessor macros, or with the architecture support of INF files= / > >> separate source files. For example, on IA32 and X64, the IoLib > >> instance should work transparently, unconditionally, and the BeIoL= ib > >> instance should byte-swap, unconditionally. > >> > >> - On other CPU arches, all the wider-than-byte MMIO functions, in > >> *both* lib instances should do something like this: > >> > >> // > >> // at file scope > >> // > >> STATIC CONST UINT16 mOne =3D 1; > >> > >> // > >> // at function scope > >> // > >> if (*(CONST UINT8 *)&mOne =3D=3D 1) { > >> // > >> // CPU in LE mode: > >> // - work transparently in the IoLib instance > >> // - byte-swap in the BeIoLib instance > >> // > >> } else { > >> // > >> // CPU in BE mode: > >> // - byte-swap in the IoLib instance > >> // - work transparently in the BeIoLib instance > >> // > >> } > > > > I suggest this approach : > > > > 1. Add BeMmio* functions in existing IoLib. BeMmio* functions will swap > the input before write and swap output after read and so on. > > Mmio* functions will not perform any byte swapping > > 2. create second instance (a copy) of this IoLib for CPUs that are Big = Endian. > We can call it BigEndianIoLib. > > In this library Mmio* functions will swap the input before write a= nd swap > output after read and so on. > > BeMmio* functions will not perform any byte swapping. > > 3. Include the instance of IoLib in dsc file based on cpu endianness th= at the > platform wants to use. i.e. > > If BIG_ENDIAN =3D=3D FALSE > > IoLib | ..\..\..\IoLib > > Else > > IoLib | ..\..\..\BigEndianIoLib > > 4. The devices that are Big endian in platform will always call BeMmio* > functions. They need not check CPU endianness. > > 5. The devices that are Little endian in platform will always call Mmio= * > functions. They need not check CPU endianness. >=20 > This can work too, but there is a downside: a large number of IoLib > instances exist in the tree already. If you add the BeMmio* functions to > the existent IoLib class, you'll have to duplicate the implementation to > all instances (identically, I think). >=20 > We've had this debate in the past. Back then it was about IoFifo > routines. I argued for an IoFifo lib class. Ultimately the IoFifo > routines were added to IoLib, and they had to be implemented for many > more library instances than client code would have actually required. > (See the series at 13a50a6fe1dc..2b631390f9f5.) In turn this runs the > risk of adding untested code. >=20 > Regarding the instances for BE CPUs: the name should likely be > BaseIoLibBigEndian or something similar. In lib instance names, the lib > class name is usually prefixed with the firmware phases where the > instance is usable, and hints about the implementation or constraints > are added as a suffix. >=20 > Also, if you want to support BE CPUs with separate IoLib instances, I'm > afraid that's going to lead to the combinatorial explosion that Leif > characterized as "burst[ing] an aneurysm". I think using the (seemingly > dynamic) "mOne" approach I suggested above, a smart compiler can > eliminate all the branches at build time. >=20 > Anyway, I don't insist; I just commented on an RFC. >=20 The implementation suggested by you is pretty clean. Just one concern on the naming convention,=20 STATIC CONST UINT16 mOne =3D 1; // // at function scope // if (*(CONST UINT8 *)&mOne =3D=3D 1) { // // CPU in LE mode: // - work transparently in the IoLib instance // - byte-swap in the BeIoLib instance // } else { // // CPU in BE mode: // - byte-swap in the IoLib instance // - work transparently in the BeIoLib instance // } If we keep it BeIoLib, then it is not justifying the case for BE CPUs. I mean, for a BE CPU architecture, it look weird to call BeIoLib APIs when = swap is needed. So, i think, we should name it IoLibSwap. > Laszlo=20 > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://emea01.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2Flist > s.01.org%2Fmailman%2Flistinfo%2Fedk2- > devel&data=3D02%7C01%7Cmeenakshi.aggarwal%40nxp.com%7C932b04d97a2 > 648845d9208d57a9ece38%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0 > %7C636549744880515349&sdata=3DSBrgvgbrsk3ViYygJdNIzy%2FuB19pWeSvHln > IMX6ae90%3D&reserved=3D0