Re: [PATCH edk2-platforms v5 21/28] Platform/Hisilicon/D06: Add PciHostBridgeLib

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

Hello all,

While grepping through the code in edk2-platforms, I noticed an issue
with this commit. Apologies for not spotting it earlier.

On 31 August 2018 at 15:27, Ming Huang <ming.huang@linaro.org> wrote:
> PciHostBridgeLib which is need by PciHostBridgeDxe, provide
> root bridges and deal with resource conflict.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ming Huang <ming.huang@linaro.org>
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
>  Platform/Hisilicon/D06/D06.dsc                                       |   2 +-
>  Platform/Hisilicon/D06/Library/PciHostBridgeLib/PciHostBridgeLib.inf |  36 ++
>  Platform/Hisilicon/D06/Library/PciHostBridgeLib/PciHostBridgeLib.c   | 635 ++++++++++++++++++++
>  3 files changed, 672 insertions(+), 1 deletion(-)
>
> diff --git a/Platform/Hisilicon/D06/D06.dsc b/Platform/Hisilicon/D06/D06.dsc
> index 2659cb7e37..83dcbab6c4 100644
> --- a/Platform/Hisilicon/D06/D06.dsc
> +++ b/Platform/Hisilicon/D06/D06.dsc
> @@ -419,7 +419,7 @@
>      <LibraryClasses>
>        PciSegmentLib|MdePkg/Library/BasePciSegmentLibPci/BasePciSegmentLibPci.inf
>        PciLib|MdePkg/Library/BasePciLibPciExpress/BasePciLibPciExpress.inf
> -      PciHostBridgeLib|MdeModulePkg/Library/PciHostBridgeLibNull/PciHostBridgeLibNull.inf
> +      PciHostBridgeLib|Platform/Hisilicon/D06/Library/PciHostBridgeLib/PciHostBridgeLib.inf
>    }
>
>    MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
> diff --git a/Platform/Hisilicon/D06/Library/PciHostBridgeLib/PciHostBridgeLib.inf b/Platform/Hisilicon/D06/Library/PciHostBridgeLib/PciHostBridgeLib.inf
> new file mode 100644
> index 0000000000..8a998681a3
> --- /dev/null
> +++ b/Platform/Hisilicon/D06/Library/PciHostBridgeLib/PciHostBridgeLib.inf
> @@ -0,0 +1,36 @@
> +## @file
> +#
> +#  Copyright (c) 2018, Hisilicon Limited. All rights reserved.<BR>
> +#  Copyright (c) 2018, Linaro Limited. 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                    = 0x0001001A
> +  BASE_NAME                      = PciHostBridgeLib
> +  FILE_GUID                      = 61b7276a-fc67-11e5-82fd-47ea9896dd5d
> +  MODULE_TYPE                    = DXE_DRIVER
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = PciHostBridgeLib|DXE_DRIVER
> +
> +[Sources]
> +  PciHostBridgeLib.c
> +
> +[Packages]
> +  MdeModulePkg/MdeModulePkg.dec
> +  MdePkg/MdePkg.dec
> +
> +[LibraryClasses]
> +  BaseLib
> +  DebugLib
> +  DevicePathLib
> +  MemoryAllocationLib
> +  UefiBootServicesTableLib
> diff --git a/Platform/Hisilicon/D06/Library/PciHostBridgeLib/PciHostBridgeLib.c b/Platform/Hisilicon/D06/Library/PciHostBridgeLib/PciHostBridgeLib.c
> new file mode 100644
> index 0000000000..d1a436d9bc
> --- /dev/null
> +++ b/Platform/Hisilicon/D06/Library/PciHostBridgeLib/PciHostBridgeLib.c
> @@ -0,0 +1,635 @@
> +/** @file
> +
> +  Copyright (c) 2018, Hisilicon Limited. All rights reserved.<BR>
> +  Copyright (c) 2018, Linaro Limited. 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 <PiDxe.h>
> +#include <Library/DebugLib.h>
> +#include <Library/DevicePathLib.h>
> +#include <Library/MemoryAllocationLib.h>
> +#include <Library/PciHostBridgeLib.h>
> +#include <Protocol/PciHostBridgeResourceAllocation.h>
> +#include <Protocol/PciRootBridgeIo.h>
> +
> +#define ENUM_HB_NUM 8
> +
> +#define EFI_PCI_SUPPORT   (EFI_PCI_ATTRIBUTE_ISA_MOTHERBOARD_IO | \
> +                           EFI_PCI_ATTRIBUTE_IDE_SECONDARY_IO | \
> +                           EFI_PCI_ATTRIBUTE_IDE_PRIMARY_IO | \
> +                           EFI_PCI_ATTRIBUTE_ISA_IO_16  | \
> +                           EFI_PCI_ATTRIBUTE_VGA_MEMORY | \
> +                           EFI_PCI_ATTRIBUTE_VGA_IO_16  | \
> +                           EFI_PCI_ATTRIBUTE_VGA_PALETTE_IO_16)
> +
> +#define EFI_PCI_ATTRIBUTE  EFI_PCI_SUPPORT
> +
> +#pragma pack(1)
> +typedef struct {
> +  ACPI_HID_DEVICE_PATH     AcpiDevicePath;
> +  EFI_DEVICE_PATH_PROTOCOL EndDevicePath;
> +} EFI_PCI_ROOT_BRIDGE_DEVICE_PATH;
> +#pragma pack ()
> +
> +STATIC EFI_PCI_ROOT_BRIDGE_DEVICE_PATH mEfiPciRootBridgeDevicePath [ENUM_HB_NUM] = {
> +//Host Bridge 0
> +  {
> +    {
> +      {
> +        ACPI_DEVICE_PATH,
> +        ACPI_DP,
> +        {
> +          (UINT8)sizeof (ACPI_HID_DEVICE_PATH),
> +          (UINT8)(sizeof (ACPI_HID_DEVICE_PATH) >> 8)
> +        }
> +      },
> +      EISA_PNP_ID(0x0A03), // PCI
> +      0
...
> +//Host Bridge 2
...
> +      EISA_PNP_ID(0x0A04), // PCI
> +      0
..
> +//Host Bridge 4
...
> +      EISA_PNP_ID(0x0A05), // PCI
> +      0
...
> +//Host Bridge 5
...
> +      EISA_PNP_ID(0x0A06), // PCI
> +      0
...
> +//Host Bridge 6
...
> +      EISA_PNP_ID(0x0A07), // PCI
> +      0
...
> +//Host Bridge 8
...
> +      EISA_PNP_ID(0x0A08), // PCI
> +      0
...
> +//Host Bridge 10
...
> +      EISA_PNP_ID(0x0A09), // PCI
> +      0
...
> +//Host Bridge 11
...
> +      EISA_PNP_ID(0x0A0A), // PCI
> +      0

This is *not* how it works. You cannot invent your own ACPI HIDs like
that. If you have multiple instances of a device, you increment the
UID.

Please synchronize these definitions with the HIDs/UIDs used in the
DSDT/SSDT for PCIe. And please make sure all host bridges are
accounted for.