[Linaro-uefi] Platforms/AMD: remove bogus 4 GB limit in PciHostBridgeDxe

Message ID 1460721170-10347-1-git-send-email-ard.biesheuvel@linaro.org
State New
Headers show

Commit Message

Ard Biesheuvel April 15, 2016, 11:52 a.m.
The copy-pasted PciHostBridgeDxe driver has some interesting restrictions
that are difficult to fulfil on Seattle, i.e., its AllocateBuffer() and
IoMap() operations require the DMA address to be below 4 GB. This would
only makes sense in the presence of 32-bit only DMA bus masters that are
not behind a SMMU, but in the Seattle case, it is completely pointless
since it does not have any RAM below 4 GB in the first place.

So simply drop these restrictions.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 .../Drivers/PciHostBridgeDxe/PciRootBridgeIo.c     | 91 +---------------------
 1 file changed, 2 insertions(+), 89 deletions(-)

Comments

Duran, Leo April 15, 2016, 2:43 p.m. | #1
Sigh!... I've actually tried that before (but maybe I missed something, so I'll try again).

Leo.

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Friday, April 15, 2016 6:53 AM
> To: linaro-uefi@lists.linaro.org; leif.lindholm@linaro.org;
> ricardo.salveti@linaro.org; Duran, Leo <leo.duran@amd.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Subject: [PATCH] Platforms/AMD: remove bogus 4 GB limit in
> PciHostBridgeDxe
> 
> The copy-pasted PciHostBridgeDxe driver has some interesting restrictions
> that are difficult to fulfil on Seattle, i.e., its AllocateBuffer() and
> IoMap() operations require the DMA address to be below 4 GB. This would
> only makes sense in the presence of 32-bit only DMA bus masters that are
> not behind a SMMU, but in the Seattle case, it is completely pointless since it
> does not have any RAM below 4 GB in the first place.
> 
> So simply drop these restrictions.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  .../Drivers/PciHostBridgeDxe/PciRootBridgeIo.c     | 91 +---------------------
>  1 file changed, 2 insertions(+), 89 deletions(-)
> 
> diff --git a/Platforms/AMD/Styx/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c
> b/Platforms/AMD/Styx/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c
> index 4ac89fb7548f..941c330a4228 100644
> --- a/Platforms/AMD/Styx/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c
> +++ b/Platforms/AMD/Styx/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c
> @@ -1715,89 +1715,7 @@ DEBUG((EFI_D_ERROR, "RootBridgeIoMap() -
> %d\n", __LINE__));
>      return EFI_INVALID_PARAMETER;
>    }
> 
> -  //
> -  // Most PCAT like chipsets can not handle performing DMA above 4GB.
> -  // If any part of the DMA transfer being mapped is above 4GB, then
> -  // map the DMA transfer to a buffer below 4GB.
> -  //
> -  PhysicalAddress = (EFI_PHYSICAL_ADDRESS) (UINTN) HostAddress;
> -  if ((PhysicalAddress + *NumberOfBytes) > 0x100000000ULL) {
> -
> -    //
> -    // Common Buffer operations can not be remapped.  If the common
> buffer
> -    // if above 4GB, then it is not possible to generate a mapping, so return
> -    // an error.
> -    //
> -    if (Operation == EfiPciOperationBusMasterCommonBuffer || Operation
> == EfiPciOperationBusMasterCommonBuffer64) {
> -      return EFI_UNSUPPORTED;
> -    }
> -
> -    //
> -    // Allocate a MAP_INFO structure to remember the mapping when
> Unmap() is
> -    // called later.
> -    //
> -    Status = gBS->AllocatePool (
> -                    EfiBootServicesData,
> -                    sizeof(MAP_INFO),
> -                    (VOID **)&MapInfo
> -                    );
> -    if (EFI_ERROR (Status)) {
> -      *NumberOfBytes = 0;
> -      return Status;
> -    }
> -
> -    //
> -    // Return a pointer to the MAP_INFO structure in Mapping
> -    //
> -    *Mapping = MapInfo;
> -
> -    //
> -    // Initialize the MAP_INFO structure
> -    //
> -    MapInfo->Operation         = Operation;
> -    MapInfo->NumberOfBytes     = *NumberOfBytes;
> -    MapInfo->NumberOfPages     = EFI_SIZE_TO_PAGES(*NumberOfBytes);
> -    MapInfo->HostAddress       = PhysicalAddress;
> -    MapInfo->MappedHostAddress = 0x00000000ffffffff;
> -
> -    //
> -    // Allocate a buffer below 4GB to map the transfer to.
> -    //
> -    Status = gBS->AllocatePages (
> -                    AllocateMaxAddress,
> -                    EfiBootServicesData,
> -                    MapInfo->NumberOfPages,
> -                    &MapInfo->MappedHostAddress
> -                    );
> -    if (EFI_ERROR (Status)) {
> -      gBS->FreePool (MapInfo);
> -      *NumberOfBytes = 0;
> -      return Status;
> -    }
> -
> -    //
> -    // If this is a read operation from the Bus Master's point of view,
> -    // then copy the contents of the real buffer into the mapped buffer
> -    // so the Bus Master can read the contents of the real buffer.
> -    //
> -    if (Operation == EfiPciOperationBusMasterRead || Operation ==
> EfiPciOperationBusMasterRead64) {
> -      CopyMem (
> -        (VOID *)(UINTN)MapInfo->MappedHostAddress,
> -        (VOID *)(UINTN)MapInfo->HostAddress,
> -        MapInfo->NumberOfBytes
> -        );
> -    }
> -
> -    //
> -    // The DeviceAddress is the address of the maped buffer below 4GB
> -    //
> -    *DeviceAddress = MapInfo->MappedHostAddress;
> -  } else {
> -    //
> -    // The transfer is below 4GB, so the DeviceAddress is simply the
> HostAddress
> -    //
> -    *DeviceAddress = PhysicalAddress;
> -  }
> +  *DeviceAddress = PhysicalAddress;
> 
>    return EFI_SUCCESS;
>  }
> @@ -1917,12 +1835,7 @@ DEBUG((EFI_D_ERROR,
> "RootBridgeIoAllocateBuffer() - %d\n", __LINE__));
>      return EFI_INVALID_PARAMETER;
>    }
> 
> -  //
> -  // Limit allocations to memory below 4GB
> -  //
> -  PhysicalAddress = (EFI_PHYSICAL_ADDRESS)(0xffffffff);
> -
> -  Status = gBS->AllocatePages (AllocateMaxAddress, MemoryType, Pages,
> &PhysicalAddress);
> +  Status = gBS->AllocatePages (AllocateAnyPages, MemoryType, Pages,
> + &PhysicalAddress);
>    if (EFI_ERROR (Status)) {
>      return Status;
>    }
> --
> 2.5.0
Ard Biesheuvel April 15, 2016, 3:19 p.m. | #2
On 15 April 2016 at 16:43, Duran, Leo <leo.duran@amd.com> wrote:
> Sigh!... I've actually tried that before (but maybe I missed something, so I'll try again).
>

Well, even if it does not fix the issue completely, we obviously need
to merge this since the original code does not make sense at all.

Thanks,
Ard.


>> -----Original Message-----
>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>> Sent: Friday, April 15, 2016 6:53 AM
>> To: linaro-uefi@lists.linaro.org; leif.lindholm@linaro.org;
>> ricardo.salveti@linaro.org; Duran, Leo <leo.duran@amd.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Subject: [PATCH] Platforms/AMD: remove bogus 4 GB limit in
>> PciHostBridgeDxe
>>
>> The copy-pasted PciHostBridgeDxe driver has some interesting restrictions
>> that are difficult to fulfil on Seattle, i.e., its AllocateBuffer() and
>> IoMap() operations require the DMA address to be below 4 GB. This would
>> only makes sense in the presence of 32-bit only DMA bus masters that are
>> not behind a SMMU, but in the Seattle case, it is completely pointless since it
>> does not have any RAM below 4 GB in the first place.
>>
>> So simply drop these restrictions.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  .../Drivers/PciHostBridgeDxe/PciRootBridgeIo.c     | 91 +---------------------
>>  1 file changed, 2 insertions(+), 89 deletions(-)
>>
>> diff --git a/Platforms/AMD/Styx/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c
>> b/Platforms/AMD/Styx/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c
>> index 4ac89fb7548f..941c330a4228 100644
>> --- a/Platforms/AMD/Styx/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c
>> +++ b/Platforms/AMD/Styx/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c
>> @@ -1715,89 +1715,7 @@ DEBUG((EFI_D_ERROR, "RootBridgeIoMap() -
>> %d\n", __LINE__));
>>      return EFI_INVALID_PARAMETER;
>>    }
>>
>> -  //
>> -  // Most PCAT like chipsets can not handle performing DMA above 4GB.
>> -  // If any part of the DMA transfer being mapped is above 4GB, then
>> -  // map the DMA transfer to a buffer below 4GB.
>> -  //
>> -  PhysicalAddress = (EFI_PHYSICAL_ADDRESS) (UINTN) HostAddress;
>> -  if ((PhysicalAddress + *NumberOfBytes) > 0x100000000ULL) {
>> -
>> -    //
>> -    // Common Buffer operations can not be remapped.  If the common
>> buffer
>> -    // if above 4GB, then it is not possible to generate a mapping, so return
>> -    // an error.
>> -    //
>> -    if (Operation == EfiPciOperationBusMasterCommonBuffer || Operation
>> == EfiPciOperationBusMasterCommonBuffer64) {
>> -      return EFI_UNSUPPORTED;
>> -    }
>> -
>> -    //
>> -    // Allocate a MAP_INFO structure to remember the mapping when
>> Unmap() is
>> -    // called later.
>> -    //
>> -    Status = gBS->AllocatePool (
>> -                    EfiBootServicesData,
>> -                    sizeof(MAP_INFO),
>> -                    (VOID **)&MapInfo
>> -                    );
>> -    if (EFI_ERROR (Status)) {
>> -      *NumberOfBytes = 0;
>> -      return Status;
>> -    }
>> -
>> -    //
>> -    // Return a pointer to the MAP_INFO structure in Mapping
>> -    //
>> -    *Mapping = MapInfo;
>> -
>> -    //
>> -    // Initialize the MAP_INFO structure
>> -    //
>> -    MapInfo->Operation         = Operation;
>> -    MapInfo->NumberOfBytes     = *NumberOfBytes;
>> -    MapInfo->NumberOfPages     = EFI_SIZE_TO_PAGES(*NumberOfBytes);
>> -    MapInfo->HostAddress       = PhysicalAddress;
>> -    MapInfo->MappedHostAddress = 0x00000000ffffffff;
>> -
>> -    //
>> -    // Allocate a buffer below 4GB to map the transfer to.
>> -    //
>> -    Status = gBS->AllocatePages (
>> -                    AllocateMaxAddress,
>> -                    EfiBootServicesData,
>> -                    MapInfo->NumberOfPages,
>> -                    &MapInfo->MappedHostAddress
>> -                    );
>> -    if (EFI_ERROR (Status)) {
>> -      gBS->FreePool (MapInfo);
>> -      *NumberOfBytes = 0;
>> -      return Status;
>> -    }
>> -
>> -    //
>> -    // If this is a read operation from the Bus Master's point of view,
>> -    // then copy the contents of the real buffer into the mapped buffer
>> -    // so the Bus Master can read the contents of the real buffer.
>> -    //
>> -    if (Operation == EfiPciOperationBusMasterRead || Operation ==
>> EfiPciOperationBusMasterRead64) {
>> -      CopyMem (
>> -        (VOID *)(UINTN)MapInfo->MappedHostAddress,
>> -        (VOID *)(UINTN)MapInfo->HostAddress,
>> -        MapInfo->NumberOfBytes
>> -        );
>> -    }
>> -
>> -    //
>> -    // The DeviceAddress is the address of the maped buffer below 4GB
>> -    //
>> -    *DeviceAddress = MapInfo->MappedHostAddress;
>> -  } else {
>> -    //
>> -    // The transfer is below 4GB, so the DeviceAddress is simply the
>> HostAddress
>> -    //
>> -    *DeviceAddress = PhysicalAddress;
>> -  }
>> +  *DeviceAddress = PhysicalAddress;
>>
>>    return EFI_SUCCESS;
>>  }
>> @@ -1917,12 +1835,7 @@ DEBUG((EFI_D_ERROR,
>> "RootBridgeIoAllocateBuffer() - %d\n", __LINE__));
>>      return EFI_INVALID_PARAMETER;
>>    }
>>
>> -  //
>> -  // Limit allocations to memory below 4GB
>> -  //
>> -  PhysicalAddress = (EFI_PHYSICAL_ADDRESS)(0xffffffff);
>> -
>> -  Status = gBS->AllocatePages (AllocateMaxAddress, MemoryType, Pages,
>> &PhysicalAddress);
>> +  Status = gBS->AllocatePages (AllocateAnyPages, MemoryType, Pages,
>> + &PhysicalAddress);
>>    if (EFI_ERROR (Status)) {
>>      return Status;
>>    }
>> --
>> 2.5.0
>
Ricardo Salveti April 21, 2016, 5:29 p.m. | #3
Ircloud is off for me, so replying here instead.

On Fri, Apr 15, 2016 at 8:52 AM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> The copy-pasted PciHostBridgeDxe driver has some interesting restrictions
> that are difficult to fulfil on Seattle, i.e., its AllocateBuffer() and
> IoMap() operations require the DMA address to be below 4 GB. This would
> only makes sense in the presence of 32-bit only DMA bus masters that are
> not behind a SMMU, but in the Seattle case, it is completely pointless
> since it does not have any RAM below 4 GB in the first place.
>
> So simply drop these restrictions.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  .../Drivers/PciHostBridgeDxe/PciRootBridgeIo.c     | 91 +---------------------
>  1 file changed, 2 insertions(+), 89 deletions(-)
>
> diff --git a/Platforms/AMD/Styx/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c b/Platforms/AMD/Styx/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c
> index 4ac89fb7548f..941c330a4228 100644
> --- a/Platforms/AMD/Styx/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c
> +++ b/Platforms/AMD/Styx/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c
> @@ -1715,89 +1715,7 @@ DEBUG((EFI_D_ERROR, "RootBridgeIoMap() - %d\n", __LINE__));
>      return EFI_INVALID_PARAMETER;
>    }
>
> -  //
> -  // Most PCAT like chipsets can not handle performing DMA above 4GB.
> -  // If any part of the DMA transfer being mapped is above 4GB, then
> -  // map the DMA transfer to a buffer below 4GB.
> -  //
> -  PhysicalAddress = (EFI_PHYSICAL_ADDRESS) (UINTN) HostAddress;
> -  if ((PhysicalAddress + *NumberOfBytes) > 0x100000000ULL) {
> -
> -    //
> -    // Common Buffer operations can not be remapped.  If the common buffer
> -    // if above 4GB, then it is not possible to generate a mapping, so return
> -    // an error.
> -    //
> -    if (Operation == EfiPciOperationBusMasterCommonBuffer || Operation == EfiPciOperationBusMasterCommonBuffer64) {
> -      return EFI_UNSUPPORTED;
> -    }
> -
> -    //
> -    // Allocate a MAP_INFO structure to remember the mapping when Unmap() is
> -    // called later.
> -    //
> -    Status = gBS->AllocatePool (
> -                    EfiBootServicesData,
> -                    sizeof(MAP_INFO),
> -                    (VOID **)&MapInfo
> -                    );
> -    if (EFI_ERROR (Status)) {
> -      *NumberOfBytes = 0;
> -      return Status;
> -    }
> -
> -    //
> -    // Return a pointer to the MAP_INFO structure in Mapping
> -    //
> -    *Mapping = MapInfo;
> -
> -    //
> -    // Initialize the MAP_INFO structure
> -    //
> -    MapInfo->Operation         = Operation;
> -    MapInfo->NumberOfBytes     = *NumberOfBytes;
> -    MapInfo->NumberOfPages     = EFI_SIZE_TO_PAGES(*NumberOfBytes);
> -    MapInfo->HostAddress       = PhysicalAddress;
> -    MapInfo->MappedHostAddress = 0x00000000ffffffff;
> -
> -    //
> -    // Allocate a buffer below 4GB to map the transfer to.
> -    //
> -    Status = gBS->AllocatePages (
> -                    AllocateMaxAddress,
> -                    EfiBootServicesData,
> -                    MapInfo->NumberOfPages,
> -                    &MapInfo->MappedHostAddress
> -                    );
> -    if (EFI_ERROR (Status)) {
> -      gBS->FreePool (MapInfo);
> -      *NumberOfBytes = 0;
> -      return Status;
> -    }
> -
> -    //
> -    // If this is a read operation from the Bus Master's point of view,
> -    // then copy the contents of the real buffer into the mapped buffer
> -    // so the Bus Master can read the contents of the real buffer.
> -    //
> -    if (Operation == EfiPciOperationBusMasterRead || Operation == EfiPciOperationBusMasterRead64) {
> -      CopyMem (
> -        (VOID *)(UINTN)MapInfo->MappedHostAddress,
> -        (VOID *)(UINTN)MapInfo->HostAddress,
> -        MapInfo->NumberOfBytes
> -        );
> -    }
> -
> -    //
> -    // The DeviceAddress is the address of the maped buffer below 4GB
> -    //
> -    *DeviceAddress = MapInfo->MappedHostAddress;
> -  } else {
> -    //
> -    // The transfer is below 4GB, so the DeviceAddress is simply the HostAddress
> -    //
> -    *DeviceAddress = PhysicalAddress;
> -  }
> +  *DeviceAddress = PhysicalAddress;

The 3 errors I had:
/tmp/build/edk2/OpenPlatformPkg/Platforms/AMD/Styx/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c:
In function 'RootBridgeIoMap':
/tmp/build/edk2/OpenPlatformPkg/Platforms/AMD/Styx/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c:1699:13:
error: unused variable 'MapInfo'
 [-Werror=unused-variable]
   MAP_INFO              *MapInfo;
             ^
/tmp/build/edk2/OpenPlatformPkg/Platforms/AMD/Styx/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c:1697:14:
error: unused variable 'Status'
[-Werror=unused-variable]
   EFI_STATUS            Status;
              ^
/tmp/build/edk2/OpenPlatformPkg/Platforms/AMD/Styx/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c:1718:18:
error: 'PhysicalAddress' may be
used uninitialized in this function [-Werror=maybe-uninitialized]
   *DeviceAddress = PhysicalAddress;
                  ^
cc1: all warnings being treated as errors

Here should *DeviceAddress be just HostAddress?

Thanks,
Ard Biesheuvel April 21, 2016, 5:31 p.m. | #4
On 21 April 2016 at 19:29, Ricardo Salveti <ricardo.salveti@linaro.org> wrote:
> Ircloud is off for me, so replying here instead.
>
> On Fri, Apr 15, 2016 at 8:52 AM, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
>> The copy-pasted PciHostBridgeDxe driver has some interesting restrictions
>> that are difficult to fulfil on Seattle, i.e., its AllocateBuffer() and
>> IoMap() operations require the DMA address to be below 4 GB. This would
>> only makes sense in the presence of 32-bit only DMA bus masters that are
>> not behind a SMMU, but in the Seattle case, it is completely pointless
>> since it does not have any RAM below 4 GB in the first place.
>>
>> So simply drop these restrictions.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  .../Drivers/PciHostBridgeDxe/PciRootBridgeIo.c     | 91 +---------------------
>>  1 file changed, 2 insertions(+), 89 deletions(-)
>>
>> diff --git a/Platforms/AMD/Styx/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c b/Platforms/AMD/Styx/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c
>> index 4ac89fb7548f..941c330a4228 100644
>> --- a/Platforms/AMD/Styx/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c
>> +++ b/Platforms/AMD/Styx/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c
>> @@ -1715,89 +1715,7 @@ DEBUG((EFI_D_ERROR, "RootBridgeIoMap() - %d\n", __LINE__));
>>      return EFI_INVALID_PARAMETER;
>>    }
>>
>> -  //
>> -  // Most PCAT like chipsets can not handle performing DMA above 4GB.
>> -  // If any part of the DMA transfer being mapped is above 4GB, then
>> -  // map the DMA transfer to a buffer below 4GB.
>> -  //
>> -  PhysicalAddress = (EFI_PHYSICAL_ADDRESS) (UINTN) HostAddress;
>> -  if ((PhysicalAddress + *NumberOfBytes) > 0x100000000ULL) {
>> -
>> -    //
>> -    // Common Buffer operations can not be remapped.  If the common buffer
>> -    // if above 4GB, then it is not possible to generate a mapping, so return
>> -    // an error.
>> -    //
>> -    if (Operation == EfiPciOperationBusMasterCommonBuffer || Operation == EfiPciOperationBusMasterCommonBuffer64) {
>> -      return EFI_UNSUPPORTED;
>> -    }
>> -
>> -    //
>> -    // Allocate a MAP_INFO structure to remember the mapping when Unmap() is
>> -    // called later.
>> -    //
>> -    Status = gBS->AllocatePool (
>> -                    EfiBootServicesData,
>> -                    sizeof(MAP_INFO),
>> -                    (VOID **)&MapInfo
>> -                    );
>> -    if (EFI_ERROR (Status)) {
>> -      *NumberOfBytes = 0;
>> -      return Status;
>> -    }
>> -
>> -    //
>> -    // Return a pointer to the MAP_INFO structure in Mapping
>> -    //
>> -    *Mapping = MapInfo;
>> -
>> -    //
>> -    // Initialize the MAP_INFO structure
>> -    //
>> -    MapInfo->Operation         = Operation;
>> -    MapInfo->NumberOfBytes     = *NumberOfBytes;
>> -    MapInfo->NumberOfPages     = EFI_SIZE_TO_PAGES(*NumberOfBytes);
>> -    MapInfo->HostAddress       = PhysicalAddress;
>> -    MapInfo->MappedHostAddress = 0x00000000ffffffff;
>> -
>> -    //
>> -    // Allocate a buffer below 4GB to map the transfer to.
>> -    //
>> -    Status = gBS->AllocatePages (
>> -                    AllocateMaxAddress,
>> -                    EfiBootServicesData,
>> -                    MapInfo->NumberOfPages,
>> -                    &MapInfo->MappedHostAddress
>> -                    );
>> -    if (EFI_ERROR (Status)) {
>> -      gBS->FreePool (MapInfo);
>> -      *NumberOfBytes = 0;
>> -      return Status;
>> -    }
>> -
>> -    //
>> -    // If this is a read operation from the Bus Master's point of view,
>> -    // then copy the contents of the real buffer into the mapped buffer
>> -    // so the Bus Master can read the contents of the real buffer.
>> -    //
>> -    if (Operation == EfiPciOperationBusMasterRead || Operation == EfiPciOperationBusMasterRead64) {
>> -      CopyMem (
>> -        (VOID *)(UINTN)MapInfo->MappedHostAddress,
>> -        (VOID *)(UINTN)MapInfo->HostAddress,
>> -        MapInfo->NumberOfBytes
>> -        );
>> -    }
>> -
>> -    //
>> -    // The DeviceAddress is the address of the maped buffer below 4GB
>> -    //
>> -    *DeviceAddress = MapInfo->MappedHostAddress;
>> -  } else {
>> -    //
>> -    // The transfer is below 4GB, so the DeviceAddress is simply the HostAddress
>> -    //
>> -    *DeviceAddress = PhysicalAddress;
>> -  }
>> +  *DeviceAddress = PhysicalAddress;
>
> The 3 errors I had:
> /tmp/build/edk2/OpenPlatformPkg/Platforms/AMD/Styx/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c:
> In function 'RootBridgeIoMap':
> /tmp/build/edk2/OpenPlatformPkg/Platforms/AMD/Styx/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c:1699:13:
> error: unused variable 'MapInfo'
>  [-Werror=unused-variable]
>    MAP_INFO              *MapInfo;
>              ^
> /tmp/build/edk2/OpenPlatformPkg/Platforms/AMD/Styx/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c:1697:14:
> error: unused variable 'Status'
> [-Werror=unused-variable]
>    EFI_STATUS            Status;
>               ^

These can both simply be dropped


> /tmp/build/edk2/OpenPlatformPkg/Platforms/AMD/Styx/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c:1718:18:
> error: 'PhysicalAddress' may be
> used uninitialized in this function [-Werror=maybe-uninitialized]
>    *DeviceAddress = PhysicalAddress;
>                   ^
> cc1: all warnings being treated as errors
>
> Here should *DeviceAddress be just HostAddress?

No, please use

*DeviceAddress = (EFI_PHYSICAL_ADDRESS) (UINTN) HostAddress;

and drop the declaration of PhysicalAddress
Ricardo Salveti April 21, 2016, 5:59 p.m. | #5
On Thu, Apr 21, 2016 at 2:31 PM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> On 21 April 2016 at 19:29, Ricardo Salveti <ricardo.salveti@linaro.org> wrote:
>> Ircloud is off for me, so replying here instead.
>>
>> On Fri, Apr 15, 2016 at 8:52 AM, Ard Biesheuvel
>> <ard.biesheuvel@linaro.org> wrote:
>>> The copy-pasted PciHostBridgeDxe driver has some interesting restrictions
>>> that are difficult to fulfil on Seattle, i.e., its AllocateBuffer() and
>>> IoMap() operations require the DMA address to be below 4 GB. This would
>>> only makes sense in the presence of 32-bit only DMA bus masters that are
>>> not behind a SMMU, but in the Seattle case, it is completely pointless
>>> since it does not have any RAM below 4 GB in the first place.
>>>
>>> So simply drop these restrictions.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> ---
>>>  .../Drivers/PciHostBridgeDxe/PciRootBridgeIo.c     | 91 +---------------------
>>>  1 file changed, 2 insertions(+), 89 deletions(-)
>>>
>>> diff --git a/Platforms/AMD/Styx/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c b/Platforms/AMD/Styx/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c
>>> index 4ac89fb7548f..941c330a4228 100644
>>> --- a/Platforms/AMD/Styx/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c
>>> +++ b/Platforms/AMD/Styx/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c
>>> @@ -1715,89 +1715,7 @@ DEBUG((EFI_D_ERROR, "RootBridgeIoMap() - %d\n", __LINE__));
>>>      return EFI_INVALID_PARAMETER;
>>>    }
>>>
>>> -  //
>>> -  // Most PCAT like chipsets can not handle performing DMA above 4GB.
>>> -  // If any part of the DMA transfer being mapped is above 4GB, then
>>> -  // map the DMA transfer to a buffer below 4GB.
>>> -  //
>>> -  PhysicalAddress = (EFI_PHYSICAL_ADDRESS) (UINTN) HostAddress;
>>> -  if ((PhysicalAddress + *NumberOfBytes) > 0x100000000ULL) {
>>> -
>>> -    //
>>> -    // Common Buffer operations can not be remapped.  If the common buffer
>>> -    // if above 4GB, then it is not possible to generate a mapping, so return
>>> -    // an error.
>>> -    //
>>> -    if (Operation == EfiPciOperationBusMasterCommonBuffer || Operation == EfiPciOperationBusMasterCommonBuffer64) {
>>> -      return EFI_UNSUPPORTED;
>>> -    }
>>> -
>>> -    //
>>> -    // Allocate a MAP_INFO structure to remember the mapping when Unmap() is
>>> -    // called later.
>>> -    //
>>> -    Status = gBS->AllocatePool (
>>> -                    EfiBootServicesData,
>>> -                    sizeof(MAP_INFO),
>>> -                    (VOID **)&MapInfo
>>> -                    );
>>> -    if (EFI_ERROR (Status)) {
>>> -      *NumberOfBytes = 0;
>>> -      return Status;
>>> -    }
>>> -
>>> -    //
>>> -    // Return a pointer to the MAP_INFO structure in Mapping
>>> -    //
>>> -    *Mapping = MapInfo;
>>> -
>>> -    //
>>> -    // Initialize the MAP_INFO structure
>>> -    //
>>> -    MapInfo->Operation         = Operation;
>>> -    MapInfo->NumberOfBytes     = *NumberOfBytes;
>>> -    MapInfo->NumberOfPages     = EFI_SIZE_TO_PAGES(*NumberOfBytes);
>>> -    MapInfo->HostAddress       = PhysicalAddress;
>>> -    MapInfo->MappedHostAddress = 0x00000000ffffffff;
>>> -
>>> -    //
>>> -    // Allocate a buffer below 4GB to map the transfer to.
>>> -    //
>>> -    Status = gBS->AllocatePages (
>>> -                    AllocateMaxAddress,
>>> -                    EfiBootServicesData,
>>> -                    MapInfo->NumberOfPages,
>>> -                    &MapInfo->MappedHostAddress
>>> -                    );
>>> -    if (EFI_ERROR (Status)) {
>>> -      gBS->FreePool (MapInfo);
>>> -      *NumberOfBytes = 0;
>>> -      return Status;
>>> -    }
>>> -
>>> -    //
>>> -    // If this is a read operation from the Bus Master's point of view,
>>> -    // then copy the contents of the real buffer into the mapped buffer
>>> -    // so the Bus Master can read the contents of the real buffer.
>>> -    //
>>> -    if (Operation == EfiPciOperationBusMasterRead || Operation == EfiPciOperationBusMasterRead64) {
>>> -      CopyMem (
>>> -        (VOID *)(UINTN)MapInfo->MappedHostAddress,
>>> -        (VOID *)(UINTN)MapInfo->HostAddress,
>>> -        MapInfo->NumberOfBytes
>>> -        );
>>> -    }
>>> -
>>> -    //
>>> -    // The DeviceAddress is the address of the maped buffer below 4GB
>>> -    //
>>> -    *DeviceAddress = MapInfo->MappedHostAddress;
>>> -  } else {
>>> -    //
>>> -    // The transfer is below 4GB, so the DeviceAddress is simply the HostAddress
>>> -    //
>>> -    *DeviceAddress = PhysicalAddress;
>>> -  }
>>> +  *DeviceAddress = PhysicalAddress;
>>
>> The 3 errors I had:
>> /tmp/build/edk2/OpenPlatformPkg/Platforms/AMD/Styx/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c:
>> In function 'RootBridgeIoMap':
>> /tmp/build/edk2/OpenPlatformPkg/Platforms/AMD/Styx/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c:1699:13:
>> error: unused variable 'MapInfo'
>>  [-Werror=unused-variable]
>>    MAP_INFO              *MapInfo;
>>              ^
>> /tmp/build/edk2/OpenPlatformPkg/Platforms/AMD/Styx/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c:1697:14:
>> error: unused variable 'Status'
>> [-Werror=unused-variable]
>>    EFI_STATUS            Status;
>>               ^
>
> These can both simply be dropped
>
>
>> /tmp/build/edk2/OpenPlatformPkg/Platforms/AMD/Styx/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c:1718:18:
>> error: 'PhysicalAddress' may be
>> used uninitialized in this function [-Werror=maybe-uninitialized]
>>    *DeviceAddress = PhysicalAddress;
>>                   ^
>> cc1: all warnings being treated as errors
>>
>> Here should *DeviceAddress be just HostAddress?
>
> No, please use
>
> *DeviceAddress = (EFI_PHYSICAL_ADDRESS) (UINTN) HostAddress;
>
> and drop the declaration of PhysicalAddress

Tried the fixed version and didn't help, but pushed to the private
dev-FDK101 branch as we would need it anyway.

Thanks,

Patch

diff --git a/Platforms/AMD/Styx/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c b/Platforms/AMD/Styx/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c
index 4ac89fb7548f..941c330a4228 100644
--- a/Platforms/AMD/Styx/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c
+++ b/Platforms/AMD/Styx/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c
@@ -1715,89 +1715,7 @@  DEBUG((EFI_D_ERROR, "RootBridgeIoMap() - %d\n", __LINE__));
     return EFI_INVALID_PARAMETER;
   }
 
-  //
-  // Most PCAT like chipsets can not handle performing DMA above 4GB.
-  // If any part of the DMA transfer being mapped is above 4GB, then
-  // map the DMA transfer to a buffer below 4GB.
-  //
-  PhysicalAddress = (EFI_PHYSICAL_ADDRESS) (UINTN) HostAddress;
-  if ((PhysicalAddress + *NumberOfBytes) > 0x100000000ULL) {
-
-    //
-    // Common Buffer operations can not be remapped.  If the common buffer
-    // if above 4GB, then it is not possible to generate a mapping, so return 
-    // an error.
-    //
-    if (Operation == EfiPciOperationBusMasterCommonBuffer || Operation == EfiPciOperationBusMasterCommonBuffer64) {
-      return EFI_UNSUPPORTED;
-    }
-
-    //
-    // Allocate a MAP_INFO structure to remember the mapping when Unmap() is
-    // called later.
-    //
-    Status = gBS->AllocatePool (
-                    EfiBootServicesData, 
-                    sizeof(MAP_INFO), 
-                    (VOID **)&MapInfo
-                    );
-    if (EFI_ERROR (Status)) {
-      *NumberOfBytes = 0;
-      return Status;
-    }
-
-    //
-    // Return a pointer to the MAP_INFO structure in Mapping
-    //
-    *Mapping = MapInfo;
-
-    //
-    // Initialize the MAP_INFO structure
-    //
-    MapInfo->Operation         = Operation;
-    MapInfo->NumberOfBytes     = *NumberOfBytes;
-    MapInfo->NumberOfPages     = EFI_SIZE_TO_PAGES(*NumberOfBytes);
-    MapInfo->HostAddress       = PhysicalAddress;
-    MapInfo->MappedHostAddress = 0x00000000ffffffff;
-
-    //
-    // Allocate a buffer below 4GB to map the transfer to.
-    //
-    Status = gBS->AllocatePages (
-                    AllocateMaxAddress, 
-                    EfiBootServicesData, 
-                    MapInfo->NumberOfPages,
-                    &MapInfo->MappedHostAddress
-                    );
-    if (EFI_ERROR (Status)) {
-      gBS->FreePool (MapInfo);
-      *NumberOfBytes = 0;
-      return Status;
-    }
-
-    //
-    // If this is a read operation from the Bus Master's point of view,
-    // then copy the contents of the real buffer into the mapped buffer
-    // so the Bus Master can read the contents of the real buffer.
-    //
-    if (Operation == EfiPciOperationBusMasterRead || Operation == EfiPciOperationBusMasterRead64) {
-      CopyMem (
-        (VOID *)(UINTN)MapInfo->MappedHostAddress, 
-        (VOID *)(UINTN)MapInfo->HostAddress,
-        MapInfo->NumberOfBytes
-        );
-    }
-
-    //
-    // The DeviceAddress is the address of the maped buffer below 4GB
-    //
-    *DeviceAddress = MapInfo->MappedHostAddress;
-  } else {
-    //
-    // The transfer is below 4GB, so the DeviceAddress is simply the HostAddress
-    //
-    *DeviceAddress = PhysicalAddress;
-  }
+  *DeviceAddress = PhysicalAddress;
 
   return EFI_SUCCESS;
 }
@@ -1917,12 +1835,7 @@  DEBUG((EFI_D_ERROR, "RootBridgeIoAllocateBuffer() - %d\n", __LINE__));
     return EFI_INVALID_PARAMETER;
   }
 
-  //
-  // Limit allocations to memory below 4GB
-  //
-  PhysicalAddress = (EFI_PHYSICAL_ADDRESS)(0xffffffff);
-
-  Status = gBS->AllocatePages (AllocateMaxAddress, MemoryType, Pages, &PhysicalAddress);
+  Status = gBS->AllocatePages (AllocateAnyPages, MemoryType, Pages, &PhysicalAddress);
   if (EFI_ERROR (Status)) {
     return Status;
   }