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

["Tomas Pilar (tpilar)" <quic_tpilar@quicinc.com>]

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.