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

[Ashish Singhal <ashishsingha@nvidia.com>]

Hi Hao,

I agree that there has been a bug all along which got exposed just now. We should submit the patch as proposed by Eugene.

Also, I have submitted the patch for enabling 64b DMA for V3. Please take that into consideration once the freeze is over so that we can fix the issue in real sense.

Eugene,

Please let me know once you have tried my patch on your board.

Thanks
Ashish

-----Original Message-----
From: Wu, Hao A <hao.a.wu@intel.com> 
Sent: Sunday, March 3, 2019 7:39 PM
To: Cohen, Eugene <eugene@hp.com>; Ashish Singhal <ashishsingha@nvidia.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: edk2-devel@lists.01.org; Kim, Sangwoo (??? SW1Lab.) <sangwoo.kim@hp.com>
Subject: RE: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems

Hi Eugene, Ashish and Ard

Sorry for the delayed response, I was out of office in the previous several days.

According to the discussion, my understanding is that (quote the comments from
Ard):

> Driver should not set the EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE 
> attribute 1. If the device does not support it; 2. If the driver does 
> not implement the 64-bit DMA mode that the device does
>    support.

Thus, for the current implementation of the SdMmcPciHcDxe driver (including the
V4 ADMA descriptor support from Ashish):

* The driver should set the DUAL_ADDRESS_CYCLE attribute only when 'SysBus64V4'
  bit set, because of the statement 2 above.

And for the previous implementation (before the change from Ashish):

* The driver should not set the DUAL_ADDRESS_CYCLE attribute at all, since the
  implementation was written to support only the 32b ADMA descriptor.

If this is true, I am fine with your proposed fix.


Eugene,

Could you help to state the reason for the fix a bit more clear in the commit log?

Also, I have filed a Bugzilla tracker for this one:
https://bugzilla.tianocore.org/show_bug.cgi?id=1583

Could you help to add this information into the commit log as well? Thanks.

Best Regards,
Hao Wu

> -----Original Message-----
> From: Ashish Singhal [mailto:ashishsingha@nvidia.com]
> Sent: Friday, March 01, 2019 11:25 PM
> To: Ard Biesheuvel; Cohen, Eugene
> Cc: Wu, Hao A; edk2-devel@lists.01.org; Kim, Sangwoo (김상우 SW1Lab.)
> Subject: RE: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 
> 64-bit systems
> 
> Acked-by: Ashish Singhal <ashishsingha@nvidia.com>
> 
> -----Original Message-----
> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Sent: Friday, March 1, 2019 4:39 AM
> To: Cohen, Eugene <eugene@hp.com>
> Cc: Ashish Singhal <ashishsingha@nvidia.com>; Wu, Hao A 
> <hao.a.wu@intel.com>; edk2-devel@lists.01.org; Kim, Sangwoo (김상우
> SW1Lab.) <sangwoo.kim@hp.com>
> Subject: Re: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 
> 64-bit systems
> 
> On Fri, 1 Mar 2019 at 11:54, Cohen, Eugene <eugene@hp.com> wrote:
> >
> > Ard,
> >
> > > 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.
> >
> > Interesting - I did not realize that there were designs that were 
> > crazy
> enough to have no addressable DRAM below 4G.
> >
> 
> You must be new here :-)
> 
> But seriously, it does make sense for an implementation to, say, put 
> all peripherals, PCIe resource windows etc in the bottom half and all 
> DRAM in the top half of a 40-bit address space, which is how the AMD 
> Seattle SoC ended with its system memory at address 0x80_0000_0000.
> Note that on this platform, we can still use 32-bit DMA if we want to 
> with the help of the SMMUs, but we haven't wired those up in UEFI (and 
> the generic host bridge driver did not have the IOMMU hooks at the
> time)
> 
> > > 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.
> >
> > Okay, that's a great reason - let's get V3 64b ADMA2 in!
> >
> > Any objection to committing the original patch in the short term?
> >
> 
> not at all
> 
> Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> 
> ----------------------------------------------------------------------
> ------------- This email message is for the sole use of the intended 
> recipient(s) and may contain confidential information.  Any 
> unauthorized review, use, disclosure or distribution is prohibited.  
> If you are not the intended recipient, please contact the sender by 
> reply email and destroy all copies of the original message.
> -----------------------------------------------------------------------------------