Re: [PATCH v3 0/2] Provide UEFILib functions for protocol uninstallation.

[Ashish Singhal <ashishsingha@nvidia.com>]

Mike,

I have refactored my change and with this I am not seeing any size bloating in DEBUG or RELEASE images I am building for my platform. The new change has been submitted for consideration.

Thanks
Ashish

-----Original Message-----
From: Ashish Singhal 
Sent: Tuesday, January 8, 2019 8:02 PM
To: 'Kinney, Michael D' <michael.d.kinney@intel.com>; edk2-devel@lists.01.org; Gao, Liming <liming.gao@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com>
Subject: RE: [PATCH v3 0/2] Provide UEFILib functions for protocol uninstallation.

Hi Mike,

Thanks for the analysis. I will investigate at my end what could be causing this. Meanwhile, to fix the issue do you want to submit PATCH v2 1/4 and 2/4 patches? These 2 patches are just what you suggested.

Thanks
Ashish

-----Original Message-----
From: Kinney, Michael D <michael.d.kinney@intel.com>
Sent: Tuesday, January 8, 2019 7:58 PM
To: Ashish Singhal <ashishsingha@nvidia.com>; edk2-devel@lists.01.org; Gao, Liming <liming.gao@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
Subject: RE: [PATCH v3 0/2] Provide UEFILib functions for protocol uninstallation.

Ashish,

When I do full platform builds and individual driver builds, the result is always a little bigger with the
V3 version of the patch.

                           	 DEBUG  DEBUG+Patch  Delta  RELEASE	RELEASE+Patch  Delta
                           	 *****  ***********  *****  *******	*************  *****
Quark IA32 FVMAIN            2337360      2339408   2048  1181168        1182992   1824
Quark IA32 FVMAIN_COMPACT     814664       815272    608   516208         516904    696
DiskIoDxe.efi                  18752        18880    128     5824           5952    128
DiskIoDxe.ROM                  10240        10240      0     3419           3569    150
OVMF X64 DXEFV               4447368      4456968   9600  2956808        2966312   9504
OVMF X64 FVMAIN_COMPACT      1164264      1164784    520   912528         913048    520

I recommend the install APIs remain unchanged, and the uninstall APIs use the same logic as the install APIs.

Best regards,

Mike

> -----Original Message-----
> From: Ashish Singhal [mailto:ashishsingha@nvidia.com]
> Sent: Tuesday, January 8, 2019 9:24 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; 
> edk2-devel@lists.01.org; Gao, Liming <liming.gao@intel.com>; Fu, 
> Siyuan <siyuan.fu@intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com>
> Subject: RE: [PATCH v3 0/2] Provide UEFILib functions for protocol 
> uninstallation.
> 
> Thanks Mike. Please let me know if you have any more questions and/or 
> comments.
> 
> Thanks
> Ashish
> 
> -----Original Message-----
> From: Kinney, Michael D <michael.d.kinney@intel.com>
> Sent: Tuesday, January 8, 2019 9:26 AM
> To: Ashish Singhal <ashishsingha@nvidia.com>; edk2- 
> devel@lists.01.org; Gao, Liming <liming.gao@intel.com>; Fu, Siyuan 
> <siyuan.fu@intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com>; Kinney, 
> Michael D <michael.d.kinney@intel.com>
> Subject: RE: [PATCH v3 0/2] Provide UEFILib functions for protocol 
> uninstallation.
> 
> Ashish,
> 
> Good point.  I was looking at the code size of a single uncompressed 
> UEFI Driver (DiskIoDxe).  I suspect that the patch provides a more 
> consistent pattern across all drivers that use these UefiLib APIs, and 
> then provides better compression on an FV that contains many UEFI 
> Drivers.
> 
> I also suspect there may be a small size increase for the single 
> compressed UEFI Driver use case for PCI Option ROMs.
> 
> I will run a few more experiments this morning.
> 
> Thanks,
> 
> Mike
> 
> > -----Original Message-----
> > From: Ashish Singhal [mailto:ashishsingha@nvidia.com]
> > Sent: Monday, January 7, 2019 2:51 PM
> > To: Kinney, Michael D <michael.d.kinney@intel.com>; 
> > edk2-devel@lists.01.org; Gao, Liming
> <liming.gao@intel.com>; Fu,
> > Siyuan <siyuan.fu@intel.com>; Wu, Jiaxin
> <jiaxin.wu@intel.com>
> > Subject: RE: [PATCH v3 0/2] Provide UEFILib functions
> for protocol
> > uninstallation.
> >
> > Hi Mike,
> >
> > I build both DEBUG and RELEASE variant of the library
> and they both
> > built a few KB less in size compared to what is in
> tip right now. Can
> > you please help me with the optimization settings you
> have enabled so
> > that I can try the same at my end? Also, if you want,
> we can look at
> > the optimization part going forward and fix the issue
> first by pushing
> > in PATCH v2 1/4 and PATCH v2 2/4 which just adds
> uninstallation APIs
> > keeping the same code structure as for install.
> >
> > Thanks
> > Ashish
> >
> > -----Original Message-----
> > From: Kinney, Michael D <michael.d.kinney@intel.com>
> > Sent: Monday, January 7, 2019 3:23 PM
> > To: Ashish Singhal <ashishsingha@nvidia.com>; edk2- 
> > devel@lists.01.org; Gao, Liming
> <liming.gao@intel.com>; Fu, Siyuan
> > <siyuan.fu@intel.com>; Wu, Jiaxin
> <jiaxin.wu@intel.com>; Kinney,
> > Michael D <michael.d.kinney@intel.com>
> > Subject: RE: [PATCH v3 0/2] Provide UEFILib functions
> for protocol
> > uninstallation.
> >
> > Hi Ashish,
> >
> > My main concern with this patch is that the generated
> code for
> > optimized RELEASE builds is not as small.
> >
> > From a source maintenance perspective, the patch you
> have provided is
> > easier to maintain.  However, the implementation of
> the APIs that
> > install protocols was done to make sure the optimizer
> produces the
> > smallest number of instructions to install the
> protocols.
> >
> > I would prefer the APIs that install protocols remain
> unchanged, and
> > that only the new APIs to uninstall the protocols be
> added.  The same
> > approach could be taken in the implementation to
> produce the exact
> > right form of the uninstall action that is guaranteed
> to succeed if
> > the uninstall API matches the API that was used to
> install.
> >
> > Thanks,
> >
> > Mike
> >
> > > -----Original Message-----
> > > From: Ashish Singhal
> [mailto:ashishsingha@nvidia.com]
> > > Sent: Monday, January 7, 2019 6:02 AM
> > > To: edk2-devel@lists.01.org; Kinney, Michael D 
> > > <michael.d.kinney@intel.com>; Gao, Liming
> > <liming.gao@intel.com>; Fu,
> > > Siyuan <siyuan.fu@intel.com>; Wu, Jiaxin
> > <jiaxin.wu@intel.com>
> > > Subject: RE: [PATCH v3 0/2] Provide UEFILib
> functions
> > for protocol
> > > uninstallation.
> > >
> > > + Maintainers
> > >
> > > -----Original Message-----
> > > From: Ashish Singhal <ashishsingha@nvidia.com>
> > > Sent: Sunday, January 6, 2019 9:38 PM
> > > To: edk2-devel@lists.01.org
> > > Cc: Ashish Singhal <ashishsingha@nvidia.com>
> > > Subject: [PATCH v3 0/2] Provide UEFILib functions
> for
> > protocol
> > > uninstallation.
> > >
> > > An issue was seen in IScsiDxe in NetworkPkg where
> > driver cleanup after
> > > initialization failure was not done right. Bug 1428
> > was filed in this
> > > regard.
> > > As per discussions with Mike, it was also discussed
> > that having
> > > UEFILib provide protocol uninstallation abstraction
> > would help to
> > > avoid these issues in the future. Bug 1429 was
> found
> > to track this.
> > > The first 2 patches take care of this.
> > >
> > > Patch number 1 also simplifies the UEFILib protocol
> > installation and
> > > uninstallation abstraction by adding a helper
> > function doing
> > > operations instead of every public function.
> > >
> > > Ashish Singhal (2):
> > >   MdePkg/UefiLib: Abstract driver model protocol
> > uninstallation
> > >   NetworkPkg/IScsiDxe: Use UEFILib APIs to
> uninstall
> > protocols.
> > >
> > >  MdePkg/Include/Library/UefiLib.h         |  103
> +++
> > >  MdePkg/Library/UefiLib/UefiDriverModel.c | 1186
> > > ++++++++----------------------
> > >  NetworkPkg/IScsiDxe/IScsiDriver.c        |   31 +-
> > >  3 files changed, 435 insertions(+), 885
> deletions(-)
> > >
> > > --
> > > 2.7.4
> > >
> > > ---------------------------------------------------
> --
> > --
> > > ----------------------------
> > > This email message is for the sole use of the
> > intended
> > > recipient(s) and may contain
> > > confidential information.  Any unauthorized review,
> > use, disclosure or
> > > distribution is prohibited.  If you are not the
> > intended recipient,
> > > please contact the sender by reply email and
> destroy
> > all copies of the
> > > original message.
> > > ---------------------------------------------------
> --
> > --
> > > ----------------------------