From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.nue.novell.com (smtp.nue.novell.com [195.135.221.5]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 6222281E10 for ; Wed, 18 Jan 2017 01:22:20 -0800 (PST) Received: from nwb-ext-pat.microfocus.com ([10.120.13.103]) by smtp.nue.novell.com with ESMTP (TLS encrypted); Wed, 18 Jan 2017 10:22:18 +0100 Received: from GaryWorkstation (nwb-a10-snat.microfocus.com [10.120.13.201]) by nwb-ext-pat.microfocus.com with ESMTP (TLS encrypted); Wed, 18 Jan 2017 09:21:48 +0000 Date: Wed, 18 Jan 2017 17:21:40 +0800 From: Gary Lin To: Laszlo Ersek Cc: "Wu, Jiaxin" , "edk2-devel@ml01.01.org" , "Justen, Jordan L" , "Long, Qin" Message-ID: <20170118092140.2kjvl5w5327e4pst@GaryWorkstation> References: <20170117045232.4765-1-glin@suse.com> <20170117045232.4765-3-glin@suse.com> <895558F6EA4E3B41AC93A00D163B72741629483A@SHSMSX103.ccr.corp.intel.com> <6e2dc3db-8657-cf9d-39d9-e604d7b2f92c@redhat.com> MIME-Version: 1.0 In-Reply-To: <6e2dc3db-8657-cf9d-39d9-e604d7b2f92c@redhat.com> User-Agent: Mutt/1.6.2 (2016-07-01) Subject: Re: [PATCH 2/3] OvmfPkg: correct the set of modules included for the IPv6 stack X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 18 Jan 2017 09:22:21 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Wed, Jan 18, 2017 at 09:17:28AM +0100, Laszlo Ersek wrote: > On 01/18/17 01:47, Wu, Jiaxin wrote: > >> Subject: Re: [edk2] [PATCH 2/3] OvmfPkg: correct the set of modules included > >> for the IPv6 stack > >> > >> On 01/17/17 05:52, Gary Lin wrote: > >>> Always use IScsiDxe from NetworkPkg when IPv6 is enabled since it provides > >>> the complete ISCSI support. > >>> > >>> NOTE: This makes OpenSSL a hard requirement when NETWORK_IP6_ENABLE > >> is > >>> true. > >>> > >>> (Based on Jiaxin's suggestion) > >>> > >>> Cc: Laszlo Ersek > >>> Cc: Justen Jordan L > >>> Cc: Wu Jiaxin > >>> Cc: Long Qin > >>> Contributed-under: TianoCore Contribution Agreement 1.0 > >>> Signed-off-by: Gary Lin > >>> --- > >>> OvmfPkg/OvmfPkgIa32.dsc | 11 ++++------- > >>> OvmfPkg/OvmfPkgIa32.fdf | 4 ---- > >>> OvmfPkg/OvmfPkgIa32X64.dsc | 11 ++++------- > >>> OvmfPkg/OvmfPkgIa32X64.fdf | 4 ---- > >>> OvmfPkg/OvmfPkgX64.dsc | 11 ++++------- > >>> OvmfPkg/OvmfPkgX64.fdf | 4 ---- > >>> 6 files changed, 12 insertions(+), 33 deletions(-) > >>> > >>> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc > >>> index 9aa66eb951..77287920e2 100644 > >>> --- a/OvmfPkg/OvmfPkgIa32.dsc > >>> +++ b/OvmfPkg/OvmfPkgIa32.dsc > >>> @@ -148,15 +148,16 @@ [LibraryClasses] > >>> > >> PlatformSecureLib|OvmfPkg/Library/PlatformSecureLib/PlatformSecureLib.inf > >>> > >> TpmMeasurementLib|SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmM > >> easurementLib.inf > >>> AuthVariableLib|SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf > >>> -!if $(NETWORK_IP6_ENABLE) == TRUE > >>> - TcpIoLib|MdeModulePkg/Library/DxeTcpIoLib/DxeTcpIoLib.inf > >>> -!endif > >>> !else > >>> > >> TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/Tpm > >> MeasurementLibNull.inf > >>> > >> AuthVariableLib|MdeModulePkg/Library/AuthVariableLibNull/AuthVariableLib > >> Null.inf > >>> !endif > >>> VarCheckLib|MdeModulePkg/Library/VarCheckLib/VarCheckLib.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 > >>> @@ -697,12 +698,8 @@ [Components] > >>> NetworkPkg/Dhcp6Dxe/Dhcp6Dxe.inf > >>> NetworkPkg/Mtftp6Dxe/Mtftp6Dxe.inf > >>> NetworkPkg/UefiPxeBcDxe/UefiPxeBcDxe.inf > >>> -!if $(SECURE_BOOT_ENABLE) == TRUE > >>> NetworkPkg/IScsiDxe/IScsiDxe.inf > >>> !else > >>> - MdeModulePkg/Universal/Network/IScsiDxe/IScsiDxe.inf > >>> -!endif > >>> -!else > >>> MdeModulePkg/Universal/Network/Tcp4Dxe/Tcp4Dxe.inf > >>> MdeModulePkg/Universal/Network/UefiPxeBcDxe/UefiPxeBcDxe.inf > >>> MdeModulePkg/Universal/Network/IScsiDxe/IScsiDxe.inf > >>> diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf > >>> index 34d57a6079..069e21b7d0 100644 > >>> --- a/OvmfPkg/OvmfPkgIa32.fdf > >>> +++ b/OvmfPkg/OvmfPkgIa32.fdf > >>> @@ -314,12 +314,8 @@ [FV.DXEFV] > >>> INF NetworkPkg/Dhcp6Dxe/Dhcp6Dxe.inf > >>> INF NetworkPkg/Mtftp6Dxe/Mtftp6Dxe.inf > >>> INF NetworkPkg/UefiPxeBcDxe/UefiPxeBcDxe.inf > >>> -!if $(SECURE_BOOT_ENABLE) == TRUE > >>> INF NetworkPkg/IScsiDxe/IScsiDxe.inf > >>> !else > >>> - INF MdeModulePkg/Universal/Network/IScsiDxe/IScsiDxe.inf > >>> -!endif > >>> -!else > >>> INF MdeModulePkg/Universal/Network/Tcp4Dxe/Tcp4Dxe.inf > >>> INF MdeModulePkg/Universal/Network/UefiPxeBcDxe/UefiPxeBcDxe.inf > >>> INF MdeModulePkg/Universal/Network/IScsiDxe/IScsiDxe.inf > >>> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc > >>> index 9537e92077..64a7c16d2f 100644 > >>> --- a/OvmfPkg/OvmfPkgIa32X64.dsc > >>> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc > >>> @@ -153,15 +153,16 @@ [LibraryClasses] > >>> > >> PlatformSecureLib|OvmfPkg/Library/PlatformSecureLib/PlatformSecureLib.inf > >>> > >> TpmMeasurementLib|SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmM > >> easurementLib.inf > >>> AuthVariableLib|SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf > >>> -!if $(NETWORK_IP6_ENABLE) == TRUE > >>> - TcpIoLib|MdeModulePkg/Library/DxeTcpIoLib/DxeTcpIoLib.inf > >>> -!endif > >>> !else > >>> > >> TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/Tpm > >> MeasurementLibNull.inf > >>> > >> AuthVariableLib|MdeModulePkg/Library/AuthVariableLibNull/AuthVariableLib > >> Null.inf > >>> !endif > >>> VarCheckLib|MdeModulePkg/Library/VarCheckLib/VarCheckLib.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 > >>> @@ -706,12 +707,8 @@ [Components.X64] > >>> NetworkPkg/Dhcp6Dxe/Dhcp6Dxe.inf > >>> NetworkPkg/Mtftp6Dxe/Mtftp6Dxe.inf > >>> NetworkPkg/UefiPxeBcDxe/UefiPxeBcDxe.inf > >>> -!if $(SECURE_BOOT_ENABLE) == TRUE > >>> NetworkPkg/IScsiDxe/IScsiDxe.inf > >>> !else > >>> - MdeModulePkg/Universal/Network/IScsiDxe/IScsiDxe.inf > >>> -!endif > >>> -!else > >>> MdeModulePkg/Universal/Network/Tcp4Dxe/Tcp4Dxe.inf > >>> MdeModulePkg/Universal/Network/UefiPxeBcDxe/UefiPxeBcDxe.inf > >>> MdeModulePkg/Universal/Network/IScsiDxe/IScsiDxe.inf > >>> diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf > >>> index df55c2b210..f29feb27b4 100644 > >>> --- a/OvmfPkg/OvmfPkgIa32X64.fdf > >>> +++ b/OvmfPkg/OvmfPkgIa32X64.fdf > >>> @@ -314,12 +314,8 @@ [FV.DXEFV] > >>> INF NetworkPkg/Dhcp6Dxe/Dhcp6Dxe.inf > >>> INF NetworkPkg/Mtftp6Dxe/Mtftp6Dxe.inf > >>> INF NetworkPkg/UefiPxeBcDxe/UefiPxeBcDxe.inf > >>> -!if $(SECURE_BOOT_ENABLE) == TRUE > >>> INF NetworkPkg/IScsiDxe/IScsiDxe.inf > >>> !else > >>> - INF MdeModulePkg/Universal/Network/IScsiDxe/IScsiDxe.inf > >>> -!endif > >>> -!else > >>> INF MdeModulePkg/Universal/Network/Tcp4Dxe/Tcp4Dxe.inf > >>> INF MdeModulePkg/Universal/Network/UefiPxeBcDxe/UefiPxeBcDxe.inf > >>> INF MdeModulePkg/Universal/Network/IScsiDxe/IScsiDxe.inf > >>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc > >>> index d15294eb72..ac4bf4f63e 100644 > >>> --- a/OvmfPkg/OvmfPkgX64.dsc > >>> +++ b/OvmfPkg/OvmfPkgX64.dsc > >>> @@ -153,15 +153,16 @@ [LibraryClasses] > >>> > >> PlatformSecureLib|OvmfPkg/Library/PlatformSecureLib/PlatformSecureLib.inf > >>> > >> TpmMeasurementLib|SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmM > >> easurementLib.inf > >>> AuthVariableLib|SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf > >>> -!if $(NETWORK_IP6_ENABLE) == TRUE > >>> - TcpIoLib|MdeModulePkg/Library/DxeTcpIoLib/DxeTcpIoLib.inf > >>> -!endif > >>> !else > >>> > >> TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/Tpm > >> MeasurementLibNull.inf > >>> > >> AuthVariableLib|MdeModulePkg/Library/AuthVariableLibNull/AuthVariableLib > >> Null.inf > >>> !endif > >>> VarCheckLib|MdeModulePkg/Library/VarCheckLib/VarCheckLib.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 > >>> @@ -704,12 +705,8 @@ [Components] > >>> NetworkPkg/Dhcp6Dxe/Dhcp6Dxe.inf > >>> NetworkPkg/Mtftp6Dxe/Mtftp6Dxe.inf > >>> NetworkPkg/UefiPxeBcDxe/UefiPxeBcDxe.inf > >>> -!if $(SECURE_BOOT_ENABLE) == TRUE > >>> NetworkPkg/IScsiDxe/IScsiDxe.inf > >>> !else > >>> - MdeModulePkg/Universal/Network/IScsiDxe/IScsiDxe.inf > >>> -!endif > >>> -!else > >>> MdeModulePkg/Universal/Network/Tcp4Dxe/Tcp4Dxe.inf > >>> MdeModulePkg/Universal/Network/UefiPxeBcDxe/UefiPxeBcDxe.inf > >>> MdeModulePkg/Universal/Network/IScsiDxe/IScsiDxe.inf > >>> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf > >>> index 5e2e1dfaf5..8d66da099f 100644 > >>> --- a/OvmfPkg/OvmfPkgX64.fdf > >>> +++ b/OvmfPkg/OvmfPkgX64.fdf > >>> @@ -314,12 +314,8 @@ [FV.DXEFV] > >>> INF NetworkPkg/Dhcp6Dxe/Dhcp6Dxe.inf > >>> INF NetworkPkg/Mtftp6Dxe/Mtftp6Dxe.inf > >>> INF NetworkPkg/UefiPxeBcDxe/UefiPxeBcDxe.inf > >>> -!if $(SECURE_BOOT_ENABLE) == TRUE > >>> INF NetworkPkg/IScsiDxe/IScsiDxe.inf > >>> !else > >>> - INF MdeModulePkg/Universal/Network/IScsiDxe/IScsiDxe.inf > >>> -!endif > >>> -!else > >>> INF MdeModulePkg/Universal/Network/Tcp4Dxe/Tcp4Dxe.inf > >>> INF MdeModulePkg/Universal/Network/UefiPxeBcDxe/UefiPxeBcDxe.inf > >>> INF MdeModulePkg/Universal/Network/IScsiDxe/IScsiDxe.inf > >>> > >> > >> For this patch, I think one of the following updates is necessary: > >> > >> (a) either restrict the subject line to identify IScsiDxe (because the > >> current patch does not fix the full set of packages related to IPv6), > >> such as > >> > >> OvmfPkg: correct the IScsiDxe module included for the IPv6 stack > >> > >> (b) or else, squash the IpSecDxe addition into this patch (with the same > >> subject). > >> > >> Perhaps I was not clear enough about this in the previous discussion. > >> > >> Either way, I think (b) might need more experimentation / additional > >> work, and a later patch, so for now, I think we should do (a). I will > >> update the subject line myself, if that's okay with you. With that: > >> > > > > Laszlo, > > > > I also agree with (a). > > > > For IpSec, we can do the below update later: > > > > 1), Include it under NETWORK_IP6_ENABLE directly but with the limit usage for IPv4. > > > > Or > > > > 2), Define new flag "IPSEC_ENABLE" for both of them. > > > > I prefer 2). > > Hmmm, I'm not so sure. Personally I've never used either IpSec or IPv6. > > If I understand correctly: > > - IPSEC_ENABLE=TRUE && NETWORK_IP6_ENABLE=FALSE would mean > IPv4 only + IpSec. While this may be a theoretically useful > combination, I wonder how often people would actually want this. > > - IPSEC_ENABLE=TRUE && NETWORK_IP6_ENABLE=TRUE -- this is a valid > combination, especially for a full-fledged build of OVMF > > - IPSEC_ENABLE=FALSE && NETWORK_IP6_ENABLE=TRUE -- as far as I > understand, this is actually an invalid (incomplete) build for the > IPv6 stack. > > - IPSEC_ENABLE=FALSE && NETWORK_IP6_ENABLE=FALSE -- the most common > build, gives you just IPv4 > > Based on the above, I think I prefer (1); that is, I believe we > shouldn't introduce IPSEC_ENABLE. For IPv6, IpSec is apparently > mandatory, so turning it off makes no sense. And in an IPv4-only build > of OVMF, I see quite limited usefulness for IpSec; turning it on with a > dedicated flag looks overkill, at least for a virtual machine firmware. > > Jordan, Gary, what do you think? IpSec is optional for me. The Ip6 driver detects the protocol dynamically, so we don't really need IpSec for IPv6. (PXEv6 and HTTPBoot v6 works for me with the current settings.) Besides, it seems we didn't provide a proper user interface (e.g. config UI or a shell command) to setup IpSec. The user probably has to find a tool, e.g. NetworkPkg/Application/IpsecConfig, to config it properly. So I feel it's not mandatory and would prefer 2). Thanks, Gary Lin > > Thanks! > Laszlo > > > > > Thanks, > > Jiaxin > > > > > > > >> Reviewed-by: Laszlo Ersek > >> > >> Thanks! > >> Laszlo > >