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

["Wu, Hao A" <hao.a.wu@intel.com>]

> -----Original Message-----
> From: Ashish Singhal [mailto:ashishsingha@nvidia.com]
> Sent: Monday, March 04, 2019 12:00 PM
> To: Wu, Hao A; Cohen, Eugene; Ard Biesheuvel
> Cc: edk2-devel@lists.01.org; Kim, Sangwoo (??? SW1Lab.)
> Subject: RE: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3
> 64-bit systems
> 
> 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.

Yes. I will co-work with Eugene to address this soon.

> 
> 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.

Sure. I will take a look at your proposed patch.
Please grant me some time for the review and test.

Best Regards,
Hao Wu

> 
> 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.
> > -----------------------------------------------------------------------------------