* [PATCH] ArmVirtPkg/ArmVirt.dsc.inc: define TcpIoLib resolution unconditionally
@ 2018-12-14 11:22 Ard Biesheuvel
2018-12-14 13:56 ` Laszlo Ersek
0 siblings, 1 reply; 6+ messages in thread
From: Ard Biesheuvel @ 2018-12-14 11:22 UTC (permalink / raw)
To: edk2-devel; +Cc: lersek, julien.grall, siyuan.fu, Ard Biesheuvel
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. 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@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
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] ArmVirtPkg/ArmVirt.dsc.inc: define TcpIoLib resolution unconditionally
2018-12-14 11:22 [PATCH] ArmVirtPkg/ArmVirt.dsc.inc: define TcpIoLib resolution unconditionally Ard Biesheuvel
@ 2018-12-14 13:56 ` Laszlo Ersek
2018-12-14 14:03 ` Ard Biesheuvel
2018-12-17 1:50 ` Fu, Siyuan
0 siblings, 2 replies; 6+ messages in thread
From: Laszlo Ersek @ 2018-12-14 13:56 UTC (permalink / raw)
To: Ard Biesheuvel, siyuan.fu; +Cc: edk2-devel, julien.grall
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:
http://mid.mail-archive.com/cdd81f4c-1bdc-8bae-63a9-58eb4eb2afbd@redhat.com
(b) What's more, my comments for the v3 version were summarily ignored
as well. See bullet (2) in:
http://mid.mail-archive.com/91d253ae-9d0c-e28b-1bda-1be98cee4340@redhat.com
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@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@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.
Laszlo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ArmVirtPkg/ArmVirt.dsc.inc: define TcpIoLib resolution unconditionally
2018-12-14 13:56 ` Laszlo Ersek
@ 2018-12-14 14:03 ` Ard Biesheuvel
2018-12-17 1:50 ` Fu, Siyuan
1 sibling, 0 replies; 6+ messages in thread
From: Ard Biesheuvel @ 2018-12-14 14:03 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: Siyuan Fu, edk2-devel@lists.01.org, Julien Grall
On Fri, 14 Dec 2018 at 14:56, Laszlo Ersek <lersek@redhat.com> wrote:
>
> 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:
>
> http://mid.mail-archive.com/cdd81f4c-1bdc-8bae-63a9-58eb4eb2afbd@redhat.com
>
>
> (b) What's more, my comments for the v3 version were summarily ignored
> as well. See bullet (2) in:
>
> http://mid.mail-archive.com/91d253ae-9d0c-e28b-1bda-1be98cee4340@redhat.com
>
> 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@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@redhat.com>
>
Thanks. Apologies for not looking more carefully whether the feedback
had in fact been incorporated before giving my R-b
Pushed as 9a67ba261fe9..a9ff32909b47
> 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.
>
I assumed [incorrectly, I suppose] that people actually build test
platforms that they modify, but apparently not :-)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ArmVirtPkg/ArmVirt.dsc.inc: define TcpIoLib resolution unconditionally
2018-12-14 13:56 ` Laszlo Ersek
2018-12-14 14:03 ` Ard Biesheuvel
@ 2018-12-17 1:50 ` Fu, Siyuan
2018-12-17 3:06 ` Gao, Liming
1 sibling, 1 reply; 6+ messages in thread
From: Fu, Siyuan @ 2018-12-17 1:50 UTC (permalink / raw)
To: Laszlo Ersek, Ard Biesheuvel
Cc: edk2-devel@lists.01.org, julien.grall@linaro.org
Hi, Laszlo
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Friday, December 14, 2018 9:56 PM
> To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Fu, Siyuan
> <siyuan.fu@intel.com>
> Cc: edk2-devel@lists.01.org; julien.grall@linaro.org
> Subject: Re: [PATCH] ArmVirtPkg/ArmVirt.dsc.inc: define TcpIoLib resolution
> unconditionally
>
> 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:
>
> http://mid.mail-archive.com/cdd81f4c-1bdc-8bae-63a9-58eb4eb2afbd@redhat.com
>
>
> (b) What's more, my comments for the v3 version were summarily ignored
> as well. See bullet (2) in:
>
> http://mid.mail-archive.com/91d253ae-9d0c-e28b-1bda-1be98cee4340@redhat.com
>
> 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@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@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.
>
> Laszlo
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.
https://lists.01.org/pipermail/edk2-devel/2018-December/034070.html
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
in future.
Best Regards,
Siyuan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ArmVirtPkg/ArmVirt.dsc.inc: define TcpIoLib resolution unconditionally
2018-12-17 1:50 ` Fu, Siyuan
@ 2018-12-17 3:06 ` Gao, Liming
2018-12-17 3:22 ` Fu, Siyuan
0 siblings, 1 reply; 6+ messages in thread
From: Gao, Liming @ 2018-12-17 3:06 UTC (permalink / raw)
To: Fu, Siyuan, Laszlo Ersek, Ard Biesheuvel; +Cc: edk2-devel@lists.01.org
Siyuan:
I know the windows GCC pre built binary can be downloaded from https://sourceforge.net/projects/edk2developertoolsforwindows/files/Tool%20Chain%20Binaries/. Although pre-built binary GCC is three years ago, they can still work. For example, after you download gcc492 arm, then you can set GCC49_ARM_PREFIX and GCC49_DLL to point ARM GCC path, then build ARM platform with -a ARM arch and -t GCC49 tool chain. Below is my setting:
set GCC49_ARM_PREFIX=D:\Tools\GCC\gcc492-arm\bin\
set GCC49_DLL=D:\Tools\GCC\gcc492-arm\dll\
Thanks
Liming
>-----Original Message-----
>From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Fu,
>Siyuan
>Sent: Monday, December 17, 2018 9:51 AM
>To: Laszlo Ersek <lersek@redhat.com>; Ard Biesheuvel
><ard.biesheuvel@linaro.org>
>Cc: edk2-devel@lists.01.org
>Subject: Re: [edk2] [PATCH] ArmVirtPkg/ArmVirt.dsc.inc: define TcpIoLib
>resolution unconditionally
>
>Hi, Laszlo
>
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Friday, December 14, 2018 9:56 PM
>> To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Fu, Siyuan
>> <siyuan.fu@intel.com>
>> Cc: edk2-devel@lists.01.org; julien.grall@linaro.org
>> Subject: Re: [PATCH] ArmVirtPkg/ArmVirt.dsc.inc: define TcpIoLib resolution
>> unconditionally
>>
>> 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:
>>
>> http://mid.mail-archive.com/cdd81f4c-1bdc-8bae-63a9-
>58eb4eb2afbd@redhat.com
>>
>>
>> (b) What's more, my comments for the v3 version were summarily ignored
>> as well. See bullet (2) in:
>>
>> http://mid.mail-archive.com/91d253ae-9d0c-e28b-1bda-
>1be98cee4340@redhat.com
>>
>> 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@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@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.
>>
>> Laszlo
>
>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.
>https://lists.01.org/pipermail/edk2-devel/2018-December/034070.html
>
>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
>in future.
>
>Best Regards,
>Siyuan
>
>_______________________________________________
>edk2-devel mailing list
>edk2-devel@lists.01.org
>https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ArmVirtPkg/ArmVirt.dsc.inc: define TcpIoLib resolution unconditionally
2018-12-17 3:06 ` Gao, Liming
@ 2018-12-17 3:22 ` Fu, Siyuan
0 siblings, 0 replies; 6+ messages in thread
From: Fu, Siyuan @ 2018-12-17 3:22 UTC (permalink / raw)
To: Gao, Liming, Laszlo Ersek, Ard Biesheuvel; +Cc: edk2-devel@lists.01.org
Thanks Liming, I will try it.
BestRegards
Fu Siyuan
> -----Original Message-----
> From: Gao, Liming
> Sent: Monday, December 17, 2018 11:07 AM
> To: Fu, Siyuan <siyuan.fu@intel.com>; Laszlo Ersek <lersek@redhat.com>; Ard
> Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: edk2-devel@lists.01.org
> Subject: RE: [PATCH] ArmVirtPkg/ArmVirt.dsc.inc: define TcpIoLib resolution
> unconditionally
>
> Siyuan:
> I know the windows GCC pre built binary can be downloaded from
> https://sourceforge.net/projects/edk2developertoolsforwindows/files/Tool%20Cha
> in%20Binaries/. Although pre-built binary GCC is three years ago, they can
> still work. For example, after you download gcc492 arm, then you can set
> GCC49_ARM_PREFIX and GCC49_DLL to point ARM GCC path, then build ARM platform
> with -a ARM arch and -t GCC49 tool chain. Below is my setting:
> set GCC49_ARM_PREFIX=D:\Tools\GCC\gcc492-arm\bin\
> set GCC49_DLL=D:\Tools\GCC\gcc492-arm\dll\
>
> Thanks
> Liming
> >-----Original Message-----
> >From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Fu,
> >Siyuan
> >Sent: Monday, December 17, 2018 9:51 AM
> >To: Laszlo Ersek <lersek@redhat.com>; Ard Biesheuvel
> ><ard.biesheuvel@linaro.org>
> >Cc: edk2-devel@lists.01.org
> >Subject: Re: [edk2] [PATCH] ArmVirtPkg/ArmVirt.dsc.inc: define TcpIoLib
> >resolution unconditionally
> >
> >Hi, Laszlo
> >
> >> -----Original Message-----
> >> From: Laszlo Ersek [mailto:lersek@redhat.com]
> >> Sent: Friday, December 14, 2018 9:56 PM
> >> To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Fu, Siyuan
> >> <siyuan.fu@intel.com>
> >> Cc: edk2-devel@lists.01.org; julien.grall@linaro.org
> >> Subject: Re: [PATCH] ArmVirtPkg/ArmVirt.dsc.inc: define TcpIoLib resolution
> >> unconditionally
> >>
> >> 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:
> >>
> >> http://mid.mail-archive.com/cdd81f4c-1bdc-8bae-63a9-
> >58eb4eb2afbd@redhat.com
> >>
> >>
> >> (b) What's more, my comments for the v3 version were summarily ignored
> >> as well. See bullet (2) in:
> >>
> >> http://mid.mail-archive.com/91d253ae-9d0c-e28b-1bda-
> >1be98cee4340@redhat.com
> >>
> >> 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@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@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.
> >>
> >> Laszlo
> >
> >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.
> >https://lists.01.org/pipermail/edk2-devel/2018-December/034070.html
> >
> >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
> >in future.
> >
> >Best Regards,
> >Siyuan
> >
> >_______________________________________________
> >edk2-devel mailing list
> >edk2-devel@lists.01.org
> >https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-12-17 3:22 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-12-14 11:22 [PATCH] ArmVirtPkg/ArmVirt.dsc.inc: define TcpIoLib resolution unconditionally Ard Biesheuvel
2018-12-14 13:56 ` Laszlo Ersek
2018-12-14 14:03 ` Ard Biesheuvel
2018-12-17 1:50 ` Fu, Siyuan
2018-12-17 3:06 ` Gao, Liming
2018-12-17 3:22 ` Fu, Siyuan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox