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

[Ard Biesheuvel <ard.biesheuvel@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.