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 7A30821E1452A for ; Wed, 21 Feb 2018 07:50:57 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id D72AD40201A0; Wed, 21 Feb 2018 15:56:55 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-45.rdu2.redhat.com [10.10.120.45]) by smtp.corp.redhat.com (Postfix) with ESMTP id DBADB21411B6; Wed, 21 Feb 2018 15:56:53 +0000 (UTC) To: Leif Lindholm Cc: edk2-devel@lists.01.org, ard.biesheuvel@linaro.org, Liming Gao , Michael D Kinney References: <20180221152525.23449-1-leif.lindholm@linaro.org> From: Laszlo Ersek Message-ID: <29ec5d11-f1be-6058-8d7f-1f4548be8fe8@redhat.com> Date: Wed, 21 Feb 2018 16:56:53 +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: <20180221152525.23449-1-leif.lindholm@linaro.org> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.6 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.6]); Wed, 21 Feb 2018 15:56:55 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.6]); Wed, 21 Feb 2018 15:56:55 +0000 (UTC) for IP:'10.11.54.6' DOMAIN:'int-mx06.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [RFC PATCH] MdePkg: add byte-swapping MMIO BaseIoLibSwap 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: Wed, 21 Feb 2018 15:50:58 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 02/21/18 16:25, Leif Lindholm wrote: > When performing MMIO to a destination of the opposite endianness to the > executing processor, this library provides automatic byte order reversal > on inputs and outputs. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Leif Lindholm > --- > > As promised in the dark and distant past: > https://www.mail-archive.com/edk2-devel@lists.01.org/msg33447.html > > This starts out as a clone of MdePkg/Include/Library/IoLib.h, > implementing simple wrappers that depend on an IoLib instance to perform > any actual accesses. > > Comments are mostly nonsense, since they've not been changed from their > originals in IoLib.h. If people feel this library is a sensible approach, > I'll fix that up before I send a v1. > > I have explicitly excluded: > - non-MMIO accesses (could you really end up with mixed endianness in > such a system?) > - bitfields (would either require duplicating code or merging this all > into the BaseIoLibIntrinsic module, which is already a bit on the > bloated side) > - buffers operations (let's avoid those until we find we need them) > > MdePkg/Include/Library/IoLibSwap.h | 395 ++++++++++++++++++++ > MdePkg/Library/BaseIoLibSwap/BaseIoLibSwap.inf | 44 +++ > MdePkg/Library/BaseIoLibSwap/BaseIoLibSwap.uni | 23 ++ > MdePkg/Library/BaseIoLibSwap/IoLibSwap.c | 491 +++++++++++++++++++++++++ > MdePkg/MdePkg.dec | 3 + > MdePkg/MdePkg.dsc | 1 + > 6 files changed, 957 insertions(+) > create mode 100644 MdePkg/Include/Library/IoLibSwap.h > create mode 100644 MdePkg/Library/BaseIoLibSwap/BaseIoLibSwap.inf > create mode 100644 MdePkg/Library/BaseIoLibSwap/BaseIoLibSwap.uni > create mode 100644 MdePkg/Library/BaseIoLibSwap/IoLibSwap.c UEFI drivers are supposed to go through PciIo instances in order to access config space and MMIO registers of PCI devices (and I assume most devices are PCI), and the related PciIo member functions are LE-only. Thus, in such device drivers (for the exceptional(?) PCI device that has custom BE registers), the byte swapping will have to occur at the call sites (or at least higher-level wrapper functions). Which makes me think that this library is primarily for BE platform devices that are exposed via DXE drivers or (maybe) PEIMs. Is that corect? If so, can you please state it in the commit message? Also I would suggest calling the new functions "BE" or "BigEndian" or something similar -- once we turn this into a lib class, byte swapping becomes an implementation detail. The API should reflect the goal, which is "talking to BE platform devices". The @file comments should be updated similarly, IMO; "non-native endianness" should simply be "big endian", and the swapping language should be dropped -- for PI/UEFI, only LE is native (it is hard-coded by the spec(s), IIRC). ... Another superficial comment: in the INF file, there's a commented out DebugLib entry under [LibraryClasses]; I guess that's unintended. To clarify, I'm neutral on this library, I'd just like its documentation to be clear on when someone should consider using it. Thanks! Laszlo