| Message ID | 1459273890-14331-3-git-send-email-lersek@redhat.com |
|---|---|
| State | Accepted |
| Commit | 6a238fa88fd0b78491201a341d45ab0dec1c2947 |
| Headers | show |
On 29 March 2016 at 19:51, Laszlo Ersek <lersek@redhat.com> wrote: > Edk2 commit 8a45f80edad4 ("MdeModulePkg: Make HII configuration settings > available to OS runtime") implements the optional UEFI feature described > in "31.2.11.1 OS Runtime Utilization" in UEFI v2.6. > > While this feature might show benefits down the road even in QEMU virtual > machines, at the moment it only presents drawbacks: > - it increases the EfiRuntimeServicesData footprint, > - it triggers HII compatibility problems between edk2 and external drivers > unconditionally, even if the end-user is not interested in HII and/or in > configuring said drivers (see > <https://www.redhat.com/archives/vfio-users/2016-March/msg00153.html> > and <http://thread.gmane.org/gmane.comp.bios.edk2.devel/9894> for an > example). > > While the feature was being introduced, popular demand for a controlling > Feature PCD rose (see > <http://thread.gmane.org/gmane.comp.bios.edk2.devel/7626>), which is why > we can set it now to FALSE. > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Laszlo Ersek <lersek@redhat.com> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > ArmVirtPkg/ArmVirt.dsc.inc | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc > index db31b2dc4cfe..14e15213b3c9 100644 > --- a/ArmVirtPkg/ArmVirt.dsc.inc > +++ b/ArmVirtPkg/ArmVirt.dsc.inc > @@ -249,6 +249,7 @@ [BuildOptions] > ################################################################################ > > [PcdsFeatureFlag.common] > + gEfiMdeModulePkgTokenSpaceGuid.PcdHiiOsRuntimeSupport|FALSE > gEfiMdePkgTokenSpaceGuid.PcdComponentNameDisable|TRUE > gEfiMdePkgTokenSpaceGuid.PcdDriverDiagnosticsDisable|TRUE > gEfiMdePkgTokenSpaceGuid.PcdComponentName2Disable|TRUE > -- > 1.8.3.1 > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 29 March 2016 at 22:08, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 29 March 2016 at 19:51, Laszlo Ersek <lersek@redhat.com> wrote: >> Edk2 commit 8a45f80edad4 ("MdeModulePkg: Make HII configuration settings >> available to OS runtime") implements the optional UEFI feature described >> in "31.2.11.1 OS Runtime Utilization" in UEFI v2.6. >> >> While this feature might show benefits down the road even in QEMU virtual >> machines, at the moment it only presents drawbacks: >> - it increases the EfiRuntimeServicesData footprint, >> - it triggers HII compatibility problems between edk2 and external drivers >> unconditionally, even if the end-user is not interested in HII and/or in >> configuring said drivers (see >> <https://www.redhat.com/archives/vfio-users/2016-March/msg00153.html> >> and <http://thread.gmane.org/gmane.comp.bios.edk2.devel/9894> for an >> example). >> >> While the feature was being introduced, popular demand for a controlling >> Feature PCD rose (see >> <http://thread.gmane.org/gmane.comp.bios.edk2.devel/7626>), which is why >> we can set it now to FALSE. >> >> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Laszlo Ersek <lersek@redhat.com> > > Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Please not that currently, the implementation of this feature in EDK2 causes problems with the generation of the new Memory Attributes table. This is due to the fact the this feature results in RuntimeServices memory to be either allocated or freed (probably the former) in its OnReadyToBoot() event handler, which is the same event that triggers the generation of the Memory Attributes table, and the latter should run last to get a correct table, which is not guaranteed. Thanks, Ard. >> --- >> ArmVirtPkg/ArmVirt.dsc.inc | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc >> index db31b2dc4cfe..14e15213b3c9 100644 >> --- a/ArmVirtPkg/ArmVirt.dsc.inc >> +++ b/ArmVirtPkg/ArmVirt.dsc.inc >> @@ -249,6 +249,7 @@ [BuildOptions] >> ################################################################################ >> >> [PcdsFeatureFlag.common] >> + gEfiMdeModulePkgTokenSpaceGuid.PcdHiiOsRuntimeSupport|FALSE >> gEfiMdePkgTokenSpaceGuid.PcdComponentNameDisable|TRUE >> gEfiMdePkgTokenSpaceGuid.PcdDriverDiagnosticsDisable|TRUE >> gEfiMdePkgTokenSpaceGuid.PcdComponentName2Disable|TRUE >> -- >> 1.8.3.1 >> _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 04/01/16 16:13, Ard Biesheuvel wrote: > On 29 March 2016 at 22:08, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >> On 29 March 2016 at 19:51, Laszlo Ersek <lersek@redhat.com> wrote: >>> Edk2 commit 8a45f80edad4 ("MdeModulePkg: Make HII configuration settings >>> available to OS runtime") implements the optional UEFI feature described >>> in "31.2.11.1 OS Runtime Utilization" in UEFI v2.6. >>> >>> While this feature might show benefits down the road even in QEMU virtual >>> machines, at the moment it only presents drawbacks: >>> - it increases the EfiRuntimeServicesData footprint, >>> - it triggers HII compatibility problems between edk2 and external drivers >>> unconditionally, even if the end-user is not interested in HII and/or in >>> configuring said drivers (see >>> <https://www.redhat.com/archives/vfio-users/2016-March/msg00153.html> >>> and <http://thread.gmane.org/gmane.comp.bios.edk2.devel/9894> for an >>> example). >>> >>> While the feature was being introduced, popular demand for a controlling >>> Feature PCD rose (see >>> <http://thread.gmane.org/gmane.comp.bios.edk2.devel/7626>), which is why >>> we can set it now to FALSE. >>> >>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>> Contributed-under: TianoCore Contribution Agreement 1.0 >>> Signed-off-by: Laszlo Ersek <lersek@redhat.com> >> >> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> > > Please not that currently, the implementation of this feature in EDK2 > causes problems with the generation of the new Memory Attributes > table. This is due to the fact the this feature results in > RuntimeServices memory to be either allocated or freed (probably the > former) in its OnReadyToBoot() event handler, which is the same event > that triggers the generation of the Memory Attributes table, and the > latter should run last to get a correct table, which is not > guaranteed. Yup, I've seen the recent updates from you and Jiewen in the other thread. But, that's just an argument for pushing these patches rather sooner than later, right? Thanks Laszlo > Thanks, > Ard. > >>> --- >>> ArmVirtPkg/ArmVirt.dsc.inc | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc >>> index db31b2dc4cfe..14e15213b3c9 100644 >>> --- a/ArmVirtPkg/ArmVirt.dsc.inc >>> +++ b/ArmVirtPkg/ArmVirt.dsc.inc >>> @@ -249,6 +249,7 @@ [BuildOptions] >>> ################################################################################ >>> >>> [PcdsFeatureFlag.common] >>> + gEfiMdeModulePkgTokenSpaceGuid.PcdHiiOsRuntimeSupport|FALSE >>> gEfiMdePkgTokenSpaceGuid.PcdComponentNameDisable|TRUE >>> gEfiMdePkgTokenSpaceGuid.PcdDriverDiagnosticsDisable|TRUE >>> gEfiMdePkgTokenSpaceGuid.PcdComponentName2Disable|TRUE >>> -- >>> 1.8.3.1 >>> _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 1 April 2016 at 16:17, Laszlo Ersek <lersek@redhat.com> wrote: > On 04/01/16 16:13, Ard Biesheuvel wrote: >> On 29 March 2016 at 22:08, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >>> On 29 March 2016 at 19:51, Laszlo Ersek <lersek@redhat.com> wrote: >>>> Edk2 commit 8a45f80edad4 ("MdeModulePkg: Make HII configuration settings >>>> available to OS runtime") implements the optional UEFI feature described >>>> in "31.2.11.1 OS Runtime Utilization" in UEFI v2.6. >>>> >>>> While this feature might show benefits down the road even in QEMU virtual >>>> machines, at the moment it only presents drawbacks: >>>> - it increases the EfiRuntimeServicesData footprint, >>>> - it triggers HII compatibility problems between edk2 and external drivers >>>> unconditionally, even if the end-user is not interested in HII and/or in >>>> configuring said drivers (see >>>> <https://www.redhat.com/archives/vfio-users/2016-March/msg00153.html> >>>> and <http://thread.gmane.org/gmane.comp.bios.edk2.devel/9894> for an >>>> example). >>>> >>>> While the feature was being introduced, popular demand for a controlling >>>> Feature PCD rose (see >>>> <http://thread.gmane.org/gmane.comp.bios.edk2.devel/7626>), which is why >>>> we can set it now to FALSE. >>>> >>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>>> Contributed-under: TianoCore Contribution Agreement 1.0 >>>> Signed-off-by: Laszlo Ersek <lersek@redha [...] >>> >>> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>> >> >> Please note that currently, the implementation of this feature in EDK2 >> causes problems with the generation of the new Memory Attributes >> table. This is due to the fact the this feature results in >> RuntimeServices memory to be either allocated or freed (probably the >> former) in its OnReadyToBoot() event handler, which is the same event >> that triggers the generation of the Memory Attributes table, and the >> latter should run last to get a correct table, which is not >> guaranteed. > > Yup, I've seen the recent updates from you and Jiewen in the other thread. > > But, that's just an argument for pushing these patches rather sooner > than later, right? > Yes, or for making PcdHiiOsRuntimeSupport default to FALSE. But I don't care either way, as long as we are aware that the features are currently incompatible, and I am only interested in one of them anyway :-) _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 04/01/16 16:20, Ard Biesheuvel wrote: > On 1 April 2016 at 16:17, Laszlo Ersek <lersek@redhat.com> wrote: >> On 04/01/16 16:13, Ard Biesheuvel wrote: >>> On 29 March 2016 at 22:08, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >>>> On 29 March 2016 at 19:51, Laszlo Ersek <lersek@redhat.com> wrote: >>>>> Edk2 commit 8a45f80edad4 ("MdeModulePkg: Make HII configuration settings >>>>> available to OS runtime") implements the optional UEFI feature described >>>>> in "31.2.11.1 OS Runtime Utilization" in UEFI v2.6. >>>>> >>>>> While this feature might show benefits down the road even in QEMU virtual >>>>> machines, at the moment it only presents drawbacks: >>>>> - it increases the EfiRuntimeServicesData footprint, >>>>> - it triggers HII compatibility problems between edk2 and external drivers >>>>> unconditionally, even if the end-user is not interested in HII and/or in >>>>> configuring said drivers (see >>>>> <https://www.redhat.com/archives/vfio-users/2016-March/msg00153.html> >>>>> and <http://thread.gmane.org/gmane.comp.bios.edk2.devel/9894> for an >>>>> example). >>>>> >>>>> While the feature was being introduced, popular demand for a controlling >>>>> Feature PCD rose (see >>>>> <http://thread.gmane.org/gmane.comp.bios.edk2.devel/7626>), which is why >>>>> we can set it now to FALSE. >>>>> >>>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>>>> Contributed-under: TianoCore Contribution Agreement 1.0 >>>>> Signed-off-by: Laszlo Ersek <lersek@redha > [...] >>>> >>>> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>>> >>> >>> Please note that currently, the implementation of this feature in EDK2 >>> causes problems with the generation of the new Memory Attributes >>> table. This is due to the fact the this feature results in >>> RuntimeServices memory to be either allocated or freed (probably the >>> former) in its OnReadyToBoot() event handler, which is the same event >>> that triggers the generation of the Memory Attributes table, and the >>> latter should run last to get a correct table, which is not >>> guaranteed. >> >> Yup, I've seen the recent updates from you and Jiewen in the other thread. >> >> But, that's just an argument for pushing these patches rather sooner >> than later, right? >> > > Yes, or for making PcdHiiOsRuntimeSupport default to FALSE. > > But I don't care either way, as long as we are aware that the features > are currently incompatible, and I am only interested in one of them > anyway :-) Given that PcdHiiOsRuntimeSupport was introduced at all after multi-lateral requests for an option to disable the feature, I don't expect the default value in the .DEC file to stay FALSE in the long term -- and that's assuming we can flip it to FALSE (from the current TRUE) in the first place. So, I think these patches should be committed. Given that I've been on vacation this week (in theory anyway -- I guess one couldn't tell from my mailing list activity), I sort of "draw the line" at committing patches, until next Monday. For the OvmfPkg patch, Jordan's review is still needed anyway, but this one -- can you please go ahead and push it? Thanks! Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 1 April 2016 at 16:32, Laszlo Ersek <lersek@redhat.com> wrote: > On 04/01/16 16:20, Ard Biesheuvel wrote: >> On 1 April 2016 at 16:17, Laszlo Ersek <lersek@redhat.com> wrote: >>> On 04/01/16 16:13, Ard Biesheuvel wrote: >>>> On 29 March 2016 at 22:08, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >>>>> On 29 March 2016 at 19:51, Laszlo Ersek <lersek@redhat.com> wrote: >>>>>> Edk2 commit 8a45f80edad4 ("MdeModulePkg: Make HII configuration settings >>>>>> available to OS runtime") implements the optional UEFI feature described >>>>>> in "31.2.11.1 OS Runtime Utilization" in UEFI v2.6. >>>>>> >>>>>> While this feature might show benefits down the road even in QEMU virtual >>>>>> machines, at the moment it only presents drawbacks: >>>>>> - it increases the EfiRuntimeServicesData footprint, >>>>>> - it triggers HII compatibility problems between edk2 and external drivers >>>>>> unconditionally, even if the end-user is not interested in HII and/or in >>>>>> configuring said drivers (see >>>>>> <https://www.redhat.com/archives/vfio-users/2016-March/msg00153.html> >>>>>> and <http://thread.gmane.org/gmane.comp.bios.edk2.devel/9894> for an >>>>>> example). >>>>>> >>>>>> While the feature was being introduced, popular demand for a controlling >>>>>> Feature PCD rose (see >>>>>> <http://thread.gmane.org/gmane.comp.bios.edk2.devel/7626>), which is why >>>>>> we can set it now to FALSE. >>>>>> >>>>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>>>>> Contributed-under: TianoCore Contribution Agreement 1.0 >>>>>> Signed-off-by: Laszlo Ersek <lersek@redha >> [...] >>>>> >>>>> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>>>> >>>> >>>> Please note that currently, the implementation of this feature in EDK2 >>>> causes problems with the generation of the new Memory Attributes >>>> table. This is due to the fact the this feature results in >>>> RuntimeServices memory to be either allocated or freed (probably the >>>> former) in its OnReadyToBoot() event handler, which is the same event >>>> that triggers the generation of the Memory Attributes table, and the >>>> latter should run last to get a correct table, which is not >>>> guaranteed. >>> >>> Yup, I've seen the recent updates from you and Jiewen in the other thread. >>> >>> But, that's just an argument for pushing these patches rather sooner >>> than later, right? >>> >> >> Yes, or for making PcdHiiOsRuntimeSupport default to FALSE. >> >> But I don't care either way, as long as we are aware that the features >> are currently incompatible, and I am only interested in one of them >> anyway :-) > > Given that PcdHiiOsRuntimeSupport was introduced at all after > multi-lateral requests for an option to disable the feature, I don't > expect the default value in the .DEC file to stay FALSE in the long term > -- and that's assuming we can flip it to FALSE (from the current TRUE) > in the first place. > > So, I think these patches should be committed. Given that I've been on > vacation this week (in theory anyway -- I guess one couldn't tell from > my mailing list activity), I sort of "draw the line" at committing > patches, until next Monday. For the OvmfPkg patch, Jordan's review is > still needed anyway, but this one -- can you please go ahead and push it? > Sure. Pushed as 6a238fa88fd0 Thanks, Ard. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc index db31b2dc4cfe..14e15213b3c9 100644 --- a/ArmVirtPkg/ArmVirt.dsc.inc +++ b/ArmVirtPkg/ArmVirt.dsc.inc @@ -249,6 +249,7 @@ [BuildOptions] ################################################################################ [PcdsFeatureFlag.common] + gEfiMdeModulePkgTokenSpaceGuid.PcdHiiOsRuntimeSupport|FALSE gEfiMdePkgTokenSpaceGuid.PcdComponentNameDisable|TRUE gEfiMdePkgTokenSpaceGuid.PcdDriverDiagnosticsDisable|TRUE gEfiMdePkgTokenSpaceGuid.PcdComponentName2Disable|TRUE
Edk2 commit 8a45f80edad4 ("MdeModulePkg: Make HII configuration settings available to OS runtime") implements the optional UEFI feature described in "31.2.11.1 OS Runtime Utilization" in UEFI v2.6. While this feature might show benefits down the road even in QEMU virtual machines, at the moment it only presents drawbacks: - it increases the EfiRuntimeServicesData footprint, - it triggers HII compatibility problems between edk2 and external drivers unconditionally, even if the end-user is not interested in HII and/or in configuring said drivers (see <https://www.redhat.com/archives/vfio-users/2016-March/msg00153.html> and <http://thread.gmane.org/gmane.comp.bios.edk2.devel/9894> for an example). While the feature was being introduced, popular demand for a controlling Feature PCD rose (see <http://thread.gmane.org/gmane.comp.bios.edk2.devel/7626>), which is why we can set it now to FALSE. Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- ArmVirtPkg/ArmVirt.dsc.inc | 1 + 1 file changed, 1 insertion(+) -- 1.8.3.1 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel