| Message ID | 1460721170-10347-1-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
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
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 >
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,
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
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,
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; }
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(-)