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

[Daniil Egranov <daniil.egranov@arm.com>]

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