From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.65; helo=mga03.intel.com; envelope-from=ruiyu.ni@intel.com; receiver=edk2-devel@lists.01.org Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id B29172034C5F4 for ; Thu, 23 Nov 2017 15:02:32 -0800 (PST) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 23 Nov 2017 15:06:49 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.44,443,1505804400"; d="scan'208,217";a="1247780363" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by fmsmga002.fm.intel.com with ESMTP; 23 Nov 2017 15:06:49 -0800 Received: from fmsmsx156.amr.corp.intel.com (10.18.116.74) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 23 Nov 2017 15:06:49 -0800 Received: from shsmsx102.ccr.corp.intel.com (10.239.4.154) by fmsmsx156.amr.corp.intel.com (10.18.116.74) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 23 Nov 2017 15:06:48 -0800 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.152]) by shsmsx102.ccr.corp.intel.com ([169.254.2.175]) with mapi id 14.03.0319.002; Fri, 24 Nov 2017 07:06:46 +0800 From: "Ni, Ruiyu" To: Laszlo Ersek CC: "Zeng, Star" , edk2-devel-01 , Ard Biesheuvel , "Dong, Eric" , Dann Frazier Thread-Topic: [edk2] [PATCH] MdeModulePkg/AtaAtapiPassThru: disable only BM-DMA at ExitBootServices() Thread-Index: AQHTTnHqYwU6/89GNUuf4hMr1aV/GaL3WQMAgCj8OOD//4BlAIABkIZAgAAvH4CAAKNmUP//oruAgADnLXE= Date: Thu, 23 Nov 2017 23:06:45 +0000 Message-ID: <2167EBC9-2532-45AE-846F-557095580BB8@intel.com> References: <20171026154819.20865-1-lersek@redhat.com> <734D49CCEBEEF84792F5B80ED585239D5BACC0A1@SHSMSX104.ccr.corp.intel.com> <0C09AFA07DD0434D9E2A0C6AEB0483103B9BABBA@shsmsx102.ccr.corp.intel.com> <734D49CCEBEEF84792F5B80ED585239D5BACDEC2@SHSMSX104.ccr.corp.intel.com> <734D49CCEBEEF84792F5B80ED585239D5BACEB5A@SHSMSX104.ccr.corp.intel.com>, In-Reply-To: Accept-Language: en-US, zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: MIME-Version: 1.0 X-Content-Filtered-By: Mailman/MimeDel 2.1.22 Subject: Re: [PATCH] MdeModulePkg/AtaAtapiPassThru: disable only BM-DMA at ExitBootServices() X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 23 Nov 2017 23:02:32 -0000 Content-Language: zh-CN Content-Type: text/plain; charset="iso-2022-jp" Content-Transfer-Encoding: quoted-printable Sent from a small-screen device =1B$B:_=1B(B 2017=1B$BG/=1B(B11=1B$B7n=1B(B24=1B$BF|!$>e8a=1B(B1:19=1B$B!$= =1B(BLaszlo Ersek > =1B$B>; Zeng, Star <= star.zeng@intel.com>; edk2- devel-01 > Cc: Ard Biesheuvel >; Dong, Eric >; Dann Frazier > 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 totall= y 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 opin= ion would be on the issue. Thanks, Laszlo -----Original Message----- From: Zeng, Star Sent: Wednesday, November 22, 2017 6:26 PM To: Ni, Ruiyu >; Laszlo Ersek= >; edk2-devel-01 > Cc: Ard Biesheuvel >; Dong, Eric >; Dann Frazier >; Zeng, Star > 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 >; edk2-devel-= 01 > Cc: Ard Biesheuvel >; Dong, Eric >; Zeng, Star >; Dann Frazier > 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 > Cc: Ard Biesheuvel >; Dong, Eric >; Zeng, Star >; Dann Frazier > 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 > Cc: Ard Biesheuvel > Cc: Dann Frazier > Cc: Eric Dong > Cc: Star Zeng > Reported-by: Aleksei Kovura > Reported-by: Dann Frazier > Reported-by: https://launchpad.net/~cjkrupp Bisected-by: Dann Frazier > Bisected-by: https://launchpad.net/~cjkrupp Suggested-by: Ard Biesheuvel > Suggested-by: Star Zeng > Ref: https://bugs.launchpad.net/ubuntu/+source/edk2/+bug/1725560 Fixes: 6fb8ddd36bde45614b0a069528cdc97077835a74 Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Laszlo Ersek > --- 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