Re: [PATCH v5] MdeModulePkg: Correct high-memory use in NvmExpressDxe

["Wu, Hao A" <hao.a.wu@intel.com>]

Acked-by: Hao A Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu

> -----Original Message-----
> From: Tomas Pilar <quic_tpilar@quicinc.com>
> Sent: Friday, February 25, 2022 12:22 AM
> To: devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Ard Biesheuvel
> <ardb+tianocore@kernel.org>; Leif Lindholm <quic_llindhol@quicinc.com>;
> Wu, Hao A <hao.a.wu@intel.com>; Ard Biesheuvel <ardb@kernel.org>
> Subject: [PATCH v5] MdeModulePkg: Correct high-memory use in
> NvmExpressDxe
> 
> Move the logic that stores starting PCI attributes and sets the
> EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE attribute to
> DriverBindingStart() before the memory that backs the
> DMA engine is allocated.
> 
> This ensures that the DMA-backing memory is not forcibly allocated
> below 4G in system address map. Otherwise the allocation fails on
> platforms that do not have any memory below the 4G mark and the drive
> initialisation fails.
> 
> Leave the PCI device enabling attribute logic in NvmeControllerInit()
> to ensure that the device is re-enabled on reset in case it was
> disabled via PCI attributes.
> 
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
> Signed-off-by: Tomas Pilar <quic_tpilar@quicinc.com>
> ---
>  MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.c    | 27
> ++++++++++++++++++++
>  MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressHci.c | 26 +------------
> ------
>  2 files changed, 28 insertions(+), 25 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.c
> b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.c
> index 9d40f67e8e..5a1eda8e8d 100644
> --- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.c
> +++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.c
> @@ -959,6 +959,33 @@ NvmExpressDriverBindingStart (
>        goto Exit;
>      }
> 
> +    //
> +    // Save original PCI attributes
> +    //
> +    Status = PciIo->Attributes (
> +                      PciIo,
> +                      EfiPciIoAttributeOperationGet,
> +                      0,
> +                      &Private->PciAttributes
> +                      );
> +
> +    if (EFI_ERROR (Status)) {
> +      return Status;
> +    }
> +
> +    //
> +    // Enable 64-bit DMA support in the PCI layer.
> +    //
> +    Status = PciIo->Attributes (
> +                      PciIo,
> +                      EfiPciIoAttributeOperationEnable,
> +                      EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE,
> +                      NULL
> +                      );
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_WARN, "NvmExpressDriverBindingStart: failed to
> enable 64-bit DMA (%r)\n", Status));
> +    }
> +
>      //
>      // 6 x 4kB aligned buffers will be carved out of this buffer.
>      // 1st 4kB boundary is the start of the admin submission queue.
> diff --git a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressHci.c
> b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressHci.c
> index ac77afe113..d87212ffb2 100644
> --- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressHci.c
> +++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressHci.c
> @@ -728,20 +728,9 @@ NvmeControllerInit (
>    UINT8                Mn[41];
> 
>    //
> -  // Save original PCI attributes and enable this controller.
> +  // Enable this controller.
>    //
>    PciIo  = Private->PciIo;
> -  Status = PciIo->Attributes (
> -                    PciIo,
> -                    EfiPciIoAttributeOperationGet,
> -                    0,
> -                    &Private->PciAttributes
> -                    );
> -
> -  if (EFI_ERROR (Status)) {
> -    return Status;
> -  }
> -
>    Status = PciIo->Attributes (
>                      PciIo,
>                      EfiPciIoAttributeOperationSupported,
> @@ -764,19 +753,6 @@ NvmeControllerInit (
>      return Status;
>    }
> 
> -  //
> -  // Enable 64-bit DMA support in the PCI layer.
> -  //
> -  Status = PciIo->Attributes (
> -                    PciIo,
> -                    EfiPciIoAttributeOperationEnable,
> -                    EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE,
> -                    NULL
> -                    );
> -  if (EFI_ERROR (Status)) {
> -    DEBUG ((DEBUG_WARN, "NvmeControllerInit: failed to enable 64-bit
> DMA (%r)\n", Status));
> -  }
> -
>    //
>    // Read the Controller Capabilities register and verify that the NVM
> command set is supported
>    //
> --
> 2.30.2