Re: [PATCH] MdeModulePkg/AtaAtapiPassThru: disable only BM-DMA at ExitBootServices()

["Yao, Jiewen" <jiewen.yao@intel.com>]

Yes, Laszlo, you are right. Back to that time, we did not realize there will be S4 issue.

Now, I agree with you that AhciReset is a better way.
And I think we also need test different UEFI OS (normal boot/S4) to see if there is other issue.

Hope AhciReset() is good enough, and won't bring compatibility issue.


Thank you
Yao Jiewen


> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo
> Ersek
> Sent: Monday, November 27, 2017 8:30 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Paolo
> Bonzini <pbonzini@redhat.com>
> Cc: edk2-devel-01 <edk2-devel@lists.01.org>; Dann Frazier
> <dannf@ubuntu.com>; Dong, Eric <eric.dong@intel.com>; Zeng, Star
> <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Subject: Re: [edk2] [PATCH] MdeModulePkg/AtaAtapiPassThru: disable only
> BM-DMA at ExitBootServices()
> 
> Hi Jiewen,
> 
> On 11/24/17 02:40, Yao, Jiewen wrote:
> > Maybe, can we revisit the original requirement on why we need disable BME at
> ExitBootService for OVMF?
> >
> > I recall we have lots of discussion at September. It is good to refresh.
> >
> > ==============================
> > [edk2] [PATCH 0/4] MdeModulePkg: some PCI HC drivers: unmap common
> buffers at ExitBootServices()
> > https://lists.01.org/pipermail/edk2-devel/2017-September/014099.html
> >
> > At ExitBootServices(), PCI and VirtIo drivers should only care about
> > aborting pending DMA on the devices. Cleaning up PciIo mappings (which
> > ultimately boil down to IOMMU mappings) for those aborted DMA operations
> > should be the job of the IOMMU driver.
> > ==============================
> >
> > I think the Driver Writer's Guide recommend to stop the transaction.
> > But it does not say you must turn off BME.
> > Clear BME is just one way to meet the recommendation.
> > Maybe we can figure out other way to halt the controller, or stop DMA
> transaction?
> > Such as stop timer event, set device reset/halt register, etc.
> > I think USB has already done that.
> >
> >
> > On the other hand, I do not think "OVMF does not support S4" is a good
> justification to add PCD.
> > Yes, it does not support at this moment. But who knows the status after 3 or 5
> years?
> > I also heard some VMM do support S4 resume Guest.
> >
> >
> > I also recommend to rollback all BME operation at EBS as first step, then go
> back to see what is best way to
> 
> I agree that, if the device and the driver offer another way to abort
> pending DMA, we can use that too.
> 
> In fact, my very first patch for AtaAtapiPassThru on this topic used
> AhciReset():
> 
> [1]
> http://mid.mail-archive.com/20170903195449.30261-5-lersek@redhat.com
> 
> But then you recommended clearing BusMasterEnable:
> 
> [2]
> http://mid.mail-archive.com/74D8A39837DF1E4DA445A8C0B3885C503A9A79
> BD@shsmsx102.ccr.corp.intel.com
> 
> 
> (The old patch [1] I referenced above also called PciIo->Unmap(). We've
> since agreed that *that* part of the idea was wrong, so I'm not
> suggesting to return to PciIo->Unmap().)
> 
> So, perhaps AhciReset() would be good enough after all, for aborting
> pending DMA.
> 
> Thanks,
> Laszlo
> 
> 
> 
> 
> >> -----Original Message-----
> >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ni,
> >> Ruiyu
> >> Sent: Friday, November 24, 2017 9:04 AM
> >> To: Paolo Bonzini <pbonzini@redhat.com>
> >> Cc: Dong, Eric <eric.dong@intel.com>; Ard Biesheuvel
> >> <ard.biesheuvel@linaro.org>; edk2-devel-01 <edk2-devel@lists.01.org>;
> Dann
> >> Frazier <dannf@ubuntu.com>; Laszlo Ersek <lersek@redhat.com>; Zeng,
> Star
> >> <star.zeng@intel.com>
> >> Subject: Re: [edk2] [PATCH] MdeModulePkg/AtaAtapiPassThru: disable only
> >> BM-DMA at ExitBootServices()
> >>
> >> Maybe win10 does some optimization in S4 path.
> >>
> >> Sent from a small-screen device
> >>
> >> 在 2017年11月24日，上午8:01，Paolo Bonzini
> >> <pbonzini@redhat.com<mailto:pbonzini@redhat.com>> 写道：
> >>
> >> On 23/11/2017 14:08, Laszlo Ersek wrote:
> >> On 11/23/17 03:20, Ni, Ruiyu wrote:
> >> I cannot explain precisely why the S4 resume fails.
> >> I can just guess: Windows might have some assumptions on the BM bit.
> >> Can we make this configurable on the platform level somehow?
> >>
> >> On one hand, I certainly don't want to break Windows 10, even in case
> >> this issue ultimately turns out to be a Windows 10 bug.
> >>
> >> On the other hand, OVMF does not support S4, and disabling BMDMA at
> >> ExitBootServices() in PCI drivers is specifically what the Driver
> >> Writers' Guide recommends. Otherwise pending DMA could corrupt OS
> memory.
> >>
> >> S4 can be done by the OS even if firmware says it doesn't support it.
> >>
> >> Once hibernation is done, it is merely a "courtesy" for the OSPM to turn
> >> off the computer using the _S4 ACPI object rather than _S5.
> >>
> >> Paolo
> >> _______________________________________________
> >> edk2-devel mailing list
> >> edk2-devel@lists.01.org
> >> https://lists.01.org/mailman/listinfo/edk2-devel
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel