* [PATCH] MdeModulePkg/AtaAtapiPassThru: disable only BM-DMA at ExitBootServices() @ 2017-10-26 15:48 Laszlo Ersek 2017-10-26 19:56 ` dann frazier ` (3 more replies) 0 siblings, 4 replies; 19+ messages in thread From: Laszlo Ersek @ 2017-10-26 15:48 UTC (permalink / raw) To: edk2-devel-01 Cc: Aleksei Kovura, Ard Biesheuvel, Dann Frazier, Eric Dong, Star Zeng Clearing I/O port decoding in the PCI command register at ExitBootServices() breaks IDE boot in Windows, on QEMU's "pc" (i440fx) machine type. (AHCI boot on "q35" is unaffected.) Windows seems repeatedly stuck, apparently waiting for a timeout of sorts. This is arguably a Windows bug; a native OS driver should not expect the firmware to leave the PCI command register in any particular state. Strictly speaking, we only need to disable BM-DMA at ExitBootServices(), in order to abort pending transfers to/from RAM, which is soon to be owned by the OS. BM-DMA is also the only bit that's explicitly named by the UEFI Driver Writers' Guide, for clearing at ExitBootServices(). I've verified that clearing only BM-DMA fixes the isse (boot time) on i440fx, and does not regress q35/AHCI. Cc: Aleksei Kovura <alex3kov@zoho.com> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> Cc: Dann Frazier <dannf@ubuntu.com> Cc: Eric Dong <eric.dong@intel.com> Cc: Star Zeng <star.zeng@intel.com> Reported-by: Aleksei Kovura <alex3kov@zoho.com> Reported-by: Dann Frazier <dannf@ubuntu.com> Reported-by: https://launchpad.net/~cjkrupp Bisected-by: Dann Frazier <dannf@ubuntu.com> Bisected-by: https://launchpad.net/~cjkrupp Suggested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> Suggested-by: Star Zeng <star.zeng@intel.com> Ref: https://bugs.launchpad.net/ubuntu/+source/edk2/+bug/1725560 Fixes: 6fb8ddd36bde45614b0a069528cdc97077835a74 Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- Notes: Repo: https://github.com/lersek/edk2.git Branch: ata_disable_only_bmdma MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h | 3 +-- MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c | 5 ++--- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h index 8d6eac706c0b..92c5bf2001cd 100644 --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h @@ -123,8 +123,7 @@ typedef struct { LIST_ENTRY NonBlockingTaskList; // - // For disabling the device (especially Bus Master DMA) at - // ExitBootServices(). + // For disabling Bus Master DMA on the device at ExitBootServices(). // EFI_EVENT ExitBootEvent; } ATA_ATAPI_PASS_THRU_INSTANCE; diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c index 09064dda18b7..e10e0d4e65f6 100644 --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c @@ -480,8 +480,7 @@ InitializeAtaAtapiPassThru ( } /** - Disable the device (especially Bus Master DMA) when exiting the boot - services. + Disable Bus Master DMA on the device when exiting the boot services. @param[in] Event Event for which this notification function is being called. @@ -506,7 +505,7 @@ AtaPassThruExitBootServices ( PciIo->Attributes ( PciIo, EfiPciIoAttributeOperationDisable, - Instance->EnabledPciAttributes, + Instance->EnabledPciAttributes & EFI_PCI_IO_ATTRIBUTE_BUS_MASTER, NULL ); } -- 2.14.1.3.gb7cf6e02401b ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] MdeModulePkg/AtaAtapiPassThru: disable only BM-DMA at ExitBootServices() 2017-10-26 15:48 [PATCH] MdeModulePkg/AtaAtapiPassThru: disable only BM-DMA at ExitBootServices() Laszlo Ersek @ 2017-10-26 19:56 ` dann frazier 2017-10-27 3:23 ` Zeng, Star ` (2 subsequent siblings) 3 siblings, 0 replies; 19+ messages in thread From: dann frazier @ 2017-10-26 19:56 UTC (permalink / raw) To: Laszlo Ersek Cc: edk2-devel-01, Aleksei Kovura, Ard Biesheuvel, Dann Frazier, Eric Dong, Star Zeng On Thu, Oct 26, 2017 at 9:48 AM, Laszlo Ersek <lersek@redhat.com> wrote: > Clearing I/O port decoding in the PCI command register at > ExitBootServices() breaks IDE boot in Windows, on QEMU's "pc" (i440fx) > machine type. (AHCI boot on "q35" is unaffected.) Windows seems repeatedly > stuck, apparently waiting for a timeout of sorts. > > This is arguably a Windows bug; a native OS driver should not expect the > firmware to leave the PCI command register in any particular state. > > Strictly speaking, we only need to disable BM-DMA at ExitBootServices(), > in order to abort pending transfers to/from RAM, which is soon to be owned > by the OS. BM-DMA is also the only bit that's explicitly named by the UEFI > Driver Writers' Guide, for clearing at ExitBootServices(). > > I've verified that clearing only BM-DMA fixes the isse (boot time) on > i440fx, and does not regress q35/AHCI. Worked for my test case (booting a Win10 install ISO in ~30s). Thanks Laszlo! Tested-by: dann frazier <dann.frazier@canonical.com> ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] MdeModulePkg/AtaAtapiPassThru: disable only BM-DMA at ExitBootServices() 2017-10-26 15:48 [PATCH] MdeModulePkg/AtaAtapiPassThru: disable only BM-DMA at ExitBootServices() Laszlo Ersek 2017-10-26 19:56 ` dann frazier @ 2017-10-27 3:23 ` Zeng, Star 2017-10-27 12:25 ` Laszlo Ersek 2017-10-27 14:57 ` Aleksei 2017-10-27 16:09 ` Laszlo Ersek 3 siblings, 1 reply; 19+ messages in thread From: Zeng, Star @ 2017-10-27 3:23 UTC (permalink / raw) To: Laszlo Ersek, edk2-devel-01 Cc: Aleksei Kovura, Ard Biesheuvel, Dann Frazier, Dong, Eric, Zeng, Star Reviewed-by: Star Zeng <star.zeng@intel.com> after the minor typo " isse " is fixed to " issue ". Thanks, Star -----Original Message----- From: Laszlo Ersek [mailto:lersek@redhat.com] Sent: Thursday, October 26, 2017 11:48 PM To: edk2-devel-01 <edk2-devel@lists.01.org> Cc: Aleksei Kovura <alex3kov@zoho.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>; Dann Frazier <dannf@ubuntu.com>; Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com> Subject: [PATCH] MdeModulePkg/AtaAtapiPassThru: disable only BM-DMA at ExitBootServices() Clearing I/O port decoding in the PCI command register at ExitBootServices() breaks IDE boot in Windows, on QEMU's "pc" (i440fx) machine type. (AHCI boot on "q35" is unaffected.) Windows seems repeatedly stuck, apparently waiting for a timeout of sorts. This is arguably a Windows bug; a native OS driver should not expect the firmware to leave the PCI command register in any particular state. Strictly speaking, we only need to disable BM-DMA at ExitBootServices(), in order to abort pending transfers to/from RAM, which is soon to be owned by the OS. BM-DMA is also the only bit that's explicitly named by the UEFI Driver Writers' Guide, for clearing at ExitBootServices(). I've verified that clearing only BM-DMA fixes the isse (boot time) on i440fx, and does not regress q35/AHCI. Cc: Aleksei Kovura <alex3kov@zoho.com> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> Cc: Dann Frazier <dannf@ubuntu.com> Cc: Eric Dong <eric.dong@intel.com> Cc: Star Zeng <star.zeng@intel.com> Reported-by: Aleksei Kovura <alex3kov@zoho.com> Reported-by: Dann Frazier <dannf@ubuntu.com> Reported-by: https://launchpad.net/~cjkrupp Bisected-by: Dann Frazier <dannf@ubuntu.com> Bisected-by: https://launchpad.net/~cjkrupp Suggested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> Suggested-by: Star Zeng <star.zeng@intel.com> Ref: https://bugs.launchpad.net/ubuntu/+source/edk2/+bug/1725560 Fixes: 6fb8ddd36bde45614b0a069528cdc97077835a74 Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- Notes: Repo: https://github.com/lersek/edk2.git Branch: ata_disable_only_bmdma MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h | 3 +-- MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c | 5 ++--- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h index 8d6eac706c0b..92c5bf2001cd 100644 --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h @@ -123,8 +123,7 @@ typedef struct { LIST_ENTRY NonBlockingTaskList; // - // For disabling the device (especially Bus Master DMA) at - // ExitBootServices(). + // For disabling Bus Master DMA on the device at ExitBootServices(). // EFI_EVENT ExitBootEvent; } ATA_ATAPI_PASS_THRU_INSTANCE; diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c index 09064dda18b7..e10e0d4e65f6 100644 --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c @@ -480,8 +480,7 @@ InitializeAtaAtapiPassThru ( } /** - Disable the device (especially Bus Master DMA) when exiting the boot - services. + Disable Bus Master DMA on the device when exiting the boot services. @param[in] Event Event for which this notification function is being called. @@ -506,7 +505,7 @@ AtaPassThruExitBootServices ( PciIo->Attributes ( PciIo, EfiPciIoAttributeOperationDisable, - Instance->EnabledPciAttributes, + Instance->EnabledPciAttributes & + EFI_PCI_IO_ATTRIBUTE_BUS_MASTER, NULL ); } -- 2.14.1.3.gb7cf6e02401b ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] MdeModulePkg/AtaAtapiPassThru: disable only BM-DMA at ExitBootServices() 2017-10-27 3:23 ` Zeng, Star @ 2017-10-27 12:25 ` Laszlo Ersek 0 siblings, 0 replies; 19+ messages in thread From: Laszlo Ersek @ 2017-10-27 12:25 UTC (permalink / raw) To: Zeng, Star, edk2-devel-01 Cc: Aleksei Kovura, Ard Biesheuvel, Dann Frazier, Dong, Eric On 10/27/17 05:23, Zeng, Star wrote: > Reviewed-by: Star Zeng <star.zeng@intel.com> after the minor typo " isse " is fixed to " issue ". Yup, I noticed that too late. I'll fix it when I push the patch (still waiting for test results from Aleksei as well). Thanks! Laszlo > > Thanks, > Star > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Thursday, October 26, 2017 11:48 PM > To: edk2-devel-01 <edk2-devel@lists.01.org> > Cc: Aleksei Kovura <alex3kov@zoho.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>; Dann Frazier <dannf@ubuntu.com>; Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com> > Subject: [PATCH] MdeModulePkg/AtaAtapiPassThru: disable only BM-DMA at ExitBootServices() > > Clearing I/O port decoding in the PCI command register at > ExitBootServices() breaks IDE boot in Windows, on QEMU's "pc" (i440fx) machine type. (AHCI boot on "q35" is unaffected.) Windows seems repeatedly stuck, apparently waiting for a timeout of sorts. > > This is arguably a Windows bug; a native OS driver should not expect the firmware to leave the PCI command register in any particular state. > > Strictly speaking, we only need to disable BM-DMA at ExitBootServices(), in order to abort pending transfers to/from RAM, which is soon to be owned by the OS. BM-DMA is also the only bit that's explicitly named by the UEFI Driver Writers' Guide, for clearing at ExitBootServices(). > > I've verified that clearing only BM-DMA fixes the isse (boot time) on i440fx, and does not regress q35/AHCI. > > Cc: Aleksei Kovura <alex3kov@zoho.com> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Cc: Dann Frazier <dannf@ubuntu.com> > Cc: Eric Dong <eric.dong@intel.com> > Cc: Star Zeng <star.zeng@intel.com> > Reported-by: Aleksei Kovura <alex3kov@zoho.com> > Reported-by: Dann Frazier <dannf@ubuntu.com> > Reported-by: https://launchpad.net/~cjkrupp > Bisected-by: Dann Frazier <dannf@ubuntu.com> > Bisected-by: https://launchpad.net/~cjkrupp > Suggested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Suggested-by: Star Zeng <star.zeng@intel.com> > Ref: https://bugs.launchpad.net/ubuntu/+source/edk2/+bug/1725560 > Fixes: 6fb8ddd36bde45614b0a069528cdc97077835a74 > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- > > Notes: > Repo: https://github.com/lersek/edk2.git > Branch: ata_disable_only_bmdma > > MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h | 3 +-- MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c | 5 ++--- > 2 files changed, 3 insertions(+), 5 deletions(-) > > diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h > index 8d6eac706c0b..92c5bf2001cd 100644 > --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h > +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h > @@ -123,8 +123,7 @@ typedef struct { > LIST_ENTRY NonBlockingTaskList; > > // > - // For disabling the device (especially Bus Master DMA) at > - // ExitBootServices(). > + // For disabling Bus Master DMA on the device at ExitBootServices(). > // > EFI_EVENT ExitBootEvent; > } ATA_ATAPI_PASS_THRU_INSTANCE; > diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c > index 09064dda18b7..e10e0d4e65f6 100644 > --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c > +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c > @@ -480,8 +480,7 @@ InitializeAtaAtapiPassThru ( } > > /** > - Disable the device (especially Bus Master DMA) when exiting the boot > - services. > + Disable Bus Master DMA on the device when exiting the boot services. > > @param[in] Event Event for which this notification function is being > called. > @@ -506,7 +505,7 @@ AtaPassThruExitBootServices ( > PciIo->Attributes ( > PciIo, > EfiPciIoAttributeOperationDisable, > - Instance->EnabledPciAttributes, > + Instance->EnabledPciAttributes & > + EFI_PCI_IO_ATTRIBUTE_BUS_MASTER, > NULL > ); > } > -- > 2.14.1.3.gb7cf6e02401b > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] MdeModulePkg/AtaAtapiPassThru: disable only BM-DMA at ExitBootServices() 2017-10-26 15:48 [PATCH] MdeModulePkg/AtaAtapiPassThru: disable only BM-DMA at ExitBootServices() Laszlo Ersek 2017-10-26 19:56 ` dann frazier 2017-10-27 3:23 ` Zeng, Star @ 2017-10-27 14:57 ` Aleksei 2017-10-27 16:09 ` Laszlo Ersek 3 siblings, 0 replies; 19+ messages in thread From: Aleksei @ 2017-10-27 14:57 UTC (permalink / raw) To: Laszlo Ersek, edk2-devel-01 Cc: Ard Biesheuvel, Dann Frazier, Eric Dong, Star Zeng Tested combinations i440fx/IDE, i440fx/AHCI, Q35/IDE, Q35/AHCI for booting Windows 7 sp1 x64 installation - all boot in reasonable time. Thanks Laszlo. Tested-by: Aleksei Kovura <alex3kov@zoho.com> /--Regards, Aleksei / On 26/10/17 18:48, Laszlo Ersek wrote: Clearing I/O port decoding in the PCI command register at ExitBootServices() breaks IDE boot in Windows, on QEMU's "pc" (i440fx) machine type. (AHCI boot on "q35" is unaffected.) Windows seems repeatedly stuck, apparently waiting for a timeout of sorts. This is arguably a Windows bug; a native OS driver should not expect the firmware to leave the PCI command register in any particular state. Strictly speaking, we only need to disable BM-DMA at ExitBootServices(), in order to abort pending transfers to/from RAM, which is soon to be owned by the OS. BM-DMA is also the only bit that's explicitly named by the UEFI Driver Writers' Guide, for clearing at ExitBootServices(). I've verified that clearing only BM-DMA fixes the isse (boot time) on i440fx, and does not regress q35/AHCI. Cc: Aleksei Kovura <alex3kov@zoho.com> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> Cc: Dann Frazier <dannf@ubuntu.com> Cc: Eric Dong <eric.dong@intel.com> Cc: Star Zeng <star.zeng@intel.com> Reported-by: Aleksei Kovura <alex3kov@zoho.com> Reported-by: Dann Frazier <dannf@ubuntu.com> Reported-by: https://launchpad.net/~cjkrupp Bisected-by: Dann Frazier <dannf@ubuntu.com> Bisected-by: https://launchpad.net/~cjkrupp Suggested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> Suggested-by: Star Zeng <star.zeng@intel.com> Ref: https://bugs.launchpad.net/ubuntu/+source/edk2/+bug/1725560 Fixes: 6fb8ddd36bde45614b0a069528cdc97077835a74 Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- Notes: Repo: https://github.com/lersek/edk2.git Branch: ata_disable_only_bmdma MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h | 3 +-- MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c | 5 ++--- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h index 8d6eac706c0b..92c5bf2001cd 100644 --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h @@ -123,8 +123,7 @@ typedef struct { LIST_ENTRY NonBlockingTaskList; // - // For disabling the device (especially Bus Master DMA) at - // ExitBootServices(). + // For disabling Bus Master DMA on the device at ExitBootServices(). // EFI_EVENT ExitBootEvent; } ATA_ATAPI_PASS_THRU_INSTANCE; diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c index 09064dda18b7..e10e0d4e65f6 100644 --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c @@ -480,8 +480,7 @@ InitializeAtaAtapiPassThru ( } /** - Disable the device (especially Bus Master DMA) when exiting the boot - services. + Disable Bus Master DMA on the device when exiting the boot services. @param[in] Event Event for which this notification function is being called. @@ -506,7 +505,7 @@ AtaPassThruExitBootServices ( PciIo->Attributes ( PciIo, EfiPciIoAttributeOperationDisable, - Instance->EnabledPciAttributes, + Instance->EnabledPciAttributes & EFI_PCI_IO_ATTRIBUTE_BUS_MASTER, NULL ); } ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] MdeModulePkg/AtaAtapiPassThru: disable only BM-DMA at ExitBootServices() 2017-10-26 15:48 [PATCH] MdeModulePkg/AtaAtapiPassThru: disable only BM-DMA at ExitBootServices() Laszlo Ersek ` (2 preceding siblings ...) 2017-10-27 14:57 ` Aleksei @ 2017-10-27 16:09 ` Laszlo Ersek 2017-11-22 10:05 ` Ni, Ruiyu 3 siblings, 1 reply; 19+ messages in thread From: Laszlo Ersek @ 2017-10-27 16:09 UTC (permalink / raw) To: edk2-devel-01 Cc: Aleksei Kovura, Dann Frazier, Eric Dong, Star Zeng, Ard Biesheuvel On 10/26/17 17:48, Laszlo Ersek wrote: > Clearing I/O port decoding in the PCI command register at > ExitBootServices() breaks IDE boot in Windows, on QEMU's "pc" (i440fx) > machine type. (AHCI boot on "q35" is unaffected.) Windows seems repeatedly > stuck, apparently waiting for a timeout of sorts. > > This is arguably a Windows bug; a native OS driver should not expect the > firmware to leave the PCI command register in any particular state. > > Strictly speaking, we only need to disable BM-DMA at ExitBootServices(), > in order to abort pending transfers to/from RAM, which is soon to be owned > by the OS. BM-DMA is also the only bit that's explicitly named by the UEFI > Driver Writers' Guide, for clearing at ExitBootServices(). > > I've verified that clearing only BM-DMA fixes the isse (boot time) on > i440fx, and does not regress q35/AHCI. > > Cc: Aleksei Kovura <alex3kov@zoho.com> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Cc: Dann Frazier <dannf@ubuntu.com> > Cc: Eric Dong <eric.dong@intel.com> > Cc: Star Zeng <star.zeng@intel.com> > Reported-by: Aleksei Kovura <alex3kov@zoho.com> > Reported-by: Dann Frazier <dannf@ubuntu.com> > Reported-by: https://launchpad.net/~cjkrupp > Bisected-by: Dann Frazier <dannf@ubuntu.com> > Bisected-by: https://launchpad.net/~cjkrupp > Suggested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Suggested-by: Star Zeng <star.zeng@intel.com> > Ref: https://bugs.launchpad.net/ubuntu/+source/edk2/+bug/1725560 > Fixes: 6fb8ddd36bde45614b0a069528cdc97077835a74 > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- > > Notes: > Repo: https://github.com/lersek/edk2.git > Branch: ata_disable_only_bmdma > > MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h | 3 +-- > MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c | 5 ++--- > 2 files changed, 3 insertions(+), 5 deletions(-) > > diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h > index 8d6eac706c0b..92c5bf2001cd 100644 > --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h > +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h > @@ -123,8 +123,7 @@ typedef struct { > LIST_ENTRY NonBlockingTaskList; > > // > - // For disabling the device (especially Bus Master DMA) at > - // ExitBootServices(). > + // For disabling Bus Master DMA on the device at ExitBootServices(). > // > EFI_EVENT ExitBootEvent; > } ATA_ATAPI_PASS_THRU_INSTANCE; > diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c > index 09064dda18b7..e10e0d4e65f6 100644 > --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c > +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c > @@ -480,8 +480,7 @@ InitializeAtaAtapiPassThru ( > } > > /** > - Disable the device (especially Bus Master DMA) when exiting the boot > - services. > + Disable Bus Master DMA on the device when exiting the boot services. > > @param[in] Event Event for which this notification function is being > called. > @@ -506,7 +505,7 @@ AtaPassThruExitBootServices ( > PciIo->Attributes ( > PciIo, > EfiPciIoAttributeOperationDisable, > - Instance->EnabledPciAttributes, > + Instance->EnabledPciAttributes & EFI_PCI_IO_ATTRIBUTE_BUS_MASTER, > NULL > ); > } > Thanks Everyone for the feedback; I fixed the typo pointed out by Star and pushed the patch as commit 76fd5a660d70. Cheers Laszlo ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] MdeModulePkg/AtaAtapiPassThru: disable only BM-DMA at ExitBootServices() 2017-10-27 16:09 ` Laszlo Ersek @ 2017-11-22 10:05 ` Ni, Ruiyu 2017-11-22 10:26 ` Zeng, Star 0 siblings, 1 reply; 19+ messages in thread From: Ni, Ruiyu @ 2017-11-22 10:05 UTC (permalink / raw) To: Laszlo Ersek, edk2-devel-01 Cc: Ard Biesheuvel, Dong, Eric, Zeng, Star, Dann Frazier Laszlo, Our QA found Win10 S4 resume fails due to your change. As you might notice that I just rolled back the BM disabling patches in PciBus module, I am thinking about maybe you need to rollback the whole ExitBootServices callback as well to fix the compatibility issue. Thanks/Ray > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of > Laszlo Ersek > Sent: Saturday, October 28, 2017 12:10 AM > To: edk2-devel-01 <edk2-devel@lists.01.org> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Dong, Eric > <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>; Dann Frazier > <dannf@ubuntu.com> > Subject: Re: [edk2] [PATCH] MdeModulePkg/AtaAtapiPassThru: disable only > BM-DMA at ExitBootServices() > > On 10/26/17 17:48, Laszlo Ersek wrote: > > Clearing I/O port decoding in the PCI command register at > > ExitBootServices() breaks IDE boot in Windows, on QEMU's "pc" (i440fx) > > machine type. (AHCI boot on "q35" is unaffected.) Windows seems > > repeatedly stuck, apparently waiting for a timeout of sorts. > > > > This is arguably a Windows bug; a native OS driver should not expect > > the firmware to leave the PCI command register in any particular state. > > > > Strictly speaking, we only need to disable BM-DMA at > > ExitBootServices(), in order to abort pending transfers to/from RAM, > > which is soon to be owned by the OS. BM-DMA is also the only bit > > that's explicitly named by the UEFI Driver Writers' Guide, for clearing at > ExitBootServices(). > > > > I've verified that clearing only BM-DMA fixes the isse (boot time) on > > i440fx, and does not regress q35/AHCI. > > > > Cc: Aleksei Kovura <alex3kov@zoho.com> > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > Cc: Dann Frazier <dannf@ubuntu.com> > > Cc: Eric Dong <eric.dong@intel.com> > > Cc: Star Zeng <star.zeng@intel.com> > > Reported-by: Aleksei Kovura <alex3kov@zoho.com> > > Reported-by: Dann Frazier <dannf@ubuntu.com> > > Reported-by: https://launchpad.net/~cjkrupp > > Bisected-by: Dann Frazier <dannf@ubuntu.com> > > Bisected-by: https://launchpad.net/~cjkrupp > > Suggested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > Suggested-by: Star Zeng <star.zeng@intel.com> > > Ref: https://bugs.launchpad.net/ubuntu/+source/edk2/+bug/1725560 > > Fixes: 6fb8ddd36bde45614b0a069528cdc97077835a74 > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > > --- > > > > Notes: > > Repo: https://github.com/lersek/edk2.git > > Branch: ata_disable_only_bmdma > > > > MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h | 3 +-- > > MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c | 5 ++--- > > 2 files changed, 3 insertions(+), 5 deletions(-) > > > > diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h > > b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h > > index 8d6eac706c0b..92c5bf2001cd 100644 > > --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h > > +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h > > @@ -123,8 +123,7 @@ typedef struct { > > LIST_ENTRY NonBlockingTaskList; > > > > // > > - // For disabling the device (especially Bus Master DMA) at > > - // ExitBootServices(). > > + // For disabling Bus Master DMA on the device at ExitBootServices(). > > // > > EFI_EVENT ExitBootEvent; > > } ATA_ATAPI_PASS_THRU_INSTANCE; > > diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c > > b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c > > index 09064dda18b7..e10e0d4e65f6 100644 > > --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c > > +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c > > @@ -480,8 +480,7 @@ InitializeAtaAtapiPassThru ( } > > > > /** > > - Disable the device (especially Bus Master DMA) when exiting the > > boot > > - services. > > + Disable Bus Master DMA on the device when exiting the boot services. > > > > @param[in] Event Event for which this notification function is being > > called. > > @@ -506,7 +505,7 @@ AtaPassThruExitBootServices ( > > PciIo->Attributes ( > > PciIo, > > EfiPciIoAttributeOperationDisable, > > - Instance->EnabledPciAttributes, > > + Instance->EnabledPciAttributes & > > + EFI_PCI_IO_ATTRIBUTE_BUS_MASTER, > > NULL > > ); > > } > > > > Thanks Everyone for the feedback; I fixed the typo pointed out by Star and > pushed the patch as commit 76fd5a660d70. > > Cheers > Laszlo > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] MdeModulePkg/AtaAtapiPassThru: disable only BM-DMA at ExitBootServices() 2017-11-22 10:05 ` Ni, Ruiyu @ 2017-11-22 10:26 ` Zeng, Star 2017-11-23 2:20 ` Ni, Ruiyu 0 siblings, 1 reply; 19+ messages in thread From: Zeng, Star @ 2017-11-22 10:26 UTC (permalink / raw) To: Ni, Ruiyu, Laszlo Ersek, edk2-devel-01 Cc: Ard Biesheuvel, Dong, Eric, Dann Frazier, Zeng, Star Ray, You may explain more why Win10 S4 resume fails with only BM disabled, then Laszlo can know the issue well. Thanks, Star -----Original Message----- From: Ni, Ruiyu Sent: Wednesday, November 22, 2017 6:06 PM To: Laszlo Ersek <lersek@redhat.com>; edk2-devel-01 <edk2-devel@lists.01.org> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>; Dann Frazier <dannf@ubuntu.com> Subject: RE: [edk2] [PATCH] MdeModulePkg/AtaAtapiPassThru: disable only BM-DMA at ExitBootServices() Laszlo, Our QA found Win10 S4 resume fails due to your change. As you might notice that I just rolled back the BM disabling patches in PciBus module, I am thinking about maybe you need to rollback the whole ExitBootServices callback as well to fix the compatibility issue. Thanks/Ray > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of > Laszlo Ersek > Sent: Saturday, October 28, 2017 12:10 AM > To: edk2-devel-01 <edk2-devel@lists.01.org> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Dong, Eric > <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>; Dann Frazier > <dannf@ubuntu.com> > Subject: Re: [edk2] [PATCH] MdeModulePkg/AtaAtapiPassThru: disable > only BM-DMA at ExitBootServices() > > On 10/26/17 17:48, Laszlo Ersek wrote: > > Clearing I/O port decoding in the PCI command register at > > ExitBootServices() breaks IDE boot in Windows, on QEMU's "pc" > > (i440fx) machine type. (AHCI boot on "q35" is unaffected.) Windows > > seems repeatedly stuck, apparently waiting for a timeout of sorts. > > > > This is arguably a Windows bug; a native OS driver should not expect > > the firmware to leave the PCI command register in any particular state. > > > > Strictly speaking, we only need to disable BM-DMA at > > ExitBootServices(), in order to abort pending transfers to/from RAM, > > which is soon to be owned by the OS. BM-DMA is also the only bit > > that's explicitly named by the UEFI Driver Writers' Guide, for > > clearing at > ExitBootServices(). > > > > I've verified that clearing only BM-DMA fixes the isse (boot time) > > on i440fx, and does not regress q35/AHCI. > > > > Cc: Aleksei Kovura <alex3kov@zoho.com> > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > Cc: Dann Frazier <dannf@ubuntu.com> > > Cc: Eric Dong <eric.dong@intel.com> > > Cc: Star Zeng <star.zeng@intel.com> > > Reported-by: Aleksei Kovura <alex3kov@zoho.com> > > Reported-by: Dann Frazier <dannf@ubuntu.com> > > Reported-by: https://launchpad.net/~cjkrupp > > Bisected-by: Dann Frazier <dannf@ubuntu.com> > > Bisected-by: https://launchpad.net/~cjkrupp > > Suggested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > Suggested-by: Star Zeng <star.zeng@intel.com> > > Ref: https://bugs.launchpad.net/ubuntu/+source/edk2/+bug/1725560 > > Fixes: 6fb8ddd36bde45614b0a069528cdc97077835a74 > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > > --- > > > > Notes: > > Repo: https://github.com/lersek/edk2.git > > Branch: ata_disable_only_bmdma > > > > MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h | 3 +-- > > MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c | 5 ++--- > > 2 files changed, 3 insertions(+), 5 deletions(-) > > > > diff --git > > a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h > > b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h > > index 8d6eac706c0b..92c5bf2001cd 100644 > > --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h > > +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h > > @@ -123,8 +123,7 @@ typedef struct { > > LIST_ENTRY NonBlockingTaskList; > > > > // > > - // For disabling the device (especially Bus Master DMA) at > > - // ExitBootServices(). > > + // For disabling Bus Master DMA on the device at ExitBootServices(). > > // > > EFI_EVENT ExitBootEvent; > > } ATA_ATAPI_PASS_THRU_INSTANCE; > > diff --git > > a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c > > b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c > > index 09064dda18b7..e10e0d4e65f6 100644 > > --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c > > +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c > > @@ -480,8 +480,7 @@ InitializeAtaAtapiPassThru ( } > > > > /** > > - Disable the device (especially Bus Master DMA) when exiting the > > boot > > - services. > > + Disable Bus Master DMA on the device when exiting the boot services. > > > > @param[in] Event Event for which this notification function is being > > called. > > @@ -506,7 +505,7 @@ AtaPassThruExitBootServices ( > > PciIo->Attributes ( > > PciIo, > > EfiPciIoAttributeOperationDisable, > > - Instance->EnabledPciAttributes, > > + Instance->EnabledPciAttributes & > > + EFI_PCI_IO_ATTRIBUTE_BUS_MASTER, > > NULL > > ); > > } > > > > Thanks Everyone for the feedback; I fixed the typo pointed out by Star > and pushed the patch as commit 76fd5a660d70. > > Cheers > Laszlo > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] MdeModulePkg/AtaAtapiPassThru: disable only BM-DMA at ExitBootServices() 2017-11-22 10:26 ` Zeng, Star @ 2017-11-23 2:20 ` Ni, Ruiyu 2017-11-23 13:08 ` Laszlo Ersek 0 siblings, 1 reply; 19+ messages in thread From: Ni, Ruiyu @ 2017-11-23 2:20 UTC (permalink / raw) To: Zeng, Star, Laszlo Ersek, edk2-devel-01 Cc: Ard Biesheuvel, Dong, Eric, Dann Frazier I cannot explain precisely why the S4 resume fails. I can just guess: Windows might have some assumptions on the BM bit. Thanks/Ray > -----Original Message----- > From: Zeng, Star > Sent: Wednesday, November 22, 2017 6:26 PM > To: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; > edk2-devel-01 <edk2-devel@lists.01.org> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Dong, Eric > <eric.dong@intel.com>; Dann Frazier <dannf@ubuntu.com>; Zeng, Star > <star.zeng@intel.com> > Subject: RE: [edk2] [PATCH] MdeModulePkg/AtaAtapiPassThru: disable only > BM-DMA at ExitBootServices() > > Ray, > > You may explain more why Win10 S4 resume fails with only BM disabled, > then Laszlo can know the issue well. > > > Thanks, > Star > -----Original Message----- > From: Ni, Ruiyu > Sent: Wednesday, November 22, 2017 6:06 PM > To: Laszlo Ersek <lersek@redhat.com>; edk2-devel-01 <edk2- > devel@lists.01.org> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Dong, Eric > <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>; Dann Frazier > <dannf@ubuntu.com> > Subject: RE: [edk2] [PATCH] MdeModulePkg/AtaAtapiPassThru: disable only > BM-DMA at ExitBootServices() > > Laszlo, > Our QA found Win10 S4 resume fails due to your change. > As you might notice that I just rolled back the BM disabling patches in PciBus > module, I am thinking about maybe you need to rollback the whole > ExitBootServices callback as well to fix the compatibility issue. > > Thanks/Ray > > > -----Original Message----- > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of > > Laszlo Ersek > > Sent: Saturday, October 28, 2017 12:10 AM > > To: edk2-devel-01 <edk2-devel@lists.01.org> > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Dong, Eric > > <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>; Dann Frazier > > <dannf@ubuntu.com> > > Subject: Re: [edk2] [PATCH] MdeModulePkg/AtaAtapiPassThru: disable > > only BM-DMA at ExitBootServices() > > > > On 10/26/17 17:48, Laszlo Ersek wrote: > > > Clearing I/O port decoding in the PCI command register at > > > ExitBootServices() breaks IDE boot in Windows, on QEMU's "pc" > > > (i440fx) machine type. (AHCI boot on "q35" is unaffected.) Windows > > > seems repeatedly stuck, apparently waiting for a timeout of sorts. > > > > > > This is arguably a Windows bug; a native OS driver should not expect > > > the firmware to leave the PCI command register in any particular state. > > > > > > Strictly speaking, we only need to disable BM-DMA at > > > ExitBootServices(), in order to abort pending transfers to/from RAM, > > > which is soon to be owned by the OS. BM-DMA is also the only bit > > > that's explicitly named by the UEFI Driver Writers' Guide, for > > > clearing at > > ExitBootServices(). > > > > > > I've verified that clearing only BM-DMA fixes the isse (boot time) > > > on i440fx, and does not regress q35/AHCI. > > > > > > Cc: Aleksei Kovura <alex3kov@zoho.com> > > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > > Cc: Dann Frazier <dannf@ubuntu.com> > > > Cc: Eric Dong <eric.dong@intel.com> > > > Cc: Star Zeng <star.zeng@intel.com> > > > Reported-by: Aleksei Kovura <alex3kov@zoho.com> > > > Reported-by: Dann Frazier <dannf@ubuntu.com> > > > Reported-by: https://launchpad.net/~cjkrupp > > > Bisected-by: Dann Frazier <dannf@ubuntu.com> > > > Bisected-by: https://launchpad.net/~cjkrupp > > > Suggested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > > Suggested-by: Star Zeng <star.zeng@intel.com> > > > Ref: https://bugs.launchpad.net/ubuntu/+source/edk2/+bug/1725560 > > > Fixes: 6fb8ddd36bde45614b0a069528cdc97077835a74 > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > > > --- > > > > > > Notes: > > > Repo: https://github.com/lersek/edk2.git > > > Branch: ata_disable_only_bmdma > > > > > > MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h | 3 +-- > > > MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c | 5 ++--- > > > 2 files changed, 3 insertions(+), 5 deletions(-) > > > > > > diff --git > > > a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h > > > b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h > > > index 8d6eac706c0b..92c5bf2001cd 100644 > > > --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h > > > +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h > > > @@ -123,8 +123,7 @@ typedef struct { > > > LIST_ENTRY NonBlockingTaskList; > > > > > > // > > > - // For disabling the device (especially Bus Master DMA) at > > > - // ExitBootServices(). > > > + // For disabling Bus Master DMA on the device at ExitBootServices(). > > > // > > > EFI_EVENT ExitBootEvent; > > > } ATA_ATAPI_PASS_THRU_INSTANCE; > > > diff --git > > > a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c > > > b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c > > > index 09064dda18b7..e10e0d4e65f6 100644 > > > --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c > > > +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c > > > @@ -480,8 +480,7 @@ InitializeAtaAtapiPassThru ( } > > > > > > /** > > > - Disable the device (especially Bus Master DMA) when exiting the > > > boot > > > - services. > > > + Disable Bus Master DMA on the device when exiting the boot services. > > > > > > @param[in] Event Event for which this notification function is being > > > called. > > > @@ -506,7 +505,7 @@ AtaPassThruExitBootServices ( > > > PciIo->Attributes ( > > > PciIo, > > > EfiPciIoAttributeOperationDisable, > > > - Instance->EnabledPciAttributes, > > > + Instance->EnabledPciAttributes & > > > + EFI_PCI_IO_ATTRIBUTE_BUS_MASTER, > > > NULL > > > ); > > > } > > > > > > > Thanks Everyone for the feedback; I fixed the typo pointed out by Star > > and pushed the patch as commit 76fd5a660d70. > > > > Cheers > > Laszlo > > _______________________________________________ > > edk2-devel mailing list > > edk2-devel@lists.01.org > > https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] MdeModulePkg/AtaAtapiPassThru: disable only BM-DMA at ExitBootServices() 2017-11-23 2:20 ` Ni, Ruiyu @ 2017-11-23 13:08 ` Laszlo Ersek 2017-11-23 14:58 ` Ni, Ruiyu 2017-11-24 0:01 ` Paolo Bonzini 0 siblings, 2 replies; 19+ messages in thread From: Laszlo Ersek @ 2017-11-23 13:08 UTC (permalink / raw) To: Ni, Ruiyu, Zeng, Star, edk2-devel-01 Cc: Ard Biesheuvel, Dong, Eric, Dann Frazier 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. So, for OVMF at least, I'd like to keep the present behavior, but for other platforms, I don't insist on disabling BMDMA, of course. We already have a PCD called "PcdAcpiS3Enable" in "MdeModulePkg.dec"; maybe we can introduce "PcdAcpiS4Enable" too. BTW, has anyone reported this S4 issue to Microsoft? (Personally I'm totally unable to talk to Microsoft -- no working communication channels seem to be accessible to me. I hope Intel might fare better.) I wonder what their opinion would be on the issue. Thanks, Laszlo >> -----Original Message----- >> From: Zeng, Star >> Sent: Wednesday, November 22, 2017 6:26 PM >> To: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; >> edk2-devel-01 <edk2-devel@lists.01.org> >> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Dong, Eric >> <eric.dong@intel.com>; Dann Frazier <dannf@ubuntu.com>; Zeng, Star >> <star.zeng@intel.com> >> Subject: RE: [edk2] [PATCH] MdeModulePkg/AtaAtapiPassThru: disable only >> BM-DMA at ExitBootServices() >> >> Ray, >> >> You may explain more why Win10 S4 resume fails with only BM disabled, >> then Laszlo can know the issue well. >> >> >> Thanks, >> Star >> -----Original Message----- >> From: Ni, Ruiyu >> Sent: Wednesday, November 22, 2017 6:06 PM >> To: Laszlo Ersek <lersek@redhat.com>; edk2-devel-01 <edk2- >> devel@lists.01.org> >> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Dong, Eric >> <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>; Dann Frazier >> <dannf@ubuntu.com> >> Subject: RE: [edk2] [PATCH] MdeModulePkg/AtaAtapiPassThru: disable only >> BM-DMA at ExitBootServices() >> >> Laszlo, >> Our QA found Win10 S4 resume fails due to your change. >> As you might notice that I just rolled back the BM disabling patches in PciBus >> module, I am thinking about maybe you need to rollback the whole >> ExitBootServices callback as well to fix the compatibility issue. >> >> Thanks/Ray >> >>> -----Original Message----- >>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of >>> Laszlo Ersek >>> Sent: Saturday, October 28, 2017 12:10 AM >>> To: edk2-devel-01 <edk2-devel@lists.01.org> >>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Dong, Eric >>> <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>; Dann Frazier >>> <dannf@ubuntu.com> >>> Subject: Re: [edk2] [PATCH] MdeModulePkg/AtaAtapiPassThru: disable >>> only BM-DMA at ExitBootServices() >>> >>> On 10/26/17 17:48, Laszlo Ersek wrote: >>>> Clearing I/O port decoding in the PCI command register at >>>> ExitBootServices() breaks IDE boot in Windows, on QEMU's "pc" >>>> (i440fx) machine type. (AHCI boot on "q35" is unaffected.) Windows >>>> seems repeatedly stuck, apparently waiting for a timeout of sorts. >>>> >>>> This is arguably a Windows bug; a native OS driver should not expect >>>> the firmware to leave the PCI command register in any particular state. >>>> >>>> Strictly speaking, we only need to disable BM-DMA at >>>> ExitBootServices(), in order to abort pending transfers to/from RAM, >>>> which is soon to be owned by the OS. BM-DMA is also the only bit >>>> that's explicitly named by the UEFI Driver Writers' Guide, for >>>> clearing at >>> ExitBootServices(). >>>> >>>> I've verified that clearing only BM-DMA fixes the isse (boot time) >>>> on i440fx, and does not regress q35/AHCI. >>>> >>>> Cc: Aleksei Kovura <alex3kov@zoho.com> >>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>>> Cc: Dann Frazier <dannf@ubuntu.com> >>>> Cc: Eric Dong <eric.dong@intel.com> >>>> Cc: Star Zeng <star.zeng@intel.com> >>>> Reported-by: Aleksei Kovura <alex3kov@zoho.com> >>>> Reported-by: Dann Frazier <dannf@ubuntu.com> >>>> Reported-by: https://launchpad.net/~cjkrupp >>>> Bisected-by: Dann Frazier <dannf@ubuntu.com> >>>> Bisected-by: https://launchpad.net/~cjkrupp >>>> Suggested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>>> Suggested-by: Star Zeng <star.zeng@intel.com> >>>> Ref: https://bugs.launchpad.net/ubuntu/+source/edk2/+bug/1725560 >>>> Fixes: 6fb8ddd36bde45614b0a069528cdc97077835a74 >>>> Contributed-under: TianoCore Contribution Agreement 1.1 >>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com> >>>> --- >>>> >>>> Notes: >>>> Repo: https://github.com/lersek/edk2.git >>>> Branch: ata_disable_only_bmdma >>>> >>>> MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h | 3 +-- >>>> MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c | 5 ++--- >>>> 2 files changed, 3 insertions(+), 5 deletions(-) >>>> >>>> diff --git >>>> a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h >>>> b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h >>>> index 8d6eac706c0b..92c5bf2001cd 100644 >>>> --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h >>>> +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h >>>> @@ -123,8 +123,7 @@ typedef struct { >>>> LIST_ENTRY NonBlockingTaskList; >>>> >>>> // >>>> - // For disabling the device (especially Bus Master DMA) at >>>> - // ExitBootServices(). >>>> + // For disabling Bus Master DMA on the device at ExitBootServices(). >>>> // >>>> EFI_EVENT ExitBootEvent; >>>> } ATA_ATAPI_PASS_THRU_INSTANCE; >>>> diff --git >>>> a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c >>>> b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c >>>> index 09064dda18b7..e10e0d4e65f6 100644 >>>> --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c >>>> +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c >>>> @@ -480,8 +480,7 @@ InitializeAtaAtapiPassThru ( } >>>> >>>> /** >>>> - Disable the device (especially Bus Master DMA) when exiting the >>>> boot >>>> - services. >>>> + Disable Bus Master DMA on the device when exiting the boot services. >>>> >>>> @param[in] Event Event for which this notification function is being >>>> called. >>>> @@ -506,7 +505,7 @@ AtaPassThruExitBootServices ( >>>> PciIo->Attributes ( >>>> PciIo, >>>> EfiPciIoAttributeOperationDisable, >>>> - Instance->EnabledPciAttributes, >>>> + Instance->EnabledPciAttributes & >>>> + EFI_PCI_IO_ATTRIBUTE_BUS_MASTER, >>>> NULL >>>> ); >>>> } >>>> >>> >>> Thanks Everyone for the feedback; I fixed the typo pointed out by Star >>> and pushed the patch as commit 76fd5a660d70. >>> >>> Cheers >>> Laszlo >>> _______________________________________________ >>> edk2-devel mailing list >>> edk2-devel@lists.01.org >>> https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] MdeModulePkg/AtaAtapiPassThru: disable only BM-DMA at ExitBootServices() 2017-11-23 13:08 ` Laszlo Ersek @ 2017-11-23 14:58 ` Ni, Ruiyu 2017-11-23 17:19 ` Laszlo Ersek 2017-11-24 0:01 ` Paolo Bonzini 1 sibling, 1 reply; 19+ messages in thread From: Ni, Ruiyu @ 2017-11-23 14:58 UTC (permalink / raw) To: Laszlo Ersek, Zeng, Star, edk2-devel-01 Cc: Ard Biesheuvel, Dong, Eric, Dann Frazier Laszlo, A S3 flag PCD was added to optimize boot flow. Let's say, setting the flag to false is to remove some unnecessary code. setting to true is also fine. But in this case, setting S4 flag to false is to add some incompatible code. It's a bit different. If you check the two patches to revert the PciBus change, Microsoft gave their Signed-off. /Ray > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Thursday, November 23, 2017 9:08 PM > To: Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>; edk2- > devel-01 <edk2-devel@lists.01.org> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Dong, Eric > <eric.dong@intel.com>; Dann Frazier <dannf@ubuntu.com> > Subject: Re: [edk2] [PATCH] MdeModulePkg/AtaAtapiPassThru: disable only BM- > DMA at ExitBootServices() > > 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. > > So, for OVMF at least, I'd like to keep the present behavior, but for other > platforms, I don't insist on disabling BMDMA, of course. > > We already have a PCD called "PcdAcpiS3Enable" in "MdeModulePkg.dec"; > maybe we can introduce "PcdAcpiS4Enable" too. > > BTW, has anyone reported this S4 issue to Microsoft? (Personally I'm totally > unable to talk to Microsoft -- no working communication channels seem to be > accessible to me. I hope Intel might fare better.) I wonder what their opinion > would be on the issue. > > Thanks, > Laszlo > > >> -----Original Message----- > >> From: Zeng, Star > >> Sent: Wednesday, November 22, 2017 6:26 PM > >> To: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; > >> edk2-devel-01 <edk2-devel@lists.01.org> > >> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Dong, Eric > >> <eric.dong@intel.com>; Dann Frazier <dannf@ubuntu.com>; Zeng, Star > >> <star.zeng@intel.com> > >> Subject: RE: [edk2] [PATCH] MdeModulePkg/AtaAtapiPassThru: disable > >> only BM-DMA at ExitBootServices() > >> > >> Ray, > >> > >> You may explain more why Win10 S4 resume fails with only BM disabled, > >> then Laszlo can know the issue well. > >> > >> > >> Thanks, > >> Star > >> -----Original Message----- > >> From: Ni, Ruiyu > >> Sent: Wednesday, November 22, 2017 6:06 PM > >> To: Laszlo Ersek <lersek@redhat.com>; edk2-devel-01 <edk2- > >> devel@lists.01.org> > >> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Dong, Eric > >> <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>; Dann Frazier > >> <dannf@ubuntu.com> > >> Subject: RE: [edk2] [PATCH] MdeModulePkg/AtaAtapiPassThru: disable > >> only BM-DMA at ExitBootServices() > >> > >> Laszlo, > >> Our QA found Win10 S4 resume fails due to your change. > >> As you might notice that I just rolled back the BM disabling patches > >> in PciBus module, I am thinking about maybe you need to rollback the > >> whole ExitBootServices callback as well to fix the compatibility issue. > >> > >> Thanks/Ray > >> > >>> -----Original Message----- > >>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf > >>> Of Laszlo Ersek > >>> Sent: Saturday, October 28, 2017 12:10 AM > >>> To: edk2-devel-01 <edk2-devel@lists.01.org> > >>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Dong, Eric > >>> <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>; Dann > >>> Frazier <dannf@ubuntu.com> > >>> Subject: Re: [edk2] [PATCH] MdeModulePkg/AtaAtapiPassThru: disable > >>> only BM-DMA at ExitBootServices() > >>> > >>> On 10/26/17 17:48, Laszlo Ersek wrote: > >>>> Clearing I/O port decoding in the PCI command register at > >>>> ExitBootServices() breaks IDE boot in Windows, on QEMU's "pc" > >>>> (i440fx) machine type. (AHCI boot on "q35" is unaffected.) Windows > >>>> seems repeatedly stuck, apparently waiting for a timeout of sorts. > >>>> > >>>> This is arguably a Windows bug; a native OS driver should not > >>>> expect the firmware to leave the PCI command register in any particular > state. > >>>> > >>>> Strictly speaking, we only need to disable BM-DMA at > >>>> ExitBootServices(), in order to abort pending transfers to/from > >>>> RAM, which is soon to be owned by the OS. BM-DMA is also the only > >>>> bit that's explicitly named by the UEFI Driver Writers' Guide, for > >>>> clearing at > >>> ExitBootServices(). > >>>> > >>>> I've verified that clearing only BM-DMA fixes the isse (boot time) > >>>> on i440fx, and does not regress q35/AHCI. > >>>> > >>>> Cc: Aleksei Kovura <alex3kov@zoho.com> > >>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > >>>> Cc: Dann Frazier <dannf@ubuntu.com> > >>>> Cc: Eric Dong <eric.dong@intel.com> > >>>> Cc: Star Zeng <star.zeng@intel.com> > >>>> Reported-by: Aleksei Kovura <alex3kov@zoho.com> > >>>> Reported-by: Dann Frazier <dannf@ubuntu.com> > >>>> Reported-by: https://launchpad.net/~cjkrupp > >>>> Bisected-by: Dann Frazier <dannf@ubuntu.com> > >>>> Bisected-by: https://launchpad.net/~cjkrupp > >>>> Suggested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > >>>> Suggested-by: Star Zeng <star.zeng@intel.com> > >>>> Ref: https://bugs.launchpad.net/ubuntu/+source/edk2/+bug/1725560 > >>>> Fixes: 6fb8ddd36bde45614b0a069528cdc97077835a74 > >>>> Contributed-under: TianoCore Contribution Agreement 1.1 > >>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com> > >>>> --- > >>>> > >>>> Notes: > >>>> Repo: https://github.com/lersek/edk2.git > >>>> Branch: ata_disable_only_bmdma > >>>> > >>>> MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h | 3 +-- > >>>> MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c | 5 ++--- > >>>> 2 files changed, 3 insertions(+), 5 deletions(-) > >>>> > >>>> diff --git > >>>> a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h > >>>> b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h > >>>> index 8d6eac706c0b..92c5bf2001cd 100644 > >>>> --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h > >>>> +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h > >>>> @@ -123,8 +123,7 @@ typedef struct { > >>>> LIST_ENTRY NonBlockingTaskList; > >>>> > >>>> // > >>>> - // For disabling the device (especially Bus Master DMA) at > >>>> - // ExitBootServices(). > >>>> + // For disabling Bus Master DMA on the device at ExitBootServices(). > >>>> // > >>>> EFI_EVENT ExitBootEvent; > >>>> } ATA_ATAPI_PASS_THRU_INSTANCE; > >>>> diff --git > >>>> a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c > >>>> b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c > >>>> index 09064dda18b7..e10e0d4e65f6 100644 > >>>> --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c > >>>> +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c > >>>> @@ -480,8 +480,7 @@ InitializeAtaAtapiPassThru ( } > >>>> > >>>> /** > >>>> - Disable the device (especially Bus Master DMA) when exiting the > >>>> boot > >>>> - services. > >>>> + Disable Bus Master DMA on the device when exiting the boot services. > >>>> > >>>> @param[in] Event Event for which this notification function is being > >>>> called. > >>>> @@ -506,7 +505,7 @@ AtaPassThruExitBootServices ( > >>>> PciIo->Attributes ( > >>>> PciIo, > >>>> EfiPciIoAttributeOperationDisable, > >>>> - Instance->EnabledPciAttributes, > >>>> + Instance->EnabledPciAttributes & > >>>> + EFI_PCI_IO_ATTRIBUTE_BUS_MASTER, > >>>> NULL > >>>> ); > >>>> } > >>>> > >>> > >>> Thanks Everyone for the feedback; I fixed the typo pointed out by > >>> Star and pushed the patch as commit 76fd5a660d70. > >>> > >>> Cheers > >>> Laszlo > >>> _______________________________________________ > >>> edk2-devel mailing list > >>> edk2-devel@lists.01.org > >>> https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] MdeModulePkg/AtaAtapiPassThru: disable only BM-DMA at ExitBootServices() 2017-11-23 14:58 ` Ni, Ruiyu @ 2017-11-23 17:19 ` Laszlo Ersek 2017-11-23 23:06 ` Ni, Ruiyu 0 siblings, 1 reply; 19+ messages in thread From: Laszlo Ersek @ 2017-11-23 17:19 UTC (permalink / raw) To: Ni, Ruiyu, Zeng, Star, edk2-devel-01 Cc: Ard Biesheuvel, Dong, Eric, Dann Frazier On 11/23/17 15:58, Ni, Ruiyu wrote: > Laszlo, > A S3 flag PCD was added to optimize boot flow. Let's say, > setting the flag to false is to remove some unnecessary code. > setting to true is also fine. > > But in this case, setting S4 flag to false is to add some incompatible code. > It's a bit different. > > If you check the two patches to revert the PciBus change, Microsoft gave > their Signed-off. Of course they did: Windows 10 is a Microsoft OS; they obviously will not want to break their own OS with firmware changes. :) Anyway, joking aside, as I said, I *do* want to keep Windows 10 in working shape. I'm just saying that this should be a platform choice. On some platforms, the AtaAtapiPassThru EBS handler does not tickle the bug in Windows 10, so on those platforms, it makes sense to do what the DWG recommends (and keep the code). If you strongly disagree with allowing customization for this code, then: (1) Can you please submit a two part series that reverts the following patches (in this order): (a) revert 76fd5a660d704538a1b14a58d03a4eef9682b01c (b) revert 6fb8ddd36bde45614b0a069528cdc97077835a74 This will keep a good git history, and I'll ACK these reverts. (2) I'll file a down-stream (Red Hat) Bugzilla to make sure we keep the EBS handler as it is now (at 8284b1791ea9) when we next rebase to upstream edk2 (as long as we don't support S4). Thanks, Laszlo > > /Ray > >> -----Original Message----- >> From: Laszlo Ersek [mailto:lersek@redhat.com] >> Sent: Thursday, November 23, 2017 9:08 PM >> To: Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>; edk2- >> devel-01 <edk2-devel@lists.01.org> >> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Dong, Eric >> <eric.dong@intel.com>; Dann Frazier <dannf@ubuntu.com> >> Subject: Re: [edk2] [PATCH] MdeModulePkg/AtaAtapiPassThru: disable only BM- >> DMA at ExitBootServices() >> >> 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. >> >> So, for OVMF at least, I'd like to keep the present behavior, but for other >> platforms, I don't insist on disabling BMDMA, of course. >> >> We already have a PCD called "PcdAcpiS3Enable" in "MdeModulePkg.dec"; >> maybe we can introduce "PcdAcpiS4Enable" too. >> >> BTW, has anyone reported this S4 issue to Microsoft? (Personally I'm totally >> unable to talk to Microsoft -- no working communication channels seem to be >> accessible to me. I hope Intel might fare better.) I wonder what their opinion >> would be on the issue. >> >> Thanks, >> Laszlo >> >>>> -----Original Message----- >>>> From: Zeng, Star >>>> Sent: Wednesday, November 22, 2017 6:26 PM >>>> To: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; >>>> edk2-devel-01 <edk2-devel@lists.01.org> >>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Dong, Eric >>>> <eric.dong@intel.com>; Dann Frazier <dannf@ubuntu.com>; Zeng, Star >>>> <star.zeng@intel.com> >>>> Subject: RE: [edk2] [PATCH] MdeModulePkg/AtaAtapiPassThru: disable >>>> only BM-DMA at ExitBootServices() >>>> >>>> Ray, >>>> >>>> You may explain more why Win10 S4 resume fails with only BM disabled, >>>> then Laszlo can know the issue well. >>>> >>>> >>>> Thanks, >>>> Star >>>> -----Original Message----- >>>> From: Ni, Ruiyu >>>> Sent: Wednesday, November 22, 2017 6:06 PM >>>> To: Laszlo Ersek <lersek@redhat.com>; edk2-devel-01 <edk2- >>>> devel@lists.01.org> >>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Dong, Eric >>>> <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>; Dann Frazier >>>> <dannf@ubuntu.com> >>>> Subject: RE: [edk2] [PATCH] MdeModulePkg/AtaAtapiPassThru: disable >>>> only BM-DMA at ExitBootServices() >>>> >>>> Laszlo, >>>> Our QA found Win10 S4 resume fails due to your change. >>>> As you might notice that I just rolled back the BM disabling patches >>>> in PciBus module, I am thinking about maybe you need to rollback the >>>> whole ExitBootServices callback as well to fix the compatibility issue. >>>> >>>> Thanks/Ray >>>> >>>>> -----Original Message----- >>>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf >>>>> Of Laszlo Ersek >>>>> Sent: Saturday, October 28, 2017 12:10 AM >>>>> To: edk2-devel-01 <edk2-devel@lists.01.org> >>>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Dong, Eric >>>>> <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>; Dann >>>>> Frazier <dannf@ubuntu.com> >>>>> Subject: Re: [edk2] [PATCH] MdeModulePkg/AtaAtapiPassThru: disable >>>>> only BM-DMA at ExitBootServices() >>>>> >>>>> On 10/26/17 17:48, Laszlo Ersek wrote: >>>>>> Clearing I/O port decoding in the PCI command register at >>>>>> ExitBootServices() breaks IDE boot in Windows, on QEMU's "pc" >>>>>> (i440fx) machine type. (AHCI boot on "q35" is unaffected.) Windows >>>>>> seems repeatedly stuck, apparently waiting for a timeout of sorts. >>>>>> >>>>>> This is arguably a Windows bug; a native OS driver should not >>>>>> expect the firmware to leave the PCI command register in any particular >> state. >>>>>> >>>>>> Strictly speaking, we only need to disable BM-DMA at >>>>>> ExitBootServices(), in order to abort pending transfers to/from >>>>>> RAM, which is soon to be owned by the OS. BM-DMA is also the only >>>>>> bit that's explicitly named by the UEFI Driver Writers' Guide, for >>>>>> clearing at >>>>> ExitBootServices(). >>>>>> >>>>>> I've verified that clearing only BM-DMA fixes the isse (boot time) >>>>>> on i440fx, and does not regress q35/AHCI. >>>>>> >>>>>> Cc: Aleksei Kovura <alex3kov@zoho.com> >>>>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>>>>> Cc: Dann Frazier <dannf@ubuntu.com> >>>>>> Cc: Eric Dong <eric.dong@intel.com> >>>>>> Cc: Star Zeng <star.zeng@intel.com> >>>>>> Reported-by: Aleksei Kovura <alex3kov@zoho.com> >>>>>> Reported-by: Dann Frazier <dannf@ubuntu.com> >>>>>> Reported-by: https://launchpad.net/~cjkrupp >>>>>> Bisected-by: Dann Frazier <dannf@ubuntu.com> >>>>>> Bisected-by: https://launchpad.net/~cjkrupp >>>>>> Suggested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>>>>> Suggested-by: Star Zeng <star.zeng@intel.com> >>>>>> Ref: https://bugs.launchpad.net/ubuntu/+source/edk2/+bug/1725560 >>>>>> Fixes: 6fb8ddd36bde45614b0a069528cdc97077835a74 >>>>>> Contributed-under: TianoCore Contribution Agreement 1.1 >>>>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com> >>>>>> --- >>>>>> >>>>>> Notes: >>>>>> Repo: https://github.com/lersek/edk2.git >>>>>> Branch: ata_disable_only_bmdma >>>>>> >>>>>> MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h | 3 +-- >>>>>> MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c | 5 ++--- >>>>>> 2 files changed, 3 insertions(+), 5 deletions(-) >>>>>> >>>>>> diff --git >>>>>> a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h >>>>>> b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h >>>>>> index 8d6eac706c0b..92c5bf2001cd 100644 >>>>>> --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h >>>>>> +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h >>>>>> @@ -123,8 +123,7 @@ typedef struct { >>>>>> LIST_ENTRY NonBlockingTaskList; >>>>>> >>>>>> // >>>>>> - // For disabling the device (especially Bus Master DMA) at >>>>>> - // ExitBootServices(). >>>>>> + // For disabling Bus Master DMA on the device at ExitBootServices(). >>>>>> // >>>>>> EFI_EVENT ExitBootEvent; >>>>>> } ATA_ATAPI_PASS_THRU_INSTANCE; >>>>>> diff --git >>>>>> a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c >>>>>> b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c >>>>>> index 09064dda18b7..e10e0d4e65f6 100644 >>>>>> --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c >>>>>> +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c >>>>>> @@ -480,8 +480,7 @@ InitializeAtaAtapiPassThru ( } >>>>>> >>>>>> /** >>>>>> - Disable the device (especially Bus Master DMA) when exiting the >>>>>> boot >>>>>> - services. >>>>>> + Disable Bus Master DMA on the device when exiting the boot services. >>>>>> >>>>>> @param[in] Event Event for which this notification function is being >>>>>> called. >>>>>> @@ -506,7 +505,7 @@ AtaPassThruExitBootServices ( >>>>>> PciIo->Attributes ( >>>>>> PciIo, >>>>>> EfiPciIoAttributeOperationDisable, >>>>>> - Instance->EnabledPciAttributes, >>>>>> + Instance->EnabledPciAttributes & >>>>>> + EFI_PCI_IO_ATTRIBUTE_BUS_MASTER, >>>>>> NULL >>>>>> ); >>>>>> } >>>>>> >>>>> >>>>> Thanks Everyone for the feedback; I fixed the typo pointed out by >>>>> Star and pushed the patch as commit 76fd5a660d70. >>>>> >>>>> Cheers >>>>> Laszlo >>>>> _______________________________________________ >>>>> edk2-devel mailing list >>>>> edk2-devel@lists.01.org >>>>> https://lists.01.org/mailman/listinfo/edk2-devel > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] MdeModulePkg/AtaAtapiPassThru: disable only BM-DMA at ExitBootServices() 2017-11-23 17:19 ` Laszlo Ersek @ 2017-11-23 23:06 ` Ni, Ruiyu 0 siblings, 0 replies; 19+ messages in thread From: Ni, Ruiyu @ 2017-11-23 23:06 UTC (permalink / raw) To: Laszlo Ersek Cc: Zeng, Star, edk2-devel-01, Ard Biesheuvel, Dong, Eric, Dann Frazier Sent from a small-screen device 在 2017年11月24日,上午1:19,Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> 写道: On 11/23/17 15:58, Ni, Ruiyu wrote: Laszlo, A S3 flag PCD was added to optimize boot flow. Let's say, setting the flag to false is to remove some unnecessary code. setting to true is also fine. But in this case, setting S4 flag to false is to add some incompatible code. It's a bit different. If you check the two patches to revert the PciBus change, Microsoft gave their Signed-off. Of course they did: Windows 10 is a Microsoft OS; they obviously will not want to break their own OS with firmware changes. :) Anyway, joking aside, as I said, I *do* want to keep Windows 10 in working shape. I'm just saying that this should be a platform choice. On some platforms, the AtaAtapiPassThru EBS handler does not tickle the bug in Windows 10, so on those platforms, it makes sense to do what the DWG recommends (and keep the code). If you strongly disagree with allowing customization for this code, then: (1) Can you please submit a two part series that reverts the following patches (in this order): (a) revert 76fd5a660d704538a1b14a58d03a4eef9682b01c (b) revert 6fb8ddd36bde45614b0a069528cdc97077835a74 This will keep a good git history, and I'll ACK these reverts. Thanks for your understanding. I do need to revert the EBS handler, because it causes a critical S4 resume failure. I will revert them in your suggested way. Hopefully, the changes in PciBus could be back in future when the OS code is changed. (2) I'll file a down-stream (Red Hat) Bugzilla to make sure we keep the EBS handler as it is now (at 8284b1791ea9) when we next rebase to upstream edk2 (as long as we don't support S4). Thanks, Laszlo /Ray -----Original Message----- From: Laszlo Ersek [mailto:lersek@redhat.com] Sent: Thursday, November 23, 2017 9:08 PM To: Ni, Ruiyu <ruiyu.ni@intel.com<mailto:ruiyu.ni@intel.com>>; Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>>; edk2- devel-01 <edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org<mailto:ard.biesheuvel@linaro.org>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Dann Frazier <dannf@ubuntu.com<mailto:dannf@ubuntu.com>> Subject: Re: [edk2] [PATCH] MdeModulePkg/AtaAtapiPassThru: disable only BM- DMA at ExitBootServices() 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. So, for OVMF at least, I'd like to keep the present behavior, but for other platforms, I don't insist on disabling BMDMA, of course. We already have a PCD called "PcdAcpiS3Enable" in "MdeModulePkg.dec"; maybe we can introduce "PcdAcpiS4Enable" too. BTW, has anyone reported this S4 issue to Microsoft? (Personally I'm totally unable to talk to Microsoft -- no working communication channels seem to be accessible to me. I hope Intel might fare better.) I wonder what their opinion would be on the issue. Thanks, Laszlo -----Original Message----- From: Zeng, Star Sent: Wednesday, November 22, 2017 6:26 PM To: Ni, Ruiyu <ruiyu.ni@intel.com<mailto:ruiyu.ni@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; edk2-devel-01 <edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org<mailto:ard.biesheuvel@linaro.org>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Dann Frazier <dannf@ubuntu.com<mailto:dannf@ubuntu.com>>; Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>> Subject: RE: [edk2] [PATCH] MdeModulePkg/AtaAtapiPassThru: disable only BM-DMA at ExitBootServices() Ray, You may explain more why Win10 S4 resume fails with only BM disabled, then Laszlo can know the issue well. Thanks, Star -----Original Message----- From: Ni, Ruiyu Sent: Wednesday, November 22, 2017 6:06 PM To: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; edk2-devel-01 <edk2- devel@lists.01.org<mailto:devel@lists.01.org>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org<mailto:ard.biesheuvel@linaro.org>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>>; Dann Frazier <dannf@ubuntu.com<mailto:dannf@ubuntu.com>> Subject: RE: [edk2] [PATCH] MdeModulePkg/AtaAtapiPassThru: disable only BM-DMA at ExitBootServices() Laszlo, Our QA found Win10 S4 resume fails due to your change. As you might notice that I just rolled back the BM disabling patches in PciBus module, I am thinking about maybe you need to rollback the whole ExitBootServices callback as well to fix the compatibility issue. Thanks/Ray -----Original Message----- From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek Sent: Saturday, October 28, 2017 12:10 AM To: edk2-devel-01 <edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org<mailto:ard.biesheuvel@linaro.org>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>>; Dann Frazier <dannf@ubuntu.com<mailto:dannf@ubuntu.com>> Subject: Re: [edk2] [PATCH] MdeModulePkg/AtaAtapiPassThru: disable only BM-DMA at ExitBootServices() On 10/26/17 17:48, Laszlo Ersek wrote: Clearing I/O port decoding in the PCI command register at ExitBootServices() breaks IDE boot in Windows, on QEMU's "pc" (i440fx) machine type. (AHCI boot on "q35" is unaffected.) Windows seems repeatedly stuck, apparently waiting for a timeout of sorts. This is arguably a Windows bug; a native OS driver should not expect the firmware to leave the PCI command register in any particular state. Strictly speaking, we only need to disable BM-DMA at ExitBootServices(), in order to abort pending transfers to/from RAM, which is soon to be owned by the OS. BM-DMA is also the only bit that's explicitly named by the UEFI Driver Writers' Guide, for clearing at ExitBootServices(). I've verified that clearing only BM-DMA fixes the isse (boot time) on i440fx, and does not regress q35/AHCI. Cc: Aleksei Kovura <alex3kov@zoho.com<mailto:alex3kov@zoho.com>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org<mailto:ard.biesheuvel@linaro.org>> Cc: Dann Frazier <dannf@ubuntu.com<mailto:dannf@ubuntu.com>> Cc: Eric Dong <eric.dong@intel.com<mailto:eric.dong@intel.com>> Cc: Star Zeng <star.zeng@intel.com<mailto:star.zeng@intel.com>> Reported-by: Aleksei Kovura <alex3kov@zoho.com<mailto:alex3kov@zoho.com>> Reported-by: Dann Frazier <dannf@ubuntu.com<mailto:dannf@ubuntu.com>> Reported-by: https://launchpad.net/~cjkrupp Bisected-by: Dann Frazier <dannf@ubuntu.com<mailto:dannf@ubuntu.com>> Bisected-by: https://launchpad.net/~cjkrupp Suggested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org<mailto:ard.biesheuvel@linaro.org>> Suggested-by: Star Zeng <star.zeng@intel.com<mailto:star.zeng@intel.com>> Ref: https://bugs.launchpad.net/ubuntu/+source/edk2/+bug/1725560 Fixes: 6fb8ddd36bde45614b0a069528cdc97077835a74 Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> --- Notes: Repo: https://github.com/lersek/edk2.git Branch: ata_disable_only_bmdma MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h | 3 +-- MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c | 5 ++--- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h index 8d6eac706c0b..92c5bf2001cd 100644 --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h @@ -123,8 +123,7 @@ typedef struct { LIST_ENTRY NonBlockingTaskList; // - // For disabling the device (especially Bus Master DMA) at - // ExitBootServices(). + // For disabling Bus Master DMA on the device at ExitBootServices(). // EFI_EVENT ExitBootEvent; } ATA_ATAPI_PASS_THRU_INSTANCE; diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c index 09064dda18b7..e10e0d4e65f6 100644 --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c @@ -480,8 +480,7 @@ InitializeAtaAtapiPassThru ( } /** - Disable the device (especially Bus Master DMA) when exiting the boot - services. + Disable Bus Master DMA on the device when exiting the boot services. @param[in] Event Event for which this notification function is being called. @@ -506,7 +505,7 @@ AtaPassThruExitBootServices ( PciIo->Attributes ( PciIo, EfiPciIoAttributeOperationDisable, - Instance->EnabledPciAttributes, + Instance->EnabledPciAttributes & + EFI_PCI_IO_ATTRIBUTE_BUS_MASTER, NULL ); } Thanks Everyone for the feedback; I fixed the typo pointed out by Star and pushed the patch as commit 76fd5a660d70. Cheers Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] MdeModulePkg/AtaAtapiPassThru: disable only BM-DMA at ExitBootServices() 2017-11-23 13:08 ` Laszlo Ersek 2017-11-23 14:58 ` Ni, Ruiyu @ 2017-11-24 0:01 ` Paolo Bonzini 2017-11-24 1:04 ` Ni, Ruiyu 1 sibling, 1 reply; 19+ messages in thread From: Paolo Bonzini @ 2017-11-24 0:01 UTC (permalink / raw) To: Laszlo Ersek, Ni, Ruiyu, Zeng, Star, edk2-devel-01 Cc: Dann Frazier, Dong, Eric, Ard Biesheuvel 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] MdeModulePkg/AtaAtapiPassThru: disable only BM-DMA at ExitBootServices() 2017-11-24 0:01 ` Paolo Bonzini @ 2017-11-24 1:04 ` Ni, Ruiyu 2017-11-24 1:40 ` Yao, Jiewen 0 siblings, 1 reply; 19+ messages in thread From: Ni, Ruiyu @ 2017-11-24 1:04 UTC (permalink / raw) To: Paolo Bonzini Cc: Laszlo Ersek, Zeng, Star, edk2-devel-01, Dann Frazier, Dong, Eric, Ard Biesheuvel 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] MdeModulePkg/AtaAtapiPassThru: disable only BM-DMA at ExitBootServices() 2017-11-24 1:04 ` Ni, Ruiyu @ 2017-11-24 1:40 ` Yao, Jiewen 2017-11-27 12:29 ` Laszlo Ersek 0 siblings, 1 reply; 19+ messages in thread From: Yao, Jiewen @ 2017-11-24 1:40 UTC (permalink / raw) To: Ni, Ruiyu, Paolo Bonzini Cc: Dong, Eric, Ard Biesheuvel, edk2-devel-01, Dann Frazier, Laszlo Ersek, Zeng, Star 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 resolve DMA operation concern without breaking the OS, and meet DWG requirement at same time. Thank you Yao Jiewen > -----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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] MdeModulePkg/AtaAtapiPassThru: disable only BM-DMA at ExitBootServices() 2017-11-24 1:40 ` Yao, Jiewen @ 2017-11-27 12:29 ` Laszlo Ersek 2017-11-27 12:58 ` Zeng, Star 2017-11-27 13:41 ` Yao, Jiewen 0 siblings, 2 replies; 19+ messages in thread From: Laszlo Ersek @ 2017-11-27 12:29 UTC (permalink / raw) To: Yao, Jiewen, Ni, Ruiyu, Paolo Bonzini Cc: Dong, Eric, Ard Biesheuvel, edk2-devel-01, Dann Frazier, Zeng, Star 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/74D8A39837DF1E4DA445A8C0B3885C503A9A79BD@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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] MdeModulePkg/AtaAtapiPassThru: disable only BM-DMA at ExitBootServices() 2017-11-27 12:29 ` Laszlo Ersek @ 2017-11-27 12:58 ` Zeng, Star 2017-11-27 13:41 ` Yao, Jiewen 1 sibling, 0 replies; 19+ messages in thread From: Zeng, Star @ 2017-11-27 12:58 UTC (permalink / raw) To: Laszlo Ersek, Yao, Jiewen, Ni, Ruiyu, Paolo Bonzini Cc: Dong, Eric, Ard Biesheuvel, edk2-devel-01, Dann Frazier, Zeng, Star It is a way also I am thinking. Hope no OS takes any assumption related to AhciReset(). Thanks, Star -----Original Message----- From: Laszlo Ersek [mailto:lersek@redhat.com] 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: 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>; Zeng, Star <star.zeng@intel.com> 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/74D8A39837DF1E4DA445A8C0B3885C503A9A79BD@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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] MdeModulePkg/AtaAtapiPassThru: disable only BM-DMA at ExitBootServices() 2017-11-27 12:29 ` Laszlo Ersek 2017-11-27 12:58 ` Zeng, Star @ 2017-11-27 13:41 ` Yao, Jiewen 1 sibling, 0 replies; 19+ messages in thread From: Yao, Jiewen @ 2017-11-27 13:41 UTC (permalink / raw) To: Laszlo Ersek, Ni, Ruiyu, Paolo Bonzini Cc: edk2-devel-01, Dann Frazier, Dong, Eric, Zeng, Star, Ard Biesheuvel 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2017-11-27 13:37 UTC | newest] Thread overview: 19+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-10-26 15:48 [PATCH] MdeModulePkg/AtaAtapiPassThru: disable only BM-DMA at ExitBootServices() Laszlo Ersek 2017-10-26 19:56 ` dann frazier 2017-10-27 3:23 ` Zeng, Star 2017-10-27 12:25 ` Laszlo Ersek 2017-10-27 14:57 ` Aleksei 2017-10-27 16:09 ` Laszlo Ersek 2017-11-22 10:05 ` Ni, Ruiyu 2017-11-22 10:26 ` Zeng, Star 2017-11-23 2:20 ` Ni, Ruiyu 2017-11-23 13:08 ` Laszlo Ersek 2017-11-23 14:58 ` Ni, Ruiyu 2017-11-23 17:19 ` Laszlo Ersek 2017-11-23 23:06 ` Ni, Ruiyu 2017-11-24 0:01 ` Paolo Bonzini 2017-11-24 1:04 ` Ni, Ruiyu 2017-11-24 1:40 ` Yao, Jiewen 2017-11-27 12:29 ` Laszlo Ersek 2017-11-27 12:58 ` Zeng, Star 2017-11-27 13:41 ` Yao, Jiewen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox