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

[Laszlo Ersek <lersek@redhat.com>]

On 10/12/18 09:29, Ard Biesheuvel wrote:
> 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.

I agree.

> 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.

I agree again; this is the cleanest approach, by the book.

(

In order to be completely honest, I have to point out that we're
currently cutting some corners on the PciRoot() / PcieRoot() difference,
in ArmVirtQemu. (Which translates to a PNPID difference, between 0x0A03
/ 0x0A08.) Please refer to the following discussions (search them for
"EISA_PNP_ID"):

- http://mid.mail-archive.com/8ba58ec8-9360-4805-c9a5-b9d5c193fdb0@redhat.com
- http://mid.mail-archive.com/b87c8a0e-e4c9-f9d3-3d53-f7bb1e27cc1c@redhat.com
- http://mid.mail-archive.com/1472840159-28957-4-git-send-email-ard.biesheuvel@linaro.org
  (and this was pushed ultimately)

)

Thanks
Laszlo