[PATCH staging][BaseToolsOpt 0/4] Enable multiple driver combination

[PATCH staging][BaseToolsOpt 0/4] Enable multiple driver combination

[Liming Gao]

Combine more drivers into the single one can reduce the image size and 
compile link time. This patch adds this support in BaseTools.

Liming Gao (4):
  BaseTools: Merge multiple drivers into one for size and link
    performance
  OvmfPkg: Update QemuVideo and VirtioGpuDxe to use NULL as
    DriverBindingHandle
  OvmfPkg: Combine QemuVideoDxe and VirtioGpuDxe to one driver
  Update Readme.MD to include multiple driver combination.

 BaseTools/Source/Python/AutoGen/GenC.py | 24 +++++++++++++++++++-----
 OvmfPkg/OvmfPkgIa32X64.dsc              |  6 ++++--
 OvmfPkg/OvmfPkgIa32X64.fdf              |  2 +-
 OvmfPkg/QemuVideoDxe/Driver.c           |  2 +-
 OvmfPkg/VirtioGpuDxe/DriverBinding.c    |  2 +-
 Readme.MD                               |  1 +
 6 files changed, 27 insertions(+), 10 deletions(-)

-- 
2.8.0.windows.1

[PATCH staging][BaseToolsOpt 1/4] BaseTools: Merge multiple drivers into one for size and link performance

[Liming Gao]

Update BaseTools to support the multiple driver combination. The merge style
reuses the library instance syntax in package.dsc file. The below example is
to combine VirtioGpu and QemuVideoDxe driver into one driver. VirtioGpu driver
entry point will be executed first, QemuVideoDxe entry point will run last.
[Components]
  OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf {
    <LibraryClasses>
      NULL|OvmfPkg/VirtioGpuDxe/VirtioGpu.inf
  }

Notes: When try combining some drivers, the compile failure may happen,
because the same function name are used in the different drivers. Those
drivers are required to be clean up first, then be combined.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Liming Gao <liming.gao@intel.com>
---
 BaseTools/Source/Python/AutoGen/GenC.py | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/BaseTools/Source/Python/AutoGen/GenC.py b/BaseTools/Source/Python/AutoGen/GenC.py
index 67aaef7..6579457 100644
--- a/BaseTools/Source/Python/AutoGen/GenC.py
+++ b/BaseTools/Source/Python/AutoGen/GenC.py
@@ -1359,10 +1359,17 @@ def CreateLibraryDestructorCode(Info, AutoGenC, AutoGenH):
 def CreateModuleEntryPointCode(Info, AutoGenC, AutoGenH):
     if Info.IsLibrary or Info.ModuleType in ['USER_DEFINED', 'SEC']:
         return
+    
+    ModuleEntryPointList = []
+    for Lib in Info.DependentLibraryList:
+        if len (Lib.ModuleEntryPointList) > 0:
+            ModuleEntryPointList = ModuleEntryPointList + Lib.ModuleEntryPointList
+    ModuleEntryPointList = ModuleEntryPointList + Info.Module.ModuleEntryPointList
+    
     #
     # Module Entry Points
     #
-    NumEntryPoints = len(Info.Module.ModuleEntryPointList)
+    NumEntryPoints = len(ModuleEntryPointList)
     if 'PI_SPECIFICATION_VERSION' in Info.Module.Specification:
         PiSpecVersion = Info.Module.Specification['PI_SPECIFICATION_VERSION']
     else:
@@ -1372,7 +1379,7 @@ def CreateModuleEntryPointCode(Info, AutoGenC, AutoGenH):
     else:
         UefiSpecVersion = '0x00000000'
     Dict = {
-        'Function'       :   Info.Module.ModuleEntryPointList,
+        'Function'       :   ModuleEntryPointList,
         'PiSpecVersion'  :   PiSpecVersion + 'U',
         'UefiSpecVersion':   UefiSpecVersion + 'U'
     }
@@ -1385,7 +1392,7 @@ def CreateModuleEntryPointCode(Info, AutoGenC, AutoGenH):
                   AUTOGEN_ERROR,
                   '%s must have exactly one entry point' % Info.ModuleType,
                   File=str(Info),
-                  ExtraData= ", ".join(Info.Module.ModuleEntryPointList)
+                  ExtraData= ", ".join(ModuleEntryPointList)
                   )
     if Info.ModuleType == 'PEI_CORE':
         AutoGenC.Append(gPeiCoreEntryPointString.Replace(Dict))
@@ -1430,11 +1437,18 @@ def CreateModuleEntryPointCode(Info, AutoGenC, AutoGenH):
 def CreateModuleUnloadImageCode(Info, AutoGenC, AutoGenH):
     if Info.IsLibrary or Info.ModuleType in ['USER_DEFINED', 'SEC']:
         return
+
+    ModuleUnloadImageList = []
+    for Lib in Info.DependentLibraryList:
+        if len (Lib.ModuleUnloadImageList) > 0:
+            ModuleUnloadImageList = ModuleUnloadImageList + Lib.ModuleUnloadImageList
+    ModuleUnloadImageList = ModuleUnloadImageList + Info.Module.ModuleUnloadImageList
+
     #
     # Unload Image Handlers
     #
-    NumUnloadImage = len(Info.Module.ModuleUnloadImageList)
-    Dict = {'Count':str(NumUnloadImage) + 'U', 'Function':Info.Module.ModuleUnloadImageList}
+    NumUnloadImage = len(ModuleUnloadImageList)
+    Dict = {'Count':str(NumUnloadImage) + 'U', 'Function':ModuleUnloadImageList}
     if NumUnloadImage < 2:
         AutoGenC.Append(gUefiUnloadImageString[NumUnloadImage].Replace(Dict))
     else:
-- 
2.8.0.windows.1

[PATCH staging][BaseToolsOpt 2/4] OvmfPkg: Update QemuVideo and VirtioGpuDxe to use NULL as DriverBindingHandle

[Liming Gao]

To combine two drivers into one, they can't share ImageHandle as
DriverBindingHandle. So, update their code to use NULL as DriverBindingHandle.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Liming Gao <liming.gao@intel.com>
---
 OvmfPkg/QemuVideoDxe/Driver.c        | 2 +-
 OvmfPkg/VirtioGpuDxe/DriverBinding.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c
index fc8025e..ce66c02 100644
--- a/OvmfPkg/QemuVideoDxe/Driver.c
+++ b/OvmfPkg/QemuVideoDxe/Driver.c
@@ -954,7 +954,7 @@ InitializeQemuVideo (
              ImageHandle,
              SystemTable,
              &gQemuVideoDriverBinding,
-             ImageHandle,
+             NULL,
              &gQemuVideoComponentName,
              &gQemuVideoComponentName2
              );
diff --git a/OvmfPkg/VirtioGpuDxe/DriverBinding.c b/OvmfPkg/VirtioGpuDxe/DriverBinding.c
index 33c1ad3..908bd1d 100644
--- a/OvmfPkg/VirtioGpuDxe/DriverBinding.c
+++ b/OvmfPkg/VirtioGpuDxe/DriverBinding.c
@@ -839,6 +839,6 @@ VirtioGpuEntryPoint (
   )
 {
   return EfiLibInstallDriverBindingComponentName2 (ImageHandle, SystemTable,
-           &mDriverBinding, ImageHandle, NULL /* ComponentName */,
+           &mDriverBinding, NULL, NULL /* ComponentName */,
            &mComponentName2);
 }
-- 
2.8.0.windows.1

[PATCH staging][BaseToolsOpt 3/4] OvmfPkg: Combine QemuVideoDxe and VirtioGpuDxe to one driver

[Liming Gao]

This is an example to show the driver combination in Platform.dsc.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Liming Gao <liming.gao@intel.com>
---
 OvmfPkg/OvmfPkgIa32X64.dsc | 6 ++++--
 OvmfPkg/OvmfPkgIa32X64.fdf | 2 +-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index e1e386c..9858d33 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -678,8 +678,10 @@
   MdeModulePkg/Universal/DisplayEngineDxe/DisplayEngineDxe.inf
   MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryTestDxe.inf
 
-  OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
-  OvmfPkg/VirtioGpuDxe/VirtioGpu.inf
+  OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf {
+    <LibraryClasses>
+      NULL|OvmfPkg/VirtioGpuDxe/VirtioGpu.inf
+  }
 
   #
   # ISA Support
diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf
index f6ed418..adad890 100644
--- a/OvmfPkg/OvmfPkgIa32X64.fdf
+++ b/OvmfPkg/OvmfPkgIa32X64.fdf
@@ -349,7 +349,7 @@ INF  RuleOverride=CSM OvmfPkg/Csm/Csm16/Csm16.inf
 !endif
 
 INF  OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
-INF  OvmfPkg/VirtioGpuDxe/VirtioGpu.inf
+#INF  OvmfPkg/VirtioGpuDxe/VirtioGpu.inf
 INF  OvmfPkg/PlatformDxe/Platform.inf
 
 !if $(SMM_REQUIRE) == TRUE
-- 
2.8.0.windows.1

[PATCH staging][BaseToolsOpt 4/4] Update Readme.MD to include multiple driver combination.

[Liming Gao]

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Liming Gao <liming.gao@intel.com>
---
 Readme.MD | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Readme.MD b/Readme.MD
index a436dd5..b5646be 100644
--- a/Readme.MD
+++ b/Readme.MD
@@ -15,6 +15,7 @@ identified to be optimized. POC code will be added in this branch for evaluation
    In Ubuntu 14.04 GCC5, OvmfPkgIa32X64 GenFds build time can be reduced from 6s to 4s.
 2) Support to merge multiple drivers into one. It should save the link time. But, it doesn't save much in the multiple build. 
    Besides, this feature can save the image size when the image is not compressed, such as PEI images.
+   POC code has been added. One example in OvmfPkgIa32X64.dsc is added to show how to combine more than drivers into single one.
 3) Reduce the extra copy actions in build process.
 4) Analyze cProfile data and enhance the parser logic. https://bugzilla.tianocore.org/show_bug.cgi?id=42
 
-- 
2.8.0.windows.1

Re: [PATCH staging][BaseToolsOpt 0/4] Enable multiple driver combination

[Laszlo Ersek]

Hi Liming,

(CC Jordan)

On 06/08/17 08:55, Liming Gao wrote:
> Combine more drivers into the single one can reduce the image size and 
> compile link time. This patch adds this support in BaseTools.
> 
> Liming Gao (4):
>   BaseTools: Merge multiple drivers into one for size and link
>     performance
>   OvmfPkg: Update QemuVideo and VirtioGpuDxe to use NULL as
>     DriverBindingHandle
>   OvmfPkg: Combine QemuVideoDxe and VirtioGpuDxe to one driver
>   Update Readme.MD to include multiple driver combination.
> 
>  BaseTools/Source/Python/AutoGen/GenC.py | 24 +++++++++++++++++++-----
>  OvmfPkg/OvmfPkgIa32X64.dsc              |  6 ++++--
>  OvmfPkg/OvmfPkgIa32X64.fdf              |  2 +-
>  OvmfPkg/QemuVideoDxe/Driver.c           |  2 +-
>  OvmfPkg/VirtioGpuDxe/DriverBinding.c    |  2 +-
>  Readme.MD                               |  1 +
>  6 files changed, 27 insertions(+), 10 deletions(-)
> 

I don't have a lot of hands-on practice with staging branches, but I
believe that ultimately such changes are meant to be merged into the
edk2 master branch. Is that right?

If that's the case, then I don't think we should do this in OvmfPkg (on
the master branch). Instead, I think example usage should be shown in:

- the commit message (it's already there, so that's great),

- in the DSC specification.

If you absolutely need an in-tree platform to use this BaseTools
feature, and you think OvmfPkg is better for this purpose than, say,
EmulatorPkg or Nt32Pkg, then:

(1) Please make this dependent on a new build flag (-D), which should
default to FALSE.

(2) Please make the same change to all three DSC files under OvmfPkg.

(3) Please introduce a new FeaturePCD which corresponds to the new build
flag, and controls the handle value in the entry points of the affected
drivers.

In particular I'm asking for (3) because the UEFI Driver Writer's Guide
(Version 1.01, 03/08/2012) says:

  6.1.4 Device drivers with one driver binding protocol

  [...] The driver entry point is responsible for installing the Driver
  Binding Protocol onto the driver’s image handle. [...]

  6.1.5 Device drivers with multiple driver binding protocols

  [...] The first instance of EFI_DRIVER_BINDING_PROTOCOL is installed
  onto the driver’s image handle, and the additional instances of the
  Driver Binding Protocol are installed onto newly created driver
  binding handles. [...]

So, in order to follow these recommendations,
- when the drivers are not combined, each driver should stick with its
non-NULL image handle,
- when the drivers are combined, one driver should stick with its image
handle, and the rest should use NULL (based on the feature PCD)


Regarding the BaseTools feature itself, I think it should be restricted
to UEFI_DRIVER modules (maybe it is already restricted, but then the
documentation should say it). I'm suggesting that becasue UEFI_DRIVERs
are supposed to have identical DEPEXes. With DXE_DRIVER modules for
example, their DEPEXes could be different, and I couldn't say how those
should be combined.

Thanks,
Laszlo

Re: [PATCH staging][BaseToolsOpt 0/4] Enable multiple driver combination

[Gao, Liming]

Laszlo:
  I really create one example to show the combined driver usage. I don't plan to push this change into OvmfPkg master. If this patch brings confuse to you, I will create one SamplePkg to include it. 
  And, I don't want to limit this feature into the specific driver type. I would like platform developer make the decision. If user combines some PEIM or some DXE drivers in its Platform.dsc, it can specify the combined driver in APRIOR list to make sure it be dispatched correctly.

Thanks
Liming
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Friday, June 9, 2017 5:03 AM
> To: Gao, Liming <liming.gao@intel.com>; edk2-devel@lists.01.org
> Cc: Justen, Jordan L <jordan.l.justen@intel.com>
> Subject: Re: [edk2] [PATCH staging][BaseToolsOpt 0/4] Enable multiple driver combination
> 
> Hi Liming,
> 
> (CC Jordan)
> 
> On 06/08/17 08:55, Liming Gao wrote:
> > Combine more drivers into the single one can reduce the image size and
> > compile link time. This patch adds this support in BaseTools.
> >
> > Liming Gao (4):
> >   BaseTools: Merge multiple drivers into one for size and link
> >     performance
> >   OvmfPkg: Update QemuVideo and VirtioGpuDxe to use NULL as
> >     DriverBindingHandle
> >   OvmfPkg: Combine QemuVideoDxe and VirtioGpuDxe to one driver
> >   Update Readme.MD to include multiple driver combination.
> >
> >  BaseTools/Source/Python/AutoGen/GenC.py | 24 +++++++++++++++++++-----
> >  OvmfPkg/OvmfPkgIa32X64.dsc              |  6 ++++--
> >  OvmfPkg/OvmfPkgIa32X64.fdf              |  2 +-
> >  OvmfPkg/QemuVideoDxe/Driver.c           |  2 +-
> >  OvmfPkg/VirtioGpuDxe/DriverBinding.c    |  2 +-
> >  Readme.MD                               |  1 +
> >  6 files changed, 27 insertions(+), 10 deletions(-)
> >
> 
> I don't have a lot of hands-on practice with staging branches, but I
> believe that ultimately such changes are meant to be merged into the
> edk2 master branch. Is that right?
> 
> If that's the case, then I don't think we should do this in OvmfPkg (on
> the master branch). Instead, I think example usage should be shown in:
> 
> - the commit message (it's already there, so that's great),
> 
> - in the DSC specification.
> 
> If you absolutely need an in-tree platform to use this BaseTools
> feature, and you think OvmfPkg is better for this purpose than, say,
> EmulatorPkg or Nt32Pkg, then:
> 
> (1) Please make this dependent on a new build flag (-D), which should
> default to FALSE.
> 
> (2) Please make the same change to all three DSC files under OvmfPkg.
> 
> (3) Please introduce a new FeaturePCD which corresponds to the new build
> flag, and controls the handle value in the entry points of the affected
> drivers.
> 
> In particular I'm asking for (3) because the UEFI Driver Writer's Guide
> (Version 1.01, 03/08/2012) says:
> 
>   6.1.4 Device drivers with one driver binding protocol
> 
>   [...] The driver entry point is responsible for installing the Driver
>   Binding Protocol onto the driver’s image handle. [...]
> 
>   6.1.5 Device drivers with multiple driver binding protocols
> 
>   [...] The first instance of EFI_DRIVER_BINDING_PROTOCOL is installed
>   onto the driver’s image handle, and the additional instances of the
>   Driver Binding Protocol are installed onto newly created driver
>   binding handles. [...]
> 
> So, in order to follow these recommendations,
> - when the drivers are not combined, each driver should stick with its
> non-NULL image handle,
> - when the drivers are combined, one driver should stick with its image
> handle, and the rest should use NULL (based on the feature PCD)
> 
> 
> Regarding the BaseTools feature itself, I think it should be restricted
> to UEFI_DRIVER modules (maybe it is already restricted, but then the
> documentation should say it). I'm suggesting that becasue UEFI_DRIVERs
> are supposed to have identical DEPEXes. With DXE_DRIVER modules for
> example, their DEPEXes could be different, and I couldn't say how those
> should be combined.
> 
> Thanks,
> Laszlo

Re: [PATCH staging][BaseToolsOpt 0/4] Enable multiple driver combination

[Laszlo Ersek]

(CC Mike)

On 06/10/17 15:49, Gao, Liming wrote:
> Laszlo:
> I really create one example to show the combined driver usage. I
> don't plan to push this change into OvmfPkg master.

Ah, I see.

> If this patch brings confuse to you, I will create one SamplePkg to
> include it.
I'd just like to understand the workflow intended for the BaseToolsOpt
branch. After all, you did modify OvmfPkg on that branch, to provide an
example. But, what is going to happen to this change later on?

I mean, patch #1 is the BaseTools feature itself, so you surely want to
bring that to edk2 master at some point, right? How can we tell later
that patch #1 should be merged into edk2 master but patch #2 and patch
#3 (the OvmfPkg example code) should not?

My understanding was that staging branches would be *merged* into edk2
master, with a git merge operation, pulling in all the changes from the
staging branch. If that's the case, I don't think we can realistically
separate out OvmfPkg (or similar) platform code at the time of merge.

If you are going to do a final rebase to edk2 master (instead of a
merge), when the BaseToolsOpt changes are ready for edk2 master, then
you can indeed drop the OvmfPkg patches at that point. But then the
example code will be lost (it will never go beyond the mailing list and
the BaseToolsOpt staging branch).

... Historically, in this message:
<http://mid.mail-archive.com/E92EE9817A31E24EB0585FDF735412F56485F2E7@ORSMSX113.amr.corp.intel.com>,
Mike seems to have suggested a final rebase / repost, for the HTTPS-TLS
feature. A rebase certainly makes it possible to drop the OvmfPkg
example code from the final version of this set, but then the example
code -- which *is* valuable -- will never be part of edk2 master. That's
not optimal IMO (unless you add the same example to the DSC spec).

So, I think SamplePkg is a good idea. You can provide a long-term
example in that package, even in edk2 master, without disturbing current
platforms.

(Please correct me if I'm incorrect about the staging branch workflow.
CC'ing Mike just to be sure.)

> And, I don't want to limit this feature into the specific driver
> type. I would like platform developer make the decision. If user
> combines some PEIM or some DXE drivers in its Platform.dsc, it can
> specify the combined driver in APRIOR list to make sure it be
> dispatched correctly.
I agree, but should we document what happens to the DEPEX sections that
were defined in the original (separate) INF files? Are they dropped? Are
they combined in some way (like, are they AND-ed together)?

It's fine if the platform developer makes the decision, but they need to
understand the DEPEX behavior to decide about it.

(My apologies if the DEPEX behavior is already documented for combined
drivers, I missed it then.)

Thanks!
Laszlo

> 
> Thanks
> Liming
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Friday, June 9, 2017 5:03 AM
>> To: Gao, Liming <liming.gao@intel.com>; edk2-devel@lists.01.org
>> Cc: Justen, Jordan L <jordan.l.justen@intel.com>
>> Subject: Re: [edk2] [PATCH staging][BaseToolsOpt 0/4] Enable multiple driver combination
>>
>> Hi Liming,
>>
>> (CC Jordan)
>>
>> On 06/08/17 08:55, Liming Gao wrote:
>>> Combine more drivers into the single one can reduce the image size and
>>> compile link time. This patch adds this support in BaseTools.
>>>
>>> Liming Gao (4):
>>>   BaseTools: Merge multiple drivers into one for size and link
>>>     performance
>>>   OvmfPkg: Update QemuVideo and VirtioGpuDxe to use NULL as
>>>     DriverBindingHandle
>>>   OvmfPkg: Combine QemuVideoDxe and VirtioGpuDxe to one driver
>>>   Update Readme.MD to include multiple driver combination.
>>>
>>>  BaseTools/Source/Python/AutoGen/GenC.py | 24 +++++++++++++++++++-----
>>>  OvmfPkg/OvmfPkgIa32X64.dsc              |  6 ++++--
>>>  OvmfPkg/OvmfPkgIa32X64.fdf              |  2 +-
>>>  OvmfPkg/QemuVideoDxe/Driver.c           |  2 +-
>>>  OvmfPkg/VirtioGpuDxe/DriverBinding.c    |  2 +-
>>>  Readme.MD                               |  1 +
>>>  6 files changed, 27 insertions(+), 10 deletions(-)
>>>
>>
>> I don't have a lot of hands-on practice with staging branches, but I
>> believe that ultimately such changes are meant to be merged into the
>> edk2 master branch. Is that right?
>>
>> If that's the case, then I don't think we should do this in OvmfPkg (on
>> the master branch). Instead, I think example usage should be shown in:
>>
>> - the commit message (it's already there, so that's great),
>>
>> - in the DSC specification.
>>
>> If you absolutely need an in-tree platform to use this BaseTools
>> feature, and you think OvmfPkg is better for this purpose than, say,
>> EmulatorPkg or Nt32Pkg, then:
>>
>> (1) Please make this dependent on a new build flag (-D), which should
>> default to FALSE.
>>
>> (2) Please make the same change to all three DSC files under OvmfPkg.
>>
>> (3) Please introduce a new FeaturePCD which corresponds to the new build
>> flag, and controls the handle value in the entry points of the affected
>> drivers.
>>
>> In particular I'm asking for (3) because the UEFI Driver Writer's Guide
>> (Version 1.01, 03/08/2012) says:
>>
>>   6.1.4 Device drivers with one driver binding protocol
>>
>>   [...] The driver entry point is responsible for installing the Driver
>>   Binding Protocol onto the driver’s image handle. [...]
>>
>>   6.1.5 Device drivers with multiple driver binding protocols
>>
>>   [...] The first instance of EFI_DRIVER_BINDING_PROTOCOL is installed
>>   onto the driver’s image handle, and the additional instances of the
>>   Driver Binding Protocol are installed onto newly created driver
>>   binding handles. [...]
>>
>> So, in order to follow these recommendations,
>> - when the drivers are not combined, each driver should stick with its
>> non-NULL image handle,
>> - when the drivers are combined, one driver should stick with its image
>> handle, and the rest should use NULL (based on the feature PCD)
>>
>>
>> Regarding the BaseTools feature itself, I think it should be restricted
>> to UEFI_DRIVER modules (maybe it is already restricted, but then the
>> documentation should say it). I'm suggesting that becasue UEFI_DRIVERs
>> are supposed to have identical DEPEXes. With DXE_DRIVER modules for
>> example, their DEPEXes could be different, and I couldn't say how those
>> should be combined.
>>
>> Thanks,
>> Laszlo

Re: [PATCH staging][BaseToolsOpt 0/4] Enable multiple driver combination

[Gao, Liming]

Laszlo:
  Thanks for your comments. I will create SamplePkg to include this usage. And, I will document the combined behavior on DEPEX in branch Readme.MD. The DEPEX will be combined together with AND when the driver will be combined. 

Thanks
Liming
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Monday, June 12, 2017 10:17 PM
> To: Gao, Liming <liming.gao@intel.com>; edk2-devel@lists.01.org
> Cc: Justen, Jordan L <jordan.l.justen@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: Re: [edk2] [PATCH staging][BaseToolsOpt 0/4] Enable multiple driver combination
> 
> (CC Mike)
> 
> On 06/10/17 15:49, Gao, Liming wrote:
> > Laszlo:
> > I really create one example to show the combined driver usage. I
> > don't plan to push this change into OvmfPkg master.
> 
> Ah, I see.
> 
> > If this patch brings confuse to you, I will create one SamplePkg to
> > include it.
> I'd just like to understand the workflow intended for the BaseToolsOpt
> branch. After all, you did modify OvmfPkg on that branch, to provide an
> example. But, what is going to happen to this change later on?
> 
> I mean, patch #1 is the BaseTools feature itself, so you surely want to
> bring that to edk2 master at some point, right? How can we tell later
> that patch #1 should be merged into edk2 master but patch #2 and patch
> #3 (the OvmfPkg example code) should not?
> 
> My understanding was that staging branches would be *merged* into edk2
> master, with a git merge operation, pulling in all the changes from the
> staging branch. If that's the case, I don't think we can realistically
> separate out OvmfPkg (or similar) platform code at the time of merge.
> 
> If you are going to do a final rebase to edk2 master (instead of a
> merge), when the BaseToolsOpt changes are ready for edk2 master, then
> you can indeed drop the OvmfPkg patches at that point. But then the
> example code will be lost (it will never go beyond the mailing list and
> the BaseToolsOpt staging branch).
> 
> ... Historically, in this message:
> <http://mid.mail-archive.com/E92EE9817A31E24EB0585FDF735412F56485F2E7@ORSMSX113.amr.corp.intel.com>,
> Mike seems to have suggested a final rebase / repost, for the HTTPS-TLS
> feature. A rebase certainly makes it possible to drop the OvmfPkg
> example code from the final version of this set, but then the example
> code -- which *is* valuable -- will never be part of edk2 master. That's
> not optimal IMO (unless you add the same example to the DSC spec).
> 
> So, I think SamplePkg is a good idea. You can provide a long-term
> example in that package, even in edk2 master, without disturbing current
> platforms.
> 
> (Please correct me if I'm incorrect about the staging branch workflow.
> CC'ing Mike just to be sure.)
> 
> > And, I don't want to limit this feature into the specific driver
> > type. I would like platform developer make the decision. If user
> > combines some PEIM or some DXE drivers in its Platform.dsc, it can
> > specify the combined driver in APRIOR list to make sure it be
> > dispatched correctly.
> I agree, but should we document what happens to the DEPEX sections that
> were defined in the original (separate) INF files? Are they dropped? Are
> they combined in some way (like, are they AND-ed together)?
> 
> It's fine if the platform developer makes the decision, but they need to
> understand the DEPEX behavior to decide about it.
> 
> (My apologies if the DEPEX behavior is already documented for combined
> drivers, I missed it then.)
> 
> Thanks!
> Laszlo
> 
> >
> > Thanks
> > Liming
> >> -----Original Message-----
> >> From: Laszlo Ersek [mailto:lersek@redhat.com]
> >> Sent: Friday, June 9, 2017 5:03 AM
> >> To: Gao, Liming <liming.gao@intel.com>; edk2-devel@lists.01.org
> >> Cc: Justen, Jordan L <jordan.l.justen@intel.com>
> >> Subject: Re: [edk2] [PATCH staging][BaseToolsOpt 0/4] Enable multiple driver combination
> >>
> >> Hi Liming,
> >>
> >> (CC Jordan)
> >>
> >> On 06/08/17 08:55, Liming Gao wrote:
> >>> Combine more drivers into the single one can reduce the image size and
> >>> compile link time. This patch adds this support in BaseTools.
> >>>
> >>> Liming Gao (4):
> >>>   BaseTools: Merge multiple drivers into one for size and link
> >>>     performance
> >>>   OvmfPkg: Update QemuVideo and VirtioGpuDxe to use NULL as
> >>>     DriverBindingHandle
> >>>   OvmfPkg: Combine QemuVideoDxe and VirtioGpuDxe to one driver
> >>>   Update Readme.MD to include multiple driver combination.
> >>>
> >>>  BaseTools/Source/Python/AutoGen/GenC.py | 24 +++++++++++++++++++-----
> >>>  OvmfPkg/OvmfPkgIa32X64.dsc              |  6 ++++--
> >>>  OvmfPkg/OvmfPkgIa32X64.fdf              |  2 +-
> >>>  OvmfPkg/QemuVideoDxe/Driver.c           |  2 +-
> >>>  OvmfPkg/VirtioGpuDxe/DriverBinding.c    |  2 +-
> >>>  Readme.MD                               |  1 +
> >>>  6 files changed, 27 insertions(+), 10 deletions(-)
> >>>
> >>
> >> I don't have a lot of hands-on practice with staging branches, but I
> >> believe that ultimately such changes are meant to be merged into the
> >> edk2 master branch. Is that right?
> >>
> >> If that's the case, then I don't think we should do this in OvmfPkg (on
> >> the master branch). Instead, I think example usage should be shown in:
> >>
> >> - the commit message (it's already there, so that's great),
> >>
> >> - in the DSC specification.
> >>
> >> If you absolutely need an in-tree platform to use this BaseTools
> >> feature, and you think OvmfPkg is better for this purpose than, say,
> >> EmulatorPkg or Nt32Pkg, then:
> >>
> >> (1) Please make this dependent on a new build flag (-D), which should
> >> default to FALSE.
> >>
> >> (2) Please make the same change to all three DSC files under OvmfPkg.
> >>
> >> (3) Please introduce a new FeaturePCD which corresponds to the new build
> >> flag, and controls the handle value in the entry points of the affected
> >> drivers.
> >>
> >> In particular I'm asking for (3) because the UEFI Driver Writer's Guide
> >> (Version 1.01, 03/08/2012) says:
> >>
> >>   6.1.4 Device drivers with one driver binding protocol
> >>
> >>   [...] The driver entry point is responsible for installing the Driver
> >>   Binding Protocol onto the driver’s image handle. [...]
> >>
> >>   6.1.5 Device drivers with multiple driver binding protocols
> >>
> >>   [...] The first instance of EFI_DRIVER_BINDING_PROTOCOL is installed
> >>   onto the driver’s image handle, and the additional instances of the
> >>   Driver Binding Protocol are installed onto newly created driver
> >>   binding handles. [...]
> >>
> >> So, in order to follow these recommendations,
> >> - when the drivers are not combined, each driver should stick with its
> >> non-NULL image handle,
> >> - when the drivers are combined, one driver should stick with its image
> >> handle, and the rest should use NULL (based on the feature PCD)
> >>
> >>
> >> Regarding the BaseTools feature itself, I think it should be restricted
> >> to UEFI_DRIVER modules (maybe it is already restricted, but then the
> >> documentation should say it). I'm suggesting that becasue UEFI_DRIVERs
> >> are supposed to have identical DEPEXes. With DXE_DRIVER modules for
> >> example, their DEPEXes could be different, and I couldn't say how those
> >> should be combined.
> >>
> >> Thanks,
> >> Laszlo