[Linaro-uefi,Linaro-uefi,v1,16/21] Hisilicon/PciHostBridgeDxe: Assign BAR resource from PciRegionBase

Io BAR should be based IoBase and Mem BAR should be based PciRegionBase.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ming Huang <huangming23@huawei.com>
Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
Signed-off-by: Yi Li <phoenix.liyi@huawei.com>
---
 .../Drivers/PciHostBridgeDxe/PciHostBridge.c       | 29 ++++++++++++++--------
 .../Drivers/PciHostBridgeDxe/PciRootBridgeIo.c     | 15 +++++++++--
 2 files changed, 31 insertions(+), 13 deletions(-)

On Mon, Mar 20, 2017 at 09:11:20PM +0800, Chenhui Sun wrote:
> Io BAR should be based IoBase and Mem BAR should be based PciRegionBase.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ming Huang <huangming23@huawei.com>
> Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
> Signed-off-by: Yi Li <phoenix.liyi@huawei.com>
> ---
>  .../Drivers/PciHostBridgeDxe/PciHostBridge.c       | 29 ++++++++++++++--------
>  .../Drivers/PciHostBridgeDxe/PciRootBridgeIo.c     | 15 +++++++++--
>  2 files changed, 31 insertions(+), 13 deletions(-)
> 
> diff --git a/Chips/Hisilicon/Drivers/PciHostBridgeDxe/PciHostBridge.c b/Chips/Hisilicon/Drivers/PciHostBridgeDxe/PciHostBridge.c
> index a970da6..2289d4b 100644
> --- a/Chips/Hisilicon/Drivers/PciHostBridgeDxe/PciHostBridge.c
> +++ b/Chips/Hisilicon/Drivers/PciHostBridgeDxe/PciHostBridge.c
> @@ -1410,9 +1410,8 @@ SetResource(
>          Ptr->ResType = 1;
>          Ptr->GenFlag = 0;
>          Ptr->SpecificFlag = 0;
> -        /* This is PCIE Device Bus which start address is the low 32bit of mem base*/
> -        Ptr->AddrRangeMin = (RootBridgeInstance->ResAllocNode[Index].Base - RootBridgeInstance->MemBase) +
> -                            (RootBridgeInstance->MemBase & 0xFFFFFFFF);
> +        /* PCIE Device Iobar address should be based on IoBase */
> +        Ptr->AddrRangeMin = RootBridgeInstance->IoBase;
>          Ptr->AddrRangeMax = 0;
>          Ptr->AddrTranslationOffset = \
>               (ResStatus == ResAllocated) ? EFI_RESOURCE_SATISFIED : EFI_RESOURCE_LESS;
> @@ -1429,9 +1428,13 @@ SetResource(
>          Ptr->GenFlag = 0;
>          Ptr->SpecificFlag = 0;
>          Ptr->AddrSpaceGranularity = 32;
> -        /* This is PCIE Device Bus which start address is the low 32bit of mem base*/
> +        /* PCIE device Bar should be based on PciRegionBase */
> +        if (RootBridgeInstance->PciRegionBase > 0xFFFFFFFF) {
> +          DEBUG((DEBUG_ERROR, "PCIE Res(TypeMem32) unsupported.\n"));

Space before "(("..

> +          return EFI_UNSUPPORTED;

The commit message describes nothing obviously to do with this
modification. Why is a RegionBase above 32 bit invalid?

> +        }
>          Ptr->AddrRangeMin = (RootBridgeInstance->ResAllocNode[Index].Base - RootBridgeInstance->MemBase) +
> -                             (RootBridgeInstance->MemBase & 0xFFFFFFFF);
> +                             (RootBridgeInstance->PciRegionBase & 0xFFFFFFFF);

The current code above has already returned if any bits > 4GB are set,
so this mask is now redundant.

>          Ptr->AddrRangeMax = 0;
>          Ptr->AddrTranslationOffset = \
>               (ResStatus == ResAllocated) ? EFI_RESOURCE_SATISFIED : EFI_RESOURCE_LESS;
> @@ -1448,9 +1451,13 @@ SetResource(
>          Ptr->GenFlag = 0;
>          Ptr->SpecificFlag = 6;
>          Ptr->AddrSpaceGranularity = 32;
> -        /* This is PCIE Device Bus which start address is the low 32bit of mem base*/
> +        /* PCIE device Bar should be based on PciRegionBase */
> +        if (RootBridgeInstance->PciRegionBase > 0xFFFFFFFF) {
> +          DEBUG((DEBUG_ERROR, "PCIE Res(TypePMem32) unsupported.\n"));
> +          return EFI_UNSUPPORTED;
> +        }
>          Ptr->AddrRangeMin = (RootBridgeInstance->ResAllocNode[Index].Base - RootBridgeInstance->MemBase) +
> -                             (RootBridgeInstance->MemBase & 0xFFFFFFFF);
> +                             (RootBridgeInstance->PciRegionBase & 0xFFFFFFFF);

Same three comments as above.

>          Ptr->AddrRangeMax = 0;
>          Ptr->AddrTranslationOffset = \
>               (ResStatus == ResAllocated) ? EFI_RESOURCE_SATISFIED : EFI_RESOURCE_LESS;
> @@ -1467,9 +1474,9 @@ SetResource(
>          Ptr->GenFlag = 0;
>          Ptr->SpecificFlag = 0;
>          Ptr->AddrSpaceGranularity = 64;
> -        /* This is PCIE Device Bus which start address is the low 32bit of mem base*/
> +        /* PCIE device Bar should be based on PciRegionBase */
>          Ptr->AddrRangeMin = (RootBridgeInstance->ResAllocNode[Index].Base - RootBridgeInstance->MemBase) +
> -                             (RootBridgeInstance->MemBase & 0xFFFFFFFFFFFFFFFF);
> +                             (RootBridgeInstance->PciRegionBase & 0xFFFFFFFFFFFFFFFF);

I don't believe these are type safe. Preferably replace the direct
literal masks on lines you are modifying with MAX_UINT32 and
MAX_UINT64 to ensure to avoiding amusing integer promotion disasters.

>          Ptr->AddrRangeMax = 0;
>          Ptr->AddrTranslationOffset = \
>               (ResStatus == ResAllocated) ? EFI_RESOURCE_SATISFIED : EFI_RESOURCE_LESS;
> @@ -1486,9 +1493,9 @@ SetResource(
>          Ptr->GenFlag = 0;
>          Ptr->SpecificFlag = 6;
>          Ptr->AddrSpaceGranularity = 64;
> -        /* This is PCIE Device Bus which start address is the low 32bit of mem base*/
> +        /* PCIE device Bar should be based on PciRegionBase */
>          Ptr->AddrRangeMin = (RootBridgeInstance->ResAllocNode[Index].Base - RootBridgeInstance->MemBase) +
> -                             (RootBridgeInstance->MemBase & 0xFFFFFFFFFFFFFFFF);
> +                             (RootBridgeInstance->PciRegionBase & 0xFFFFFFFFFFFFFFFF);
>          Ptr->AddrRangeMax = 0;
>          Ptr->AddrTranslationOffset = \
>               (ResStatus == ResAllocated) ? EFI_RESOURCE_SATISFIED : EFI_RESOURCE_LESS;

In general, there is a spectacular amount of duplicated code in this
function. It would be worth refactoring so that the lines that are
identical between all cases are not repeated.

At the same time it would be useful to replace the hard-coded numerals
in this switch statement with more descriptive #defines.

And a default: target with an assert would look a bit neater.

> diff --git a/Chips/Hisilicon/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c b/Chips/Hisilicon/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c
> index 30619f5..7902831 100644
> --- a/Chips/Hisilicon/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c
> +++ b/Chips/Hisilicon/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c
> @@ -2312,8 +2312,19 @@ RootBridgeIoConfiguration (
>    PrivateData = DRIVER_INSTANCE_FROM_PCI_ROOT_BRIDGE_IO_THIS (This);
>    for (Index = 0; Index < TypeMax; Index++) {
>      if (PrivateData->ResAllocNode[Index].Status == ResAllocated) {
> -      Configuration.SpaceDesp[Index].AddrRangeMin = PrivateData->ResAllocNode[Index].Base;
> -      Configuration.SpaceDesp[Index].AddrRangeMax = PrivateData->ResAllocNode[Index].Base + PrivateData->ResAllocNode[Index].Length - 1;
> +      switch (Index) {
> +      case TypeIo:
> +        Configuration.SpaceDesp[Index].AddrRangeMin = PrivateData->IoBase;
> +        break;
> +      case TypeBus:
> +        Configuration.SpaceDesp[Index].AddrRangeMin = PrivateData->ResAllocNode[Index].Base;
> +        break;
> +      default:
> +      /* PCIE Device bar address should be base on PciRegionBase */
> +      Configuration.SpaceDesp[Index].AddrRangeMin = (PrivateData->ResAllocNode[Index].Base - PrivateData->MemBase) +
> +                             (PrivateData->PciRegionBase & 0xFFFFFFFFFFFFFFFF);

Please replace this hand-coded mask with MAX_UINT64.

/
    Leif

> +      }
> +      Configuration.SpaceDesp[Index].AddrRangeMax = Configuration.SpaceDesp[Index].AddrRangeMin + PrivateData->ResAllocNode[Index].Length - 1;
>        Configuration.SpaceDesp[Index].AddrLen      = PrivateData->ResAllocNode[Index].Length;
>      }
>    }
> -- 
> 1.9.1
>