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

[Laszlo Ersek <lersek@redhat.com>]

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
>