From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=217.140.101.70; helo=foss.arm.com; envelope-from=daniil.egranov@arm.com; receiver=edk2-devel@lists.01.org Received: from foss.arm.com (foss.arm.com [217.140.101.70]) by ml01.01.org (Postfix) with ESMTP id 8BF3F2034A87B for ; Thu, 26 Oct 2017 22:54:18 -0700 (PDT) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 4FE851529; Thu, 26 Oct 2017 22:58:05 -0700 (PDT) Received: from [10.118.34.10] (dbox2.austin.arm.com [10.118.34.10]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id D5EFE3F25D; Thu, 26 Oct 2017 22:58:04 -0700 (PDT) To: Ard Biesheuvel Cc: "Ni, Ruiyu" , "edk2-devel@lists.01.org" , "leif.lindholm@linaro.org" , "Zeng, Star" References: <20171009011539.45115-1-daniil.egranov@arm.com> <734D49CCEBEEF84792F5B80ED585239D5BA8A854@SHSMSX104.ccr.corp.intel.com> <5a1c0f14-90e6-7cba-4189-fce9607dc5dd@arm.com> From: Daniil Egranov Message-ID: <9c321c94-0db4-e72e-e2e8-8f21734fe721@arm.com> Date: Fri, 27 Oct 2017 00:58:04 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: Subject: Re: [PATCH] MdeModulePkg/PciHostBridgeDxe: Fixed PCI DMA Map/Umap bounce buffer X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 27 Oct 2017 05:54:18 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US 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 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 >>> wrote: >>>> On 9 October 2017 at 08:42, Ni, Ruiyu 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 ; >>>>>> 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