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

[Phil Dennis-Jordan <lists@philjordan.eu>]

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