Re: [PATCH 2/5] EmbeddedPkg: introduce platform PCI I/O registration library

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 1 November 2016 at 21:57, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Mon, Oct 31, 2016 at 06:13:07PM +0000, Ard Biesheuvel wrote:
>> This introduces the PlatformPciIoDeviceRegistrationLib library class and
>> a default implementation to help platforms expose platform devices that
>> may be driven by a PCI driver.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  EmbeddedPkg/EmbeddedPkg.dec                                                                   |   1 +
>>  EmbeddedPkg/EmbeddedPkg.dsc                                                                   |   2 +
>>  EmbeddedPkg/Include/Library/PlatformPciIoDeviceRegistrationLib.h                              |  46 ++++++++
>>  EmbeddedPkg/Library/PlatformPciIoDeviceRegistrationLib/PlatformPciIoDeviceRegistrationLib.c   | 115 ++++++++++++++++++++
>>  EmbeddedPkg/Library/PlatformPciIoDeviceRegistrationLib/PlatformPciIoDeviceRegistrationLib.inf |  34 ++++++
>>  5 files changed, 198 insertions(+)
>>
>> diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec
>> index 84a6f4d01077..226036858880 100644
>> --- a/EmbeddedPkg/EmbeddedPkg.dec
>> +++ b/EmbeddedPkg/EmbeddedPkg.dec
>> @@ -44,6 +44,7 @@ [LibraryClasses.common]
>>    EblNetworkLib|Include/Library/EblNetworkLib.h
>>    GdbSerialLib|Include/Library/GdbSerialLib.h
>>    DebugAgentTimerLib|Include/Library/DebugAgentTimerLib.h
>> +  PlatformPciIoDeviceRegistrationLib|Include/Library/PlatformPciIoDeviceRegistrationLib.h
>>
>>
>>  [Guids.common]
>> diff --git a/EmbeddedPkg/EmbeddedPkg.dsc b/EmbeddedPkg/EmbeddedPkg.dsc
>> index eb7af800f0b2..d47c836379c9 100644
>> --- a/EmbeddedPkg/EmbeddedPkg.dsc
>> +++ b/EmbeddedPkg/EmbeddedPkg.dsc
>> @@ -290,5 +290,7 @@ [Components.common]
>>    EmbeddedPkg/Library/PrePiHobLib/PrePiHobLib.inf
>>    EmbeddedPkg/Library/PrePiMemoryAllocationLib/PrePiMemoryAllocationLib.inf
>>
>> +  EmbeddedPkg/Library/PlatformPciIoDeviceRegistrationLib/PlatformPciIoDeviceRegistrationLib.inf
>> +
>>  [Components.IA32, Components.X64, Components.IPF, Components.ARM]
>>    EmbeddedPkg/GdbStub/GdbStub.inf
>> diff --git a/EmbeddedPkg/Include/Library/PlatformPciIoDeviceRegistrationLib.h b/EmbeddedPkg/Include/Library/PlatformPciIoDeviceRegistrationLib.h
>> new file mode 100644
>> index 000000000000..07f163abc701
>> --- /dev/null
>> +++ b/EmbeddedPkg/Include/Library/PlatformPciIoDeviceRegistrationLib.h
>> @@ -0,0 +1,46 @@
>> +/** @file
>> +  Copyright (c) 2016, Linaro, Ltd. 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.
>> +
>> +**/
>> +
>> +#ifndef __PLATFORM_PCI_IO_DEVICE_REGISTRATION_LIB_H__
>> +#define __PLATFORM_PCI_IO_DEVICE_REGISTRATION_LIB_H__
>> +
>> +#include <Protocol/PlatformPciIo.h>
>> +
>> +/**
>> +  Register a platform device for PCI I/O protocol emulation
>> +
>> +  @param[in]      BaseAddress     The MMIO base address of the platform device
>> +  @param[in]      DeviceType      The type of platform device
>> +  @param[in]      DmaType         Whether the device is DMA coherent
>> +  @param[in]      InitFunc        Initialization routine to be invoked when the
>> +                                  device is enabled
>> +  @param[in,out]  Handle          The handle onto which to install the platform
>> +                                  PCI I/O protocol has been installed.
>> +                                  If Handle is NULL or *Handle is NULL, a new
>> +                                  handle will be allocated.
>> +
>> +  @retval EFI_SUCCESS             The registration succeeded.
>> +  @retval other                   The registration failed.
>> +
>> +**/
>> +EFI_STATUS
>> +EFIAPI
>> +PlatformPciIoRegisterDevice (
>> +  IN      EFI_PHYSICAL_ADDRESS          BaseAddress,
>> +  IN      PLATFORM_PCI_IO_DEVICE_TYPE   DeviceType,
>> +  IN      PLATFORM_PCI_IO_DMA_TYPE      DmaType,
>> +  IN      PLATFORM_PCI_IO_INIT          InitFunc,
>> +  IN OUT  EFI_HANDLE                    *Handle OPTIONAL
>> +  );
>> +
>> +#endif
>> diff --git a/EmbeddedPkg/Library/PlatformPciIoDeviceRegistrationLib/PlatformPciIoDeviceRegistrationLib.c b/EmbeddedPkg/Library/PlatformPciIoDeviceRegistrationLib/PlatformPciIoDeviceRegistrationLib.c
>> new file mode 100644
>> index 000000000000..1ce5abb137df
>> --- /dev/null
>> +++ b/EmbeddedPkg/Library/PlatformPciIoDeviceRegistrationLib/PlatformPciIoDeviceRegistrationLib.c
>> @@ -0,0 +1,115 @@
>> +/** @file
>> +  Copyright (c) 2016, Linaro, Ltd. 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 <Library/BaseLib.h>
>> +#include <Library/BaseMemoryLib.h>
>> +#include <Library/DevicePathLib.h>
>> +#include <Library/MemoryAllocationLib.h>
>> +#include <Library/PlatformPciIoDeviceRegistrationLib.h>
>> +#include <Library/UefiBootServicesTableLib.h>
>> +
>> +#include <Protocol/DevicePath.h>
>> +#include <Protocol/PlatformPciIo.h>
>> +
>> +#pragma pack (1)
>> +typedef struct {
>> +  VENDOR_DEVICE_PATH                  Vendor;
>> +  UINT64                              BaseAddress;
>> +  EFI_DEVICE_PATH_PROTOCOL            End;
>> +} PLATFORM_PCI_IO_DEVICE_PATH;
>> +
>> +#pragma pack ()
>> +
>> +/**
>> +  Register a platform device for PCI I/O protocol emulation
>> +
>> +  @param[in]      BaseAddress     The MMIO base address of the platform device
>> +  @param[in]      DeviceType      The type of platform device
>> +  @param[in]      DmaType         Whether the device is DMA coherent
>> +  @param[in]      InitFunc        Initialization routine to be invoked when the
>> +                                  device is enabled
>> +  @param[in,out]  Handle          The handle onto which to install the platform
>> +                                  PCI I/O protocol has been installed.
>> +                                  If Handle is NULL or *Handle is NULL, a new
>> +                                  handle will be allocated.
>> +
>> +  @retval EFI_SUCCESS             The registration succeeded.
>> +  @retval other                   The registration failed.
>> +
>> +**/
>> +EFI_STATUS
>> +EFIAPI
>> +PlatformPciIoRegisterDevice (
>> +  IN      EFI_PHYSICAL_ADDRESS          BaseAddress,
>> +  IN      PLATFORM_PCI_IO_DEVICE_TYPE   DeviceType,
>> +  IN      PLATFORM_PCI_IO_DMA_TYPE      DmaType,
>> +  IN      PLATFORM_PCI_IO_INIT          InitFunc,
>> +  IN OUT  EFI_HANDLE                    *Handle
>> +  )
>> +{
>> +  PLATFORM_PCI_IO               *Device;
>> +  PLATFORM_PCI_IO_DEVICE_PATH   *DevicePath;
>> +  EFI_HANDLE                    LocalHandle;
>> +  EFI_STATUS                    Status;
>> +
>> +  if (DeviceType >= PlatformPciIoDeviceMax || DmaType >= PlatformPciIoDmaMax) {
>> +    return EFI_INVALID_PARAMETER;
>> +  }
>> +
>> +  if (Handle == NULL) {
>> +    Handle = &LocalHandle;
>> +    LocalHandle = NULL;
>> +  }
>> +
>> +  Device = (PLATFORM_PCI_IO *)AllocateZeroPool (sizeof *Device);
>> +  if (Device == NULL) {
>> +    return EFI_OUT_OF_RESOURCES;
>> +  }
>> +
>> +  Device->BaseAddress = BaseAddress;
>> +  Device->DeviceType = DeviceType;
>> +  Device->DmaType = DmaType;
>> +  Device->Initialize = InitFunc;
>> +
>> +  DevicePath = (PLATFORM_PCI_IO_DEVICE_PATH *)CreateDeviceNode (
>> +                                                HARDWARE_DEVICE_PATH,
>> +                                                HW_VENDOR_DP,
>> +                                                sizeof (*DevicePath));
>> +  if (DevicePath == NULL) {
>> +    Status = EFI_OUT_OF_RESOURCES;
>> +    goto FreeDevice;
>> +  }
>> +
>> +  CopyGuid (&DevicePath->Vendor.Guid, &gPlatformPciIoProtocolGuid);
>> +  DevicePath->BaseAddress = BaseAddress;
>> +
>> +  SetDevicePathNodeLength (&DevicePath->Vendor,
>> +    sizeof (*DevicePath) - sizeof (DevicePath->End));
>> +  SetDevicePathEndNode (&DevicePath->End);
>> +
>> +  Status = gBS->InstallMultipleProtocolInterfaces (Handle,
>> +                  &gPlatformPciIoProtocolGuid, Device,
>> +                  &gEfiDevicePathProtocolGuid, DevicePath);
>> +  if (EFI_ERROR (Status)) {
>> +    goto FreeDevicePath;
>> +  }
>> +  return EFI_SUCCESS;
>> +
>> +FreeDevicePath:
>> +  FreePool (DevicePath);
>> +
>> +FreeDevice:
>> +  FreePool (Device);
>> +
>> +  return Status;
>> +}
>> diff --git a/EmbeddedPkg/Library/PlatformPciIoDeviceRegistrationLib/PlatformPciIoDeviceRegistrationLib.inf b/EmbeddedPkg/Library/PlatformPciIoDeviceRegistrationLib/PlatformPciIoDeviceRegistrationLib.inf
>> new file mode 100644
>> index 000000000000..282db3ab59ab
>> --- /dev/null
>> +++ b/EmbeddedPkg/Library/PlatformPciIoDeviceRegistrationLib/PlatformPciIoDeviceRegistrationLib.inf
>> @@ -0,0 +1,34 @@
>> +# @file
>> +# Copyright (c) 2016, Linaro, Ltd. 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.
>> +#
>> +
>> +[Defines]
>> +  INF_VERSION                    = 0x00010017
>
> Not 0019?
>

What can I say, I'm old school :-)

On the one hand, I'm happy to change it, but on the other hand, I
think it is good practice to expose the lowest version number you are
compatible with, in case anyone wants to backport this.