From: Phil Dennis-Jordan <phil@philjordan.eu>
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.
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
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 <jordan.l.justen@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
---
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.<BR>
+ 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.
+
+**/
+
+#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.<BR>
+ 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) );
+ 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;
+}
+
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.<BR>
+ 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;
+}
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.<BR>
+ 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
+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;
+}
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.<BR>
+ 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 <Library/DebugLib.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
+ )
+{
+ ASSERT (FALSE);
+ return 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;
+}
--
2.3.2 (Apple Git-55)
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On 04/03/17 00:44, Phil Dennis-Jordan wrote:
> From: Phil Dennis-Jordan <phil@philjordan.eu>
>
> 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 <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
Please add here:
Suggested-by: Jordan Justen <jordan.l.justen@intel.com>
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 <phil@philjordan.eu>
> ---
>
> 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.<BR>
> + 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.<BR>
> + 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 <https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html>, 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.<BR>
> + 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.<BR>
> + 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.<BR>
> + 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 <Library/DebugLib.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
> + )
> +{
> + 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 <lersek@redhat.com>
Jordan, do you have any comments? (For the whole series too, of course?)
Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On Tue, Apr 4, 2017 at 8:19 PM, Laszlo Ersek <lersek@redhat.com> wrote:
> On 04/03/17 00:44, Phil Dennis-Jordan wrote:
>> From: Phil Dennis-Jordan <phil@philjordan.eu>
>>
>> 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 <jordan.l.justen@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>
> Please add here:
>
> Suggested-by: Jordan Justen <jordan.l.justen@intel.com>
>
> 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 <phil@philjordan.eu>
>> ---
>>
>> 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.<BR>
>> + 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.<BR>
>> + 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 <https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html>, 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.<BR>
>> + 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.<BR>
>> + 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.<BR>
>> + 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 <Library/DebugLib.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
>> + )
>> +{
>> + 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 <lersek@redhat.com>
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On 04/05/17 11:16, Phil Dennis-Jordan wrote:
> On Tue, Apr 4, 2017 at 8:19 PM, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 04/03/17 00:44, Phil Dennis-Jordan wrote:
>>> +/**
>>> + 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 <https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html>, 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.
Either a comment or removal would be fine, I think.
I feel slightly more attracted to removal because the "N" diverges from
IoWrite32() in "MdePkg/Library/BaseIoLibIntrinsic/IoLibGcc.c", and that
difference seems to deserve justification.
Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
© 2016 - 2024 Red Hat, Inc.