[edk2] [PATCH] ArmVirtPkg/ArmVirt.dsc.inc: define TcpIoLib resolution unconditionally
siyuan.fu at intel.com
Sun Dec 16 17:50:38 PST 2018
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek at redhat.com]
> Sent: Friday, December 14, 2018 9:56 PM
> To: Ard Biesheuvel <ard.biesheuvel at linaro.org>; Fu, Siyuan
> <siyuan.fu at intel.com>
> Cc: edk2-devel at lists.01.org; julien.grall at linaro.org
> Subject: Re: [PATCH] ArmVirtPkg/ArmVirt.dsc.inc: define TcpIoLib resolution
> On 12/14/18 12:22, Ard Biesheuvel wrote:
> > Commit 9a67ba261fe9 ("ArmVirtPkg: Replace obsoleted network drivers
> > from platform DSC/FDF") failed to take into account that the now
> > unconditionally included IScsiDxe.inf from NetworkPkg requires a
> > resolution for TcpIoLib.
> I don't understand why this happened.
> (a) I warned *precisely* about this issue, when I reviewed the v2
> version of said commit. See bullet (5) in the following message:
> (b) What's more, my comments for the v3 version were summarily ignored
> as well. See bullet (2) in:
> And now, the BaseCryptLib, OpensslLib and IntrinsicLib resolutions for
> [LibraryClasses.common.UEFI_DRIVER] have been added to
> "ArmVirtPkg/ArmVirtQemuKernel.dsc", despite their being redundant and my
> having pointed out that fact. Worse, "ArmVirtQemuKernel.dsc" uses
> "OpensslLib.inf", while "OpensslLibCrypto.inf" from "ArmVirt.dsc.inc" is
> perfectly sufficient.
> Commit 9a67ba261fe9 does not carry my R-b, and that's not a random fact.
> The v3 patch was *not* ready for being pushed, to my eyes. And I was
> pretty explicit about that.
> > Since specifying such a resolution is harmless
> > for platforms that have no networking enabled, let's just fix things
> > by dropping the conditionals around it.
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel at linaro.org>
> > ---
> > ArmVirtPkg/ArmVirt.dsc.inc | 2 --
> > 1 file changed, 2 deletions(-)
> > diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
> > index c3549c84d4c6..89c2db074711 100644
> > --- a/ArmVirtPkg/ArmVirt.dsc.inc
> > +++ b/ArmVirtPkg/ArmVirt.dsc.inc
> > @@ -80,9 +80,7 @@
> > DpcLib|MdeModulePkg/Library/DxeDpcLib/DxeDpcLib.inf
> > UdpIoLib|MdeModulePkg/Library/DxeUdpIoLib/DxeUdpIoLib.inf
> > IpIoLib|MdeModulePkg/Library/DxeIpIoLib/DxeIpIoLib.inf
> > -!if $(NETWORK_IP6_ENABLE) == TRUE
> > TcpIoLib|MdeModulePkg/Library/DxeTcpIoLib/DxeTcpIoLib.inf
> > -!endif
> > !if $(HTTP_BOOT_ENABLE) == TRUE
> > HttpLib|MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.inf
> > !endif
> I'm *very strongly* tempted to simply revert 9a67ba261fe9, for blatantly
> ignoring my explicit requests for updates. However, that would only
> result in my having to review more (possibly incomplete) iterations of
> the patch.
> At least, this incremental fix is in line with my request in (a) -- "we
> should make the current TcpIoLib class resolution unconditional". Please
> go ahead and push it.
> Reviewed-by: Laszlo Ersek <lersek at redhat.com>
> I should really file a TianoCore BZ about the wrong / redundant
> OpensslLib resolution in ArmVirtQemuKernel.dsc too. It's difficult for
> me to find te motivation for that right now, seeing the disregard for my
> earlier reviews.
My apologies for missed your review email of the v3 patch and pushed the
changes. The original patch set was made one month ago and I didn't carefully
checked all the review feedback emails when I started to work on it again
in the last week.
I have created a new patch upon this fix to remove the redundant libraries
and adjust driver order, please check this patch mail.
And I'm also sorry that this new patch are also not tested for build, I tried
to search the wiki patch for setting up the ARM build toolchain on my windows
OS but failed to make it. I will appreciate if you can help to provide a guide
or any link for ARM package build on windows machine, so I could test my patch
More information about the edk2-devel