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

["Gao, Liming" <liming.gao@intel.com>]

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