Re: [PATCH v2 2/3] OvmfPkg/QemuVideoDxe: Helper functions for unaligned port I/O.

[Laszlo Ersek <lersek@redhat.com>]

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