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

[Laszlo Ersek <lersek@redhat.com>]

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