Re: [RFC 0/6] DxeBmDmaLib

[Laszlo Ersek <lersek@redhat.com>]

On 01/12/17 17:04, Duran, Leo wrote:
> 
> 
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Thursday, January 12, 2017 3:51 AM
>> To: Duran, Leo <leo.duran@amd.com>; edk2-devel@ml01.01.org
>> Cc: feng.tian@intel.com; Singh, Brijesh <brijesh.singh@amd.com>;
>> ard.biesheuvel@linaro.org; jordan.l.justen@intel.com;
>> prince.agyeman@intel.com; star.zeng@intel.com
>> Subject: Re: [edk2] [RFC 0/6] DxeBmDmaLib
>>
>> On 01/10/17 01:16, Leo Duran wrote:
>>> This patch-set provides an abstraction layer for DMA operations
>>> implemented by the PciHostBridgeDxe driver. The intent is to then
>>> allow override of this library as may be required by specific hardware
>>> implementations, such as AMD's Secure Encrypted Virtualization (SEV).
>>>
>>> This new (BmDmaLib class) library is modeled after the existing DmaLib
>>> plus an extra DmaAbove4GB (BOOLEAN) parameter in the Map and
>> Allocate
>>> interfaces, so that decisions can be made about the need to allocate
>>> DMA buffers below the 4GB boundary.
>>
>> * Ard, I remember you worked on >=4GB DMA stuff before, can you please
>> look over this series?
>>
>> * Leo, can you confirm that patches #1 and #6 implement nothing more than
>> code movement? If they implement more than that (judging from the new
>> DmaAbove4GB parameter above), can you specify how exactly?
>>
>> The source code changes in patches #2 and #5 look okay, but before I'm
>> comfortable acking them, I'd like to understand the scope of the code
>> movement between #1 and #6.
> [Duran, Leo] 
> Laszlo, it's more than just code movement.
> - Patch#1 clones the existing implementation from PciHostBridgeDxe,
>  but the ported code is simplified a bit by the addition of the 'DmaAbove4GB' parameter.
> - Patch#6 removes the ported code from PciHostBridgeDxe to leverage the library.

Okay, thanks. Makes sense.

I'm aware that the exact library interfaces are still under discussion
(wrt. generality for example), but as long as we keep the above scheme,
you can add (and carry forward), to patches #2 and #5, my

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks!
Laszlo



> 
>>
>> * Also, I suggest the following subject line template for patches #2 through
>> #5:
>>
>>   XxxPkg: resolve BmDmaLib class for PciHostBridgeDxe driver
>>
>> * For patch #1, I suggest restricting the client module types to DXE_DRIVER in
>> the INF file:
>>
>>   LIBRARY_CLASS                  = BmDmaLib|DXE_DRIVER
>>
>> Thanks
>> Laszlo
> [Duran, Leo] 
> OK,  sounds good to me. Thanks.
> 
>>
>>
>>>
>>> Leo Duran (6):
>>>   MdeModulePkg: Add DxeBmDmaLib library
>>>   ArmVirtPkg: Modify .DSC files that include PciHostBridgeDxe driver
>>>   CorebootPayloadPkg: Modify .DSC files that include PciHostBridgeDxe
>>>     driver
>>>   MdeModulePkg: Modify .DSC files that include PciHostBridgeDxe driver
>>>   OvmfPkg: Modify .DSC files that include PciHostBridgeDxe driver
>>>   MdeModulePkg: Modify PciHostBridgeDxe to use new BmDmaLib library.
>>>
>>>  ArmVirtPkg/ArmVirtQemu.dsc                         |   1 +
>>>  ArmVirtPkg/ArmVirtQemuKernel.dsc                   |   1 +
>>>  CorebootPayloadPkg/CorebootPayloadPkgIa32.dsc      |   1 +
>>>  CorebootPayloadPkg/CorebootPayloadPkgIa32X64.dsc   |   1 +
>>>  .../Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf  |   1 +
>>>  .../Bus/Pci/PciHostBridgeDxe/PciRootBridge.h       |  13 +-
>>>  .../Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c     | 272 ++++------------
>>>  MdeModulePkg/Include/Library/BmDmaLib.h            | 161 ++++++++++
>>>  MdeModulePkg/Library/DxeBmDmaLib/DxeBmDmaLib.c     | 351
>> +++++++++++++++++++++
>>>  MdeModulePkg/Library/DxeBmDmaLib/DxeBmDmaLib.inf   |  41 +++
>>>  MdeModulePkg/MdeModulePkg.dsc                      |   3 +
>>>  OvmfPkg/OvmfPkgIa32.dsc                            |   1 +
>>>  OvmfPkg/OvmfPkgIa32X64.dsc                         |   1 +
>>>  OvmfPkg/OvmfPkgX64.dsc                             |   1 +
>>>  14 files changed, 629 insertions(+), 220 deletions(-)  create mode
>>> 100644 MdeModulePkg/Include/Library/BmDmaLib.h
>>>  create mode 100644
>> MdeModulePkg/Library/DxeBmDmaLib/DxeBmDmaLib.c
>>>  create mode 100644
>> MdeModulePkg/Library/DxeBmDmaLib/DxeBmDmaLib.inf
>>>
>