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

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

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
>
>