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

["Ard Biesheuvel" <ardb@kernel.org>]

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
>