From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (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 491D521939308 for ; Tue, 4 Apr 2017 01:19:09 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id A18E621742; Tue, 4 Apr 2017 08:19:08 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com A18E621742 Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=lersek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com A18E621742 Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-91.phx2.redhat.com [10.3.116.91]) by smtp.corp.redhat.com (Postfix) with ESMTP id 27E417812D; Tue, 4 Apr 2017 08:19:06 +0000 (UTC) To: Phil Dennis-Jordan , edk2-devel@lists.01.org References: <1491173097-37305-1-git-send-email-lists@philjordan.eu> <1491173097-37305-3-git-send-email-lists@philjordan.eu> Cc: Jordan Justen , Phil Dennis-Jordan From: Laszlo Ersek Message-ID: <13cff49c-1034-eca5-5ad1-7d3d4e8bc6d7@redhat.com> Date: Tue, 4 Apr 2017 10:19:06 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <1491173097-37305-3-git-send-email-lists@philjordan.eu> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Tue, 04 Apr 2017 08:19:08 +0000 (UTC) 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: Tue, 04 Apr 2017 08:19:09 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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. > + 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 Jordan, do you have any comments? (For the whole series too, of course?) Thanks Laszlo