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::22e; helo=mail-io0-x22e.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-io0-x22e.google.com (mail-io0-x22e.google.com [IPv6:2607:f8b0:4001:c06::22e]) (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 7DCA62034A893 for ; Fri, 27 Oct 2017 00:19:58 -0700 (PDT) Received: by mail-io0-x22e.google.com with SMTP id 101so10771495ioj.3 for ; Fri, 27 Oct 2017 00:23:45 -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=2FDVW36SC7B85rP7RLxmoPdwQiOu7nT81O35aohrV1g=; b=H/uBj2f9RxwdxP4YALUVJXBCcRQAXAbqegKjX5UnwrB9qnldLzbTe8VZFyTmdDoLwL nNhCGMjY1lq6EnEdLaf4qlqoRwlnfQPlJdijSmqJVcYvcn17dVtH6j2pc8wM+EjJbxv3 HdrRHrV1K5cNpkwLri7iTGdx6kJUR4BaU4AdQ= 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=2FDVW36SC7B85rP7RLxmoPdwQiOu7nT81O35aohrV1g=; b=NExnz1Nc78TNMipNni4KYnwqjtApU0vUhMzcrPPUBuIbhsIvpw/ZSTqDa7iDZwK8eJ X1PLniZ3OkgeechFicxuVyjW9h82VYAm183vcUwy84JEoNKV7jXeGmYq7rSqwpWsFH0A nvZ39t2Dtsh6Ea2S+/NWBt5if06wL+qXnItrmn2cnuODGzzosanK7d2gQHbGIgcN93qx J4yhmZSI3916SYs63oY/pNRMO0Sda2TO0c9ONioAMmQO9lzDXvVK7IHJysTKmz/+DqSl JeumgA+iwASHgMpcbR4J20SYHHN4zQOpmPEy3FBGABMXGaqs1n6Hq/GfadGfOlZaOIRk ie5w== X-Gm-Message-State: AMCzsaUcb6fcoUirxU/JOUGgIb5zGUC+A77/FUDQ4rE6aJ+yTRFHKrYa wKupIujuO+1yjW9UhVuO7HE7J6kbhA9xezTJFRb8UQ== X-Google-Smtp-Source: ABhQp+Rd0QbeeRC+FfauhWBarZt/YvcBO6dIwy7xvISrGcGc/eu1oJ1XwsowozF+rypOn01Qt314V02Rnf2KVqEvbUw= X-Received: by 10.107.174.234 with SMTP id n103mr31265035ioo.43.1509089024629; Fri, 27 Oct 2017 00:23:44 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.131.167 with HTTP; Fri, 27 Oct 2017 00:23:44 -0700 (PDT) In-Reply-To: <9c321c94-0db4-e72e-e2e8-8f21734fe721@arm.com> References: <20171009011539.45115-1-daniil.egranov@arm.com> <734D49CCEBEEF84792F5B80ED585239D5BA8A854@SHSMSX104.ccr.corp.intel.com> <5a1c0f14-90e6-7cba-4189-fce9607dc5dd@arm.com> <9c321c94-0db4-e72e-e2e8-8f21734fe721@arm.com> From: Ard Biesheuvel Date: Fri, 27 Oct 2017 08:23:44 +0100 Message-ID: To: Daniil Egranov Cc: "Ni, Ruiyu" , "edk2-devel@lists.01.org" , "leif.lindholm@linaro.org" , "Zeng, Star" 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 07:19:58 -0000 Content-Type: text/plain; charset="UTF-8" On 27 October 2017 at 06:58, Daniil Egranov 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 >> 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. > 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 > >