Re: [PATCH 2/2] MdeModulePkg/PciBus: Revert "Enable BM on P2P bridges on demand"

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

Reviewed-by: Jiewen.yao@intel.com

> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Monday, November 20, 2017 11:06 AM
> To: edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Michael Turner
> <michael.turner@microsoft.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
> Subject: [PATCH 2/2] MdeModulePkg/PciBus: Revert "Enable BM on P2P bridges
> on demand"
> 
> This reverts commit 5db417ed2522367290c365831f9d6628d31c346c.
>  "MdeModulePkg/PciBusDxe: Enable Bus Master on P2P bridges on demand"
> 
> We met some compatibility issues when doing Windows S4 resume.
> Reverting the BME disabling patches to fix the S4 resume issue.
> 
> Signed-off-by: Ruiyu Ni <Ruiyu.ni@intel.com>
> Signed-off-by: Michael Turner <michael.turner@microsoft.com>
> Cc: Michael D Kinney <Michael.d.kinney@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> ---
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c     | 16
> +++-------------
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c | 18
> +++---------------
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c                |  8 ++++----
>  3 files changed, 10 insertions(+), 32 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
> index 97bb971a59..e76c8f0046 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
> @@ -1,7 +1,7 @@
>  /** @file
>    Supporting functions implementaion for PCI devices management.
> 
> -Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2006 - 2015, Intel Corporation. All rights reserved.<BR>
>  This program and the accompanying materials
>  are licensed and made available under the terms and conditions of the BSD
> License
>  which accompanies this distribution.  The full text of the license may be found
> at
> @@ -711,12 +711,7 @@ StartPciDevicesOnBridge (
>                               0,
>                               &Supports
>                               );
> -        //
> -        // By default every bridge's IO and MMIO spaces are enabled.
> -        // Bridge's Bus Master will be enabled when any device behind it
> requests
> -        // to enable Bus Master.
> -        //
> -        Supports &= (UINT64) (EFI_PCI_IO_ATTRIBUTE_IO |
> EFI_PCI_IO_ATTRIBUTE_MEMORY);
> +        Supports &= (UINT64)EFI_PCI_DEVICE_ENABLE;
>          PciIoDevice->PciIo.Attributes (
>                               &(PciIoDevice->PciIo),
>                               EfiPciIoAttributeOperationEnable,
> @@ -768,12 +763,7 @@ StartPciDevicesOnBridge (
>                               0,
>                               &Supports
>                               );
> -        //
> -        // By default every bridge's IO and MMIO spaces are enabled.
> -        // Bridge's Bus Master will be enabled when any device behind it
> requests
> -        // to enable Bus Master.
> -        //
> -        Supports &= (UINT64) (EFI_PCI_IO_ATTRIBUTE_IO |
> EFI_PCI_IO_ATTRIBUTE_MEMORY);
> +        Supports &= (UINT64)EFI_PCI_DEVICE_ENABLE;
>          PciIoDevice->PciIo.Attributes (
>                               &(PciIoDevice->PciIo),
>                               EfiPciIoAttributeOperationEnable,
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> index f73756a31e..81171c82d9 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> @@ -1218,12 +1218,11 @@ DetermineDeviceAttribute (
>        return Status;
>      }
>      //
> -    // Assume the PCI Root Bridge supports DAC and Bus Master.
> +    // Assume the PCI Root Bridge supports DAC
>      //
>      PciIoDevice->Supports |=
> (UINT64)(EFI_PCI_IO_ATTRIBUTE_EMBEDDED_DEVICE |
>                                EFI_PCI_IO_ATTRIBUTE_EMBEDDED_ROM |
> -
> EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE |
> -                              EFI_PCI_IO_ATTRIBUTE_BUS_MASTER);
> +
> EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE);
> 
>    } else {
> 
> @@ -1234,16 +1233,9 @@ DetermineDeviceAttribute (
>      //
>      Command = EFI_PCI_COMMAND_IO_SPACE     |
>                EFI_PCI_COMMAND_MEMORY_SPACE |
> +              EFI_PCI_COMMAND_BUS_MASTER   |
>                EFI_PCI_COMMAND_VGA_PALETTE_SNOOP;
> 
> -    //
> -    // Per PCI-to-PCI Bridge Architecture all PCI-to-PCI bridges are Bus Master
> capable.
> -    // So only test the Bus Master capability for PCI devices.
> -    //
> -    if (!IS_PCI_BRIDGE(&PciIoDevice->Pci)) {
> -      Command |= EFI_PCI_COMMAND_BUS_MASTER;
> -    }
> -
>      BridgeControl = EFI_PCI_BRIDGE_CONTROL_ISA |
> EFI_PCI_BRIDGE_CONTROL_VGA | EFI_PCI_BRIDGE_CONTROL_VGA_16;
> 
>      //
> @@ -1253,11 +1245,7 @@ DetermineDeviceAttribute (
> 
>      //
>      // Set the supported attributes for specified PCI device
> -    // Per PCI-to-PCI Bridge Architecture all PCI-to-PCI bridges are Bus Master
> capable.
>      //
> -    if (IS_PCI_BRIDGE(&PciIoDevice->Pci)) {
> -      Command |= EFI_PCI_COMMAND_BUS_MASTER;
> -    }
>      PciSetDeviceAttribute (PciIoDevice, Command, BridgeControl,
> EFI_SET_SUPPORTS);
> 
>      //
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
> index 659f480d71..cc7125e4fc 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
> @@ -1348,8 +1348,7 @@ ModifyRootBridgeAttributes (
>    //
>    Attributes &= ~(UINT64)(EFI_PCI_IO_ATTRIBUTE_EMBEDDED_DEVICE |
>                            EFI_PCI_IO_ATTRIBUTE_EMBEDDED_ROM |
> -                          EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE |
> -                          EFI_PCI_IO_ATTRIBUTE_BUS_MASTER);
> +                          EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE);
> 
>    //
>    // Record the new attribute of the Root Bridge
> @@ -1727,11 +1726,12 @@ PciIoAttributes (
>    }
>    //
>    // The upstream bridge should be also set to revelant attribute
> -  // expect for IO and Mem
> +  // expect for IO, Mem and BusMaster
>    //
>    UpStreamAttributes = Attributes &
>                         (~(EFI_PCI_IO_ATTRIBUTE_IO     |
> -                          EFI_PCI_IO_ATTRIBUTE_MEMORY
> +                          EFI_PCI_IO_ATTRIBUTE_MEMORY |
> +                          EFI_PCI_IO_ATTRIBUTE_BUS_MASTER
>                            )
>                          );
>    UpStreamBridge = PciIoDevice->Parent;
> --
> 2.15.0.gvfs.1.preview.4