* [PATCH] MdeModulePkg/PciHostBridgeDxe: Fixed PCI DMA Map/Umap bounce buffer @ 2017-10-09 1:15 Daniil Egranov 2017-10-09 7:42 ` Ni, Ruiyu 0 siblings, 1 reply; 8+ messages in thread From: Daniil Egranov @ 2017-10-09 1:15 UTC (permalink / raw) To: edk2-devel; +Cc: star.zeng, leif.lindholm, ard.biesheuvel, Daniil Egranov The patch corrects the logic of transferring data between a bounce buffer and a real buffer above 4GB: 1. In the case of mapping a bounce buffer for the write operation, data from a real buffer should be copied into a bounce buffer. 2.In the case of unmapping a bounce buffer for the read operation, data should be copied from a bounce buffer into a real buffer. The patch resolves a Juno board issue with the the grub and SATA drives. Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Daniil Egranov <daniil.egranov@arm.com> --- MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c index dc06c16dc0..877fa2fd13 100644 --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c @@ -1153,12 +1153,12 @@ RootBridgeIoMap ( } // - // If this is a read operation from the Bus Master's point of view, + // If this is a write 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) { + if (Operation == EfiPciOperationBusMasterWrite || + Operation == EfiPciOperationBusMasterWrite64) { CopyMem ( (VOID *) (UINTN) MapInfo->MappedHostAddress, (VOID *) (UINTN) MapInfo->HostAddress, @@ -1256,12 +1256,12 @@ RootBridgeIoUnmap ( RemoveEntryList (&MapInfo->Link); // - // If this is a write operation from the Bus Master's point of view, + // If this is a read operation from the Bus Master's point of view, // then copy the contents of the mapped buffer into the real buffer // so the processor can read the contents of the real buffer. // - if (MapInfo->Operation == EfiPciOperationBusMasterWrite || - MapInfo->Operation == EfiPciOperationBusMasterWrite64) { + if (MapInfo->Operation == EfiPciOperationBusMasterRead || + MapInfo->Operation == EfiPciOperationBusMasterRead64) { CopyMem ( (VOID *) (UINTN) MapInfo->HostAddress, (VOID *) (UINTN) MapInfo->MappedHostAddress, -- 2.11.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] MdeModulePkg/PciHostBridgeDxe: Fixed PCI DMA Map/Umap bounce buffer 2017-10-09 1:15 [PATCH] MdeModulePkg/PciHostBridgeDxe: Fixed PCI DMA Map/Umap bounce buffer Daniil Egranov @ 2017-10-09 7:42 ` Ni, Ruiyu 2017-10-09 10:40 ` Ard Biesheuvel 0 siblings, 1 reply; 8+ messages in thread From: Ni, Ruiyu @ 2017-10-09 7:42 UTC (permalink / raw) To: Daniil Egranov, edk2-devel@lists.01.org Cc: leif.lindholm@linaro.org, Zeng, Star, ard.biesheuvel@linaro.org The "read"/"write" is from the Bus Master's point of view. Thanks/Ray > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Daniil > Egranov > Sent: Monday, October 9, 2017 9:16 AM > To: edk2-devel@lists.01.org > Cc: leif.lindholm@linaro.org; Zeng, Star <star.zeng@intel.com>; > ard.biesheuvel@linaro.org > Subject: [edk2] [PATCH] MdeModulePkg/PciHostBridgeDxe: Fixed PCI DMA > Map/Umap bounce buffer > > The patch corrects the logic of transferring data between a bounce buffer and a > real buffer above 4GB: > 1. In the case of mapping a bounce buffer for the write operation, data from a > real buffer should be copied into a bounce buffer. > 2.In the case of unmapping a bounce buffer for the read operation, data should > be copied from a bounce buffer into a real buffer. > > The patch resolves a Juno board issue with the the grub and SATA drives. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Daniil Egranov <daniil.egranov@arm.com> > --- > MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c > b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c > index dc06c16dc0..877fa2fd13 100644 > --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c > +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c > @@ -1153,12 +1153,12 @@ RootBridgeIoMap ( > } > > // > - // If this is a read operation from the Bus Master's point of view, > + // If this is a write 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) { > + if (Operation == EfiPciOperationBusMasterWrite || > + Operation == EfiPciOperationBusMasterWrite64) { > CopyMem ( > (VOID *) (UINTN) MapInfo->MappedHostAddress, > (VOID *) (UINTN) MapInfo->HostAddress, @@ -1256,12 +1256,12 @@ > RootBridgeIoUnmap ( > RemoveEntryList (&MapInfo->Link); > > // > - // If this is a write operation from the Bus Master's point of view, > + // If this is a read operation from the Bus Master's point of view, > // then copy the contents of the mapped buffer into the real buffer > // so the processor can read the contents of the real buffer. > // > - if (MapInfo->Operation == EfiPciOperationBusMasterWrite || > - MapInfo->Operation == EfiPciOperationBusMasterWrite64) { > + if (MapInfo->Operation == EfiPciOperationBusMasterRead || > + MapInfo->Operation == EfiPciOperationBusMasterRead64) { > CopyMem ( > (VOID *) (UINTN) MapInfo->HostAddress, > (VOID *) (UINTN) MapInfo->MappedHostAddress, > -- > 2.11.0 > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] MdeModulePkg/PciHostBridgeDxe: Fixed PCI DMA Map/Umap bounce buffer 2017-10-09 7:42 ` Ni, Ruiyu @ 2017-10-09 10:40 ` Ard Biesheuvel 2017-10-09 12:23 ` Ard Biesheuvel 0 siblings, 1 reply; 8+ messages in thread From: Ard Biesheuvel @ 2017-10-09 10:40 UTC (permalink / raw) To: Ni, Ruiyu Cc: Daniil Egranov, edk2-devel@lists.01.org, leif.lindholm@linaro.org, Zeng, Star On 9 October 2017 at 08:42, Ni, Ruiyu <ruiyu.ni@intel.com> wrote: > The "read"/"write" is from the Bus Master's point of view. > > > Thanks/Ray > >> -----Original Message----- >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Daniil >> Egranov >> Sent: Monday, October 9, 2017 9:16 AM >> To: edk2-devel@lists.01.org >> Cc: leif.lindholm@linaro.org; Zeng, Star <star.zeng@intel.com>; >> ard.biesheuvel@linaro.org >> Subject: [edk2] [PATCH] MdeModulePkg/PciHostBridgeDxe: Fixed PCI DMA >> Map/Umap bounce buffer >> >> The patch corrects the logic of transferring data between a bounce buffer and a >> real buffer above 4GB: >> 1. In the case of mapping a bounce buffer for the write operation, data from a >> real buffer should be copied into a bounce buffer. >> 2.In the case of unmapping a bounce buffer for the read operation, data should >> be copied from a bounce buffer into a real buffer. >> >> The patch resolves a Juno board issue with the the grub and SATA drives. >> I am intrigued by this. So as I suggested, this has to do with 64-bit DMA, but not in the way I suspected. UEFI itself never hits this code path, because it runs entirely < 32 GB, but as soon as GRUB starts allocating loader data and use it for DMA, the bounce buffering kicks in because apparently, the SATA controller is not 64-bit DMA capable. Are you using the SataSiI3132Dxe driver on Juno? Does this help at all? diff --git a/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c index 2fb5fd68db01..a938563ebdd6 100644 --- a/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c +++ b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c @@ -104,7 +104,7 @@ SiI3132AtaPassThruCommand ( } Status = PciIo->Map ( - PciIo, EfiPciIoOperationBusMasterRead, + PciIo, EfiPciIoOperationBusMasterWrite, Packet->InDataBuffer, &InDataBufferLength, &PhysInDataBuffer, &PciAllocMapping ); if (EFI_ERROR (Status)) { @@ -139,7 +139,7 @@ SiI3132AtaPassThruCommand ( OutDataBufferLength = Packet->OutTransferLength * SataDevice->BlockSize; Status = PciIo->Map ( - PciIo, EfiPciIoOperationBusMasterWrite, + PciIo, EfiPciIoOperationBusMasterRead, Packet->OutDataBuffer, &OutDataBufferLength, &PhysOutDataBuffer, &PciAllocMapping ); if (EFI_ERROR (Status)) { ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] MdeModulePkg/PciHostBridgeDxe: Fixed PCI DMA Map/Umap bounce buffer 2017-10-09 10:40 ` Ard Biesheuvel @ 2017-10-09 12:23 ` Ard Biesheuvel 2017-10-10 3:41 ` Daniil Egranov 0 siblings, 1 reply; 8+ messages in thread From: Ard Biesheuvel @ 2017-10-09 12:23 UTC (permalink / raw) To: Ni, Ruiyu Cc: Daniil Egranov, edk2-devel@lists.01.org, leif.lindholm@linaro.org, Zeng, Star On 9 October 2017 at 11:40, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 9 October 2017 at 08:42, Ni, Ruiyu <ruiyu.ni@intel.com> wrote: >> The "read"/"write" is from the Bus Master's point of view. >> >> >> Thanks/Ray >> >>> -----Original Message----- >>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Daniil >>> Egranov >>> Sent: Monday, October 9, 2017 9:16 AM >>> To: edk2-devel@lists.01.org >>> Cc: leif.lindholm@linaro.org; Zeng, Star <star.zeng@intel.com>; >>> ard.biesheuvel@linaro.org >>> Subject: [edk2] [PATCH] MdeModulePkg/PciHostBridgeDxe: Fixed PCI DMA >>> Map/Umap bounce buffer >>> >>> The patch corrects the logic of transferring data between a bounce buffer and a >>> real buffer above 4GB: >>> 1. In the case of mapping a bounce buffer for the write operation, data from a >>> real buffer should be copied into a bounce buffer. >>> 2.In the case of unmapping a bounce buffer for the read operation, data should >>> be copied from a bounce buffer into a real buffer. >>> >>> The patch resolves a Juno board issue with the the grub and SATA drives. >>> > > I am intrigued by this. > > So as I suggested, this has to do with 64-bit DMA, but not in the way > I suspected. UEFI itself never hits this code path, because it runs > entirely < 32 GB, but as soon as GRUB starts allocating loader data > and use it for DMA, the bounce buffering kicks in because apparently, > the SATA controller is not 64-bit DMA capable. > > Are you using the SataSiI3132Dxe driver on Juno? Does this help at all? > > diff --git a/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c > b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c > index 2fb5fd68db01..a938563ebdd6 100644 > --- a/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c > +++ b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c > @@ -104,7 +104,7 @@ SiI3132AtaPassThruCommand ( > } > > Status = PciIo->Map ( > - PciIo, EfiPciIoOperationBusMasterRead, > + PciIo, EfiPciIoOperationBusMasterWrite, > Packet->InDataBuffer, &InDataBufferLength, > &PhysInDataBuffer, &PciAllocMapping > ); > if (EFI_ERROR (Status)) { > @@ -139,7 +139,7 @@ SiI3132AtaPassThruCommand ( > OutDataBufferLength = Packet->OutTransferLength * SataDevice->BlockSize; > > Status = PciIo->Map ( > - PciIo, EfiPciIoOperationBusMasterWrite, > + PciIo, EfiPciIoOperationBusMasterRead, > Packet->OutDataBuffer, &OutDataBufferLength, > &PhysOutDataBuffer, &PciAllocMapping > ); > if (EFI_ERROR (Status)) { Also, it might make sense to find out if the hardware is really not 64-bit DMA capable, or whether the driver simply fails to set the EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE attribute. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] MdeModulePkg/PciHostBridgeDxe: Fixed PCI DMA Map/Umap bounce buffer 2017-10-09 12:23 ` Ard Biesheuvel @ 2017-10-10 3:41 ` Daniil Egranov 2017-10-10 8:59 ` Ard Biesheuvel 0 siblings, 1 reply; 8+ messages in thread From: Daniil Egranov @ 2017-10-10 3:41 UTC (permalink / raw) To: Ard Biesheuvel, Ni, Ruiyu Cc: Zeng, Star, edk2-devel@lists.01.org, leif.lindholm@linaro.org Hi Ard, Ray, Thanks for your comments. On 10/09/2017 07:23 AM, Ard Biesheuvel wrote: > On 9 October 2017 at 11:40, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >> On 9 October 2017 at 08:42, Ni, Ruiyu <ruiyu.ni@intel.com> wrote: >>> The "read"/"write" is from the Bus Master's point of view. >>> >>> >>> Thanks/Ray >>> >>>> -----Original Message----- >>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Daniil >>>> Egranov >>>> Sent: Monday, October 9, 2017 9:16 AM >>>> To: edk2-devel@lists.01.org >>>> Cc: leif.lindholm@linaro.org; Zeng, Star <star.zeng@intel.com>; >>>> ard.biesheuvel@linaro.org >>>> Subject: [edk2] [PATCH] MdeModulePkg/PciHostBridgeDxe: Fixed PCI DMA >>>> Map/Umap bounce buffer >>>> >>>> The patch corrects the logic of transferring data between a bounce buffer and a >>>> real buffer above 4GB: >>>> 1. In the case of mapping a bounce buffer for the write operation, data from a >>>> real buffer should be copied into a bounce buffer. >>>> 2.In the case of unmapping a bounce buffer for the read operation, data should >>>> be copied from a bounce buffer into a real buffer. >>>> >>>> The patch resolves a Juno board issue with the the grub and SATA drives. >>>> >> I am intrigued by this. >> >> So as I suggested, this has to do with 64-bit DMA, but not in the way >> I suspected. UEFI itself never hits this code path, because it runs >> entirely < 32 GB, but as soon as GRUB starts allocating loader data >> and use it for DMA, the bounce buffering kicks in because apparently, >> the SATA controller is not 64-bit DMA capable. >> >> Are you using the SataSiI3132Dxe driver on Juno? Does this help at all? >> >> diff --git a/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c >> b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c >> index 2fb5fd68db01..a938563ebdd6 100644 >> --- a/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c >> +++ b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c >> @@ -104,7 +104,7 @@ SiI3132AtaPassThruCommand ( >> } >> >> Status = PciIo->Map ( >> - PciIo, EfiPciIoOperationBusMasterRead, >> + PciIo, EfiPciIoOperationBusMasterWrite, >> Packet->InDataBuffer, &InDataBufferLength, >> &PhysInDataBuffer, &PciAllocMapping >> ); >> if (EFI_ERROR (Status)) { >> @@ -139,7 +139,7 @@ SiI3132AtaPassThruCommand ( >> OutDataBufferLength = Packet->OutTransferLength * SataDevice->BlockSize; >> >> Status = PciIo->Map ( >> - PciIo, EfiPciIoOperationBusMasterWrite, >> + PciIo, EfiPciIoOperationBusMasterRead, >> Packet->OutDataBuffer, &OutDataBufferLength, >> &PhysOutDataBuffer, &PciAllocMapping >> ); >> if (EFI_ERROR (Status)) { > Also, it might make sense to find out if the hardware is really not > 64-bit DMA capable, or whether the driver simply fails to set the > EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE attribute. > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel Swapping the EfiPciIoOperationBusMasterRead and EfiPciIoOperationBusMasterWrite operations in the SiI3132AtaPassThru.c fixes the problem as well. The driver's patch will be the correct fix for this issue. I think i missed the point what these operations are from the Bus Master's perspective. The old PciHostBridge Juno driver was using NullDmaLib so it was direct mapping. That explains why the SataSiI3132Dxe worked with the original host bridge driver and failed with the new one. Thanks, Daniil ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] MdeModulePkg/PciHostBridgeDxe: Fixed PCI DMA Map/Umap bounce buffer 2017-10-10 3:41 ` Daniil Egranov @ 2017-10-10 8:59 ` Ard Biesheuvel 2017-10-27 5:58 ` Daniil Egranov 0 siblings, 1 reply; 8+ messages in thread From: Ard Biesheuvel @ 2017-10-10 8:59 UTC (permalink / raw) To: Daniil Egranov Cc: Ni, Ruiyu, Zeng, Star, edk2-devel@lists.01.org, leif.lindholm@linaro.org On 10 October 2017 at 04:41, Daniil Egranov <daniil.egranov@arm.com> wrote: > Hi Ard, Ray, > > Thanks for your comments. > > > On 10/09/2017 07:23 AM, Ard Biesheuvel wrote: >> >> On 9 October 2017 at 11:40, Ard Biesheuvel <ard.biesheuvel@linaro.org> >> wrote: >>> >>> On 9 October 2017 at 08:42, Ni, Ruiyu <ruiyu.ni@intel.com> wrote: >>>> >>>> The "read"/"write" is from the Bus Master's point of view. >>>> >>>> >>>> Thanks/Ray >>>> >>>>> -----Original Message----- >>>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of >>>>> Daniil >>>>> Egranov >>>>> Sent: Monday, October 9, 2017 9:16 AM >>>>> To: edk2-devel@lists.01.org >>>>> Cc: leif.lindholm@linaro.org; Zeng, Star <star.zeng@intel.com>; >>>>> ard.biesheuvel@linaro.org >>>>> Subject: [edk2] [PATCH] MdeModulePkg/PciHostBridgeDxe: Fixed PCI DMA >>>>> Map/Umap bounce buffer >>>>> >>>>> The patch corrects the logic of transferring data between a bounce >>>>> buffer and a >>>>> real buffer above 4GB: >>>>> 1. In the case of mapping a bounce buffer for the write operation, data >>>>> from a >>>>> real buffer should be copied into a bounce buffer. >>>>> 2.In the case of unmapping a bounce buffer for the read operation, data >>>>> should >>>>> be copied from a bounce buffer into a real buffer. >>>>> >>>>> The patch resolves a Juno board issue with the the grub and SATA >>>>> drives. >>>>> >>> I am intrigued by this. >>> >>> So as I suggested, this has to do with 64-bit DMA, but not in the way >>> I suspected. UEFI itself never hits this code path, because it runs >>> entirely < 32 GB, but as soon as GRUB starts allocating loader data >>> and use it for DMA, the bounce buffering kicks in because apparently, >>> the SATA controller is not 64-bit DMA capable. >>> >>> Are you using the SataSiI3132Dxe driver on Juno? Does this help at all? >>> >>> diff --git a/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c >>> b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c >>> index 2fb5fd68db01..a938563ebdd6 100644 >>> --- a/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c >>> +++ b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c >>> @@ -104,7 +104,7 @@ SiI3132AtaPassThruCommand ( >>> } >>> >>> Status = PciIo->Map ( >>> - PciIo, EfiPciIoOperationBusMasterRead, >>> + PciIo, EfiPciIoOperationBusMasterWrite, >>> Packet->InDataBuffer, &InDataBufferLength, >>> &PhysInDataBuffer, &PciAllocMapping >>> ); >>> if (EFI_ERROR (Status)) { >>> @@ -139,7 +139,7 @@ SiI3132AtaPassThruCommand ( >>> OutDataBufferLength = Packet->OutTransferLength * >>> SataDevice->BlockSize; >>> >>> Status = PciIo->Map ( >>> - PciIo, EfiPciIoOperationBusMasterWrite, >>> + PciIo, EfiPciIoOperationBusMasterRead, >>> Packet->OutDataBuffer, &OutDataBufferLength, >>> &PhysOutDataBuffer, &PciAllocMapping >>> ); >>> if (EFI_ERROR (Status)) { >> >> Also, it might make sense to find out if the hardware is really not >> 64-bit DMA capable, or whether the driver simply fails to set the >> EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE attribute. >> _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.01.org >> https://lists.01.org/mailman/listinfo/edk2-devel > > Swapping the EfiPciIoOperationBusMasterRead and > EfiPciIoOperationBusMasterWrite operations in the SiI3132AtaPassThru.c fixes > the problem as well. The driver's patch will be the correct fix for this > issue. I think i missed the point what these operations are from the Bus > Master's perspective. > Good! > The old PciHostBridge Juno driver was using NullDmaLib so it was direct > mapping. That explains why the SataSiI3132Dxe worked with the original host > bridge driver and failed with the new one. > NullDmaLib has nothing to do with this. The difference between the old driver and the generic one is that the old driver always enables 64-bit DMA, while the generic one only does so if the driver sets the EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE attribute. So to fix this driver, we should a) fix the swapped constants above b) set EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE in the Start() hook c) add code to disable DMA at ExitBootServices() [or the controller may scribble over RAM when the kernel takes over] d) replace mbStarted with a per-controller attribute, given that this is a UEFI driver model implementation that could theoretically drive multiple hardware instances concurrently. Thanks, Ard. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] MdeModulePkg/PciHostBridgeDxe: Fixed PCI DMA Map/Umap bounce buffer 2017-10-10 8:59 ` Ard Biesheuvel @ 2017-10-27 5:58 ` Daniil Egranov 2017-10-27 7:23 ` Ard Biesheuvel 0 siblings, 1 reply; 8+ messages in thread From: Daniil Egranov @ 2017-10-27 5:58 UTC (permalink / raw) To: Ard Biesheuvel Cc: Ni, Ruiyu, edk2-devel@lists.01.org, leif.lindholm@linaro.org, Zeng, Star Hi Ard, Thanks for you comments. Please see my comments below. On 10/10/2017 03:59 AM, Ard Biesheuvel wrote: > On 10 October 2017 at 04:41, Daniil Egranov <daniil.egranov@arm.com> wrote: >> Hi Ard, Ray, >> >> Thanks for your comments. >> >> >> On 10/09/2017 07:23 AM, Ard Biesheuvel wrote: >>> On 9 October 2017 at 11:40, Ard Biesheuvel <ard.biesheuvel@linaro.org> >>> wrote: >>>> On 9 October 2017 at 08:42, Ni, Ruiyu <ruiyu.ni@intel.com> wrote: >>>>> The "read"/"write" is from the Bus Master's point of view. >>>>> >>>>> >>>>> Thanks/Ray >>>>> >>>>>> -----Original Message----- >>>>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of >>>>>> Daniil >>>>>> Egranov >>>>>> Sent: Monday, October 9, 2017 9:16 AM >>>>>> To: edk2-devel@lists.01.org >>>>>> Cc: leif.lindholm@linaro.org; Zeng, Star <star.zeng@intel.com>; >>>>>> ard.biesheuvel@linaro.org >>>>>> Subject: [edk2] [PATCH] MdeModulePkg/PciHostBridgeDxe: Fixed PCI DMA >>>>>> Map/Umap bounce buffer >>>>>> >>>>>> The patch corrects the logic of transferring data between a bounce >>>>>> buffer and a >>>>>> real buffer above 4GB: >>>>>> 1. In the case of mapping a bounce buffer for the write operation, data >>>>>> from a >>>>>> real buffer should be copied into a bounce buffer. >>>>>> 2.In the case of unmapping a bounce buffer for the read operation, data >>>>>> should >>>>>> be copied from a bounce buffer into a real buffer. >>>>>> >>>>>> The patch resolves a Juno board issue with the the grub and SATA >>>>>> drives. >>>>>> >>>> I am intrigued by this. >>>> >>>> So as I suggested, this has to do with 64-bit DMA, but not in the way >>>> I suspected. UEFI itself never hits this code path, because it runs >>>> entirely < 32 GB, but as soon as GRUB starts allocating loader data >>>> and use it for DMA, the bounce buffering kicks in because apparently, >>>> the SATA controller is not 64-bit DMA capable. >>>> >>>> Are you using the SataSiI3132Dxe driver on Juno? Does this help at all? >>>> >>>> diff --git a/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c >>>> b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c >>>> index 2fb5fd68db01..a938563ebdd6 100644 >>>> --- a/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c >>>> +++ b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c >>>> @@ -104,7 +104,7 @@ SiI3132AtaPassThruCommand ( >>>> } >>>> >>>> Status = PciIo->Map ( >>>> - PciIo, EfiPciIoOperationBusMasterRead, >>>> + PciIo, EfiPciIoOperationBusMasterWrite, >>>> Packet->InDataBuffer, &InDataBufferLength, >>>> &PhysInDataBuffer, &PciAllocMapping >>>> ); >>>> if (EFI_ERROR (Status)) { >>>> @@ -139,7 +139,7 @@ SiI3132AtaPassThruCommand ( >>>> OutDataBufferLength = Packet->OutTransferLength * >>>> SataDevice->BlockSize; >>>> >>>> Status = PciIo->Map ( >>>> - PciIo, EfiPciIoOperationBusMasterWrite, >>>> + PciIo, EfiPciIoOperationBusMasterRead, >>>> Packet->OutDataBuffer, &OutDataBufferLength, >>>> &PhysOutDataBuffer, &PciAllocMapping >>>> ); >>>> if (EFI_ERROR (Status)) { >>> Also, it might make sense to find out if the hardware is really not >>> 64-bit DMA capable, or whether the driver simply fails to set the >>> EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE attribute. >>> _______________________________________________ >>> edk2-devel mailing list >>> edk2-devel@lists.01.org >>> https://lists.01.org/mailman/listinfo/edk2-devel >> Swapping the EfiPciIoOperationBusMasterRead and >> EfiPciIoOperationBusMasterWrite operations in the SiI3132AtaPassThru.c fixes >> the problem as well. The driver's patch will be the correct fix for this >> issue. I think i missed the point what these operations are from the Bus >> Master's perspective. >> > Good! > >> The old PciHostBridge Juno driver was using NullDmaLib so it was direct >> mapping. That explains why the SataSiI3132Dxe worked with the original host >> bridge driver and failed with the new one. >> > NullDmaLib has nothing to do with this. The difference between the old > driver and the generic one is that the old driver always enables > 64-bit DMA, while the generic one only does so if the driver sets the > EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE attribute. So to fix this > driver, we should I meant what the NullDmaLib masked out this issue. > a) fix the swapped constants above > b) set EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE in the Start() hook The PCIe is a serial bus protocol and does not implement DAC. The meaning of this attribute is understandable but the name is incorrect. PCIe designed with native 64-bit addressing so in context of PCIe this attribute is not valid ... and I doubt what any legacy PCI devices are still exist/usable. Anyway, I set this attribute in the patch. In Juno case, bounce buffer is not used anymore. > c) add code to disable DMA at ExitBootServices() [or the controller > may scribble over RAM when the kernel takes over] > d) replace mbStarted with a per-controller attribute, given that this > is a UEFI driver model implementation that could theoretically drive > multiple hardware instances concurrently. I sent a set of patches with all changes. Please take a look. Thanks, Daniil > Thanks, > Ard. > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] MdeModulePkg/PciHostBridgeDxe: Fixed PCI DMA Map/Umap bounce buffer 2017-10-27 5:58 ` Daniil Egranov @ 2017-10-27 7:23 ` Ard Biesheuvel 0 siblings, 0 replies; 8+ messages in thread From: Ard Biesheuvel @ 2017-10-27 7:23 UTC (permalink / raw) To: Daniil Egranov Cc: Ni, Ruiyu, edk2-devel@lists.01.org, leif.lindholm@linaro.org, Zeng, Star On 27 October 2017 at 06:58, Daniil Egranov <daniil.egranov@arm.com> wrote: > Hi Ard, > > Thanks for you comments. Please see my comments below. > > > > On 10/10/2017 03:59 AM, Ard Biesheuvel wrote: >> >> On 10 October 2017 at 04:41, Daniil Egranov <daniil.egranov@arm.com> >> wrote: >>> >>> Hi Ard, Ray, >>> >>> Thanks for your comments. >>> >>> >>> On 10/09/2017 07:23 AM, Ard Biesheuvel wrote: >>>> >>>> On 9 October 2017 at 11:40, Ard Biesheuvel <ard.biesheuvel@linaro.org> >>>> wrote: >>>>> >>>>> On 9 October 2017 at 08:42, Ni, Ruiyu <ruiyu.ni@intel.com> wrote: >>>>>> >>>>>> The "read"/"write" is from the Bus Master's point of view. >>>>>> >>>>>> >>>>>> Thanks/Ray >>>>>> >>>>>>> -----Original Message----- >>>>>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf >>>>>>> Of >>>>>>> Daniil >>>>>>> Egranov >>>>>>> Sent: Monday, October 9, 2017 9:16 AM >>>>>>> To: edk2-devel@lists.01.org >>>>>>> Cc: leif.lindholm@linaro.org; Zeng, Star <star.zeng@intel.com>; >>>>>>> ard.biesheuvel@linaro.org >>>>>>> Subject: [edk2] [PATCH] MdeModulePkg/PciHostBridgeDxe: Fixed PCI DMA >>>>>>> Map/Umap bounce buffer >>>>>>> >>>>>>> The patch corrects the logic of transferring data between a bounce >>>>>>> buffer and a >>>>>>> real buffer above 4GB: >>>>>>> 1. In the case of mapping a bounce buffer for the write operation, >>>>>>> data >>>>>>> from a >>>>>>> real buffer should be copied into a bounce buffer. >>>>>>> 2.In the case of unmapping a bounce buffer for the read operation, >>>>>>> data >>>>>>> should >>>>>>> be copied from a bounce buffer into a real buffer. >>>>>>> >>>>>>> The patch resolves a Juno board issue with the the grub and SATA >>>>>>> drives. >>>>>>> >>>>> I am intrigued by this. >>>>> >>>>> So as I suggested, this has to do with 64-bit DMA, but not in the way >>>>> I suspected. UEFI itself never hits this code path, because it runs >>>>> entirely < 32 GB, but as soon as GRUB starts allocating loader data >>>>> and use it for DMA, the bounce buffering kicks in because apparently, >>>>> the SATA controller is not 64-bit DMA capable. >>>>> >>>>> Are you using the SataSiI3132Dxe driver on Juno? Does this help at all? >>>>> >>>>> diff --git a/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c >>>>> b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c >>>>> index 2fb5fd68db01..a938563ebdd6 100644 >>>>> --- a/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c >>>>> +++ b/EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c >>>>> @@ -104,7 +104,7 @@ SiI3132AtaPassThruCommand ( >>>>> } >>>>> >>>>> Status = PciIo->Map ( >>>>> - PciIo, EfiPciIoOperationBusMasterRead, >>>>> + PciIo, EfiPciIoOperationBusMasterWrite, >>>>> Packet->InDataBuffer, &InDataBufferLength, >>>>> &PhysInDataBuffer, &PciAllocMapping >>>>> ); >>>>> if (EFI_ERROR (Status)) { >>>>> @@ -139,7 +139,7 @@ SiI3132AtaPassThruCommand ( >>>>> OutDataBufferLength = Packet->OutTransferLength * >>>>> SataDevice->BlockSize; >>>>> >>>>> Status = PciIo->Map ( >>>>> - PciIo, EfiPciIoOperationBusMasterWrite, >>>>> + PciIo, EfiPciIoOperationBusMasterRead, >>>>> Packet->OutDataBuffer, &OutDataBufferLength, >>>>> &PhysOutDataBuffer, &PciAllocMapping >>>>> ); >>>>> if (EFI_ERROR (Status)) { >>>> >>>> Also, it might make sense to find out if the hardware is really not >>>> 64-bit DMA capable, or whether the driver simply fails to set the >>>> EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE attribute. >>>> _______________________________________________ >>>> edk2-devel mailing list >>>> edk2-devel@lists.01.org >>>> https://lists.01.org/mailman/listinfo/edk2-devel >>> >>> Swapping the EfiPciIoOperationBusMasterRead and >>> EfiPciIoOperationBusMasterWrite operations in the SiI3132AtaPassThru.c >>> fixes >>> the problem as well. The driver's patch will be the correct fix for this >>> issue. I think i missed the point what these operations are from the Bus >>> Master's perspective. >>> >> Good! >> >>> The old PciHostBridge Juno driver was using NullDmaLib so it was direct >>> mapping. That explains why the SataSiI3132Dxe worked with the original >>> host >>> bridge driver and failed with the new one. >>> >> NullDmaLib has nothing to do with this. The difference between the old >> driver and the generic one is that the old driver always enables >> 64-bit DMA, while the generic one only does so if the driver sets the >> EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE attribute. So to fix this >> driver, we should > > > I meant what the NullDmaLib masked out this issue. > >> a) fix the swapped constants above >> b) set EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE in the Start() hook > > > The PCIe is a serial bus protocol and does not implement DAC. The meaning of > this attribute is understandable but the name is incorrect. PCIe designed > with native 64-bit addressing so in context of PCIe this attribute is not > valid ... and I doubt what any legacy PCI devices are still exist/usable. > I agree the naming is a bit obsolete but there are plenty of PCIe devices that only support 32-bit DMA so the fact that PCIe implements 64-bit addressing natively is not entirely relevant. > Anyway, I set this attribute in the patch. In Juno case, bounce buffer is > not used anymore. > Good! >> c) add code to disable DMA at ExitBootServices() [or the controller >> may scribble over RAM when the kernel takes over] >> d) replace mbStarted with a per-controller attribute, given that this >> is a UEFI driver model implementation that could theoretically drive >> multiple hardware instances concurrently. > > > I sent a set of patches with all changes. Please take a look. > > Thanks, > Daniil > >> Thanks, >> Ard. >> _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.01.org >> https://lists.01.org/mailman/listinfo/edk2-devel > > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-10-27 7:19 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-10-09 1:15 [PATCH] MdeModulePkg/PciHostBridgeDxe: Fixed PCI DMA Map/Umap bounce buffer Daniil Egranov 2017-10-09 7:42 ` Ni, Ruiyu 2017-10-09 10:40 ` Ard Biesheuvel 2017-10-09 12:23 ` Ard Biesheuvel 2017-10-10 3:41 ` Daniil Egranov 2017-10-10 8:59 ` Ard Biesheuvel 2017-10-27 5:58 ` Daniil Egranov 2017-10-27 7:23 ` Ard Biesheuvel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox