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 742CE2244E403 for ; Mon, 16 Apr 2018 12:32:47 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 2A99340704AE; Mon, 16 Apr 2018 19:32:46 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-213.rdu2.redhat.com [10.10.120.213]) by smtp.corp.redhat.com (Postfix) with ESMTP id 1BA282026DFD; Mon, 16 Apr 2018 19:32:44 +0000 (UTC) To: Leif Lindholm Cc: "Kinney, Michael D" , "edk2-devel@lists.01.org" , "Gao, Liming" References: <20180413174211.858-1-leif.lindholm@linaro.org> <20180413193143.t45tua3yi7sopk4d@bivouac.eciton.net> <20180416100712.6v642ycksvmoffvt@bivouac.eciton.net> From: Laszlo Ersek Message-ID: Date: Mon, 16 Apr 2018 21:32:44 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <20180416100712.6v642ycksvmoffvt@bivouac.eciton.net> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.4 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.7]); Mon, 16 Apr 2018 19:32:46 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.7]); Mon, 16 Apr 2018 19:32:46 +0000 (UTC) for IP:'10.11.54.4' DOMAIN:'int-mx04.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [PATCH] MdePkg: add big-endian MMIO BaseBeIoLib X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 16 Apr 2018 19:32:47 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 04/16/18 12:07, Leif Lindholm wrote: > On Fri, Apr 13, 2018 at 11:32:35PM +0000, Kinney, Michael D wrote: >> Leif, >> >> I am curious why a Swap class/instances is not sufficient. >> >> Currently EDK II follows the UEFI/PI specs, which for all supported >> CPU architectures use little endian ABI. The BaseIoLib follows the >> endianness of the CPU. If UEFI/PI added a CPU that was big endian, I >> would expect BaseIoLib when built for that CPU would perform big >> endian operations. >> >> Am I missing something? > > If you did add a big-endian CPU, you could then find yourself in the > exact opposite situation and require a little-endian i/o access > library. Which would be implemented exactly as the contents of > IoLibSwap.c. > > The header file necessarily needs to be endianness-specific, and if > the coding style had permitted functions in header files, my automatic > reaction would have been to make all of these static inline helper > functions (even with the code duplication). First, to remind myself of the previous discussion (and please correct me if I remember incorrectly): whether swapping is needed or not depends on both CPU byte order and device byte order. However, the library API names that device drivers use should reflect device byte order *only* (regardless of CPU byte order). This way, (a) developers that write device drivers can focus on the devices, regardless of what CPU the driver is compiled for, (b) library classes for both LE and BE devices can be used together in the same driver module, (c) assuming we introduce a CPU with BE byte order, the same driver source will work (for both LE and BE devices), only the lib instances will have to be switched around. (This might even happen dynamically, via function pointers.) Now, after staring at this patch long and hard, here's my understanding. Crucially, you take the IoLib class to mean "it doesn't swap". You don't take it to mean "it talks to LE devices". This is evident from the source file "IoLibSwap.c", where you add the swapping on top of IoLib. That's fine, but it will have consequences: (1) We need a separate library class called IoSwapLib (not IoLibSwap), implemented under "MdePkg/Library/BaseIoSwapLib/BaseIoSwapLib.c", and without the preprocessor trickery. There should be one library instance only, adding nothing but byte swapping on top of IoLib. (2) We need separate library classes called BeIoLib and LeIoLib. These provide the ultimate APIs that I describe near the top. In total we should have four library instances that are explicit about device endianness. This means four INF files, and a single shared C source file, *with* the preprocessor trickery. (2.1) BaseBeIoLib.inf: implements the BeIoLib functions on top of IoLib, that is, without swapping -- this means that it is suitable for talking to BE devices on BE CPUs. (2.2) BaseBeIoLibSwap.inf: implements the BeIoLib functions on top of IoSwapLib -- talks to BE devices on LE CPUs. (2.3) BaseLeIoLib.inf: implements LeIoLib functions on top of IoLib -- talks to LE devices on LE CPUs. (2.4) BaseLeIoLibSwap.inf: implements LeIoLib functions on top of IoSwapLib -- talks to LE devices on BE CPUs. IMO, it's fine if you only want to add the BeIoLib class now, with its BaseBeIoLibSwap instance only. But the IoSwapLib and BeIoLib classes must exist separately nonetheless. And that's because your currently proposed C source file is unable to express case (2.1): you can generate the "Be" prefix alright, but the internals will always swap, and do that on top of IoLib (which *never* swaps). So you will end up swapping once in the Be*() functions, which is wrong for talking to BE devices on BE CPUs. I guess -- relaxing my initial point a bit -- it's also OK if you do not introduce the BeIoLib class at all (let alone LeIoLib), saying that drivers are generally expected to compute at startup whether they need to byte-swap or not (considering both CPU and device byte order), and then they need to flip a few function pointers between IoLib versus IoSwapLib for all further use. In that case however the patch should not say "Be" or "Le" in any lib class, instance, or API names, at all. Some other random comments I have: (3) You mention that no UNI file is being duplicated, but I do see one. (4) Please consider adopting https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-10 https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-23 so that patches roughly advance from "abstract" to "concrete". (5) You skipped the following family of functions: MmioBitField(Read|Write|Or|And|AndThenOr)(16|32|64). I don't think that's right: the mask values at the least are expressed in host byte order, and they'll need conversion. An earlier (independent) discussion was at: http://mid.mail-archive.com/48d7d1a9-503f-2a72-803d-f63a8bc67e9c@redhat.com (6) Please don't use "__" and "_C" prefixes for identifiers (see __CONCATENATE and _CONCATENATE); according to the C standard, "All identifiers that begin with an underscore and either an uppercase letter or another underscore are always reserved for any use". I know Linux uses "__" prefixes liberally; that doesn't make them any less wrong :) ... Obviously I don't insist on these patches being implemented "my way"; I'm stating my opinion because you CC'd me :) (Thanks for that!) Thanks, Laszlo