Re: [PATCH] MdeModulePkg/PciHostBridgeDxe: Fixed PCI DMA Map/Umap bounce buffer

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

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.