Re: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On Fri, 1 Mar 2019 at 01:19, Ashish Singhal <ashishsingha@nvidia.com> wrote:
>
> Eugene,
>
> Small question. Did the issue appear after the V4 patch went in? Looking at the code before that patch, we were enabling 64b dma in pci based on capability register already despite of driver supporting only 32b dma.
>

I think this may have been an oversight on my part when I originally
added the DUAL_ADDRESS_CYCLE handling.

The following commit added EFI_PCI_ATTRIBUTE_DUAL_ADDRESS_CYCLE to the
host bridge driver

commit e58a71d9c50ba641b5ab19f5ce2cbf772187de4d
Author: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Date:   Mon Sep 5 09:55:16 2016 +0100

    MdeModulePkg/PciHostBridgeDxe: restrict 64-bit DMA to devices that
support it

    Currently, the EFI_PCI_ATTRIBUTE_DUAL_ADDRESS_CYCLE attribute is completely
    ignored by the PCI host bridge driver, which means that, on an
implementation
    that supports DMA above 4 GB, allocations above 4 GB may be provided to
    devices that have not expressed support for it.

and the SDHCI driver was fixed accordingly in

Author: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Date:   Mon Sep 5 09:51:48 2016 +0100

    MdeModulePkg/SdMmcPciHcDxe: enable 64-bit PCI DMA

    PCI controller drivers must set the EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE
    attribute if the controller supports 64-bit DMA addressing.

So before these changes, we were in the exact same situation, but
since PC platforms never enable DMA above 4 GB in the first place,
nobody ever noticed until we started running this code on arm64
platforms that have no 32-bit addressable DRAM to begin with.

The obvious conclusion is that the driver should not set the
EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE attribute if the device does
not support it, or, which seems to be our case, if the driver does not
implement the 64-bit DMA mode that the driver does support. However,
since there are platforms for which bounce buffering is not an option
(since there is no 32-bit addressable memory to bounce to), this is
not just a performance optimization, and so it would be useful to fix
the code so it can drive all 64-bit DMA capable hardware.