From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id C36072243693D for ; Fri, 23 Feb 2018 02:41:29 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 85D857C6A0; Fri, 23 Feb 2018 10:47:30 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-112.rdu2.redhat.com [10.10.120.112]) by smtp.corp.redhat.com (Postfix) with ESMTP id 51BBFB300D; Fri, 23 Feb 2018 10:47:29 +0000 (UTC) To: Udit Kumar , Leif Lindholm Cc: "michael.d.kinney@intel.com" , "edk2-devel@lists.01.org" , "ard.biesheuvel@linaro.org" 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> From: Laszlo Ersek Message-ID: <58b7fe3f-8e39-02a2-76a9-cf904280d298@redhat.com> Date: Fri, 23 Feb 2018 11:47:28 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.11.54.5 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.2]); Fri, 23 Feb 2018 10:47:30 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.2]); Fri, 23 Feb 2018 10:47:30 +0000 (UTC) for IP:'10.11.54.5' DOMAIN:'int-mx05.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' 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 10:41:30 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 02/23/18 11:25, Udit Kumar wrote: > > >> -----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=4 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 >> pointers? >> - 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 >> endianness, 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 BeIoLib 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 = 1; >> >> // >> // at function scope >> // >> if (*(CONST UINT8 *)&mOne == 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 >> // >> } > > You meant, sort of cpu_to_le and cpu_to_be sort of APIs I'm lost. I don't know how to put it any clearer. Let me try with actual code. (a) Suggested for "MdePkg/Library/BaseIoLibIntrinsic/IoLib.c", which is used on IA32 and X64, therefore CPU byte order is little endian only: > UINT32 > EFIAPI > MmioWrite32 ( > IN UINTN Address, > IN UINT32 Value > ) > { > ASSERT ((Address & 3) == 0); > > MemoryFence (); > *(volatile UINT32*)Address = Value; > MemoryFence (); > > return Value; > } In other words, no change to the current implementation. (b) Suggested for "MdePkg/Library/BaseBeIoLibIntrinsic/IoLib.c", also to be used on IA32 and X64. Because the CPU byte order is LE only, this variant will byte-swap unconditionally. > UINT32 > EFIAPI > BeMmioWrite32 ( > IN UINTN Address, > IN UINT32 Value > ) > { > ASSERT ((Address & 3) == 0); > > MemoryFence (); > *(volatile UINT32*)Address = SwapBytes32 (Value); > MemoryFence (); > > return Value; > } (c) Suggested for "MdePkg/Library/BaseIoLibIntrinsic/IoLibArm.c", which is used on ARM and AARCH64. And here I'll assume that the CPU byte order on those can be either LE or BE. > UINT32 > EFIAPI > MmioWrite32 ( > IN UINTN Address, > IN UINT32 Value > ) > { > ASSERT ((Address & 3) == 0); > *(volatile UINT32*)Address = (*(CONST UINT8 *)&mOne == 1) ? > Value : > SwapBytes32 (Value); > return Value; > } (d) Suggested for "MdePkg/Library/BaseBeIoLibIntrinsic/BeIoLibArm.c", which is to be used on ARM and AARCH64. And here I'll assume that the CPU byte order on those can be either LE or BE. > UINT32 > EFIAPI > BeMmioWrite32 ( > IN UINTN Address, > IN UINT32 Value > ) > { > ASSERT ((Address & 3) == 0); > *(volatile UINT32*)Address = (*(CONST UINT8 *)&mOne == 1) ? > SwapBytes32 (Value) : > Value; > return Value; > } Laszlo