From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ua0-x230.google.com (mail-ua0-x230.google.com [IPv6:2607:f8b0:400c:c08::230]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 32F7921DFA7BB for ; Wed, 5 Apr 2017 02:16:34 -0700 (PDT) Received: by mail-ua0-x230.google.com with SMTP id g30so4273791uab.0 for ; Wed, 05 Apr 2017 02:16:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=philjordan-eu.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=40dIdX6P/+6NjR7Gm9COW6sqXJETNw7588GZqxVFk7w=; b=utUWblsxk6RSURKNIbDQxv2j5MSpY6MlptWoaRPVFxZhiEcZlSQjFUU9X06HvoWmFi TU2saDVmPBgFm/2XF/BPuKt0Q5Kkk2IpGAgBlcfNWo5jiOlTd+oFVDPRK2yYd27FWqmv sc5IO4CWmNeuqYFmTLCaT5sVd0koXVTkY3owdp9y7rM2rcC5lx6Hv+2HfHZDqM3zT78y IrwVzAyScl07cEwM/Z9oipw3pwnvVgrHAffNFIBGFRp2bWy/c2PuzEwAYEMSbOjTYfbD 3W1bHHlERecubZPpzgiRje70RLp8zB+x8G/4uUixxOg30X2u36cPLdgIGkyxYzlkrmWt pz8w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=40dIdX6P/+6NjR7Gm9COW6sqXJETNw7588GZqxVFk7w=; b=qnNLlurbwgu63Wjm2+oR/6FrmnO9toPzHuOT0qCVnV6+44nlkwHr0SgcUit7dW4T50 8iguiUtUNbNd/IKIYUThXIvpsTIuyIMe6WBNSI/60J4QRKbgTcELimJAHhs7yPnSg2gx f6zYYnAzxF4cFA5zIvKeaJhAhXxudJZkBD1Ayyvb43gMB4HOlA2imTkk0lg/Jrpu3mgq c4OaT/hBW2bWtEG6Gb5s8j+N/ehi6q4uZ/co/0gG6c0uwqeh6tqlM4xPNIoJn3by3yvc vgLSwZxGSFQg9NNieee1AM4lZxG0RdYrYY7jd0EOIVEqxTNbiq9MS9E4wQcUzV5Yfd64 jceA== X-Gm-Message-State: AFeK/H1xCD8izW2OiXX8M1NJJPNHzWaq8bgDhCGgRMKptlhxSLA5oUVQ37FxBxRnIYHRB/O6t6xXaRfLeZFvNg== X-Received: by 10.176.16.83 with SMTP id g19mr509518uab.168.1491383792609; Wed, 05 Apr 2017 02:16:32 -0700 (PDT) MIME-Version: 1.0 Received: by 10.31.58.83 with HTTP; Wed, 5 Apr 2017 02:16:12 -0700 (PDT) In-Reply-To: <13cff49c-1034-eca5-5ad1-7d3d4e8bc6d7@redhat.com> References: <1491173097-37305-1-git-send-email-lists@philjordan.eu> <1491173097-37305-3-git-send-email-lists@philjordan.eu> <13cff49c-1034-eca5-5ad1-7d3d4e8bc6d7@redhat.com> From: Phil Dennis-Jordan Date: Wed, 5 Apr 2017 21:16:12 +1200 Message-ID: To: Laszlo Ersek Cc: edk2-devel-01 , Jordan Justen , Phil Dennis-Jordan Subject: Re: [PATCH v2 2/3] OvmfPkg/QemuVideoDxe: Helper functions for unaligned port I/O. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 05 Apr 2017 09:16:34 -0000 Content-Type: text/plain; charset=UTF-8 On Tue, Apr 4, 2017 at 8:19 PM, Laszlo Ersek wrote: > On 04/03/17 00:44, Phil Dennis-Jordan wrote: >> From: Phil Dennis-Jordan >> >> The VMWare SVGA2 display device implemented by Qemu (-vga vmware) uses >> an I/O-type BAR which is laid out such that some register offsets are >> not aligned to the read/write width with which they are expected to be >> accessed. (The register value port has an offset of 1 and requires >> 32 bit wide read/write access.) >> >> The EFI_PCI_IO_PROTOCOL's Pci.Read/Pci.Write functions do not support >> such unaligned I/O. > > Pci.Read and Pci.Write are for accessing config space; I think you mean > Io.Read and Io.Write. > >> >> Before a driver for this device can be added to QemuVideoDxe, helper >> functions for unaligned I/O are therefore required. This adds the >> functions UnalignedIoWrite32 and UnalignedIoRead32, based on IoLib's >> IoWrite32 and Read32, for the Ia32 and X64 architectures. Port I/O > > s/Read32/IoRead32/ I believe > >> requires inline assembly, so implementations are provided for the GCC, >> ICC, and Microsoft compiler families. Such I/O is not possible on other >> architectures, a dummy (ASSERT()ing) implementation is therefore >> provided to satisfy the linker. >> >> Cc: Jordan Justen >> Cc: Laszlo Ersek > > Please add here: > > Suggested-by: Jordan Justen > > as the idea comes from Jordan (from 4 years ago or so); is that correct? > >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Phil Dennis-Jordan >> --- >> >> Notes: >> v2: >> - Separate commit for the unaligned I/O helper functions. [Laszlo] >> - Dummy implementations return values despite ASSERT(). [Laszlo] >> - Build failure in ArmVirtPkg fixed. [Laszlo] >> - More consistent API docs and function ordering. >> >> OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf | 6 ++ >> OvmfPkg/QemuVideoDxe/UnalignedIoInternal.h | 59 ++++++++++++++ >> OvmfPkg/QemuVideoDxe/UnalignedIoGcc.c | 69 +++++++++++++++++ >> OvmfPkg/QemuVideoDxe/UnalignedIoIcc.c | 79 +++++++++++++++++++ >> OvmfPkg/QemuVideoDxe/UnalignedIoMsc.c | 81 ++++++++++++++++++++ >> OvmfPkg/QemuVideoDxe/UnalignedIoUnsupported.c | 65 ++++++++++++++++ >> 6 files changed, 359 insertions(+) >> >> diff --git a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf >> index affb6ffd88e0..346a5aed94fa 100644 >> --- a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf >> +++ b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf >> @@ -41,6 +41,12 @@ [Sources.common] >> >> [Sources.Ia32, Sources.X64] >> VbeShim.c >> + UnalignedIoGcc.c | GCC >> + UnalignedIoMsc.c | MSFT >> + UnalignedIoIcc.c | INTEL >> + >> +[Sources.IPF, Sources.EBC, Sources.ARM, Sources.AARCH64] >> + UnalignedIoUnsupported.c >> >> [Packages] >> MdePkg/MdePkg.dec >> diff --git a/OvmfPkg/QemuVideoDxe/UnalignedIoInternal.h b/OvmfPkg/QemuVideoDxe/UnalignedIoInternal.h >> new file mode 100644 >> index 000000000000..a069f3b98087 >> --- /dev/null >> +++ b/OvmfPkg/QemuVideoDxe/UnalignedIoInternal.h >> @@ -0,0 +1,59 @@ >> +/** @file >> + Unaligned port I/O, with implementations for various x86 compilers and a dummy >> + for platforms which do not support unaligned port I/O. >> + >> + Copyright (c) 2017, Phil Dennis-Jordan.
>> + This program and the accompanying materials >> + are licensed and made available under the terms and conditions of the BSD License >> + which accompanies this distribution. The full text of the license may be found at >> + http://opensource.org/licenses/bsd-license.php. >> + >> + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, >> + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. >> + >> +**/ > > Can you please rewrap all new files added in this commit to 79 > characters? (Even comments that you are copying from under MdePkg.) > >> + >> +#ifndef _UNALIGNED_IO_INTERNAL_H_ >> +#define _UNALIGNED_IO_INTERNAL_H_ >> + >> +/** >> + Performs a 32-bit write to the specified, possibly unaligned I/O-type address. >> + >> + Writes the 32-bit I/O port specified by Port with the value specified by Value >> + and returns Value. This function must guarantee that all I/O read and write >> + operations are serialized. >> + >> + If 32-bit unaligned I/O port operations are not supported, then ASSERT(). >> + >> + @param[in] Port I/O port address >> + @param[in] Value 32-bit word to write >> + >> + @return The value written to the I/O port. >> + >> +**/ >> +UINT32 >> +UnalignedIoWrite32 ( >> + IN UINTN Port, >> + IN UINT32 Value >> + ); >> + >> +/** >> + Reads 32-bit word from the specified, possibly unaligned I/O-type address. >> + >> + Reads the 32-bit I/O port specified by Port. The 32-bit read value is returned. >> + This function must guarantee that all I/O read and write operations are >> + serialized. >> + >> + If 32-bit unaligned I/O port operations are not supported, then ASSERT(). >> + >> + @param[in] Port I/O port from which to read. >> + >> + @return The value read from the specified location. >> + >> +**/ >> +UINT32 >> +UnalignedIoRead32 ( >> + IN UINTN Port >> + ); >> + >> +#endif >> diff --git a/OvmfPkg/QemuVideoDxe/UnalignedIoGcc.c b/OvmfPkg/QemuVideoDxe/UnalignedIoGcc.c >> new file mode 100644 >> index 000000000000..8bb74c784c06 >> --- /dev/null >> +++ b/OvmfPkg/QemuVideoDxe/UnalignedIoGcc.c >> @@ -0,0 +1,69 @@ >> +/** @file >> + Unaligned Port I/O. This file has compiler specifics for GCC as there is no >> + ANSI C standard for doing IO. >> + >> + Based on IoLibGcc.c. >> + >> + Copyright (c) 2006 - 2010, Intel Corporation. All rights reserved.
>> + This program and the accompanying materials >> + are licensed and made available under the terms and conditions of the BSD License >> + which accompanies this distribution. The full text of the license may be found at >> + http://opensource.org/licenses/bsd-license.php. >> + >> + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, >> + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. >> + >> +**/ >> + >> + >> +#include "UnalignedIoInternal.h" >> + >> +/** >> + Performs a 32-bit write to the specified, possibly unaligned I/O-type address. >> + >> + Writes the 32-bit I/O port specified by Port with the value specified by Value >> + and returns Value. This function must guarantee that all I/O read and write >> + operations are serialized. >> + >> + If 32-bit unaligned I/O port operations are not supported, then ASSERT(). >> + >> + @param[in] Port I/O port address >> + @param[in] Value 32-bit word to write >> + >> + @return The value written to the I/O port. >> + >> +**/ >> +UINT32 >> +UnalignedIoWrite32 ( >> + IN UINTN Port, >> + IN UINT32 Value >> + ) >> +{ >> + __asm__ __volatile__ ( "outl %0, %1" : : "a"(Value), "Nd"((UINT16)Port) ); > > Please insert a space after each second quote character: > > "a" (Value) > > "Nd" ((UINT16)Port) > > Also, a question: what does the N character (constraint?) do in the > input operand specification? I tried to check the gcc inline assembly > docs at , and I > couldn't find it. Thanks. The N constraint is x86-specific and indicates an 8-bit unsigned immediate value can be used as the operand. This enables emitting the OUT imm8, EAX instruction variant, see https://gcc.gnu.org/onlinedocs/gcc/Machine-Constraints.html In practice, this is most likely not going to happen here, even if link time optimisation happens to inline the call, as the port number isn't hardcoded. I can remove it if you like. >> + return Value; >> +} >> + >> +/** >> + Reads a 32-bit word from the specified, possibly unaligned I/O-type address. >> + >> + Reads the 32-bit I/O port specified by Port. The 32-bit read value is returned. >> + This function must guarantee that all I/O read and write operations are >> + serialized. >> + >> + If 32-bit unaligned I/O port operations are not supported, then ASSERT(). >> + >> + @param[in] Port I/O port from which to read. >> + >> + @return The value read from the specified location. >> + >> +**/ >> +UINT32 >> +UnalignedIoRead32 ( >> + IN UINTN Port >> + ) >> +{ >> + UINT32 Data; >> + __asm__ __volatile__ ( "inl %1, %0" : "=a"(Data) : "Nd"((UINT16)Port) ); >> + return Data; >> +} >> + > > Same comment about inserting spaces. > >> diff --git a/OvmfPkg/QemuVideoDxe/UnalignedIoIcc.c b/OvmfPkg/QemuVideoDxe/UnalignedIoIcc.c >> new file mode 100644 >> index 000000000000..ac365a8b6be5 >> --- /dev/null >> +++ b/OvmfPkg/QemuVideoDxe/UnalignedIoIcc.c >> @@ -0,0 +1,79 @@ >> +/** @file >> + Unaligned port I/O. This file has compiler specifics for ICC as there >> + is no ANSI C standard for doing IO. >> + >> + Based on IoLibIcc.c. >> + >> + Copyright (c) 2006 - 2008, Intel Corporation. All rights reserved.
>> + This program and the accompanying materials are >> + licensed and made available under the terms and conditions of the BSD License >> + which accompanies this distribution. The full text of the license may be found at >> + http://opensource.org/licenses/bsd-license.php. >> + >> + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, >> + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. >> + >> +**/ >> + >> + >> +#include "UnalignedIoInternal.h" >> + >> +/** >> + Performs a 32-bit write to the specified, possibly unaligned I/O-type address. >> + >> + Writes the 32-bit I/O port specified by Port with the value specified by Value >> + and returns Value. This function must guarantee that all I/O read and write >> + operations are serialized. >> + >> + If 32-bit unaligned I/O port operations are not supported, then ASSERT(). >> + >> + @param Port The I/O port to write. >> + @param Value The value to write to the I/O port. >> + >> + @return The value written the I/O port. >> + >> +**/ >> +UINT32 >> +UnalignedIoWrite32 ( >> + IN UINTN Port, >> + IN UINT32 Value >> + ) >> +{ >> + __asm { >> + mov eax, dword ptr [Value] >> + mov dx, word ptr [Port] >> + out dx, eax >> + } >> + >> + return Value; >> +} >> + >> +/** >> + Reads a 32-bit word from the specified, possibly unaligned I/O-type address. >> + >> + Reads the 32-bit I/O port specified by Port. The 32-bit read value is returned. >> + This function must guarantee that all I/O read and write operations are >> + serialized. >> + >> + If 32-bit unaligned I/O port operations are not supported, then ASSERT(). >> + >> + @param Port The I/O port to read. >> + >> + @return The value read. >> + >> +**/ >> +UINT32 >> +UnalignedIoRead32 ( >> + IN UINTN Port >> + ) >> +{ >> + UINT32 Data; >> + >> + __asm { >> + mov dx, word ptr [Port] >> + in eax, dx >> + mov dword ptr [Data], eax >> + } >> + >> + return Data; >> +} > > OK, these appear to be verbatim copies from > "MdePkg/Library/BaseIoLibIntrinsic/IoLibIcc.c". > >> diff --git a/OvmfPkg/QemuVideoDxe/UnalignedIoMsc.c b/OvmfPkg/QemuVideoDxe/UnalignedIoMsc.c >> new file mode 100644 >> index 000000000000..2eda40a47e2b >> --- /dev/null >> +++ b/OvmfPkg/QemuVideoDxe/UnalignedIoMsc.c >> @@ -0,0 +1,81 @@ >> +/** @file >> + Unaligned port I/O. This file has compiler specifics for Microsoft C as there >> + is no ANSI C standard for doing IO. >> + >> + Based on IoLibMsc.c >> + >> + Copyright (c) 2006 - 2010, Intel Corporation. All rights reserved.
>> + This program and the accompanying materials >> + are licensed and made available under the terms and conditions of the BSD License >> + which accompanies this distribution. The full text of the license may be found at >> + http://opensource.org/licenses/bsd-license.php. >> + >> + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, >> + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. >> + >> +**/ >> + >> + >> +#include "UnalignedIoInternal.h" >> + >> +unsigned long _inpd (unsigned short port); >> +unsigned long _outpd (unsigned short port, unsigned long dataword ); >> +void _ReadWriteBarrier (void); >> + >> +/** >> + Performs a 32-bit write to the specified, possibly unaligned I/O-type address. >> + >> + Writes the 32-bit I/O port specified by Port with the value specified by Value >> + and returns Value. This function must guarantee that all I/O read and write >> + operations are serialized. >> + >> + If 32-bit I/O port operations are not supported, then ASSERT(). >> + If Port is not aligned on a 32-bit boundary, then ASSERT(). >> + >> + @param Port The I/O port to write. >> + @param Value The value to write to the I/O port. >> + >> + @return The value written to the I/O port. >> + >> +**/ >> +UINT32 >> +EFIAPI > > Please drop EFIAPI here. Your internal header file doesn't specify it > (which is fine, as it is not a public library interface), so we > shouldn't add EFIAPI here either (even if it would indeed compile, as > this file is for VS only, and there EFIAPI is the only / default calling > convention). > >> +UnalignedIoWrite32 ( >> + IN UINTN Port, >> + IN UINT32 Value >> + ) >> +{ >> + _ReadWriteBarrier (); >> + _outpd ((UINT16)Port, Value); >> + _ReadWriteBarrier (); >> + return Value; >> +} >> + >> +/** >> + Reads a 32-bit word from the specified, possibly unaligned I/O-type address. >> + >> + Reads the 32-bit I/O port specified by Port. The 32-bit read value is returned. >> + This function must guarantee that all I/O read and write operations are >> + serialized. >> + >> + If 32-bit I/O port operations are not supported, then ASSERT(). >> + If Port is not aligned on a 32-bit boundary, then ASSERT(). >> + >> + @param Port The I/O port to read. >> + >> + @return The value read. >> + >> +**/ >> +UINT32 >> +EFIAPI >> +UnalignedIoRead32 ( >> + IN UINTN Port >> + ) >> +{ >> + UINT32 Value; >> + >> + _ReadWriteBarrier (); >> + Value = _inpd ((UINT16)Port); >> + _ReadWriteBarrier (); >> + return Value; >> +} > > Seems OK. (We'll only know for sure if someone builds this on VS :)) > >> diff --git a/OvmfPkg/QemuVideoDxe/UnalignedIoUnsupported.c b/OvmfPkg/QemuVideoDxe/UnalignedIoUnsupported.c >> new file mode 100644 >> index 000000000000..1d37ecb7bec0 >> --- /dev/null >> +++ b/OvmfPkg/QemuVideoDxe/UnalignedIoUnsupported.c >> @@ -0,0 +1,65 @@ >> +/** @file >> + Unaligned port I/O dummy implementation for platforms which do not support it. >> + >> + Copyright (c) 2017, Phil Dennis-Jordan.
>> + This program and the accompanying materials >> + are licensed and made available under the terms and conditions of the BSD License >> + which accompanies this distribution. The full text of the license may be found at >> + http://opensource.org/licenses/bsd-license.php. >> + >> + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, >> + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. >> + >> +**/ >> + >> + >> +#include "UnalignedIoInternal.h" >> +#include >> + >> +/** >> + Performs a 32-bit write to the specified, possibly unaligned I/O-type address. >> + >> + Writes the 32-bit I/O port specified by Port with the value specified by Value >> + and returns Value. This function must guarantee that all I/O read and write >> + operations are serialized. >> + >> + If 32-bit unaligned I/O port operations are not supported, then ASSERT(). >> + >> + @param[in] Port I/O port address >> + @param[in] Value 32-bit word to write >> + >> + @return The value written to the I/O port. >> + >> +**/ >> +UINT32 >> +UnalignedIoWrite32 ( >> + IN UINTN Port, >> + IN UINT32 Value >> + ) >> +{ >> + ASSERT (FALSE); >> + return 0; > > Well, not really relevant, but I still suggest to return Value, not 0. > >> +} >> + >> +/** >> + Reads a 32-bit word from the specified, possibly unaligned I/O-type address. >> + >> + Reads the 32-bit I/O port specified by Port. The 32-bit read value is returned. >> + This function must guarantee that all I/O read and write operations are >> + serialized. >> + >> + If 32-bit unaligned I/O port operations are not supported, then ASSERT(). >> + >> + @param[in] Port I/O port from which to read. >> + >> + @return The value read from the specified location. >> + >> +**/ >> +UINT32 >> +UnalignedIoRead32 ( >> + IN UINTN Port >> + ) >> +{ >> + ASSERT (FALSE); >> + return 0; >> +} >> > > With those changes: > > Reviewed-by: Laszlo Ersek Thanks for the detailed review Laszlo, I've gone through and implemented all of your suggestions and requests and will be posting a new version of the patch series shortly. Phil > Jordan, do you have any comments? (For the whole series too, of course?) > > Thanks > Laszlo