1. Patchew
  2. EDK2
  3. View series

[edk2-devel] [PATCH v3] MdeModulePkg: Correct high-memory use in NvmExpressDxe

Tomas Pilar (tpilar) posted 1 patch 2 years, 2 months ago
Failed in applying to current master (apply log)
There is a newer version of this series
.../Bus/Pci/NvmExpressDxe/NvmExpress.c        | 27 +++++++++++++++++++
.../Bus/Pci/NvmExpressDxe/NvmExpressHci.c     | 26 +-----------------
2 files changed, 28 insertions(+), 25 deletions(-)
[edk2-devel] [PATCH v3] MdeModulePkg: Correct high-memory use in NvmExpressDxe
Posted by Tomas Pilar (tpilar) 2 years, 2 months ago 
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 <leif@nuviainc.com>
Signed-off-by: Tomas Pilar <quic_tpilar@quicinc.com>
---
 .../Bus/Pci/NvmExpressDxe/NvmExpress.c        | 27 +++++++++++++++++++
 .../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..b70499e3be 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, "NvmeControllerInit: 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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#86974): https://edk2.groups.io/g/devel/message/86974
Mute This Topic: https://groups.io/mt/89364574/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v3] MdeModulePkg: Correct high-memory use in NvmExpressDxe
Posted by Ard Biesheuvel 2 years, 2 months ago 
(+ Hao Wu)

On Thu, 24 Feb 2022 at 14:29, Tomas Pilar (tpilar)
<quic_tpilar@quicinc.com> wrote:
>
> 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 <leif@nuviainc.com>
> Signed-off-by: Tomas Pilar <quic_tpilar@quicinc.com>

This is still not exactly what I had in mind, but I think it's
actually better: the manipulation of the MMIO/IO decode and bus master
attributes occurs in the same place as before (and therefore
potentially multiple times), whereas recording the original value of
the attributes now occurs only once. In that sense, this patch fixes
more than one bug.

Reviewed-by: Ard Biesheuvel <ardb@kernel.org>


> ---
>  .../Bus/Pci/NvmExpressDxe/NvmExpress.c        | 27 +++++++++++++++++++
>  .../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..b70499e3be 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, "NvmeControllerInit: 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
>
>
> 
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#86976): https://edk2.groups.io/g/devel/message/86976
Mute This Topic: https://groups.io/mt/89364574/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v3] MdeModulePkg: Correct high-memory use in NvmExpressDxe
Posted by Rebecca Cran 2 years, 2 months ago 
On 2/24/22 06:29, Tomas Pilar (tpilar) wrote:
> +    //
> +    // 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));
> +    }
> +

The message should probably mention NvmExpressDriverBindingStart instead 
of NvmeControllerInit.


-- 
Rebecca Cran



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#86991): https://edk2.groups.io/g/devel/message/86991
Mute This Topic: https://groups.io/mt/89364574/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.