On 27 October 2017 at 06:58, Daniil Egranov <[email protected]> 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 <[email protected]> >> 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 <[email protected]> >>>> wrote: >>>>> >>>>> On 9 October 2017 at 08:42, Ni, Ruiyu <[email protected]> wrote: >>>>>> >>>>>> The "read"/"write" is from the Bus Master's point of view. >>>>>> >>>>>> >>>>>> Thanks/Ray >>>>>> >>>>>>> -----Original Message----- >>>>>>> From: edk2-devel [mailto:[email protected]] On Behalf >>>>>>> Of >>>>>>> Daniil >>>>>>> Egranov >>>>>>> Sent: Monday, October 9, 2017 9:16 AM >>>>>>> To: [email protected] >>>>>>> Cc: [email protected]; Zeng, Star <[email protected]>; >>>>>>> [email protected] >>>>>>> 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 >>>> [email protected] >>>> 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 >> [email protected] >> https://lists.01.org/mailman/listinfo/edk2-devel > > _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
