From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c06::231; helo=mail-io0-x231.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-io0-x231.google.com (mail-io0-x231.google.com [IPv6:2607:f8b0:4001:c06::231]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 42DDE2095B09A for ; Tue, 10 Oct 2017 01:56:00 -0700 (PDT) Received: by mail-io0-x231.google.com with SMTP id m81so13887702ioi.13 for ; Tue, 10 Oct 2017 01:59:29 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=spbxBEXcuArsSSmWYOuQD0xFtFv7rjnqJFPeuHIxulQ=; b=eRmSFKv7G01PRjEN319oOP0LBT//IrzNmlIkYrUWv/MspkCJl19uTHOK9lhguscq1B dMy+9r6BM2324sAEfNE2Fl0qOtoWmCW2jpfmAFPVVAHlyqEJ1Weo6WP0DMC9gaYcEUDH KezKUTLxTpxCe5GwUBZKqlRT9tVs/HS9GEcfM= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=spbxBEXcuArsSSmWYOuQD0xFtFv7rjnqJFPeuHIxulQ=; b=IdAqIW5kwOKKOpApDIpC34aq41c8BQyABpiYIQ5xyuWMDXAV2w7OSzkhmmFiwU7ntp PxHqSoaZfxvETJGeu0MRFCgltbEV1t69l7XjH9LvcmEKyVUAcPMw+lMp0vr5WJ7MAykl z7h+xIMPlLlc2/ONe4fegzH+TRpcs7tDj9hvZcZL7KD/CcVzsdhjKsG+zMHioyl61ujG EhW5977dzcmxU8Ek/TWIY3gj77qA0jlfVMP7aJVSVpoGdIqVg0XMG9bKkkOQMgtIJKLO 1MJR+T28zpRBXWQ0p4R3zXwrw8TW/iqAxwYHi39ZWGAeiARFFVwpUN5PLyKPciQE+Kgh d4PA== X-Gm-Message-State: AMCzsaXjVaDfYEu7Ln6Bw+pWbD46itIGtSfZiZZLzl2k8MmOKMadU41g lYMGRPMIibKAJNen7h5EmymGMsi3LBMwgGBQ/rgY9A== X-Google-Smtp-Source: AOwi7QAa7m8DYMTzBAMYGVCTTZpFi9l+OFqJhS/Q8kOH/UgLSMHYvhtlMAG7949W5eaXKu7qN436+bLMyLZTzzwrgZY= X-Received: by 10.107.154.138 with SMTP id c132mr11180188ioe.95.1507625968422; Tue, 10 Oct 2017 01:59:28 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.131.167 with HTTP; Tue, 10 Oct 2017 01:59:27 -0700 (PDT) In-Reply-To: <5a1c0f14-90e6-7cba-4189-fce9607dc5dd@arm.com> References: <20171009011539.45115-1-daniil.egranov@arm.com> <734D49CCEBEEF84792F5B80ED585239D5BA8A854@SHSMSX104.ccr.corp.intel.com> <5a1c0f14-90e6-7cba-4189-fce9607dc5dd@arm.com> From: Ard Biesheuvel Date: Tue, 10 Oct 2017 09:59:27 +0100 Message-ID: To: Daniil Egranov Cc: "Ni, Ruiyu" , "Zeng, Star" , "edk2-devel@lists.01.org" , "leif.lindholm@linaro.org" 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: Tue, 10 Oct 2017 08:56:01 -0000 Content-Type: text/plain; charset="UTF-8" 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 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.