From: Gary Lin <glin@suse.com>
To: Laszlo Ersek <lersek@redhat.com>
Cc: "Wu, Jiaxin" <jiaxin.wu@intel.com>,
"edk2-devel@ml01.01.org" <edk2-devel@ml01.01.org>,
"Justen, Jordan L" <jordan.l.justen@intel.com>,
"Long, Qin" <qin.long@intel.com>
Subject: Re: [PATCH 2/3] OvmfPkg: correct the set of modules included for the IPv6 stack
Date: Wed, 18 Jan 2017 17:21:40 +0800 [thread overview]
Message-ID: <20170118092140.2kjvl5w5327e4pst@GaryWorkstation> (raw)
In-Reply-To: <6e2dc3db-8657-cf9d-39d9-e604d7b2f92c@redhat.com>
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 <lersek@redhat.com>
> >>> Cc: Justen Jordan L <jordan.l.justen@intel.com>
> >>> Cc: Wu Jiaxin <jiaxin.wu@intel.com>
> >>> Cc: Long Qin <qin.long@intel.com>
> >>> Contributed-under: TianoCore Contribution Agreement 1.0
> >>> Signed-off-by: Gary Lin <glin@suse.com>
> >>> ---
> >>> 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 <lersek@redhat.com>
> >>
> >> Thanks!
> >> Laszlo
>
>
next prev parent reply other threads:[~2017-01-18 9:22 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-01-17 4:52 [PATCH 0/3] Enable HTTPS Boot in OVMF Gary Lin
2017-01-17 4:52 ` [PATCH 1/3] OvmfPkg: always resolve OpenSslLib, IntrinsicLib and BaseCryptLib Gary Lin
2017-01-17 8:03 ` Wu, Jiaxin
2017-01-17 9:13 ` Laszlo Ersek
2017-01-17 4:52 ` [PATCH 2/3] OvmfPkg: correct the set of modules included for the IPv6 stack Gary Lin
2017-01-17 8:04 ` Wu, Jiaxin
2017-01-17 9:22 ` Laszlo Ersek
2017-01-18 0:47 ` Wu, Jiaxin
2017-01-18 8:17 ` Laszlo Ersek
2017-01-18 9:21 ` Gary Lin [this message]
2017-01-19 3:09 ` Wu, Jiaxin
2017-01-19 8:36 ` Laszlo Ersek
2017-01-17 4:52 ` [PATCH 3/3] OvmfPkg: pull in TLS modules with -D TLS_ENABLE (also enabling HTTPS) Gary Lin
2017-01-17 8:04 ` Wu, Jiaxin
2017-01-17 9:24 ` Laszlo Ersek
2017-01-17 8:13 ` [PATCH 0/3] Enable HTTPS Boot in OVMF Long, Qin
2017-01-17 8:25 ` Jordan Justen
2017-01-17 20:13 ` Laszlo Ersek
2017-01-18 1:59 ` Gary Lin
2017-01-17 9:49 ` Laszlo Ersek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170118092140.2kjvl5w5327e4pst@GaryWorkstation \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox