* [PATCH v2] MdeModulePkg: Correct high-memory use in NvmExpressDxe
@ 2022-02-24 12:57 Tomas Pilar (tpilar)
2022-02-24 13:13 ` [edk2-devel] " Ard Biesheuvel
0 siblings, 1 reply; 5+ messages in thread
From: Tomas Pilar (tpilar) @ 2022-02-24 12:57 UTC (permalink / raw)
To: devel; +Cc: Ray Ni, Ard Biesheuvel, Leif Lindholm
Move the logic that sets EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE Pci
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.
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>
---
MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.c | 13 +++++++++++++
MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressHci.c | 13 -------------
2 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.c b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.c
index 9d40f67e8e..1f0fc5bb68 100644
--- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.c
+++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.c
@@ -959,6 +959,19 @@ NvmExpressDriverBindingStart (
goto Exit;
}
+ //
+ // 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..748cb0ba24 100644
--- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressHci.c
+++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressHci.c
@@ -764,19 +764,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
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [edk2-devel] [PATCH v2] MdeModulePkg: Correct high-memory use in NvmExpressDxe
2022-02-24 12:57 [PATCH v2] MdeModulePkg: Correct high-memory use in NvmExpressDxe Tomas Pilar (tpilar)
@ 2022-02-24 13:13 ` Ard Biesheuvel
2022-02-24 13:14 ` Tomas Pilar (tpilar)
0 siblings, 1 reply; 5+ messages in thread
From: Ard Biesheuvel @ 2022-02-24 13:13 UTC (permalink / raw)
To: edk2-devel-groups-io, Tomas Pilar; +Cc: Ray Ni, Ard Biesheuvel, Leif Lindholm
On Thu, 24 Feb 2022 at 13:58, Tomas Pilar (tpilar)
<quic_tpilar@quicinc.com> wrote:
>
> Move the logic that sets EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE Pci
>
> 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.
>
>
>
> 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>
Ehm, nope, that is not exactly what I meant.
The existing code stores the original PCI attributes in the controller
private data, enables MMIO/IO decoding and bus mastering, and only
then sets the dual address cycle attribute.
All of that needs to move, so that the captured attributes are accurate.
>
> ---
>
> MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.c | 13 +++++++++++++
>
> MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressHci.c | 13 -------------
>
> 2 files changed, 13 insertions(+), 13 deletions(-)
>
>
>
> diff --git a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.c b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.c
>
> index 9d40f67e8e..1f0fc5bb68 100644
>
> --- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.c
>
> +++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.c
>
> @@ -959,6 +959,19 @@ NvmExpressDriverBindingStart (
>
> goto Exit;
>
> }
>
>
>
> + //
>
> + // 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..748cb0ba24 100644
>
> --- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressHci.c
>
> +++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressHci.c
>
> @@ -764,19 +764,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
>
>
>
>
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [edk2-devel] [PATCH v2] MdeModulePkg: Correct high-memory use in NvmExpressDxe
2022-02-24 13:13 ` [edk2-devel] " Ard Biesheuvel
@ 2022-02-24 13:14 ` Tomas Pilar (tpilar)
2022-02-24 13:19 ` Tomas Pilar (tpilar)
0 siblings, 1 reply; 5+ messages in thread
From: Tomas Pilar (tpilar) @ 2022-02-24 13:14 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel-groups-io
Cc: Ray Ni, Ard Biesheuvel, Leif Lindholm
On 24/02/2022 13:13, Ard Biesheuvel wrote:
> On Thu, 24 Feb 2022 at 13:58, Tomas Pilar (tpilar)
> <quic_tpilar@quicinc.com> wrote:
>> Move the logic that sets EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE Pci
>>
>> 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.
>>
>>
>>
>> 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>
> Ehm, nope, that is not exactly what I meant.
>
> The existing code stores the original PCI attributes in the controller
> private data, enables MMIO/IO decoding and bus mastering, and only
> then sets the dual address cycle attribute.
>
> All of that needs to move, so that the captured attributes are accurate.
>
>
Okay, I was wondering. My thought was that we probably want to re-enable
bus mastering
on reset so I kept that bit of code in the original location.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [edk2-devel] [PATCH v2] MdeModulePkg: Correct high-memory use in NvmExpressDxe
2022-02-24 13:14 ` Tomas Pilar (tpilar)
@ 2022-02-24 13:19 ` Tomas Pilar (tpilar)
2022-02-24 13:20 ` Tomas Pilar (tpilar)
0 siblings, 1 reply; 5+ messages in thread
From: Tomas Pilar (tpilar) @ 2022-02-24 13:19 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel-groups-io
Cc: Ray Ni, Ard Biesheuvel, Leif Lindholm
On 24/02/2022 13:14, Tomas Pilar (tpilar) wrote:
>
>
> On 24/02/2022 13:13, Ard Biesheuvel wrote:
>> On Thu, 24 Feb 2022 at 13:58, Tomas Pilar (tpilar)
>> <quic_tpilar@quicinc.com> wrote:
>>> Move the logic that sets EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE Pci
>>>
>>> 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.
>>>
>>>
>>>
>>> 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>
>> Ehm, nope, that is not exactly what I meant.
>>
>> The existing code stores the original PCI attributes in the controller
>> private data, enables MMIO/IO decoding and bus mastering, and only
>> then sets the dual address cycle attribute.
>>
>> All of that needs to move, so that the captured attributes are accurate.
>>
>>
> Okay, I was wondering. My thought was that we probably want to
> re-enable bus mastering
> on reset so I kept that bit of code in the original location.
>
Also, doesn't this code:
if (!EFI_ERROR (Status)) {
Supports &= (UINT64)EFI_PCI_DEVICE_ENABLE;
Status = PciIo->Attributes (
PciIo,
EfiPciIoAttributeOperationEnable,
Supports,
NULL
);
}
*strip* the PCI_DEVICE_ENABLE set of attributes rather than add them? I
am somewhat confused about this.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [edk2-devel] [PATCH v2] MdeModulePkg: Correct high-memory use in NvmExpressDxe
2022-02-24 13:19 ` Tomas Pilar (tpilar)
@ 2022-02-24 13:20 ` Tomas Pilar (tpilar)
0 siblings, 0 replies; 5+ messages in thread
From: Tomas Pilar (tpilar) @ 2022-02-24 13:20 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel-groups-io
Cc: Ray Ni, Ard Biesheuvel, Leif Lindholm
On 24/02/2022 13:19, Tomas Pilar (tpilar) wrote:
>
>
> On 24/02/2022 13:14, Tomas Pilar (tpilar) wrote:
>>
>>
>> On 24/02/2022 13:13, Ard Biesheuvel wrote:
>>> On Thu, 24 Feb 2022 at 13:58, Tomas Pilar (tpilar)
>>> <quic_tpilar@quicinc.com> wrote:
>>>> Move the logic that sets EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE Pci
>>>>
>>>> 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.
>>>>
>>>>
>>>>
>>>> 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>
>>> Ehm, nope, that is not exactly what I meant.
>>>
>>> The existing code stores the original PCI attributes in the controller
>>> private data, enables MMIO/IO decoding and bus mastering, and only
>>> then sets the dual address cycle attribute.
>>>
>>> All of that needs to move, so that the captured attributes are
>>> accurate.
>>>
>>>
>> Okay, I was wondering. My thought was that we probably want to
>> re-enable bus mastering
>> on reset so I kept that bit of code in the original location.
>>
>
> Also, doesn't this code:
>
> if (!EFI_ERROR (Status)) {
> Supports &= (UINT64)EFI_PCI_DEVICE_ENABLE;
> Status = PciIo->Attributes (
> PciIo,
> EfiPciIoAttributeOperationEnable,
> Supports,
> NULL
> );
> }
>
> *strip* the PCI_DEVICE_ENABLE set of attributes rather than add them?
> I am somewhat confused about this.
>
Wait no. I just cannot read code, ignore that last bit. I'll prepare a
patch.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-02-24 13:20 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-02-24 12:57 [PATCH v2] MdeModulePkg: Correct high-memory use in NvmExpressDxe Tomas Pilar (tpilar)
2022-02-24 13:13 ` [edk2-devel] " Ard Biesheuvel
2022-02-24 13:14 ` Tomas Pilar (tpilar)
2022-02-24 13:19 ` Tomas Pilar (tpilar)
2022-02-24 13:20 ` 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