* [PATCH] MdeModulePkg: Correct high-memory use in NvmExpressDxe
@ 2022-02-24 11:47 Tomas Pilar
2022-02-24 11:58 ` Ard Biesheuvel
0 siblings, 1 reply; 4+ messages in thread
From: Tomas Pilar @ 2022-02-24 11:47 UTC (permalink / raw)
To: devel; +Cc: Ray Ni, Ard Biesheuvel, Leif Lindholm
Delay and move the allocation and mapping of memory that backs the DMA
engine in NvmExpress devices to NvmeInit() to ensure that
the allocation only happens after the
EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE attribute is set
on the PciIo controller.
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.
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 | 41 -------------
.../Bus/Pci/NvmExpressDxe/NvmExpressHci.c | 57 ++++++++++++++++---
2 files changed, 49 insertions(+), 49 deletions(-)
diff --git a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.c b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.c
index 9d40f67e8e..cc921756f5 100644
--- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.c
+++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.c
@@ -912,8 +912,6 @@ NvmExpressDriverBindingStart (
NVME_CONTROLLER_PRIVATE_DATA *Private;
EFI_DEVICE_PATH_PROTOCOL *ParentDevicePath;
UINT32 NamespaceId;
- EFI_PHYSICAL_ADDRESS MappedAddr;
- UINTN Bytes;
EFI_NVM_EXPRESS_PASS_THRU_PROTOCOL *Passthru;
DEBUG ((DEBUG_INFO, "NvmExpressDriverBindingStart: start\n"));
@@ -959,45 +957,6 @@ NvmExpressDriverBindingStart (
goto Exit;
}
- //
- // 6 x 4kB aligned buffers will be carved out of this buffer.
- // 1st 4kB boundary is the start of the admin submission queue.
- // 2nd 4kB boundary is the start of the admin completion queue.
- // 3rd 4kB boundary is the start of I/O submission queue #1.
- // 4th 4kB boundary is the start of I/O completion queue #1.
- // 5th 4kB boundary is the start of I/O submission queue #2.
- // 6th 4kB boundary is the start of I/O completion queue #2.
- //
- // Allocate 6 pages of memory, then map it for bus master read and write.
- //
- Status = PciIo->AllocateBuffer (
- PciIo,
- AllocateAnyPages,
- EfiBootServicesData,
- 6,
- (VOID **)&Private->Buffer,
- 0
- );
- if (EFI_ERROR (Status)) {
- goto Exit;
- }
-
- Bytes = EFI_PAGES_TO_SIZE (6);
- Status = PciIo->Map (
- PciIo,
- EfiPciIoOperationBusMasterCommonBuffer,
- Private->Buffer,
- &Bytes,
- &MappedAddr,
- &Private->Mapping
- );
-
- if (EFI_ERROR (Status) || (Bytes != EFI_PAGES_TO_SIZE (6))) {
- goto Exit;
- }
-
- Private->BufferPciAddr = (UINT8 *)(UINTN)MappedAddr;
-
Private->Signature = NVME_CONTROLLER_PRIVATE_DATA_SIGNATURE;
Private->ControllerHandle = Controller;
Private->ImageHandle = This->DriverBindingHandle;
diff --git a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressHci.c b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressHci.c
index ac77afe113..359373300e 100644
--- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressHci.c
+++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressHci.c
@@ -718,14 +718,16 @@ NvmeControllerInit (
IN NVME_CONTROLLER_PRIVATE_DATA *Private
)
{
- EFI_STATUS Status;
- EFI_PCI_IO_PROTOCOL *PciIo;
- UINT64 Supports;
- NVME_AQA Aqa;
- NVME_ASQ Asq;
- NVME_ACQ Acq;
- UINT8 Sn[21];
- UINT8 Mn[41];
+ EFI_STATUS Status;
+ EFI_PCI_IO_PROTOCOL *PciIo;
+ UINT64 Supports;
+ NVME_AQA Aqa;
+ NVME_ASQ Asq;
+ NVME_ACQ Acq;
+ UINT8 Sn[21];
+ UINT8 Mn[41];
+ UINTN Bytes;
+ EFI_PHYSICAL_ADDRESS MappedAddr;
//
// Save original PCI attributes and enable this controller.
@@ -777,6 +779,45 @@ NvmeControllerInit (
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.
+ // 2nd 4kB boundary is the start of the admin completion queue.
+ // 3rd 4kB boundary is the start of I/O submission queue #1.
+ // 4th 4kB boundary is the start of I/O completion queue #1.
+ // 5th 4kB boundary is the start of I/O submission queue #2.
+ // 6th 4kB boundary is the start of I/O completion queue #2.
+ //
+ // Allocate 6 pages of memory, then map it for bus master read and write.
+ //
+ Status = PciIo->AllocateBuffer (
+ PciIo,
+ AllocateAnyPages,
+ EfiBootServicesData,
+ 6,
+ (VOID **)&Private->Buffer,
+ 0
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ Bytes = EFI_PAGES_TO_SIZE (6);
+ Status = PciIo->Map (
+ PciIo,
+ EfiPciIoOperationBusMasterCommonBuffer,
+ Private->Buffer,
+ &Bytes,
+ &MappedAddr,
+ &Private->Mapping
+ );
+
+ if (EFI_ERROR (Status) || (Bytes != EFI_PAGES_TO_SIZE (6))) {
+ return Status;
+ }
+
+ Private->BufferPciAddr = (UINT8 *)(UINTN)MappedAddr;
+
//
// Read the Controller Capabilities register and verify that the NVM command set is supported
//
--
2.30.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] MdeModulePkg: Correct high-memory use in NvmExpressDxe
2022-02-24 11:47 [PATCH] MdeModulePkg: Correct high-memory use in NvmExpressDxe Tomas Pilar
@ 2022-02-24 11:58 ` Ard Biesheuvel
2022-02-24 12:09 ` Ard Biesheuvel
0 siblings, 1 reply; 4+ messages in thread
From: Ard Biesheuvel @ 2022-02-24 11:58 UTC (permalink / raw)
To: Tomas Pilar; +Cc: edk2-devel-groups-io, Ray Ni, Ard Biesheuvel, Leif Lindholm
On Thu, 24 Feb 2022 at 12:48, Tomas Pilar <quic_tpilar@quicinc.com> wrote:
>
> Delay and move the allocation and mapping of memory that backs the DMA
> engine in NvmExpress devices to NvmeInit() to ensure that
> the allocation only happens after the
> EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE attribute is set
> on the PciIo controller.
>
> 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.
>
> 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>
NvmeControllerInit () can be called multiple times, no? So you should
probably make sure that the buffer is not allocated and mapped again
if one already exists.
> ---
> .../Bus/Pci/NvmExpressDxe/NvmExpress.c | 41 -------------
> .../Bus/Pci/NvmExpressDxe/NvmExpressHci.c | 57 ++++++++++++++++---
> 2 files changed, 49 insertions(+), 49 deletions(-)
>
> diff --git a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.c b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.c
> index 9d40f67e8e..cc921756f5 100644
> --- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.c
> +++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.c
> @@ -912,8 +912,6 @@ NvmExpressDriverBindingStart (
> NVME_CONTROLLER_PRIVATE_DATA *Private;
> EFI_DEVICE_PATH_PROTOCOL *ParentDevicePath;
> UINT32 NamespaceId;
> - EFI_PHYSICAL_ADDRESS MappedAddr;
> - UINTN Bytes;
> EFI_NVM_EXPRESS_PASS_THRU_PROTOCOL *Passthru;
>
> DEBUG ((DEBUG_INFO, "NvmExpressDriverBindingStart: start\n"));
> @@ -959,45 +957,6 @@ NvmExpressDriverBindingStart (
> goto Exit;
> }
>
> - //
> - // 6 x 4kB aligned buffers will be carved out of this buffer.
> - // 1st 4kB boundary is the start of the admin submission queue.
> - // 2nd 4kB boundary is the start of the admin completion queue.
> - // 3rd 4kB boundary is the start of I/O submission queue #1.
> - // 4th 4kB boundary is the start of I/O completion queue #1.
> - // 5th 4kB boundary is the start of I/O submission queue #2.
> - // 6th 4kB boundary is the start of I/O completion queue #2.
> - //
> - // Allocate 6 pages of memory, then map it for bus master read and write.
> - //
> - Status = PciIo->AllocateBuffer (
> - PciIo,
> - AllocateAnyPages,
> - EfiBootServicesData,
> - 6,
> - (VOID **)&Private->Buffer,
> - 0
> - );
> - if (EFI_ERROR (Status)) {
> - goto Exit;
> - }
> -
> - Bytes = EFI_PAGES_TO_SIZE (6);
> - Status = PciIo->Map (
> - PciIo,
> - EfiPciIoOperationBusMasterCommonBuffer,
> - Private->Buffer,
> - &Bytes,
> - &MappedAddr,
> - &Private->Mapping
> - );
> -
> - if (EFI_ERROR (Status) || (Bytes != EFI_PAGES_TO_SIZE (6))) {
> - goto Exit;
> - }
> -
> - Private->BufferPciAddr = (UINT8 *)(UINTN)MappedAddr;
> -
> Private->Signature = NVME_CONTROLLER_PRIVATE_DATA_SIGNATURE;
> Private->ControllerHandle = Controller;
> Private->ImageHandle = This->DriverBindingHandle;
> diff --git a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressHci.c b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressHci.c
> index ac77afe113..359373300e 100644
> --- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressHci.c
> +++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressHci.c
> @@ -718,14 +718,16 @@ NvmeControllerInit (
> IN NVME_CONTROLLER_PRIVATE_DATA *Private
> )
> {
> - EFI_STATUS Status;
> - EFI_PCI_IO_PROTOCOL *PciIo;
> - UINT64 Supports;
> - NVME_AQA Aqa;
> - NVME_ASQ Asq;
> - NVME_ACQ Acq;
> - UINT8 Sn[21];
> - UINT8 Mn[41];
> + EFI_STATUS Status;
> + EFI_PCI_IO_PROTOCOL *PciIo;
> + UINT64 Supports;
> + NVME_AQA Aqa;
> + NVME_ASQ Asq;
> + NVME_ACQ Acq;
> + UINT8 Sn[21];
> + UINT8 Mn[41];
> + UINTN Bytes;
> + EFI_PHYSICAL_ADDRESS MappedAddr;
>
> //
> // Save original PCI attributes and enable this controller.
> @@ -777,6 +779,45 @@ NvmeControllerInit (
> 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.
> + // 2nd 4kB boundary is the start of the admin completion queue.
> + // 3rd 4kB boundary is the start of I/O submission queue #1.
> + // 4th 4kB boundary is the start of I/O completion queue #1.
> + // 5th 4kB boundary is the start of I/O submission queue #2.
> + // 6th 4kB boundary is the start of I/O completion queue #2.
> + //
> + // Allocate 6 pages of memory, then map it for bus master read and write.
> + //
> + Status = PciIo->AllocateBuffer (
> + PciIo,
> + AllocateAnyPages,
> + EfiBootServicesData,
> + 6,
> + (VOID **)&Private->Buffer,
> + 0
> + );
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> +
> + Bytes = EFI_PAGES_TO_SIZE (6);
> + Status = PciIo->Map (
> + PciIo,
> + EfiPciIoOperationBusMasterCommonBuffer,
> + Private->Buffer,
> + &Bytes,
> + &MappedAddr,
> + &Private->Mapping
> + );
> +
> + if (EFI_ERROR (Status) || (Bytes != EFI_PAGES_TO_SIZE (6))) {
> + return Status;
> + }
> +
> + Private->BufferPciAddr = (UINT8 *)(UINTN)MappedAddr;
> +
> //
> // Read the Controller Capabilities register and verify that the NVM command set is supported
> //
> --
> 2.30.2
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] MdeModulePkg: Correct high-memory use in NvmExpressDxe
2022-02-24 11:58 ` Ard Biesheuvel
@ 2022-02-24 12:09 ` Ard Biesheuvel
2022-02-24 12:33 ` Tomas Pilar (tpilar)
0 siblings, 1 reply; 4+ messages in thread
From: Ard Biesheuvel @ 2022-02-24 12:09 UTC (permalink / raw)
To: Tomas Pilar; +Cc: edk2-devel-groups-io, Ray Ni, Ard Biesheuvel, Leif Lindholm
On Thu, 24 Feb 2022 at 12:58, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Thu, 24 Feb 2022 at 12:48, Tomas Pilar <quic_tpilar@quicinc.com> wrote:
> >
> > Delay and move the allocation and mapping of memory that backs the DMA
> > engine in NvmExpress devices to NvmeInit() to ensure that
> > the allocation only happens after the
> > EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE attribute is set
> > on the PciIo controller.
> >
> > 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.
> >
> > 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>
>
> NvmeControllerInit () can be called multiple times, no? So you should
> probably make sure that the buffer is not allocated and mapped again
> if one already exists.
>
Alternatively, you could move all PCI attribute handling into the
start() hook. Other drivers keep it there as well, and it seems like a
more natural place for it (given the fact that NvmeControllerInit() is
also called on a host controller reset)
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] MdeModulePkg: Correct high-memory use in NvmExpressDxe
2022-02-24 12:09 ` Ard Biesheuvel
@ 2022-02-24 12:33 ` Tomas Pilar (tpilar)
0 siblings, 0 replies; 4+ messages in thread
From: Tomas Pilar (tpilar) @ 2022-02-24 12:33 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: edk2-devel-groups-io, Ray Ni, Ard Biesheuvel, Leif Lindholm
On 24/02/2022 12:09, Ard Biesheuvel wrote:
> On Thu, 24 Feb 2022 at 12:58, Ard Biesheuvel <ardb@kernel.org> wrote:
>> On Thu, 24 Feb 2022 at 12:48, Tomas Pilar <quic_tpilar@quicinc.com> wrote:
>>> Delay and move the allocation and mapping of memory that backs the DMA
>>> engine in NvmExpress devices to NvmeInit() to ensure that
>>> the allocation only happens after the
>>> EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE attribute is set
>>> on the PciIo controller.
>>>
>>> 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.
>>>
>>> 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>
>> NvmeControllerInit () can be called multiple times, no? So you should
>> probably make sure that the buffer is not allocated and mapped again
>> if one already exists.
>>
> Alternatively, you could move all PCI attribute handling into the
> start() hook. Other drivers keep it there as well, and it seems like a
> more natural place for it (given the fact that NvmeControllerInit() is
> also called on a host controller reset)
Brilliant idea, I'll do this!
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-02-24 12:33 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-02-24 11:47 [PATCH] MdeModulePkg: Correct high-memory use in NvmExpressDxe Tomas Pilar
2022-02-24 11:58 ` Ard Biesheuvel
2022-02-24 12:09 ` Ard Biesheuvel
2022-02-24 12:33 ` Tomas Pilar (tpilar)
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox