public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [RFC] Expose HII package list via C variables
@ 2021-08-25 21:21 Marvin Häuser
  2021-08-25 22:33 ` [edk2-devel] " Michael D Kinney
  2021-08-26 18:19 ` Ni, Ray
  0 siblings, 2 replies; 17+ messages in thread
From: Marvin Häuser @ 2021-08-25 21:21 UTC (permalink / raw)
  To: devel@edk2.groups.io
  Cc: Andrew Fish, leif, michael.d.kinney, ray.ni, zhichao.gao,
	Jian J Wang, Hao A Wu, dandan.bi, eric.dong, Bret Barkelew,
	Vitaly Cheptsov

Good day everyone,

Currently, the HII package list is stored in a PE/COFF resource section 
[1]. I propose to store it in a C variable (byte array with a pointer to 
it and its size exposed) instead. DxeCore would have a guard to toggle 
the deprecated support for the automatic protocol installation. This has 
the following advantages:

1. Fixes BZ (incl. future toolchains): 
https://bugzilla.tianocore.org/show_bug.cgi?id=557
2. Universal method across all toolchains and output file formats
3. Saves error-prone parsing work
4. Saves protocol install/locate work, the data is available right away
5. The omission of a dedicated section can save space
6. Terse file formats can support this and remain terse :)
7. Removes a dependency on the PE/COFF format specifically

A *very rough* PoC diff can be found here: 
https://github.com/mhaeuser/edk2/compare/master...wip_hii_cvar
If the feedback is positive, I will clean it up of course. OVMF boots 
with everything working fine.

I'd explicitly like feedback on the following:
A. Is this an acceptable solution to BZ 557 (Andrew?)?
B. Is this an acceptable solution for the "HII workflow" (MdeModule 
maintainers?)?
C. Is it acceptable to make support UEFI-side support for the old 
mechanism optional (Stewards?)?
D. Can an acceptable alternative be found for the removed ShellPkg code 
(Shell maintainers?)?

As you can see the BaseTools part also is rough, but that is more a 
question of "how" rather than "whether", so I'll postpone asking about it.

Thanks for your time and feedback!

Best regards,
Marvin


[1] "Once the image is loaded, LoadImage() installs 
EFI_HII_PACKAGE_LIST_PROTOCOL on the handle if
the image contains a custom PE/COFF resource with the type 'HII'."
- UEFI 2.9, 7.4, "EFI_BOOT_SERVICES.LoadImage()"

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [edk2-devel] [RFC] Expose HII package list via C variables
  2021-08-25 21:21 [RFC] Expose HII package list via C variables Marvin Häuser
@ 2021-08-25 22:33 ` Michael D Kinney
  2021-08-26  8:50   ` Marvin Häuser
  2021-08-26 18:19 ` Ni, Ray
  1 sibling, 1 reply; 17+ messages in thread
From: Michael D Kinney @ 2021-08-25 22:33 UTC (permalink / raw)
  To: devel@edk2.groups.io, mhaeuser@posteo.de, Kinney, Michael D
  Cc: Andrew Fish, leif@nuviainc.com, Ni, Ray, Gao, Zhichao,
	Wang, Jian J, Wu, Hao A, Bi, Dandan, Dong, Eric, Bret Barkelew,
	Vitaly Cheptsov

Hi Marvin,

I think this feature is already there and supported.

HII can either be in a global variable or in a PE/COFF resource section.
The default is a global variable because HII was implemented before the
PE/COFF resource section feature was added to the UEFI Specification.

There is an INF [Defines] section statement to enable the PE/COFF
section. See UefiHiiResource in the following link.

https://tianocore-docs.github.io/edk2-InfSpecification/draft/3_edk_ii_inf_file_format/34_[defines]_section.html#34-defines-section

How is your proposal different than this existing capability?

Thanks,

Mike

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Marvin Häuser
> Sent: Wednesday, August 25, 2021 2:21 PM
> To: devel@edk2.groups.io
> Cc: Andrew Fish <afish@apple.com>; leif@nuviainc.com; Kinney, Michael D <michael.d.kinney@intel.com>; Ni, Ray
> <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> <hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Dong, Eric <eric.dong@intel.com>; Bret Barkelew
> <Bret.Barkelew@microsoft.com>; Vitaly Cheptsov <vit9696@protonmail.com>
> Subject: [edk2-devel] [RFC] Expose HII package list via C variables
> 
> Good day everyone,
> 
> Currently, the HII package list is stored in a PE/COFF resource section
> [1]. I propose to store it in a C variable (byte array with a pointer to
> it and its size exposed) instead. DxeCore would have a guard to toggle
> the deprecated support for the automatic protocol installation. This has
> the following advantages:
> 
> 1. Fixes BZ (incl. future toolchains):
> https://bugzilla.tianocore.org/show_bug.cgi?id=557
> 2. Universal method across all toolchains and output file formats
> 3. Saves error-prone parsing work
> 4. Saves protocol install/locate work, the data is available right away
> 5. The omission of a dedicated section can save space
> 6. Terse file formats can support this and remain terse :)
> 7. Removes a dependency on the PE/COFF format specifically
> 
> A *very rough* PoC diff can be found here:
> https://github.com/mhaeuser/edk2/compare/master...wip_hii_cvar
> If the feedback is positive, I will clean it up of course. OVMF boots
> with everything working fine.
> 
> I'd explicitly like feedback on the following:
> A. Is this an acceptable solution to BZ 557 (Andrew?)?
> B. Is this an acceptable solution for the "HII workflow" (MdeModule
> maintainers?)?
> C. Is it acceptable to make support UEFI-side support for the old
> mechanism optional (Stewards?)?
> D. Can an acceptable alternative be found for the removed ShellPkg code
> (Shell maintainers?)?
> 
> As you can see the BaseTools part also is rough, but that is more a
> question of "how" rather than "whether", so I'll postpone asking about it.
> 
> Thanks for your time and feedback!
> 
> Best regards,
> Marvin
> 
> 
> [1] "Once the image is loaded, LoadImage() installs
> EFI_HII_PACKAGE_LIST_PROTOCOL on the handle if
> the image contains a custom PE/COFF resource with the type 'HII'."
> - UEFI 2.9, 7.4, "EFI_BOOT_SERVICES.LoadImage()"
> 
> 
> 
> 


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [edk2-devel] [RFC] Expose HII package list via C variables
  2021-08-25 22:33 ` [edk2-devel] " Michael D Kinney
@ 2021-08-26  8:50   ` Marvin Häuser
  2021-08-26 14:37     ` Michael D Kinney
  0 siblings, 1 reply; 17+ messages in thread
From: Marvin Häuser @ 2021-08-26  8:50 UTC (permalink / raw)
  To: devel, michael.d.kinney
  Cc: devel, Andrew Fish, leif, Ni, Ray, Gao, Zhichao, Wang, Jian J,
	Wu, Hao A, Bi, Dandan, Dong, Eric, Bret Barkelew, Vitaly Cheptsov

Hey Mike,

Thanks for your reply!

Well, this switch is not well-documented. Looking now at both the 
generation code and the emitted code, it does not generate a package 
list like my code does, but separate data variables (strings and images) 
that cannot easily be passed to HiiDatabase as-is. However apparently 
there are drivers that use this functionality successfully by composing 
the package list at runtime [1].

Looking with this information now at the pattern of using HII C 
variables (which I did not know existed before) vs the PE/COFF HII 
section, most that use latter are Shell applications, which I guess 
means the section has actually been introduced to resolve D.? There are 
exceptions such as LogoDxe [2], which use the PE/COFF section while D. 
is not a problem, hence I got confused, sorry. I think these modules 
should be updated in any case. Do you agree?

So, for modules that use C variables already, my patch would save some 
runtime generation code and dynamic memory allocation for the HII 
package list. This was not my goal (as I said, I didn't realise HII C 
variables already were a thing in the first place), but the changes are 
small enough that it might be worth considering anyway, in my opinion. 
If a HII package list is generated for both Shell and non-Shell apps, 
this also means code paths can be unified. For example, there could be a 
library class with constructor and destructor to add/remove packages 
from the HII database for all modules that use such, Shell or not. For 
BaseTools it means that there is no real need for separate Python and C 
paths as ideally they just generate the exact same data.

Now to D., the only usage for this seems to be that Shell can locate the 
help text in the executable without executing it, yet it is fully loaded 
anyway [3]. To be honest, I find it hard to justify loading an 
executable (PE/COFF loading, memory permission application, the full 
process) to retrieve a help text and then unloading it again, especially 
with the HII code being on a core dispatcher level. 1. to 7. still hold 
true in my opinion. Was there any discussion I could read through why 
Shell apps cannot simply support a "--help" or "-?" command and output 
the string themselves? Pushing the burden to the Shell apps does 
preserve the "drawback" that a full loading is required (which honestly 
does not matter for a debugging application like Shell), however it does 
relieve the burden of PE/COFF HII parsing from the core dispatcher 
(which matters a lot in my opinion to keep the core simple). It would 
simply be a normal Shell app execution as any other however. If someone 
wants to avoid the PE/COFF burden altogether, they can still provide a 
.man file.

As for my points 6. and 7., maybe I should provide some context. Due to 
many issues with TE files, platforms started abandoning them and 
returned to PE/COFF Images. I think a big reason for this is that TE is 
not really a sound and complete format, but a stripped version of 
PE/COFF with none of the necessary fixups applied. I'm currently 
sketching a possible alternative [4], and I would really like to not 
having to specify a HII section type, while still preserving 
compatibility with all of the UEFI Image types and use-cases [4].

Thanks again!

Best regards,
Marvin


[1] 
https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0eab4ac688d5/MdeModulePkg/Application/BootManagerMenuApp/BootManagerMenu.c#L929-L934

[2] 
https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0eab4ac688d5/MdeModulePkg/Logo/LogoDxe.inf#L23

[3] 
https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0eab4ac688d5/ShellPkg/Application/Shell/ShellManParser.c#L646-L671

[4] 
https://github.com/mhaeuser/edk2/blob/ue_poc/MdePkg/Include/IndustryStandard/UeImage.h

26.08.2021 00:34:12 Michael D Kinney <michael.d.kinney@intel.com>:

> Hi Marvin,
> 
> I think this feature is already there and supported.
> 
> HII can either be in a global variable or in a PE/COFF resource section.
> The default is a global variable because HII was implemented before the
> PE/COFF resource section feature was added to the UEFI Specification.
> 
> There is an INF [Defines] section statement to enable the PE/COFF
> section. See UefiHiiResource in the following link.
> 
> https://tianocore-docs.github.io/edk2-InfSpecification/draft/3_edk_ii_inf_file_format/34_[defines]_section.html#34-defines-section
> 
> How is your proposal different than this existing capability?
> 
> Thanks,
> 
> Mike
> 
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Marvin Häuser
>> Sent: Wednesday, August 25, 2021 2:21 PM
>> To: devel@edk2.groups.io
>> Cc: Andrew Fish <afish@apple.com>; leif@nuviainc.com; Kinney, Michael D <michael.d.kinney@intel.com>; Ni, Ray
>> <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
>> <hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Dong, Eric <eric.dong@intel.com>; Bret Barkelew
>> <Bret.Barkelew@microsoft.com>; Vitaly Cheptsov <vit9696@protonmail.com>
>> Subject: [edk2-devel] [RFC] Expose HII package list via C variables
>> 
>> Good day everyone,
>> 
>> Currently, the HII package list is stored in a PE/COFF resource section
>> [1]. I propose to store it in a C variable (byte array with a pointer to
>> it and its size exposed) instead. DxeCore would have a guard to toggle
>> the deprecated support for the automatic protocol installation. This has
>> the following advantages:
>> 
>> 1. Fixes BZ (incl. future toolchains):
>> https://bugzilla.tianocore.org/show_bug.cgi?id=557
>> 2. Universal method across all toolchains and output file formats
>> 3. Saves error-prone parsing work
>> 4. Saves protocol install/locate work, the data is available right away
>> 5. The omission of a dedicated section can save space
>> 6. Terse file formats can support this and remain terse :)
>> 7. Removes a dependency on the PE/COFF format specifically
>> 
>> A *very rough* PoC diff can be found here:
>> https://github.com/mhaeuser/edk2/compare/master...wip_hii_cvar
>> If the feedback is positive, I will clean it up of course. OVMF boots
>> with everything working fine.
>> 
>> I'd explicitly like feedback on the following:
>> A. Is this an acceptable solution to BZ 557 (Andrew?)?
>> B. Is this an acceptable solution for the "HII workflow" (MdeModule
>> maintainers?)?
>> C. Is it acceptable to make support UEFI-side support for the old
>> mechanism optional (Stewards?)?
>> D. Can an acceptable alternative be found for the removed ShellPkg code
>> (Shell maintainers?)?
>> 
>> As you can see the BaseTools part also is rough, but that is more a
>> question of "how" rather than "whether", so I'll postpone asking about it.
>> 
>> Thanks for your time and feedback!
>> 
>> Best regards,
>> Marvin
>> 
>> 
>> [1] "Once the image is loaded, LoadImage() installs
>> EFI_HII_PACKAGE_LIST_PROTOCOL on the handle if
>> the image contains a custom PE/COFF resource with the type 'HII'."
>> - UEFI 2.9, 7.4, "EFI_BOOT_SERVICES.LoadImage()"
>> 
>> 
>> 
>> 
> 
> 
> 
> 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [edk2-devel] [RFC] Expose HII package list via C variables
  2021-08-26  8:50   ` Marvin Häuser
@ 2021-08-26 14:37     ` Michael D Kinney
  2021-08-26 14:47       ` Marvin Häuser
  2021-08-26 16:01       ` Tim Lewis
  0 siblings, 2 replies; 17+ messages in thread
From: Michael D Kinney @ 2021-08-26 14:37 UTC (permalink / raw)
  To: devel@edk2.groups.io, mhaeuser@posteo.de, Kinney, Michael D
  Cc: Andrew Fish, leif@nuviainc.com, Ni, Ray, Gao, Zhichao,
	Wang, Jian J, Wu, Hao A, Bi, Dandan, Dong, Eric, Bret Barkelew,
	Vitaly Cheptsov

Marvin,

One constraint in this discussion is that the HII content in 
a PE/COFF resource section is defined in the UEFI Specification,
Which means UEFI Apps and UEFI Drivers from media and option ROMs
that are not part of the system FW image are allowed to use this 
feature,  This means the system FW PE/COFF loader must support
loading HII content from this PE/COFF resource section to be UEFI
conformant.  So we cannot remove this feature from the PE/COFF 
loader without changes to the UEFI Specification.  Even if
changes to the UEFI Specification we made, we would have to
continue to support this feature for backward compatibility 
with existing UEFI Apps/Drivers that may be using this 
feature.

Thanks,

Mike

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Marvin Häuser
> Sent: Thursday, August 26, 2021 1:51 AM
> To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: devel@edk2.groups.io; Andrew Fish <afish@apple.com>; leif@nuviainc.com; Ni, Ray <ray.ni@intel.com>; Gao, Zhichao
> <zhichao.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan
> <dandan.bi@intel.com>; Dong, Eric <eric.dong@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>; Vitaly Cheptsov
> <vit9696@protonmail.com>
> Subject: Re: [edk2-devel] [RFC] Expose HII package list via C variables
> 
> Hey Mike,
> 
> Thanks for your reply!
> 
> Well, this switch is not well-documented. Looking now at both the
> generation code and the emitted code, it does not generate a package
> list like my code does, but separate data variables (strings and images)
> that cannot easily be passed to HiiDatabase as-is. However apparently
> there are drivers that use this functionality successfully by composing
> the package list at runtime [1].
> 
> Looking with this information now at the pattern of using HII C
> variables (which I did not know existed before) vs the PE/COFF HII
> section, most that use latter are Shell applications, which I guess
> means the section has actually been introduced to resolve D.? There are
> exceptions such as LogoDxe [2], which use the PE/COFF section while D.
> is not a problem, hence I got confused, sorry. I think these modules
> should be updated in any case. Do you agree?
> 
> So, for modules that use C variables already, my patch would save some
> runtime generation code and dynamic memory allocation for the HII
> package list. This was not my goal (as I said, I didn't realise HII C
> variables already were a thing in the first place), but the changes are
> small enough that it might be worth considering anyway, in my opinion.
> If a HII package list is generated for both Shell and non-Shell apps,
> this also means code paths can be unified. For example, there could be a
> library class with constructor and destructor to add/remove packages
> from the HII database for all modules that use such, Shell or not. For
> BaseTools it means that there is no real need for separate Python and C
> paths as ideally they just generate the exact same data.
> 
> Now to D., the only usage for this seems to be that Shell can locate the
> help text in the executable without executing it, yet it is fully loaded
> anyway [3]. To be honest, I find it hard to justify loading an
> executable (PE/COFF loading, memory permission application, the full
> process) to retrieve a help text and then unloading it again, especially
> with the HII code being on a core dispatcher level. 1. to 7. still hold
> true in my opinion. Was there any discussion I could read through why
> Shell apps cannot simply support a "--help" or "-?" command and output
> the string themselves? Pushing the burden to the Shell apps does
> preserve the "drawback" that a full loading is required (which honestly
> does not matter for a debugging application like Shell), however it does
> relieve the burden of PE/COFF HII parsing from the core dispatcher
> (which matters a lot in my opinion to keep the core simple). It would
> simply be a normal Shell app execution as any other however. If someone
> wants to avoid the PE/COFF burden altogether, they can still provide a
> .man file.
> 
> As for my points 6. and 7., maybe I should provide some context. Due to
> many issues with TE files, platforms started abandoning them and
> returned to PE/COFF Images. I think a big reason for this is that TE is
> not really a sound and complete format, but a stripped version of
> PE/COFF with none of the necessary fixups applied. I'm currently
> sketching a possible alternative [4], and I would really like to not
> having to specify a HII section type, while still preserving
> compatibility with all of the UEFI Image types and use-cases [4].
> 
> Thanks again!
> 
> Best regards,
> Marvin
> 
> 
> [1]
> https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0eab4ac688d5/MdeModulePkg/Application/BootManagerMenuAp
> p/BootManagerMenu.c#L929-L934
> 
> [2]
> https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0eab4ac688d5/MdeModulePkg/Logo/LogoDxe.inf#L23
> 
> [3]
> https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0eab4ac688d5/ShellPkg/Application/Shell/ShellManParser.
> c#L646-L671
> 
> [4]
> https://github.com/mhaeuser/edk2/blob/ue_poc/MdePkg/Include/IndustryStandard/UeImage.h
> 
> 26.08.2021 00:34:12 Michael D Kinney <michael.d.kinney@intel.com>:
> 
> > Hi Marvin,
> >
> > I think this feature is already there and supported.
> >
> > HII can either be in a global variable or in a PE/COFF resource section.
> > The default is a global variable because HII was implemented before the
> > PE/COFF resource section feature was added to the UEFI Specification.
> >
> > There is an INF [Defines] section statement to enable the PE/COFF
> > section. See UefiHiiResource in the following link.
> >
> > https://tianocore-docs.github.io/edk2-InfSpecification/draft/3_edk_ii_inf_file_format/34_[defines]_section.html#34-
> defines-section
> >
> > How is your proposal different than this existing capability?
> >
> > Thanks,
> >
> > Mike
> >
> >> -----Original Message-----
> >> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Marvin Häuser
> >> Sent: Wednesday, August 25, 2021 2:21 PM
> >> To: devel@edk2.groups.io
> >> Cc: Andrew Fish <afish@apple.com>; leif@nuviainc.com; Kinney, Michael D <michael.d.kinney@intel.com>; Ni, Ray
> >> <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> >> <hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Dong, Eric <eric.dong@intel.com>; Bret Barkelew
> >> <Bret.Barkelew@microsoft.com>; Vitaly Cheptsov <vit9696@protonmail.com>
> >> Subject: [edk2-devel] [RFC] Expose HII package list via C variables
> >>
> >> Good day everyone,
> >>
> >> Currently, the HII package list is stored in a PE/COFF resource section
> >> [1]. I propose to store it in a C variable (byte array with a pointer to
> >> it and its size exposed) instead. DxeCore would have a guard to toggle
> >> the deprecated support for the automatic protocol installation. This has
> >> the following advantages:
> >>
> >> 1. Fixes BZ (incl. future toolchains):
> >> https://bugzilla.tianocore.org/show_bug.cgi?id=557
> >> 2. Universal method across all toolchains and output file formats
> >> 3. Saves error-prone parsing work
> >> 4. Saves protocol install/locate work, the data is available right away
> >> 5. The omission of a dedicated section can save space
> >> 6. Terse file formats can support this and remain terse :)
> >> 7. Removes a dependency on the PE/COFF format specifically
> >>
> >> A *very rough* PoC diff can be found here:
> >> https://github.com/mhaeuser/edk2/compare/master...wip_hii_cvar
> >> If the feedback is positive, I will clean it up of course. OVMF boots
> >> with everything working fine.
> >>
> >> I'd explicitly like feedback on the following:
> >> A. Is this an acceptable solution to BZ 557 (Andrew?)?
> >> B. Is this an acceptable solution for the "HII workflow" (MdeModule
> >> maintainers?)?
> >> C. Is it acceptable to make support UEFI-side support for the old
> >> mechanism optional (Stewards?)?
> >> D. Can an acceptable alternative be found for the removed ShellPkg code
> >> (Shell maintainers?)?
> >>
> >> As you can see the BaseTools part also is rough, but that is more a
> >> question of "how" rather than "whether", so I'll postpone asking about it.
> >>
> >> Thanks for your time and feedback!
> >>
> >> Best regards,
> >> Marvin
> >>
> >>
> >> [1] "Once the image is loaded, LoadImage() installs
> >> EFI_HII_PACKAGE_LIST_PROTOCOL on the handle if
> >> the image contains a custom PE/COFF resource with the type 'HII'."
> >> - UEFI 2.9, 7.4, "EFI_BOOT_SERVICES.LoadImage()"
> >>
> >>
> >>
> >>
> >
> >
> >
> >
> 
> 
> 
> 


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [edk2-devel] [RFC] Expose HII package list via C variables
  2021-08-26 14:37     ` Michael D Kinney
@ 2021-08-26 14:47       ` Marvin Häuser
  2021-08-26 16:01       ` Tim Lewis
  1 sibling, 0 replies; 17+ messages in thread
From: Marvin Häuser @ 2021-08-26 14:47 UTC (permalink / raw)
  To: devel, michael.d.kinney
  Cc: Andrew Fish, leif@nuviainc.com, Ni, Ray, Gao, Zhichao,
	Wang, Jian J, Wu, Hao A, Bi, Dandan, Dong, Eric, Bret Barkelew,
	Vitaly Cheptsov

Hey Mike,

Yes, I am aware. My goal for that would be to declare support as 
optional to support drivers up until UEFI vX (whatever would be the spec 
to endorse drivers to move away from the PE/COFF section model), much 
like EFI 1.10 backwards-compatibility worked with UEFI 2.x, and have 
some enabled-by-default FixedPcd for HII PE/COFF support. Project 
Amaranth at ISP RAS for example would disable support for this right 
away, as there is no urgent dependency on any external software to 
backwards-support. Production consumer platforms would drop support 
eventually in the far future, just like with all the EFI 1.10 specifics. 
Possibly a DEBUG_WARN could be issued whenever such a section is 
encountered, to help identify modules relying on this model.

Considering the nature of the usage right now, i.e. Shell manual 
queries, I sure hope there aren't actually any dependencies on this. The 
new PE/COFF loader is yet to undergo extensive real-world platform 
testing on full-UEFI scale (it is already used to load OS loaders on a 
wider scale, but not e.g. Option ROMs), and so are the UEFI spec changes 
that are to be proposed for hardening the parser, so I hope this can 
just be squeezed into both. If you can give a sort of general vote 
whether this change can be endorsed or not, ignoring any real-world 
dependencies for the moment, I will clean up the changes, integrate them 
into any upcoming PE/COFF loader testing, and provide findings whenever 
they arrive.

Best regards,
Marvin

On 26/08/2021 16:37, Michael D Kinney wrote:
> Marvin,
>
> One constraint in this discussion is that the HII content in
> a PE/COFF resource section is defined in the UEFI Specification,
> Which means UEFI Apps and UEFI Drivers from media and option ROMs
> that are not part of the system FW image are allowed to use this
> feature,  This means the system FW PE/COFF loader must support
> loading HII content from this PE/COFF resource section to be UEFI
> conformant.  So we cannot remove this feature from the PE/COFF
> loader without changes to the UEFI Specification.  Even if
> changes to the UEFI Specification we made, we would have to
> continue to support this feature for backward compatibility
> with existing UEFI Apps/Drivers that may be using this
> feature.
>
> Thanks,
>
> Mike
>
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Marvin Häuser
>> Sent: Thursday, August 26, 2021 1:51 AM
>> To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
>> Cc: devel@edk2.groups.io; Andrew Fish <afish@apple.com>; leif@nuviainc.com; Ni, Ray <ray.ni@intel.com>; Gao, Zhichao
>> <zhichao.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan
>> <dandan.bi@intel.com>; Dong, Eric <eric.dong@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>; Vitaly Cheptsov
>> <vit9696@protonmail.com>
>> Subject: Re: [edk2-devel] [RFC] Expose HII package list via C variables
>>
>> Hey Mike,
>>
>> Thanks for your reply!
>>
>> Well, this switch is not well-documented. Looking now at both the
>> generation code and the emitted code, it does not generate a package
>> list like my code does, but separate data variables (strings and images)
>> that cannot easily be passed to HiiDatabase as-is. However apparently
>> there are drivers that use this functionality successfully by composing
>> the package list at runtime [1].
>>
>> Looking with this information now at the pattern of using HII C
>> variables (which I did not know existed before) vs the PE/COFF HII
>> section, most that use latter are Shell applications, which I guess
>> means the section has actually been introduced to resolve D.? There are
>> exceptions such as LogoDxe [2], which use the PE/COFF section while D.
>> is not a problem, hence I got confused, sorry. I think these modules
>> should be updated in any case. Do you agree?
>>
>> So, for modules that use C variables already, my patch would save some
>> runtime generation code and dynamic memory allocation for the HII
>> package list. This was not my goal (as I said, I didn't realise HII C
>> variables already were a thing in the first place), but the changes are
>> small enough that it might be worth considering anyway, in my opinion.
>> If a HII package list is generated for both Shell and non-Shell apps,
>> this also means code paths can be unified. For example, there could be a
>> library class with constructor and destructor to add/remove packages
>> from the HII database for all modules that use such, Shell or not. For
>> BaseTools it means that there is no real need for separate Python and C
>> paths as ideally they just generate the exact same data.
>>
>> Now to D., the only usage for this seems to be that Shell can locate the
>> help text in the executable without executing it, yet it is fully loaded
>> anyway [3]. To be honest, I find it hard to justify loading an
>> executable (PE/COFF loading, memory permission application, the full
>> process) to retrieve a help text and then unloading it again, especially
>> with the HII code being on a core dispatcher level. 1. to 7. still hold
>> true in my opinion. Was there any discussion I could read through why
>> Shell apps cannot simply support a "--help" or "-?" command and output
>> the string themselves? Pushing the burden to the Shell apps does
>> preserve the "drawback" that a full loading is required (which honestly
>> does not matter for a debugging application like Shell), however it does
>> relieve the burden of PE/COFF HII parsing from the core dispatcher
>> (which matters a lot in my opinion to keep the core simple). It would
>> simply be a normal Shell app execution as any other however. If someone
>> wants to avoid the PE/COFF burden altogether, they can still provide a
>> .man file.
>>
>> As for my points 6. and 7., maybe I should provide some context. Due to
>> many issues with TE files, platforms started abandoning them and
>> returned to PE/COFF Images. I think a big reason for this is that TE is
>> not really a sound and complete format, but a stripped version of
>> PE/COFF with none of the necessary fixups applied. I'm currently
>> sketching a possible alternative [4], and I would really like to not
>> having to specify a HII section type, while still preserving
>> compatibility with all of the UEFI Image types and use-cases [4].
>>
>> Thanks again!
>>
>> Best regards,
>> Marvin
>>
>>
>> [1]
>> https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0eab4ac688d5/MdeModulePkg/Application/BootManagerMenuAp
>> p/BootManagerMenu.c#L929-L934
>>
>> [2]
>> https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0eab4ac688d5/MdeModulePkg/Logo/LogoDxe.inf#L23
>>
>> [3]
>> https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0eab4ac688d5/ShellPkg/Application/Shell/ShellManParser.
>> c#L646-L671
>>
>> [4]
>> https://github.com/mhaeuser/edk2/blob/ue_poc/MdePkg/Include/IndustryStandard/UeImage.h
>>
>> 26.08.2021 00:34:12 Michael D Kinney <michael.d.kinney@intel.com>:
>>
>>> Hi Marvin,
>>>
>>> I think this feature is already there and supported.
>>>
>>> HII can either be in a global variable or in a PE/COFF resource section.
>>> The default is a global variable because HII was implemented before the
>>> PE/COFF resource section feature was added to the UEFI Specification.
>>>
>>> There is an INF [Defines] section statement to enable the PE/COFF
>>> section. See UefiHiiResource in the following link.
>>>
>>> https://tianocore-docs.github.io/edk2-InfSpecification/draft/3_edk_ii_inf_file_format/34_[defines]_section.html#34-
>> defines-section
>>> How is your proposal different than this existing capability?
>>>
>>> Thanks,
>>>
>>> Mike
>>>
>>>> -----Original Message-----
>>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Marvin Häuser
>>>> Sent: Wednesday, August 25, 2021 2:21 PM
>>>> To: devel@edk2.groups.io
>>>> Cc: Andrew Fish <afish@apple.com>; leif@nuviainc.com; Kinney, Michael D <michael.d.kinney@intel.com>; Ni, Ray
>>>> <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
>>>> <hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Dong, Eric <eric.dong@intel.com>; Bret Barkelew
>>>> <Bret.Barkelew@microsoft.com>; Vitaly Cheptsov <vit9696@protonmail.com>
>>>> Subject: [edk2-devel] [RFC] Expose HII package list via C variables
>>>>
>>>> Good day everyone,
>>>>
>>>> Currently, the HII package list is stored in a PE/COFF resource section
>>>> [1]. I propose to store it in a C variable (byte array with a pointer to
>>>> it and its size exposed) instead. DxeCore would have a guard to toggle
>>>> the deprecated support for the automatic protocol installation. This has
>>>> the following advantages:
>>>>
>>>> 1. Fixes BZ (incl. future toolchains):
>>>> https://bugzilla.tianocore.org/show_bug.cgi?id=557
>>>> 2. Universal method across all toolchains and output file formats
>>>> 3. Saves error-prone parsing work
>>>> 4. Saves protocol install/locate work, the data is available right away
>>>> 5. The omission of a dedicated section can save space
>>>> 6. Terse file formats can support this and remain terse :)
>>>> 7. Removes a dependency on the PE/COFF format specifically
>>>>
>>>> A *very rough* PoC diff can be found here:
>>>> https://github.com/mhaeuser/edk2/compare/master...wip_hii_cvar
>>>> If the feedback is positive, I will clean it up of course. OVMF boots
>>>> with everything working fine.
>>>>
>>>> I'd explicitly like feedback on the following:
>>>> A. Is this an acceptable solution to BZ 557 (Andrew?)?
>>>> B. Is this an acceptable solution for the "HII workflow" (MdeModule
>>>> maintainers?)?
>>>> C. Is it acceptable to make support UEFI-side support for the old
>>>> mechanism optional (Stewards?)?
>>>> D. Can an acceptable alternative be found for the removed ShellPkg code
>>>> (Shell maintainers?)?
>>>>
>>>> As you can see the BaseTools part also is rough, but that is more a
>>>> question of "how" rather than "whether", so I'll postpone asking about it.
>>>>
>>>> Thanks for your time and feedback!
>>>>
>>>> Best regards,
>>>> Marvin
>>>>
>>>>
>>>> [1] "Once the image is loaded, LoadImage() installs
>>>> EFI_HII_PACKAGE_LIST_PROTOCOL on the handle if
>>>> the image contains a custom PE/COFF resource with the type 'HII'."
>>>> - UEFI 2.9, 7.4, "EFI_BOOT_SERVICES.LoadImage()"
>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>>
>>
>>
>>
>
>
> 
>
>


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [edk2-devel] [RFC] Expose HII package list via C variables
  2021-08-26 14:37     ` Michael D Kinney
  2021-08-26 14:47       ` Marvin Häuser
@ 2021-08-26 16:01       ` Tim Lewis
  2021-08-26 16:07         ` Marvin Häuser
  1 sibling, 1 reply; 17+ messages in thread
From: Tim Lewis @ 2021-08-26 16:01 UTC (permalink / raw)
  To: devel, michael.d.kinney, mhaeuser
  Cc: 'Andrew Fish', leif, 'Ni, Ray',
	'Gao, Zhichao', 'Wang, Jian J',
	'Wu, Hao A', 'Bi, Dandan', 'Dong, Eric',
	'Bret Barkelew', 'Vitaly Cheptsov'

Hi Marvin --

I would like to add some historical perspective on this. One of the design requirements back when HII was first introduced into the UEFI specification after Intel's initial contribution was that of binary editability. In order to be able to reliably find, extract and then re-insert the HII data into the binary, it needed to be discoverable and not affect the offsets of the code and data in the binary. 

While it was possible to put some sort of signature in the read-only data sections of the binary and leave padding for possible future edits, it was felt that using a PE/COFF section similar to the resource sections was better. Resource sections are commonly in use for PE/COFF files (in Windows) and the similar idea existed in the then-extant Mac binary format (resource fork?). 

Thanks,

Tim Lewis
CTO, Insyde Software
www.insyde.com

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael D Kinney
Sent: Thursday, August 26, 2021 7:37 AM
To: devel@edk2.groups.io; mhaeuser@posteo.de; Kinney, Michael D <michael.d.kinney@intel.com>
Cc: Andrew Fish <afish@apple.com>; leif@nuviainc.com; Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Dong, Eric <eric.dong@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>; Vitaly Cheptsov <vit9696@protonmail.com>
Subject: Re: [edk2-devel] [RFC] Expose HII package list via C variables

Marvin,

One constraint in this discussion is that the HII content in a PE/COFF resource section is defined in the UEFI Specification, Which means UEFI Apps and UEFI Drivers from media and option ROMs that are not part of the system FW image are allowed to use this feature,  This means the system FW PE/COFF loader must support loading HII content from this PE/COFF resource section to be UEFI conformant.  So we cannot remove this feature from the PE/COFF loader without changes to the UEFI Specification.  Even if changes to the UEFI Specification we made, we would have to continue to support this feature for backward compatibility with existing UEFI Apps/Drivers that may be using this feature.

Thanks,

Mike

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Marvin 
> Häuser
> Sent: Thursday, August 26, 2021 1:51 AM
> To: devel@edk2.groups.io; Kinney, Michael D 
> <michael.d.kinney@intel.com>
> Cc: devel@edk2.groups.io; Andrew Fish <afish@apple.com>; 
> leif@nuviainc.com; Ni, Ray <ray.ni@intel.com>; Gao, Zhichao 
> <zhichao.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao 
> A <hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Dong, Eric 
> <eric.dong@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>; 
> Vitaly Cheptsov <vit9696@protonmail.com>
> Subject: Re: [edk2-devel] [RFC] Expose HII package list via C 
> variables
> 
> Hey Mike,
> 
> Thanks for your reply!
> 
> Well, this switch is not well-documented. Looking now at both the 
> generation code and the emitted code, it does not generate a package 
> list like my code does, but separate data variables (strings and 
> images) that cannot easily be passed to HiiDatabase as-is. However 
> apparently there are drivers that use this functionality successfully 
> by composing the package list at runtime [1].
> 
> Looking with this information now at the pattern of using HII C 
> variables (which I did not know existed before) vs the PE/COFF HII 
> section, most that use latter are Shell applications, which I guess 
> means the section has actually been introduced to resolve D.? There 
> are exceptions such as LogoDxe [2], which use the PE/COFF section while D.
> is not a problem, hence I got confused, sorry. I think these modules 
> should be updated in any case. Do you agree?
> 
> So, for modules that use C variables already, my patch would save some 
> runtime generation code and dynamic memory allocation for the HII 
> package list. This was not my goal (as I said, I didn't realise HII C 
> variables already were a thing in the first place), but the changes 
> are small enough that it might be worth considering anyway, in my opinion.
> If a HII package list is generated for both Shell and non-Shell apps, 
> this also means code paths can be unified. For example, there could be 
> a library class with constructor and destructor to add/remove packages 
> from the HII database for all modules that use such, Shell or not. For 
> BaseTools it means that there is no real need for separate Python and 
> C paths as ideally they just generate the exact same data.
> 
> Now to D., the only usage for this seems to be that Shell can locate 
> the help text in the executable without executing it, yet it is fully 
> loaded anyway [3]. To be honest, I find it hard to justify loading an 
> executable (PE/COFF loading, memory permission application, the full
> process) to retrieve a help text and then unloading it again, 
> especially with the HII code being on a core dispatcher level. 1. to 
> 7. still hold true in my opinion. Was there any discussion I could 
> read through why Shell apps cannot simply support a "--help" or "-?" 
> command and output the string themselves? Pushing the burden to the 
> Shell apps does preserve the "drawback" that a full loading is 
> required (which honestly does not matter for a debugging application 
> like Shell), however it does relieve the burden of PE/COFF HII parsing 
> from the core dispatcher (which matters a lot in my opinion to keep 
> the core simple). It would simply be a normal Shell app execution as 
> any other however. If someone wants to avoid the PE/COFF burden 
> altogether, they can still provide a .man file.
> 
> As for my points 6. and 7., maybe I should provide some context. Due 
> to many issues with TE files, platforms started abandoning them and 
> returned to PE/COFF Images. I think a big reason for this is that TE 
> is not really a sound and complete format, but a stripped version of 
> PE/COFF with none of the necessary fixups applied. I'm currently 
> sketching a possible alternative [4], and I would really like to not 
> having to specify a HII section type, while still preserving 
> compatibility with all of the UEFI Image types and use-cases [4].
> 
> Thanks again!
> 
> Best regards,
> Marvin
> 
> 
> [1]
> https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0ea
> b4ac688d5/MdeModulePkg/Application/BootManagerMenuAp
> p/BootManagerMenu.c#L929-L934
> 
> [2]
> https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0ea
> b4ac688d5/MdeModulePkg/Logo/LogoDxe.inf#L23
> 
> [3]
> https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0eab4ac688d5/ShellPkg/Application/Shell/ShellManParser.
> c#L646-L671
> 
> [4]
> https://github.com/mhaeuser/edk2/blob/ue_poc/MdePkg/Include/IndustrySt
> andard/UeImage.h
> 
> 26.08.2021 00:34:12 Michael D Kinney <michael.d.kinney@intel.com>:
> 
> > Hi Marvin,
> >
> > I think this feature is already there and supported.
> >
> > HII can either be in a global variable or in a PE/COFF resource section.
> > The default is a global variable because HII was implemented before 
> > the PE/COFF resource section feature was added to the UEFI Specification.
> >
> > There is an INF [Defines] section statement to enable the PE/COFF 
> > section. See UefiHiiResource in the following link.
> >
> > https://tianocore-docs.github.io/edk2-InfSpecification/draft/3_edk_i
> > i_inf_file_format/34_[defines]_section.html#34-
> defines-section
> >
> > How is your proposal different than this existing capability?
> >
> > Thanks,
> >
> > Mike
> >
> >> -----Original Message-----
> >> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of 
> >> Marvin Häuser
> >> Sent: Wednesday, August 25, 2021 2:21 PM
> >> To: devel@edk2.groups.io
> >> Cc: Andrew Fish <afish@apple.com>; leif@nuviainc.com; Kinney, 
> >> Michael D <michael.d.kinney@intel.com>; Ni, Ray <ray.ni@intel.com>; 
> >> Gao, Zhichao <zhichao.gao@intel.com>; Wang, Jian J 
> >> <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan 
> >> <dandan.bi@intel.com>; Dong, Eric <eric.dong@intel.com>; Bret 
> >> Barkelew <Bret.Barkelew@microsoft.com>; Vitaly Cheptsov 
> >> <vit9696@protonmail.com>
> >> Subject: [edk2-devel] [RFC] Expose HII package list via C variables
> >>
> >> Good day everyone,
> >>
> >> Currently, the HII package list is stored in a PE/COFF resource 
> >> section [1]. I propose to store it in a C variable (byte array with 
> >> a pointer to it and its size exposed) instead. DxeCore would have a 
> >> guard to toggle the deprecated support for the automatic protocol 
> >> installation. This has the following advantages:
> >>
> >> 1. Fixes BZ (incl. future toolchains):
> >> https://bugzilla.tianocore.org/show_bug.cgi?id=557
> >> 2. Universal method across all toolchains and output file formats 
> >> 3. Saves error-prone parsing work 4. Saves protocol install/locate 
> >> work, the data is available right away 5. The omission of a 
> >> dedicated section can save space 6. Terse file formats can support 
> >> this and remain terse :) 7. Removes a dependency on the PE/COFF 
> >> format specifically
> >>
> >> A *very rough* PoC diff can be found here:
> >> https://github.com/mhaeuser/edk2/compare/master...wip_hii_cvar
> >> If the feedback is positive, I will clean it up of course. OVMF 
> >> boots with everything working fine.
> >>
> >> I'd explicitly like feedback on the following:
> >> A. Is this an acceptable solution to BZ 557 (Andrew?)?
> >> B. Is this an acceptable solution for the "HII workflow" (MdeModule 
> >> maintainers?)?
> >> C. Is it acceptable to make support UEFI-side support for the old 
> >> mechanism optional (Stewards?)?
> >> D. Can an acceptable alternative be found for the removed ShellPkg 
> >> code (Shell maintainers?)?
> >>
> >> As you can see the BaseTools part also is rough, but that is more a 
> >> question of "how" rather than "whether", so I'll postpone asking about it.
> >>
> >> Thanks for your time and feedback!
> >>
> >> Best regards,
> >> Marvin
> >>
> >>
> >> [1] "Once the image is loaded, LoadImage() installs 
> >> EFI_HII_PACKAGE_LIST_PROTOCOL on the handle if the image contains a 
> >> custom PE/COFF resource with the type 'HII'."
> >> - UEFI 2.9, 7.4, "EFI_BOOT_SERVICES.LoadImage()"
> >>
> >>
> >>
> >>
> >
> >
> >
> >
> 
> 
> 
> 








^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [edk2-devel] [RFC] Expose HII package list via C variables
  2021-08-26 16:01       ` Tim Lewis
@ 2021-08-26 16:07         ` Marvin Häuser
  2021-08-26 16:44           ` Michael D Kinney
                             ` (2 more replies)
  0 siblings, 3 replies; 17+ messages in thread
From: Marvin Häuser @ 2021-08-26 16:07 UTC (permalink / raw)
  To: tim.lewis, devel, michael.d.kinney
  Cc: 'Andrew Fish', leif, 'Ni, Ray',
	'Gao, Zhichao', 'Wang, Jian J',
	'Wu, Hao A', 'Bi, Dandan', 'Dong, Eric',
	'Bret Barkelew', 'Vitaly Cheptsov'

Hey Tim,

Interesting, do you happen to have some tool or code that performs such 
edits at hand to take a look at? Seems like most modules already use 
variables and thus cannot be modified in such a way even right now?

That's the kind of information I am looking for, thanks a lot!

Best regards,
Marvin

On 26/08/2021 18:01, tim.lewis@insyde.com wrote:
> Hi Marvin --
>
> I would like to add some historical perspective on this. One of the design requirements back when HII was first introduced into the UEFI specification after Intel's initial contribution was that of binary editability. In order to be able to reliably find, extract and then re-insert the HII data into the binary, it needed to be discoverable and not affect the offsets of the code and data in the binary.
>
> While it was possible to put some sort of signature in the read-only data sections of the binary and leave padding for possible future edits, it was felt that using a PE/COFF section similar to the resource sections was better. Resource sections are commonly in use for PE/COFF files (in Windows) and the similar idea existed in the then-extant Mac binary format (resource fork?).
>
> Thanks,
>
> Tim Lewis
> CTO, Insyde Software
> www.insyde.com
>
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael D Kinney
> Sent: Thursday, August 26, 2021 7:37 AM
> To: devel@edk2.groups.io; mhaeuser@posteo.de; Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Andrew Fish <afish@apple.com>; leif@nuviainc.com; Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Dong, Eric <eric.dong@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>; Vitaly Cheptsov <vit9696@protonmail.com>
> Subject: Re: [edk2-devel] [RFC] Expose HII package list via C variables
>
> Marvin,
>
> One constraint in this discussion is that the HII content in a PE/COFF resource section is defined in the UEFI Specification, Which means UEFI Apps and UEFI Drivers from media and option ROMs that are not part of the system FW image are allowed to use this feature,  This means the system FW PE/COFF loader must support loading HII content from this PE/COFF resource section to be UEFI conformant.  So we cannot remove this feature from the PE/COFF loader without changes to the UEFI Specification.  Even if changes to the UEFI Specification we made, we would have to continue to support this feature for backward compatibility with existing UEFI Apps/Drivers that may be using this feature.
>
> Thanks,
>
> Mike
>
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Marvin
>> Häuser
>> Sent: Thursday, August 26, 2021 1:51 AM
>> To: devel@edk2.groups.io; Kinney, Michael D
>> <michael.d.kinney@intel.com>
>> Cc: devel@edk2.groups.io; Andrew Fish <afish@apple.com>;
>> leif@nuviainc.com; Ni, Ray <ray.ni@intel.com>; Gao, Zhichao
>> <zhichao.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao
>> A <hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Dong, Eric
>> <eric.dong@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>;
>> Vitaly Cheptsov <vit9696@protonmail.com>
>> Subject: Re: [edk2-devel] [RFC] Expose HII package list via C
>> variables
>>
>> Hey Mike,
>>
>> Thanks for your reply!
>>
>> Well, this switch is not well-documented. Looking now at both the
>> generation code and the emitted code, it does not generate a package
>> list like my code does, but separate data variables (strings and
>> images) that cannot easily be passed to HiiDatabase as-is. However
>> apparently there are drivers that use this functionality successfully
>> by composing the package list at runtime [1].
>>
>> Looking with this information now at the pattern of using HII C
>> variables (which I did not know existed before) vs the PE/COFF HII
>> section, most that use latter are Shell applications, which I guess
>> means the section has actually been introduced to resolve D.? There
>> are exceptions such as LogoDxe [2], which use the PE/COFF section while D.
>> is not a problem, hence I got confused, sorry. I think these modules
>> should be updated in any case. Do you agree?
>>
>> So, for modules that use C variables already, my patch would save some
>> runtime generation code and dynamic memory allocation for the HII
>> package list. This was not my goal (as I said, I didn't realise HII C
>> variables already were a thing in the first place), but the changes
>> are small enough that it might be worth considering anyway, in my opinion.
>> If a HII package list is generated for both Shell and non-Shell apps,
>> this also means code paths can be unified. For example, there could be
>> a library class with constructor and destructor to add/remove packages
>> from the HII database for all modules that use such, Shell or not. For
>> BaseTools it means that there is no real need for separate Python and
>> C paths as ideally they just generate the exact same data.
>>
>> Now to D., the only usage for this seems to be that Shell can locate
>> the help text in the executable without executing it, yet it is fully
>> loaded anyway [3]. To be honest, I find it hard to justify loading an
>> executable (PE/COFF loading, memory permission application, the full
>> process) to retrieve a help text and then unloading it again,
>> especially with the HII code being on a core dispatcher level. 1. to
>> 7. still hold true in my opinion. Was there any discussion I could
>> read through why Shell apps cannot simply support a "--help" or "-?"
>> command and output the string themselves? Pushing the burden to the
>> Shell apps does preserve the "drawback" that a full loading is
>> required (which honestly does not matter for a debugging application
>> like Shell), however it does relieve the burden of PE/COFF HII parsing
>> from the core dispatcher (which matters a lot in my opinion to keep
>> the core simple). It would simply be a normal Shell app execution as
>> any other however. If someone wants to avoid the PE/COFF burden
>> altogether, they can still provide a .man file.
>>
>> As for my points 6. and 7., maybe I should provide some context. Due
>> to many issues with TE files, platforms started abandoning them and
>> returned to PE/COFF Images. I think a big reason for this is that TE
>> is not really a sound and complete format, but a stripped version of
>> PE/COFF with none of the necessary fixups applied. I'm currently
>> sketching a possible alternative [4], and I would really like to not
>> having to specify a HII section type, while still preserving
>> compatibility with all of the UEFI Image types and use-cases [4].
>>
>> Thanks again!
>>
>> Best regards,
>> Marvin
>>
>>
>> [1]
>> https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0ea
>> b4ac688d5/MdeModulePkg/Application/BootManagerMenuAp
>> p/BootManagerMenu.c#L929-L934
>>
>> [2]
>> https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0ea
>> b4ac688d5/MdeModulePkg/Logo/LogoDxe.inf#L23
>>
>> [3]
>> https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0eab4ac688d5/ShellPkg/Application/Shell/ShellManParser.
>> c#L646-L671
>>
>> [4]
>> https://github.com/mhaeuser/edk2/blob/ue_poc/MdePkg/Include/IndustrySt
>> andard/UeImage.h
>>
>> 26.08.2021 00:34:12 Michael D Kinney <michael.d.kinney@intel.com>:
>>
>>> Hi Marvin,
>>>
>>> I think this feature is already there and supported.
>>>
>>> HII can either be in a global variable or in a PE/COFF resource section.
>>> The default is a global variable because HII was implemented before
>>> the PE/COFF resource section feature was added to the UEFI Specification.
>>>
>>> There is an INF [Defines] section statement to enable the PE/COFF
>>> section. See UefiHiiResource in the following link.
>>>
>>> https://tianocore-docs.github.io/edk2-InfSpecification/draft/3_edk_i
>>> i_inf_file_format/34_[defines]_section.html#34-
>> defines-section
>>> How is your proposal different than this existing capability?
>>>
>>> Thanks,
>>>
>>> Mike
>>>
>>>> -----Original Message-----
>>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
>>>> Marvin Häuser
>>>> Sent: Wednesday, August 25, 2021 2:21 PM
>>>> To: devel@edk2.groups.io
>>>> Cc: Andrew Fish <afish@apple.com>; leif@nuviainc.com; Kinney,
>>>> Michael D <michael.d.kinney@intel.com>; Ni, Ray <ray.ni@intel.com>;
>>>> Gao, Zhichao <zhichao.gao@intel.com>; Wang, Jian J
>>>> <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan
>>>> <dandan.bi@intel.com>; Dong, Eric <eric.dong@intel.com>; Bret
>>>> Barkelew <Bret.Barkelew@microsoft.com>; Vitaly Cheptsov
>>>> <vit9696@protonmail.com>
>>>> Subject: [edk2-devel] [RFC] Expose HII package list via C variables
>>>>
>>>> Good day everyone,
>>>>
>>>> Currently, the HII package list is stored in a PE/COFF resource
>>>> section [1]. I propose to store it in a C variable (byte array with
>>>> a pointer to it and its size exposed) instead. DxeCore would have a
>>>> guard to toggle the deprecated support for the automatic protocol
>>>> installation. This has the following advantages:
>>>>
>>>> 1. Fixes BZ (incl. future toolchains):
>>>> https://bugzilla.tianocore.org/show_bug.cgi?id=557
>>>> 2. Universal method across all toolchains and output file formats
>>>> 3. Saves error-prone parsing work 4. Saves protocol install/locate
>>>> work, the data is available right away 5. The omission of a
>>>> dedicated section can save space 6. Terse file formats can support
>>>> this and remain terse :) 7. Removes a dependency on the PE/COFF
>>>> format specifically
>>>>
>>>> A *very rough* PoC diff can be found here:
>>>> https://github.com/mhaeuser/edk2/compare/master...wip_hii_cvar
>>>> If the feedback is positive, I will clean it up of course. OVMF
>>>> boots with everything working fine.
>>>>
>>>> I'd explicitly like feedback on the following:
>>>> A. Is this an acceptable solution to BZ 557 (Andrew?)?
>>>> B. Is this an acceptable solution for the "HII workflow" (MdeModule
>>>> maintainers?)?
>>>> C. Is it acceptable to make support UEFI-side support for the old
>>>> mechanism optional (Stewards?)?
>>>> D. Can an acceptable alternative be found for the removed ShellPkg
>>>> code (Shell maintainers?)?
>>>>
>>>> As you can see the BaseTools part also is rough, but that is more a
>>>> question of "how" rather than "whether", so I'll postpone asking about it.
>>>>
>>>> Thanks for your time and feedback!
>>>>
>>>> Best regards,
>>>> Marvin
>>>>
>>>>
>>>> [1] "Once the image is loaded, LoadImage() installs
>>>> EFI_HII_PACKAGE_LIST_PROTOCOL on the handle if the image contains a
>>>> custom PE/COFF resource with the type 'HII'."
>>>> - UEFI 2.9, 7.4, "EFI_BOOT_SERVICES.LoadImage()"
>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>>
>>
>>
>>
>
>
> 
>
>
>


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [edk2-devel] [RFC] Expose HII package list via C variables
  2021-08-26 16:07         ` Marvin Häuser
@ 2021-08-26 16:44           ` Michael D Kinney
  2021-08-26 17:29             ` Marvin Häuser
  2021-08-26 19:50             ` Andrew Fish
  2021-08-26 16:49           ` Samer El-Haj-Mahmoud
  2021-08-26 17:07           ` Tim Lewis
  2 siblings, 2 replies; 17+ messages in thread
From: Michael D Kinney @ 2021-08-26 16:44 UTC (permalink / raw)
  To: Marvin Häuser, tim.lewis@insyde.com, devel@edk2.groups.io,
	Kinney, Michael D
  Cc: 'Andrew Fish', leif@nuviainc.com, Ni, Ray, Gao, Zhichao,
	Wang, Jian J, Wu, Hao A, Bi, Dandan, Dong, Eric,
	'Bret Barkelew', 'Vitaly Cheptsov'

Marvin,

The main reason most components in the EDK II repos continue to use the variables is 
because there are incomplete tools to generate PE/COFF resource sections for all
the OS/compiler combinations that EDK II supports.

The preference would be to use PE/COFF resource sections and we would have converted
all components to that style long ago if the tools existed to be aligned with the
UEFI Specification instead of an EDK II specific implementation feature.

Also, it is not a good idea to only look at the open source EDK II repos to
understand how a feature is used.  There are many downstream consumers of the
EDK II repos.

The UEFI Driver Writer's Guide *only* documents the PE/COFF resource section
method.

https://tianocore-docs.github.io/edk2-UefiDriverWritersGuide/draft/7_driver_entry_point/74_adding_hii_packages_feature.html#74-adding-hii-packages-feature

Best regards,

Mike

> -----Original Message-----
> From: Marvin Häuser <mhaeuser@posteo.de>
> Sent: Thursday, August 26, 2021 9:07 AM
> To: tim.lewis@insyde.com; devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: 'Andrew Fish' <afish@apple.com>; leif@nuviainc.com; Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>;
> Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Dong, Eric
> <eric.dong@intel.com>; 'Bret Barkelew' <Bret.Barkelew@microsoft.com>; 'Vitaly Cheptsov' <vit9696@protonmail.com>
> Subject: Re: [edk2-devel] [RFC] Expose HII package list via C variables
> 
> Hey Tim,
> 
> Interesting, do you happen to have some tool or code that performs such
> edits at hand to take a look at? Seems like most modules already use
> variables and thus cannot be modified in such a way even right now?
> 
> That's the kind of information I am looking for, thanks a lot!
> 
> Best regards,
> Marvin
> 
> On 26/08/2021 18:01, tim.lewis@insyde.com wrote:
> > Hi Marvin --
> >
> > I would like to add some historical perspective on this. One of the design requirements back when HII was first
> introduced into the UEFI specification after Intel's initial contribution was that of binary editability. In order to be
> able to reliably find, extract and then re-insert the HII data into the binary, it needed to be discoverable and not
> affect the offsets of the code and data in the binary.
> >
> > While it was possible to put some sort of signature in the read-only data sections of the binary and leave padding for
> possible future edits, it was felt that using a PE/COFF section similar to the resource sections was better. Resource
> sections are commonly in use for PE/COFF files (in Windows) and the similar idea existed in the then-extant Mac binary
> format (resource fork?).
> >
> > Thanks,
> >
> > Tim Lewis
> > CTO, Insyde Software
> > www.insyde.com
> >
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael D Kinney
> > Sent: Thursday, August 26, 2021 7:37 AM
> > To: devel@edk2.groups.io; mhaeuser@posteo.de; Kinney, Michael D <michael.d.kinney@intel.com>
> > Cc: Andrew Fish <afish@apple.com>; leif@nuviainc.com; Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>;
> Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Dong, Eric
> <eric.dong@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>; Vitaly Cheptsov <vit9696@protonmail.com>
> > Subject: Re: [edk2-devel] [RFC] Expose HII package list via C variables
> >
> > Marvin,
> >
> > One constraint in this discussion is that the HII content in a PE/COFF resource section is defined in the UEFI
> Specification, Which means UEFI Apps and UEFI Drivers from media and option ROMs that are not part of the system FW image
> are allowed to use this feature,  This means the system FW PE/COFF loader must support loading HII content from this
> PE/COFF resource section to be UEFI conformant.  So we cannot remove this feature from the PE/COFF loader without changes
> to the UEFI Specification.  Even if changes to the UEFI Specification we made, we would have to continue to support this
> feature for backward compatibility with existing UEFI Apps/Drivers that may be using this feature.
> >
> > Thanks,
> >
> > Mike
> >
> >> -----Original Message-----
> >> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Marvin
> >> Häuser
> >> Sent: Thursday, August 26, 2021 1:51 AM
> >> To: devel@edk2.groups.io; Kinney, Michael D
> >> <michael.d.kinney@intel.com>
> >> Cc: devel@edk2.groups.io; Andrew Fish <afish@apple.com>;
> >> leif@nuviainc.com; Ni, Ray <ray.ni@intel.com>; Gao, Zhichao
> >> <zhichao.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao
> >> A <hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Dong, Eric
> >> <eric.dong@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>;
> >> Vitaly Cheptsov <vit9696@protonmail.com>
> >> Subject: Re: [edk2-devel] [RFC] Expose HII package list via C
> >> variables
> >>
> >> Hey Mike,
> >>
> >> Thanks for your reply!
> >>
> >> Well, this switch is not well-documented. Looking now at both the
> >> generation code and the emitted code, it does not generate a package
> >> list like my code does, but separate data variables (strings and
> >> images) that cannot easily be passed to HiiDatabase as-is. However
> >> apparently there are drivers that use this functionality successfully
> >> by composing the package list at runtime [1].
> >>
> >> Looking with this information now at the pattern of using HII C
> >> variables (which I did not know existed before) vs the PE/COFF HII
> >> section, most that use latter are Shell applications, which I guess
> >> means the section has actually been introduced to resolve D.? There
> >> are exceptions such as LogoDxe [2], which use the PE/COFF section while D.
> >> is not a problem, hence I got confused, sorry. I think these modules
> >> should be updated in any case. Do you agree?
> >>
> >> So, for modules that use C variables already, my patch would save some
> >> runtime generation code and dynamic memory allocation for the HII
> >> package list. This was not my goal (as I said, I didn't realise HII C
> >> variables already were a thing in the first place), but the changes
> >> are small enough that it might be worth considering anyway, in my opinion.
> >> If a HII package list is generated for both Shell and non-Shell apps,
> >> this also means code paths can be unified. For example, there could be
> >> a library class with constructor and destructor to add/remove packages
> >> from the HII database for all modules that use such, Shell or not. For
> >> BaseTools it means that there is no real need for separate Python and
> >> C paths as ideally they just generate the exact same data.
> >>
> >> Now to D., the only usage for this seems to be that Shell can locate
> >> the help text in the executable without executing it, yet it is fully
> >> loaded anyway [3]. To be honest, I find it hard to justify loading an
> >> executable (PE/COFF loading, memory permission application, the full
> >> process) to retrieve a help text and then unloading it again,
> >> especially with the HII code being on a core dispatcher level. 1. to
> >> 7. still hold true in my opinion. Was there any discussion I could
> >> read through why Shell apps cannot simply support a "--help" or "-?"
> >> command and output the string themselves? Pushing the burden to the
> >> Shell apps does preserve the "drawback" that a full loading is
> >> required (which honestly does not matter for a debugging application
> >> like Shell), however it does relieve the burden of PE/COFF HII parsing
> >> from the core dispatcher (which matters a lot in my opinion to keep
> >> the core simple). It would simply be a normal Shell app execution as
> >> any other however. If someone wants to avoid the PE/COFF burden
> >> altogether, they can still provide a .man file.
> >>
> >> As for my points 6. and 7., maybe I should provide some context. Due
> >> to many issues with TE files, platforms started abandoning them and
> >> returned to PE/COFF Images. I think a big reason for this is that TE
> >> is not really a sound and complete format, but a stripped version of
> >> PE/COFF with none of the necessary fixups applied. I'm currently
> >> sketching a possible alternative [4], and I would really like to not
> >> having to specify a HII section type, while still preserving
> >> compatibility with all of the UEFI Image types and use-cases [4].
> >>
> >> Thanks again!
> >>
> >> Best regards,
> >> Marvin
> >>
> >>
> >> [1]
> >> https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0ea
> >> b4ac688d5/MdeModulePkg/Application/BootManagerMenuAp
> >> p/BootManagerMenu.c#L929-L934
> >>
> >> [2]
> >> https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0ea
> >> b4ac688d5/MdeModulePkg/Logo/LogoDxe.inf#L23
> >>
> >> [3]
> >>
> https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0eab4ac688d5/ShellPkg/Application/Shell/ShellManParser.
> >> c#L646-L671
> >>
> >> [4]
> >> https://github.com/mhaeuser/edk2/blob/ue_poc/MdePkg/Include/IndustrySt
> >> andard/UeImage.h
> >>
> >> 26.08.2021 00:34:12 Michael D Kinney <michael.d.kinney@intel.com>:
> >>
> >>> Hi Marvin,
> >>>
> >>> I think this feature is already there and supported.
> >>>
> >>> HII can either be in a global variable or in a PE/COFF resource section.
> >>> The default is a global variable because HII was implemented before
> >>> the PE/COFF resource section feature was added to the UEFI Specification.
> >>>
> >>> There is an INF [Defines] section statement to enable the PE/COFF
> >>> section. See UefiHiiResource in the following link.
> >>>
> >>> https://tianocore-docs.github.io/edk2-InfSpecification/draft/3_edk_i
> >>> i_inf_file_format/34_[defines]_section.html#34-
> >> defines-section
> >>> How is your proposal different than this existing capability?
> >>>
> >>> Thanks,
> >>>
> >>> Mike
> >>>
> >>>> -----Original Message-----
> >>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> >>>> Marvin Häuser
> >>>> Sent: Wednesday, August 25, 2021 2:21 PM
> >>>> To: devel@edk2.groups.io
> >>>> Cc: Andrew Fish <afish@apple.com>; leif@nuviainc.com; Kinney,
> >>>> Michael D <michael.d.kinney@intel.com>; Ni, Ray <ray.ni@intel.com>;
> >>>> Gao, Zhichao <zhichao.gao@intel.com>; Wang, Jian J
> >>>> <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan
> >>>> <dandan.bi@intel.com>; Dong, Eric <eric.dong@intel.com>; Bret
> >>>> Barkelew <Bret.Barkelew@microsoft.com>; Vitaly Cheptsov
> >>>> <vit9696@protonmail.com>
> >>>> Subject: [edk2-devel] [RFC] Expose HII package list via C variables
> >>>>
> >>>> Good day everyone,
> >>>>
> >>>> Currently, the HII package list is stored in a PE/COFF resource
> >>>> section [1]. I propose to store it in a C variable (byte array with
> >>>> a pointer to it and its size exposed) instead. DxeCore would have a
> >>>> guard to toggle the deprecated support for the automatic protocol
> >>>> installation. This has the following advantages:
> >>>>
> >>>> 1. Fixes BZ (incl. future toolchains):
> >>>> https://bugzilla.tianocore.org/show_bug.cgi?id=557
> >>>> 2. Universal method across all toolchains and output file formats
> >>>> 3. Saves error-prone parsing work 4. Saves protocol install/locate
> >>>> work, the data is available right away 5. The omission of a
> >>>> dedicated section can save space 6. Terse file formats can support
> >>>> this and remain terse :) 7. Removes a dependency on the PE/COFF
> >>>> format specifically
> >>>>
> >>>> A *very rough* PoC diff can be found here:
> >>>> https://github.com/mhaeuser/edk2/compare/master...wip_hii_cvar
> >>>> If the feedback is positive, I will clean it up of course. OVMF
> >>>> boots with everything working fine.
> >>>>
> >>>> I'd explicitly like feedback on the following:
> >>>> A. Is this an acceptable solution to BZ 557 (Andrew?)?
> >>>> B. Is this an acceptable solution for the "HII workflow" (MdeModule
> >>>> maintainers?)?
> >>>> C. Is it acceptable to make support UEFI-side support for the old
> >>>> mechanism optional (Stewards?)?
> >>>> D. Can an acceptable alternative be found for the removed ShellPkg
> >>>> code (Shell maintainers?)?
> >>>>
> >>>> As you can see the BaseTools part also is rough, but that is more a
> >>>> question of "how" rather than "whether", so I'll postpone asking about it.
> >>>>
> >>>> Thanks for your time and feedback!
> >>>>
> >>>> Best regards,
> >>>> Marvin
> >>>>
> >>>>
> >>>> [1] "Once the image is loaded, LoadImage() installs
> >>>> EFI_HII_PACKAGE_LIST_PROTOCOL on the handle if the image contains a
> >>>> custom PE/COFF resource with the type 'HII'."
> >>>> - UEFI 2.9, 7.4, "EFI_BOOT_SERVICES.LoadImage()"
> >>>>
> >>>>
> >>>>
> >>>>
> >>>
> >>>
> >>>
> >>
> >>
> >>
> >
> >
> > 
> >
> >
> >


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [edk2-devel] [RFC] Expose HII package list via C variables
  2021-08-26 16:07         ` Marvin Häuser
  2021-08-26 16:44           ` Michael D Kinney
@ 2021-08-26 16:49           ` Samer El-Haj-Mahmoud
  2021-08-26 17:02             ` Marvin Häuser
  2021-08-26 17:07           ` Tim Lewis
  2 siblings, 1 reply; 17+ messages in thread
From: Samer El-Haj-Mahmoud @ 2021-08-26 16:49 UTC (permalink / raw)
  To: devel@edk2.groups.io, mhaeuser@posteo.de, tim.lewis@insyde.com,
	michael.d.kinney@intel.com
  Cc: 'Andrew Fish', leif@nuviainc.com, 'Ni, Ray',
	'Gao, Zhichao', 'Wang, Jian J',
	'Wu, Hao A', 'Bi, Dandan', 'Dong, Eric',
	'Bret Barkelew', 'Vitaly Cheptsov',
	Samer El-Haj-Mahmoud

I am aware of some proprietary tools (not open source) that depend on this functionality.

The feature has been used, especially on some server designs, to allow exporting HII packages and consuming them offline by out-of-band or OS-based management applications for instance. The need for this seems to be declining though, as implementations move to using standard Redfish data instead, which in turn can be produced from HII packages, either during boot (like what is done in EDK2 RedfishPkg), or at build / release time.

Thanks,
--Samer


> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Marvin
> Häuser via groups.io
> Sent: Thursday, August 26, 2021 12:07 PM
> To: tim.lewis@insyde.com; devel@edk2.groups.io;
> michael.d.kinney@intel.com
> Cc: 'Andrew Fish' <afish@apple.com>; leif@nuviainc.com; 'Ni, Ray'
> <ray.ni@intel.com>; 'Gao, Zhichao' <zhichao.gao@intel.com>; 'Wang, Jian J'
> <jian.j.wang@intel.com>; 'Wu, Hao A' <hao.a.wu@intel.com>; 'Bi, Dandan'
> <dandan.bi@intel.com>; 'Dong, Eric' <eric.dong@intel.com>; 'Bret Barkelew'
> <Bret.Barkelew@microsoft.com>; 'Vitaly Cheptsov'
> <vit9696@protonmail.com>
> Subject: Re: [edk2-devel] [RFC] Expose HII package list via C variables
>
> Hey Tim,
>
> Interesting, do you happen to have some tool or code that performs such
> edits at hand to take a look at? Seems like most modules already use
> variables and thus cannot be modified in such a way even right now?
>
> That's the kind of information I am looking for, thanks a lot!
>
> Best regards,
> Marvin
>
> On 26/08/2021 18:01, tim.lewis@insyde.com wrote:
> > Hi Marvin --
> >
> > I would like to add some historical perspective on this. One of the design
> requirements back when HII was first introduced into the UEFI specification
> after Intel's initial contribution was that of binary editability. In order to be
> able to reliably find, extract and then re-insert the HII data into the binary, it
> needed to be discoverable and not affect the offsets of the code and data in
> the binary.
> >
> > While it was possible to put some sort of signature in the read-only data
> sections of the binary and leave padding for possible future edits, it was felt
> that using a PE/COFF section similar to the resource sections was better.
> Resource sections are commonly in use for PE/COFF files (in Windows) and
> the similar idea existed in the then-extant Mac binary format (resource
> fork?).
> >
> > Thanks,
> >
> > Tim Lewis
> > CTO, Insyde Software
> > www.insyde.com
> >
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> Michael D Kinney
> > Sent: Thursday, August 26, 2021 7:37 AM
> > To: devel@edk2.groups.io; mhaeuser@posteo.de; Kinney, Michael D
> <michael.d.kinney@intel.com>
> > Cc: Andrew Fish <afish@apple.com>; leif@nuviainc.com; Ni, Ray
> <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Wang, Jian J
> <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan
> <dandan.bi@intel.com>; Dong, Eric <eric.dong@intel.com>; Bret Barkelew
> <Bret.Barkelew@microsoft.com>; Vitaly Cheptsov
> <vit9696@protonmail.com>
> > Subject: Re: [edk2-devel] [RFC] Expose HII package list via C variables
> >
> > Marvin,
> >
> > One constraint in this discussion is that the HII content in a PE/COFF
> resource section is defined in the UEFI Specification, Which means UEFI Apps
> and UEFI Drivers from media and option ROMs that are not part of the
> system FW image are allowed to use this feature,  This means the system FW
> PE/COFF loader must support loading HII content from this PE/COFF resource
> section to be UEFI conformant.  So we cannot remove this feature from the
> PE/COFF loader without changes to the UEFI Specification.  Even if changes to
> the UEFI Specification we made, we would have to continue to support this
> feature for backward compatibility with existing UEFI Apps/Drivers that may
> be using this feature.
> >
> > Thanks,
> >
> > Mike
> >
> >> -----Original Message-----
> >> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> Marvin
> >> Häuser
> >> Sent: Thursday, August 26, 2021 1:51 AM
> >> To: devel@edk2.groups.io; Kinney, Michael D
> >> <michael.d.kinney@intel.com>
> >> Cc: devel@edk2.groups.io; Andrew Fish <afish@apple.com>;
> >> leif@nuviainc.com; Ni, Ray <ray.ni@intel.com>; Gao, Zhichao
> >> <zhichao.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu,
> Hao
> >> A <hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Dong, Eric
> >> <eric.dong@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>;
> >> Vitaly Cheptsov <vit9696@protonmail.com>
> >> Subject: Re: [edk2-devel] [RFC] Expose HII package list via C
> >> variables
> >>
> >> Hey Mike,
> >>
> >> Thanks for your reply!
> >>
> >> Well, this switch is not well-documented. Looking now at both the
> >> generation code and the emitted code, it does not generate a package
> >> list like my code does, but separate data variables (strings and
> >> images) that cannot easily be passed to HiiDatabase as-is. However
> >> apparently there are drivers that use this functionality successfully
> >> by composing the package list at runtime [1].
> >>
> >> Looking with this information now at the pattern of using HII C
> >> variables (which I did not know existed before) vs the PE/COFF HII
> >> section, most that use latter are Shell applications, which I guess
> >> means the section has actually been introduced to resolve D.? There
> >> are exceptions such as LogoDxe [2], which use the PE/COFF section while
> D.
> >> is not a problem, hence I got confused, sorry. I think these modules
> >> should be updated in any case. Do you agree?
> >>
> >> So, for modules that use C variables already, my patch would save some
> >> runtime generation code and dynamic memory allocation for the HII
> >> package list. This was not my goal (as I said, I didn't realise HII C
> >> variables already were a thing in the first place), but the changes
> >> are small enough that it might be worth considering anyway, in my
> opinion.
> >> If a HII package list is generated for both Shell and non-Shell apps,
> >> this also means code paths can be unified. For example, there could be
> >> a library class with constructor and destructor to add/remove packages
> >> from the HII database for all modules that use such, Shell or not. For
> >> BaseTools it means that there is no real need for separate Python and
> >> C paths as ideally they just generate the exact same data.
> >>
> >> Now to D., the only usage for this seems to be that Shell can locate
> >> the help text in the executable without executing it, yet it is fully
> >> loaded anyway [3]. To be honest, I find it hard to justify loading an
> >> executable (PE/COFF loading, memory permission application, the full
> >> process) to retrieve a help text and then unloading it again,
> >> especially with the HII code being on a core dispatcher level. 1. to
> >> 7. still hold true in my opinion. Was there any discussion I could
> >> read through why Shell apps cannot simply support a "--help" or "-?"
> >> command and output the string themselves? Pushing the burden to the
> >> Shell apps does preserve the "drawback" that a full loading is
> >> required (which honestly does not matter for a debugging application
> >> like Shell), however it does relieve the burden of PE/COFF HII parsing
> >> from the core dispatcher (which matters a lot in my opinion to keep
> >> the core simple). It would simply be a normal Shell app execution as
> >> any other however. If someone wants to avoid the PE/COFF burden
> >> altogether, they can still provide a .man file.
> >>
> >> As for my points 6. and 7., maybe I should provide some context. Due
> >> to many issues with TE files, platforms started abandoning them and
> >> returned to PE/COFF Images. I think a big reason for this is that TE
> >> is not really a sound and complete format, but a stripped version of
> >> PE/COFF with none of the necessary fixups applied. I'm currently
> >> sketching a possible alternative [4], and I would really like to not
> >> having to specify a HII section type, while still preserving
> >> compatibility with all of the UEFI Image types and use-cases [4].
> >>
> >> Thanks again!
> >>
> >> Best regards,
> >> Marvin
> >>
> >>
> >> [1]
> >>
> https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0e
> a
> >> b4ac688d5/MdeModulePkg/Application/BootManagerMenuAp
> >> p/BootManagerMenu.c#L929-L934
> >>
> >> [2]
> >>
> https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0e
> a
> >> b4ac688d5/MdeModulePkg/Logo/LogoDxe.inf#L23
> >>
> >> [3]
> >>
> https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0e
> ab4ac688d5/ShellPkg/Application/Shell/ShellManParser.
> >> c#L646-L671
> >>
> >> [4]
> >>
> https://github.com/mhaeuser/edk2/blob/ue_poc/MdePkg/Include/Industr
> ySt
> >> andard/UeImage.h
> >>
> >> 26.08.2021 00:34:12 Michael D Kinney <michael.d.kinney@intel.com>:
> >>
> >>> Hi Marvin,
> >>>
> >>> I think this feature is already there and supported.
> >>>
> >>> HII can either be in a global variable or in a PE/COFF resource section.
> >>> The default is a global variable because HII was implemented before
> >>> the PE/COFF resource section feature was added to the UEFI
> Specification.
> >>>
> >>> There is an INF [Defines] section statement to enable the PE/COFF
> >>> section. See UefiHiiResource in the following link.
> >>>
> >>> https://tianocore-docs.github.io/edk2-InfSpecification/draft/3_edk_i
> >>> i_inf_file_format/34_[defines]_section.html#34-
> >> defines-section
> >>> How is your proposal different than this existing capability?
> >>>
> >>> Thanks,
> >>>
> >>> Mike
> >>>
> >>>> -----Original Message-----
> >>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> >>>> Marvin Häuser
> >>>> Sent: Wednesday, August 25, 2021 2:21 PM
> >>>> To: devel@edk2.groups.io
> >>>> Cc: Andrew Fish <afish@apple.com>; leif@nuviainc.com; Kinney,
> >>>> Michael D <michael.d.kinney@intel.com>; Ni, Ray <ray.ni@intel.com>;
> >>>> Gao, Zhichao <zhichao.gao@intel.com>; Wang, Jian J
> >>>> <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi,
> Dandan
> >>>> <dandan.bi@intel.com>; Dong, Eric <eric.dong@intel.com>; Bret
> >>>> Barkelew <Bret.Barkelew@microsoft.com>; Vitaly Cheptsov
> >>>> <vit9696@protonmail.com>
> >>>> Subject: [edk2-devel] [RFC] Expose HII package list via C variables
> >>>>
> >>>> Good day everyone,
> >>>>
> >>>> Currently, the HII package list is stored in a PE/COFF resource
> >>>> section [1]. I propose to store it in a C variable (byte array with
> >>>> a pointer to it and its size exposed) instead. DxeCore would have a
> >>>> guard to toggle the deprecated support for the automatic protocol
> >>>> installation. This has the following advantages:
> >>>>
> >>>> 1. Fixes BZ (incl. future toolchains):
> >>>> https://bugzilla.tianocore.org/show_bug.cgi?id=557
> >>>> 2. Universal method across all toolchains and output file formats
> >>>> 3. Saves error-prone parsing work 4. Saves protocol install/locate
> >>>> work, the data is available right away 5. The omission of a
> >>>> dedicated section can save space 6. Terse file formats can support
> >>>> this and remain terse :) 7. Removes a dependency on the PE/COFF
> >>>> format specifically
> >>>>
> >>>> A *very rough* PoC diff can be found here:
> >>>> https://github.com/mhaeuser/edk2/compare/master...wip_hii_cvar
> >>>> If the feedback is positive, I will clean it up of course. OVMF
> >>>> boots with everything working fine.
> >>>>
> >>>> I'd explicitly like feedback on the following:
> >>>> A. Is this an acceptable solution to BZ 557 (Andrew?)?
> >>>> B. Is this an acceptable solution for the "HII workflow" (MdeModule
> >>>> maintainers?)?
> >>>> C. Is it acceptable to make support UEFI-side support for the old
> >>>> mechanism optional (Stewards?)?
> >>>> D. Can an acceptable alternative be found for the removed ShellPkg
> >>>> code (Shell maintainers?)?
> >>>>
> >>>> As you can see the BaseTools part also is rough, but that is more a
> >>>> question of "how" rather than "whether", so I'll postpone asking about
> it.
> >>>>
> >>>> Thanks for your time and feedback!
> >>>>
> >>>> Best regards,
> >>>> Marvin
> >>>>
> >>>>
> >>>> [1] "Once the image is loaded, LoadImage() installs
> >>>> EFI_HII_PACKAGE_LIST_PROTOCOL on the handle if the image contains
> a
> >>>> custom PE/COFF resource with the type 'HII'."
> >>>> - UEFI 2.9, 7.4, "EFI_BOOT_SERVICES.LoadImage()"
> >>>>
> >>>>
> >>>>
> >>>>
> >>>
> >>>
> >>>
> >>
> >>
> >>
> >
> >
> >
> >
> >
> >
>
>
>
> 
>

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [edk2-devel] [RFC] Expose HII package list via C variables
  2021-08-26 16:49           ` Samer El-Haj-Mahmoud
@ 2021-08-26 17:02             ` Marvin Häuser
  0 siblings, 0 replies; 17+ messages in thread
From: Marvin Häuser @ 2021-08-26 17:02 UTC (permalink / raw)
  To: Samer El-Haj-Mahmoud, devel@edk2.groups.io, tim.lewis@insyde.com,
	michael.d.kinney@intel.com
  Cc: 'Andrew Fish', leif@nuviainc.com, 'Ni, Ray',
	'Gao, Zhichao', 'Wang, Jian J',
	'Wu, Hao A', 'Bi, Dandan', 'Dong, Eric',
	'Bret Barkelew', 'Vitaly Cheptsov'

Good day Samer,

Thanks a lot for your reply!

On 26/08/2021 18:49, Samer El-Haj-Mahmoud wrote:
> I am aware of some proprietary tools (not open source) that depend on this functionality.
>
> The feature has been used, especially on some server designs, to allow exporting HII packages and consuming them offline by out-of-band or OS-based management applications for instance.

OS-based, so we are talking about DXE Runtime Drivers with HII support? 
Can you share how they retrieve the data, i.e. do they parse the PE/COFF 
header at OS runtime, or is there maybe some OS loader support that 
collects all installed packages before kernel handover? As I mentioned, 
part of the rationale is to support formats other than PE/COFF 
(especially a new terse format), so the former mechanisms would break 
for the long-term future no matter what (if adoption is progressing of 
course). In the latter case, functionality should not really be affected 
I believe, or am I missing something?

The goal is not only to reduce core dispatcher parsing work, but also to 
abstract away file format specifics with UEFI concepts (e.g. I replaced 
all parsing of PE/COFF to retrieve the PDB with a modification of the 
Debug Image Info Table to store the PDB file path). Publishing to the OS 
could work with some OS-proprietary mechanism, or maybe with the help of 
the SystemTable ConfigTable?

> The need for this seems to be declining though, as implementations move to using standard Redfish data instead, which in turn can be produced from HII packages, either during boot (like what is done in EDK2 RedfishPkg), or at build / release time.

How is this data retrieved then, some dynamic database the OS loader 
passes along?

Thanks!

Best regards,
Marvin

>
> Thanks,
> --Samer
>
>
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Marvin
>> Häuser via groups.io
>> Sent: Thursday, August 26, 2021 12:07 PM
>> To: tim.lewis@insyde.com; devel@edk2.groups.io;
>> michael.d.kinney@intel.com
>> Cc: 'Andrew Fish' <afish@apple.com>; leif@nuviainc.com; 'Ni, Ray'
>> <ray.ni@intel.com>; 'Gao, Zhichao' <zhichao.gao@intel.com>; 'Wang, Jian J'
>> <jian.j.wang@intel.com>; 'Wu, Hao A' <hao.a.wu@intel.com>; 'Bi, Dandan'
>> <dandan.bi@intel.com>; 'Dong, Eric' <eric.dong@intel.com>; 'Bret Barkelew'
>> <Bret.Barkelew@microsoft.com>; 'Vitaly Cheptsov'
>> <vit9696@protonmail.com>
>> Subject: Re: [edk2-devel] [RFC] Expose HII package list via C variables
>>
>> Hey Tim,
>>
>> Interesting, do you happen to have some tool or code that performs such
>> edits at hand to take a look at? Seems like most modules already use
>> variables and thus cannot be modified in such a way even right now?
>>
>> That's the kind of information I am looking for, thanks a lot!
>>
>> Best regards,
>> Marvin
>>
>> On 26/08/2021 18:01, tim.lewis@insyde.com wrote:
>>> Hi Marvin --
>>>
>>> I would like to add some historical perspective on this. One of the design
>> requirements back when HII was first introduced into the UEFI specification
>> after Intel's initial contribution was that of binary editability. In order to be
>> able to reliably find, extract and then re-insert the HII data into the binary, it
>> needed to be discoverable and not affect the offsets of the code and data in
>> the binary.
>>> While it was possible to put some sort of signature in the read-only data
>> sections of the binary and leave padding for possible future edits, it was felt
>> that using a PE/COFF section similar to the resource sections was better.
>> Resource sections are commonly in use for PE/COFF files (in Windows) and
>> the similar idea existed in the then-extant Mac binary format (resource
>> fork?).
>>> Thanks,
>>>
>>> Tim Lewis
>>> CTO, Insyde Software
>>> www.insyde.com
>>>
>>> -----Original Message-----
>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
>> Michael D Kinney
>>> Sent: Thursday, August 26, 2021 7:37 AM
>>> To: devel@edk2.groups.io; mhaeuser@posteo.de; Kinney, Michael D
>> <michael.d.kinney@intel.com>
>>> Cc: Andrew Fish <afish@apple.com>; leif@nuviainc.com; Ni, Ray
>> <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Wang, Jian J
>> <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan
>> <dandan.bi@intel.com>; Dong, Eric <eric.dong@intel.com>; Bret Barkelew
>> <Bret.Barkelew@microsoft.com>; Vitaly Cheptsov
>> <vit9696@protonmail.com>
>>> Subject: Re: [edk2-devel] [RFC] Expose HII package list via C variables
>>>
>>> Marvin,
>>>
>>> One constraint in this discussion is that the HII content in a PE/COFF
>> resource section is defined in the UEFI Specification, Which means UEFI Apps
>> and UEFI Drivers from media and option ROMs that are not part of the
>> system FW image are allowed to use this feature,  This means the system FW
>> PE/COFF loader must support loading HII content from this PE/COFF resource
>> section to be UEFI conformant.  So we cannot remove this feature from the
>> PE/COFF loader without changes to the UEFI Specification.  Even if changes to
>> the UEFI Specification we made, we would have to continue to support this
>> feature for backward compatibility with existing UEFI Apps/Drivers that may
>> be using this feature.
>>> Thanks,
>>>
>>> Mike
>>>
>>>> -----Original Message-----
>>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
>> Marvin
>>>> Häuser
>>>> Sent: Thursday, August 26, 2021 1:51 AM
>>>> To: devel@edk2.groups.io; Kinney, Michael D
>>>> <michael.d.kinney@intel.com>
>>>> Cc: devel@edk2.groups.io; Andrew Fish <afish@apple.com>;
>>>> leif@nuviainc.com; Ni, Ray <ray.ni@intel.com>; Gao, Zhichao
>>>> <zhichao.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu,
>> Hao
>>>> A <hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Dong, Eric
>>>> <eric.dong@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>;
>>>> Vitaly Cheptsov <vit9696@protonmail.com>
>>>> Subject: Re: [edk2-devel] [RFC] Expose HII package list via C
>>>> variables
>>>>
>>>> Hey Mike,
>>>>
>>>> Thanks for your reply!
>>>>
>>>> Well, this switch is not well-documented. Looking now at both the
>>>> generation code and the emitted code, it does not generate a package
>>>> list like my code does, but separate data variables (strings and
>>>> images) that cannot easily be passed to HiiDatabase as-is. However
>>>> apparently there are drivers that use this functionality successfully
>>>> by composing the package list at runtime [1].
>>>>
>>>> Looking with this information now at the pattern of using HII C
>>>> variables (which I did not know existed before) vs the PE/COFF HII
>>>> section, most that use latter are Shell applications, which I guess
>>>> means the section has actually been introduced to resolve D.? There
>>>> are exceptions such as LogoDxe [2], which use the PE/COFF section while
>> D.
>>>> is not a problem, hence I got confused, sorry. I think these modules
>>>> should be updated in any case. Do you agree?
>>>>
>>>> So, for modules that use C variables already, my patch would save some
>>>> runtime generation code and dynamic memory allocation for the HII
>>>> package list. This was not my goal (as I said, I didn't realise HII C
>>>> variables already were a thing in the first place), but the changes
>>>> are small enough that it might be worth considering anyway, in my
>> opinion.
>>>> If a HII package list is generated for both Shell and non-Shell apps,
>>>> this also means code paths can be unified. For example, there could be
>>>> a library class with constructor and destructor to add/remove packages
>>>> from the HII database for all modules that use such, Shell or not. For
>>>> BaseTools it means that there is no real need for separate Python and
>>>> C paths as ideally they just generate the exact same data.
>>>>
>>>> Now to D., the only usage for this seems to be that Shell can locate
>>>> the help text in the executable without executing it, yet it is fully
>>>> loaded anyway [3]. To be honest, I find it hard to justify loading an
>>>> executable (PE/COFF loading, memory permission application, the full
>>>> process) to retrieve a help text and then unloading it again,
>>>> especially with the HII code being on a core dispatcher level. 1. to
>>>> 7. still hold true in my opinion. Was there any discussion I could
>>>> read through why Shell apps cannot simply support a "--help" or "-?"
>>>> command and output the string themselves? Pushing the burden to the
>>>> Shell apps does preserve the "drawback" that a full loading is
>>>> required (which honestly does not matter for a debugging application
>>>> like Shell), however it does relieve the burden of PE/COFF HII parsing
>>>> from the core dispatcher (which matters a lot in my opinion to keep
>>>> the core simple). It would simply be a normal Shell app execution as
>>>> any other however. If someone wants to avoid the PE/COFF burden
>>>> altogether, they can still provide a .man file.
>>>>
>>>> As for my points 6. and 7., maybe I should provide some context. Due
>>>> to many issues with TE files, platforms started abandoning them and
>>>> returned to PE/COFF Images. I think a big reason for this is that TE
>>>> is not really a sound and complete format, but a stripped version of
>>>> PE/COFF with none of the necessary fixups applied. I'm currently
>>>> sketching a possible alternative [4], and I would really like to not
>>>> having to specify a HII section type, while still preserving
>>>> compatibility with all of the UEFI Image types and use-cases [4].
>>>>
>>>> Thanks again!
>>>>
>>>> Best regards,
>>>> Marvin
>>>>
>>>>
>>>> [1]
>>>>
>> https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0e
>> a
>>>> b4ac688d5/MdeModulePkg/Application/BootManagerMenuAp
>>>> p/BootManagerMenu.c#L929-L934
>>>>
>>>> [2]
>>>>
>> https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0e
>> a
>>>> b4ac688d5/MdeModulePkg/Logo/LogoDxe.inf#L23
>>>>
>>>> [3]
>>>>
>> https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0e
>> ab4ac688d5/ShellPkg/Application/Shell/ShellManParser.
>>>> c#L646-L671
>>>>
>>>> [4]
>>>>
>> https://github.com/mhaeuser/edk2/blob/ue_poc/MdePkg/Include/Industr
>> ySt
>>>> andard/UeImage.h
>>>>
>>>> 26.08.2021 00:34:12 Michael D Kinney <michael.d.kinney@intel.com>:
>>>>
>>>>> Hi Marvin,
>>>>>
>>>>> I think this feature is already there and supported.
>>>>>
>>>>> HII can either be in a global variable or in a PE/COFF resource section.
>>>>> The default is a global variable because HII was implemented before
>>>>> the PE/COFF resource section feature was added to the UEFI
>> Specification.
>>>>> There is an INF [Defines] section statement to enable the PE/COFF
>>>>> section. See UefiHiiResource in the following link.
>>>>>
>>>>> https://tianocore-docs.github.io/edk2-InfSpecification/draft/3_edk_i
>>>>> i_inf_file_format/34_[defines]_section.html#34-
>>>> defines-section
>>>>> How is your proposal different than this existing capability?
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Mike
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
>>>>>> Marvin Häuser
>>>>>> Sent: Wednesday, August 25, 2021 2:21 PM
>>>>>> To: devel@edk2.groups.io
>>>>>> Cc: Andrew Fish <afish@apple.com>; leif@nuviainc.com; Kinney,
>>>>>> Michael D <michael.d.kinney@intel.com>; Ni, Ray <ray.ni@intel.com>;
>>>>>> Gao, Zhichao <zhichao.gao@intel.com>; Wang, Jian J
>>>>>> <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi,
>> Dandan
>>>>>> <dandan.bi@intel.com>; Dong, Eric <eric.dong@intel.com>; Bret
>>>>>> Barkelew <Bret.Barkelew@microsoft.com>; Vitaly Cheptsov
>>>>>> <vit9696@protonmail.com>
>>>>>> Subject: [edk2-devel] [RFC] Expose HII package list via C variables
>>>>>>
>>>>>> Good day everyone,
>>>>>>
>>>>>> Currently, the HII package list is stored in a PE/COFF resource
>>>>>> section [1]. I propose to store it in a C variable (byte array with
>>>>>> a pointer to it and its size exposed) instead. DxeCore would have a
>>>>>> guard to toggle the deprecated support for the automatic protocol
>>>>>> installation. This has the following advantages:
>>>>>>
>>>>>> 1. Fixes BZ (incl. future toolchains):
>>>>>> https://bugzilla.tianocore.org/show_bug.cgi?id=557
>>>>>> 2. Universal method across all toolchains and output file formats
>>>>>> 3. Saves error-prone parsing work 4. Saves protocol install/locate
>>>>>> work, the data is available right away 5. The omission of a
>>>>>> dedicated section can save space 6. Terse file formats can support
>>>>>> this and remain terse :) 7. Removes a dependency on the PE/COFF
>>>>>> format specifically
>>>>>>
>>>>>> A *very rough* PoC diff can be found here:
>>>>>> https://github.com/mhaeuser/edk2/compare/master...wip_hii_cvar
>>>>>> If the feedback is positive, I will clean it up of course. OVMF
>>>>>> boots with everything working fine.
>>>>>>
>>>>>> I'd explicitly like feedback on the following:
>>>>>> A. Is this an acceptable solution to BZ 557 (Andrew?)?
>>>>>> B. Is this an acceptable solution for the "HII workflow" (MdeModule
>>>>>> maintainers?)?
>>>>>> C. Is it acceptable to make support UEFI-side support for the old
>>>>>> mechanism optional (Stewards?)?
>>>>>> D. Can an acceptable alternative be found for the removed ShellPkg
>>>>>> code (Shell maintainers?)?
>>>>>>
>>>>>> As you can see the BaseTools part also is rough, but that is more a
>>>>>> question of "how" rather than "whether", so I'll postpone asking about
>> it.
>>>>>> Thanks for your time and feedback!
>>>>>>
>>>>>> Best regards,
>>>>>> Marvin
>>>>>>
>>>>>>
>>>>>> [1] "Once the image is loaded, LoadImage() installs
>>>>>> EFI_HII_PACKAGE_LIST_PROTOCOL on the handle if the image contains
>> a
>>>>>> custom PE/COFF resource with the type 'HII'."
>>>>>> - UEFI 2.9, 7.4, "EFI_BOOT_SERVICES.LoadImage()"
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>>>
>>>
>>>
>>
>>
>> 
>>
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [edk2-devel] [RFC] Expose HII package list via C variables
  2021-08-26 16:07         ` Marvin Häuser
  2021-08-26 16:44           ` Michael D Kinney
  2021-08-26 16:49           ` Samer El-Haj-Mahmoud
@ 2021-08-26 17:07           ` Tim Lewis
  2 siblings, 0 replies; 17+ messages in thread
From: Tim Lewis @ 2021-08-26 17:07 UTC (permalink / raw)
  To: 'Marvin Häuser', devel, michael.d.kinney
  Cc: 'Andrew Fish', leif, 'Ni, Ray',
	'Gao, Zhichao', 'Wang, Jian J',
	'Wu, Hao A', 'Bi, Dandan', 'Dong, Eric',
	'Bret Barkelew', 'Vitaly Cheptsov'

Hi Marvin --

Sorry, nothing I can share. Thanks,

Tim

-----Original Message-----
From: Marvin Häuser <mhaeuser@posteo.de> 
Sent: Thursday, August 26, 2021 9:07 AM
To: tim.lewis@insyde.com; devel@edk2.groups.io; michael.d.kinney@intel.com
Cc: 'Andrew Fish' <afish@apple.com>; leif@nuviainc.com; 'Ni, Ray' <ray.ni@intel.com>; 'Gao, Zhichao' <zhichao.gao@intel.com>; 'Wang, Jian J' <jian.j.wang@intel.com>; 'Wu, Hao A' <hao.a.wu@intel.com>; 'Bi, Dandan' <dandan.bi@intel.com>; 'Dong, Eric' <eric.dong@intel.com>; 'Bret Barkelew' <Bret.Barkelew@microsoft.com>; 'Vitaly Cheptsov' <vit9696@protonmail.com>
Subject: Re: [edk2-devel] [RFC] Expose HII package list via C variables

Hey Tim,

Interesting, do you happen to have some tool or code that performs such edits at hand to take a look at? Seems like most modules already use variables and thus cannot be modified in such a way even right now?

That's the kind of information I am looking for, thanks a lot!

Best regards,
Marvin

On 26/08/2021 18:01, tim.lewis@insyde.com wrote:
> Hi Marvin --
>
> I would like to add some historical perspective on this. One of the design requirements back when HII was first introduced into the UEFI specification after Intel's initial contribution was that of binary editability. In order to be able to reliably find, extract and then re-insert the HII data into the binary, it needed to be discoverable and not affect the offsets of the code and data in the binary.
>
> While it was possible to put some sort of signature in the read-only data sections of the binary and leave padding for possible future edits, it was felt that using a PE/COFF section similar to the resource sections was better. Resource sections are commonly in use for PE/COFF files (in Windows) and the similar idea existed in the then-extant Mac binary format (resource fork?).
>
> Thanks,
>
> Tim Lewis
> CTO, Insyde Software
> www.insyde.com
>
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael 
> D Kinney
> Sent: Thursday, August 26, 2021 7:37 AM
> To: devel@edk2.groups.io; mhaeuser@posteo.de; Kinney, Michael D 
> <michael.d.kinney@intel.com>
> Cc: Andrew Fish <afish@apple.com>; leif@nuviainc.com; Ni, Ray 
> <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Wang, Jian J 
> <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan 
> <dandan.bi@intel.com>; Dong, Eric <eric.dong@intel.com>; Bret Barkelew 
> <Bret.Barkelew@microsoft.com>; Vitaly Cheptsov 
> <vit9696@protonmail.com>
> Subject: Re: [edk2-devel] [RFC] Expose HII package list via C 
> variables
>
> Marvin,
>
> One constraint in this discussion is that the HII content in a PE/COFF resource section is defined in the UEFI Specification, Which means UEFI Apps and UEFI Drivers from media and option ROMs that are not part of the system FW image are allowed to use this feature,  This means the system FW PE/COFF loader must support loading HII content from this PE/COFF resource section to be UEFI conformant.  So we cannot remove this feature from the PE/COFF loader without changes to the UEFI Specification.  Even if changes to the UEFI Specification we made, we would have to continue to support this feature for backward compatibility with existing UEFI Apps/Drivers that may be using this feature.
>
> Thanks,
>
> Mike
>
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Marvin 
>> Häuser
>> Sent: Thursday, August 26, 2021 1:51 AM
>> To: devel@edk2.groups.io; Kinney, Michael D 
>> <michael.d.kinney@intel.com>
>> Cc: devel@edk2.groups.io; Andrew Fish <afish@apple.com>; 
>> leif@nuviainc.com; Ni, Ray <ray.ni@intel.com>; Gao, Zhichao 
>> <zhichao.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, 
>> Hao A <hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Dong, 
>> Eric <eric.dong@intel.com>; Bret Barkelew 
>> <Bret.Barkelew@microsoft.com>; Vitaly Cheptsov 
>> <vit9696@protonmail.com>
>> Subject: Re: [edk2-devel] [RFC] Expose HII package list via C 
>> variables
>>
>> Hey Mike,
>>
>> Thanks for your reply!
>>
>> Well, this switch is not well-documented. Looking now at both the 
>> generation code and the emitted code, it does not generate a package 
>> list like my code does, but separate data variables (strings and
>> images) that cannot easily be passed to HiiDatabase as-is. However 
>> apparently there are drivers that use this functionality successfully 
>> by composing the package list at runtime [1].
>>
>> Looking with this information now at the pattern of using HII C 
>> variables (which I did not know existed before) vs the PE/COFF HII 
>> section, most that use latter are Shell applications, which I guess 
>> means the section has actually been introduced to resolve D.? There 
>> are exceptions such as LogoDxe [2], which use the PE/COFF section while D.
>> is not a problem, hence I got confused, sorry. I think these modules 
>> should be updated in any case. Do you agree?
>>
>> So, for modules that use C variables already, my patch would save 
>> some runtime generation code and dynamic memory allocation for the 
>> HII package list. This was not my goal (as I said, I didn't realise 
>> HII C variables already were a thing in the first place), but the 
>> changes are small enough that it might be worth considering anyway, in my opinion.
>> If a HII package list is generated for both Shell and non-Shell apps, 
>> this also means code paths can be unified. For example, there could 
>> be a library class with constructor and destructor to add/remove 
>> packages from the HII database for all modules that use such, Shell 
>> or not. For BaseTools it means that there is no real need for 
>> separate Python and C paths as ideally they just generate the exact same data.
>>
>> Now to D., the only usage for this seems to be that Shell can locate 
>> the help text in the executable without executing it, yet it is fully 
>> loaded anyway [3]. To be honest, I find it hard to justify loading an 
>> executable (PE/COFF loading, memory permission application, the full
>> process) to retrieve a help text and then unloading it again, 
>> especially with the HII code being on a core dispatcher level. 1. to 
>> 7. still hold true in my opinion. Was there any discussion I could 
>> read through why Shell apps cannot simply support a "--help" or "-?"
>> command and output the string themselves? Pushing the burden to the 
>> Shell apps does preserve the "drawback" that a full loading is 
>> required (which honestly does not matter for a debugging application 
>> like Shell), however it does relieve the burden of PE/COFF HII 
>> parsing from the core dispatcher (which matters a lot in my opinion 
>> to keep the core simple). It would simply be a normal Shell app 
>> execution as any other however. If someone wants to avoid the PE/COFF 
>> burden altogether, they can still provide a .man file.
>>
>> As for my points 6. and 7., maybe I should provide some context. Due 
>> to many issues with TE files, platforms started abandoning them and 
>> returned to PE/COFF Images. I think a big reason for this is that TE 
>> is not really a sound and complete format, but a stripped version of 
>> PE/COFF with none of the necessary fixups applied. I'm currently 
>> sketching a possible alternative [4], and I would really like to not 
>> having to specify a HII section type, while still preserving 
>> compatibility with all of the UEFI Image types and use-cases [4].
>>
>> Thanks again!
>>
>> Best regards,
>> Marvin
>>
>>
>> [1]
>> https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0e
>> a b4ac688d5/MdeModulePkg/Application/BootManagerMenuAp
>> p/BootManagerMenu.c#L929-L934
>>
>> [2]
>> https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0e
>> a
>> b4ac688d5/MdeModulePkg/Logo/LogoDxe.inf#L23
>>
>> [3]
>> https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0eab4ac688d5/ShellPkg/Application/Shell/ShellManParser.
>> c#L646-L671
>>
>> [4]
>> https://github.com/mhaeuser/edk2/blob/ue_poc/MdePkg/Include/IndustryS
>> t
>> andard/UeImage.h
>>
>> 26.08.2021 00:34:12 Michael D Kinney <michael.d.kinney@intel.com>:
>>
>>> Hi Marvin,
>>>
>>> I think this feature is already there and supported.
>>>
>>> HII can either be in a global variable or in a PE/COFF resource section.
>>> The default is a global variable because HII was implemented before 
>>> the PE/COFF resource section feature was added to the UEFI Specification.
>>>
>>> There is an INF [Defines] section statement to enable the PE/COFF 
>>> section. See UefiHiiResource in the following link.
>>>
>>> https://tianocore-docs.github.io/edk2-InfSpecification/draft/3_edk_i
>>> i_inf_file_format/34_[defines]_section.html#34-
>> defines-section
>>> How is your proposal different than this existing capability?
>>>
>>> Thanks,
>>>
>>> Mike
>>>
>>>> -----Original Message-----
>>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of 
>>>> Marvin Häuser
>>>> Sent: Wednesday, August 25, 2021 2:21 PM
>>>> To: devel@edk2.groups.io
>>>> Cc: Andrew Fish <afish@apple.com>; leif@nuviainc.com; Kinney, 
>>>> Michael D <michael.d.kinney@intel.com>; Ni, Ray <ray.ni@intel.com>; 
>>>> Gao, Zhichao <zhichao.gao@intel.com>; Wang, Jian J 
>>>> <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan 
>>>> <dandan.bi@intel.com>; Dong, Eric <eric.dong@intel.com>; Bret 
>>>> Barkelew <Bret.Barkelew@microsoft.com>; Vitaly Cheptsov 
>>>> <vit9696@protonmail.com>
>>>> Subject: [edk2-devel] [RFC] Expose HII package list via C variables
>>>>
>>>> Good day everyone,
>>>>
>>>> Currently, the HII package list is stored in a PE/COFF resource 
>>>> section [1]. I propose to store it in a C variable (byte array with 
>>>> a pointer to it and its size exposed) instead. DxeCore would have a 
>>>> guard to toggle the deprecated support for the automatic protocol 
>>>> installation. This has the following advantages:
>>>>
>>>> 1. Fixes BZ (incl. future toolchains):
>>>> https://bugzilla.tianocore.org/show_bug.cgi?id=557
>>>> 2. Universal method across all toolchains and output file formats 
>>>> 3. Saves error-prone parsing work 4. Saves protocol install/locate 
>>>> work, the data is available right away 5. The omission of a 
>>>> dedicated section can save space 6. Terse file formats can support 
>>>> this and remain terse :) 7. Removes a dependency on the PE/COFF 
>>>> format specifically
>>>>
>>>> A *very rough* PoC diff can be found here:
>>>> https://github.com/mhaeuser/edk2/compare/master...wip_hii_cvar
>>>> If the feedback is positive, I will clean it up of course. OVMF 
>>>> boots with everything working fine.
>>>>
>>>> I'd explicitly like feedback on the following:
>>>> A. Is this an acceptable solution to BZ 557 (Andrew?)?
>>>> B. Is this an acceptable solution for the "HII workflow" (MdeModule 
>>>> maintainers?)?
>>>> C. Is it acceptable to make support UEFI-side support for the old 
>>>> mechanism optional (Stewards?)?
>>>> D. Can an acceptable alternative be found for the removed ShellPkg 
>>>> code (Shell maintainers?)?
>>>>
>>>> As you can see the BaseTools part also is rough, but that is more a 
>>>> question of "how" rather than "whether", so I'll postpone asking about it.
>>>>
>>>> Thanks for your time and feedback!
>>>>
>>>> Best regards,
>>>> Marvin
>>>>
>>>>
>>>> [1] "Once the image is loaded, LoadImage() installs 
>>>> EFI_HII_PACKAGE_LIST_PROTOCOL on the handle if the image contains a 
>>>> custom PE/COFF resource with the type 'HII'."
>>>> - UEFI 2.9, 7.4, "EFI_BOOT_SERVICES.LoadImage()"
>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>>
>>
>>
>>
>
>
> 
>
>
>



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [edk2-devel] [RFC] Expose HII package list via C variables
  2021-08-26 16:44           ` Michael D Kinney
@ 2021-08-26 17:29             ` Marvin Häuser
  2021-08-26 19:50             ` Andrew Fish
  1 sibling, 0 replies; 17+ messages in thread
From: Marvin Häuser @ 2021-08-26 17:29 UTC (permalink / raw)
  To: devel, michael.d.kinney, tim.lewis@insyde.com
  Cc: 'Andrew Fish', leif@nuviainc.com, Ni, Ray, Gao, Zhichao,
	Wang, Jian J, Wu, Hao A, Bi, Dandan, Dong, Eric,
	'Bret Barkelew', 'Vitaly Cheptsov'

Hey Mike,

On 26/08/2021 18:44, Michael D Kinney wrote:
> Marvin,
>
> The main reason most components in the EDK II repos continue to use the variables is
> because there are incomplete tools to generate PE/COFF resource sections for all
> the OS/compiler combinations that EDK II supports.
>
> The preference would be to use PE/COFF resource sections and we would have converted
> all components to that style long ago if the tools existed to be aligned with the
> UEFI Specification instead of an EDK II specific implementation feature.

If it's not broken... :)

> Also, it is not a good idea to only look at the open source EDK II repos to
> understand how a feature is used.  There are many downstream consumers of the
> EDK II repos.

Well, that is why it is a RFC. I find it unfortunate that there are many 
features present (e.g. fix-address loading) with no explanation of why 
they were introduced.

> The UEFI Driver Writer's Guide *only* documents the PE/COFF resource section
> method.
>
> https://tianocore-docs.github.io/edk2-UefiDriverWritersGuide/draft/7_driver_entry_point/74_adding_hii_packages_feature.html#74-adding-hii-packages-feature

Yes, that is part of the reason I did not know about the current 
variable feature.

Looking at the current feedback from others, there are (still) 
proprietary use-cases that cannot be updated easily. New proposal:
1. Import my new code to generate package lists into C variables
2. Remove the current HII variable code and update the modules accordingly
3. Introduce a FixedPcd to disable HII lookup in DxeCore like suggested, 
lookup enabled by default
4. Do not deprecate the PE/COFF section on spec-level, at least for now

1. + 2. gives us easy unified code paths, as the data will be equivalent 
for both methods, and there can be a library class with 
constructor/destructor to register the HII data with HiiDatabase, no 
matter whether a module uses PE/COFF or C HII information. The platform 
can then decide on its own whether to support the mechanism or not, and 
modules can easily be toggled between them. In my opinion, variables 
should be preferred unless there is a need for external parsing of the 
data. This is not really "EDK II vs spec", because this is not about an 
interface, but about private module data lookup.
3. Allows trees like Project Amaranth to disable the additional parsing 
at their own discretion. This technically does violate the spec, and 
should be documented accordingly, but if it is known ahead of time which 
software is run, this is a safe decision to make, and it reduces the 
attack surface.
4. If it will ever be deprecated, it can be a natural decision from 
shifted use-cases.

Comments? :)

Best regards,
Marvin

>
> Best regards,
>
> Mike
>
>> -----Original Message-----
>> From: Marvin Häuser <mhaeuser@posteo.de>
>> Sent: Thursday, August 26, 2021 9:07 AM
>> To: tim.lewis@insyde.com; devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
>> Cc: 'Andrew Fish' <afish@apple.com>; leif@nuviainc.com; Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>;
>> Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Dong, Eric
>> <eric.dong@intel.com>; 'Bret Barkelew' <Bret.Barkelew@microsoft.com>; 'Vitaly Cheptsov' <vit9696@protonmail.com>
>> Subject: Re: [edk2-devel] [RFC] Expose HII package list via C variables
>>
>> Hey Tim,
>>
>> Interesting, do you happen to have some tool or code that performs such
>> edits at hand to take a look at? Seems like most modules already use
>> variables and thus cannot be modified in such a way even right now?
>>
>> That's the kind of information I am looking for, thanks a lot!
>>
>> Best regards,
>> Marvin
>>
>> On 26/08/2021 18:01, tim.lewis@insyde.com wrote:
>>> Hi Marvin --
>>>
>>> I would like to add some historical perspective on this. One of the design requirements back when HII was first
>> introduced into the UEFI specification after Intel's initial contribution was that of binary editability. In order to be
>> able to reliably find, extract and then re-insert the HII data into the binary, it needed to be discoverable and not
>> affect the offsets of the code and data in the binary.
>>> While it was possible to put some sort of signature in the read-only data sections of the binary and leave padding for
>> possible future edits, it was felt that using a PE/COFF section similar to the resource sections was better. Resource
>> sections are commonly in use for PE/COFF files (in Windows) and the similar idea existed in the then-extant Mac binary
>> format (resource fork?).
>>> Thanks,
>>>
>>> Tim Lewis
>>> CTO, Insyde Software
>>> www.insyde.com
>>>
>>> -----Original Message-----
>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael D Kinney
>>> Sent: Thursday, August 26, 2021 7:37 AM
>>> To: devel@edk2.groups.io; mhaeuser@posteo.de; Kinney, Michael D <michael.d.kinney@intel.com>
>>> Cc: Andrew Fish <afish@apple.com>; leif@nuviainc.com; Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>;
>> Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Dong, Eric
>> <eric.dong@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>; Vitaly Cheptsov <vit9696@protonmail.com>
>>> Subject: Re: [edk2-devel] [RFC] Expose HII package list via C variables
>>>
>>> Marvin,
>>>
>>> One constraint in this discussion is that the HII content in a PE/COFF resource section is defined in the UEFI
>> Specification, Which means UEFI Apps and UEFI Drivers from media and option ROMs that are not part of the system FW image
>> are allowed to use this feature,  This means the system FW PE/COFF loader must support loading HII content from this
>> PE/COFF resource section to be UEFI conformant.  So we cannot remove this feature from the PE/COFF loader without changes
>> to the UEFI Specification.  Even if changes to the UEFI Specification we made, we would have to continue to support this
>> feature for backward compatibility with existing UEFI Apps/Drivers that may be using this feature.
>>> Thanks,
>>>
>>> Mike
>>>
>>>> -----Original Message-----
>>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Marvin
>>>> Häuser
>>>> Sent: Thursday, August 26, 2021 1:51 AM
>>>> To: devel@edk2.groups.io; Kinney, Michael D
>>>> <michael.d.kinney@intel.com>
>>>> Cc: devel@edk2.groups.io; Andrew Fish <afish@apple.com>;
>>>> leif@nuviainc.com; Ni, Ray <ray.ni@intel.com>; Gao, Zhichao
>>>> <zhichao.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao
>>>> A <hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Dong, Eric
>>>> <eric.dong@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>;
>>>> Vitaly Cheptsov <vit9696@protonmail.com>
>>>> Subject: Re: [edk2-devel] [RFC] Expose HII package list via C
>>>> variables
>>>>
>>>> Hey Mike,
>>>>
>>>> Thanks for your reply!
>>>>
>>>> Well, this switch is not well-documented. Looking now at both the
>>>> generation code and the emitted code, it does not generate a package
>>>> list like my code does, but separate data variables (strings and
>>>> images) that cannot easily be passed to HiiDatabase as-is. However
>>>> apparently there are drivers that use this functionality successfully
>>>> by composing the package list at runtime [1].
>>>>
>>>> Looking with this information now at the pattern of using HII C
>>>> variables (which I did not know existed before) vs the PE/COFF HII
>>>> section, most that use latter are Shell applications, which I guess
>>>> means the section has actually been introduced to resolve D.? There
>>>> are exceptions such as LogoDxe [2], which use the PE/COFF section while D.
>>>> is not a problem, hence I got confused, sorry. I think these modules
>>>> should be updated in any case. Do you agree?
>>>>
>>>> So, for modules that use C variables already, my patch would save some
>>>> runtime generation code and dynamic memory allocation for the HII
>>>> package list. This was not my goal (as I said, I didn't realise HII C
>>>> variables already were a thing in the first place), but the changes
>>>> are small enough that it might be worth considering anyway, in my opinion.
>>>> If a HII package list is generated for both Shell and non-Shell apps,
>>>> this also means code paths can be unified. For example, there could be
>>>> a library class with constructor and destructor to add/remove packages
>>>> from the HII database for all modules that use such, Shell or not. For
>>>> BaseTools it means that there is no real need for separate Python and
>>>> C paths as ideally they just generate the exact same data.
>>>>
>>>> Now to D., the only usage for this seems to be that Shell can locate
>>>> the help text in the executable without executing it, yet it is fully
>>>> loaded anyway [3]. To be honest, I find it hard to justify loading an
>>>> executable (PE/COFF loading, memory permission application, the full
>>>> process) to retrieve a help text and then unloading it again,
>>>> especially with the HII code being on a core dispatcher level. 1. to
>>>> 7. still hold true in my opinion. Was there any discussion I could
>>>> read through why Shell apps cannot simply support a "--help" or "-?"
>>>> command and output the string themselves? Pushing the burden to the
>>>> Shell apps does preserve the "drawback" that a full loading is
>>>> required (which honestly does not matter for a debugging application
>>>> like Shell), however it does relieve the burden of PE/COFF HII parsing
>>>> from the core dispatcher (which matters a lot in my opinion to keep
>>>> the core simple). It would simply be a normal Shell app execution as
>>>> any other however. If someone wants to avoid the PE/COFF burden
>>>> altogether, they can still provide a .man file.
>>>>
>>>> As for my points 6. and 7., maybe I should provide some context. Due
>>>> to many issues with TE files, platforms started abandoning them and
>>>> returned to PE/COFF Images. I think a big reason for this is that TE
>>>> is not really a sound and complete format, but a stripped version of
>>>> PE/COFF with none of the necessary fixups applied. I'm currently
>>>> sketching a possible alternative [4], and I would really like to not
>>>> having to specify a HII section type, while still preserving
>>>> compatibility with all of the UEFI Image types and use-cases [4].
>>>>
>>>> Thanks again!
>>>>
>>>> Best regards,
>>>> Marvin
>>>>
>>>>
>>>> [1]
>>>> https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0ea
>>>> b4ac688d5/MdeModulePkg/Application/BootManagerMenuAp
>>>> p/BootManagerMenu.c#L929-L934
>>>>
>>>> [2]
>>>> https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0ea
>>>> b4ac688d5/MdeModulePkg/Logo/LogoDxe.inf#L23
>>>>
>>>> [3]
>>>>
>> https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0eab4ac688d5/ShellPkg/Application/Shell/ShellManParser.
>>>> c#L646-L671
>>>>
>>>> [4]
>>>> https://github.com/mhaeuser/edk2/blob/ue_poc/MdePkg/Include/IndustrySt
>>>> andard/UeImage.h
>>>>
>>>> 26.08.2021 00:34:12 Michael D Kinney <michael.d.kinney@intel.com>:
>>>>
>>>>> Hi Marvin,
>>>>>
>>>>> I think this feature is already there and supported.
>>>>>
>>>>> HII can either be in a global variable or in a PE/COFF resource section.
>>>>> The default is a global variable because HII was implemented before
>>>>> the PE/COFF resource section feature was added to the UEFI Specification.
>>>>>
>>>>> There is an INF [Defines] section statement to enable the PE/COFF
>>>>> section. See UefiHiiResource in the following link.
>>>>>
>>>>> https://tianocore-docs.github.io/edk2-InfSpecification/draft/3_edk_i
>>>>> i_inf_file_format/34_[defines]_section.html#34-
>>>> defines-section
>>>>> How is your proposal different than this existing capability?
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Mike
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
>>>>>> Marvin Häuser
>>>>>> Sent: Wednesday, August 25, 2021 2:21 PM
>>>>>> To: devel@edk2.groups.io
>>>>>> Cc: Andrew Fish <afish@apple.com>; leif@nuviainc.com; Kinney,
>>>>>> Michael D <michael.d.kinney@intel.com>; Ni, Ray <ray.ni@intel.com>;
>>>>>> Gao, Zhichao <zhichao.gao@intel.com>; Wang, Jian J
>>>>>> <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan
>>>>>> <dandan.bi@intel.com>; Dong, Eric <eric.dong@intel.com>; Bret
>>>>>> Barkelew <Bret.Barkelew@microsoft.com>; Vitaly Cheptsov
>>>>>> <vit9696@protonmail.com>
>>>>>> Subject: [edk2-devel] [RFC] Expose HII package list via C variables
>>>>>>
>>>>>> Good day everyone,
>>>>>>
>>>>>> Currently, the HII package list is stored in a PE/COFF resource
>>>>>> section [1]. I propose to store it in a C variable (byte array with
>>>>>> a pointer to it and its size exposed) instead. DxeCore would have a
>>>>>> guard to toggle the deprecated support for the automatic protocol
>>>>>> installation. This has the following advantages:
>>>>>>
>>>>>> 1. Fixes BZ (incl. future toolchains):
>>>>>> https://bugzilla.tianocore.org/show_bug.cgi?id=557
>>>>>> 2. Universal method across all toolchains and output file formats
>>>>>> 3. Saves error-prone parsing work 4. Saves protocol install/locate
>>>>>> work, the data is available right away 5. The omission of a
>>>>>> dedicated section can save space 6. Terse file formats can support
>>>>>> this and remain terse :) 7. Removes a dependency on the PE/COFF
>>>>>> format specifically
>>>>>>
>>>>>> A *very rough* PoC diff can be found here:
>>>>>> https://github.com/mhaeuser/edk2/compare/master...wip_hii_cvar
>>>>>> If the feedback is positive, I will clean it up of course. OVMF
>>>>>> boots with everything working fine.
>>>>>>
>>>>>> I'd explicitly like feedback on the following:
>>>>>> A. Is this an acceptable solution to BZ 557 (Andrew?)?
>>>>>> B. Is this an acceptable solution for the "HII workflow" (MdeModule
>>>>>> maintainers?)?
>>>>>> C. Is it acceptable to make support UEFI-side support for the old
>>>>>> mechanism optional (Stewards?)?
>>>>>> D. Can an acceptable alternative be found for the removed ShellPkg
>>>>>> code (Shell maintainers?)?
>>>>>>
>>>>>> As you can see the BaseTools part also is rough, but that is more a
>>>>>> question of "how" rather than "whether", so I'll postpone asking about it.
>>>>>>
>>>>>> Thanks for your time and feedback!
>>>>>>
>>>>>> Best regards,
>>>>>> Marvin
>>>>>>
>>>>>>
>>>>>> [1] "Once the image is loaded, LoadImage() installs
>>>>>> EFI_HII_PACKAGE_LIST_PROTOCOL on the handle if the image contains a
>>>>>> custom PE/COFF resource with the type 'HII'."
>>>>>> - UEFI 2.9, 7.4, "EFI_BOOT_SERVICES.LoadImage()"
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>>>
>>>
>>>
>
>
> 
>
>


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC] Expose HII package list via C variables
  2021-08-25 21:21 [RFC] Expose HII package list via C variables Marvin Häuser
  2021-08-25 22:33 ` [edk2-devel] " Michael D Kinney
@ 2021-08-26 18:19 ` Ni, Ray
  2021-08-26 18:32   ` [edk2-devel] " Marvin Häuser
  1 sibling, 1 reply; 17+ messages in thread
From: Ni, Ray @ 2021-08-26 18:19 UTC (permalink / raw)
  To: Marvin Häuser, devel@edk2.groups.io
  Cc: Andrew Fish, leif@nuviainc.com, Kinney, Michael D, Gao, Zhichao,
	Wang, Jian J, Wu, Hao A, Bi, Dandan, Dong, Eric, Bret Barkelew,
	Vitaly Cheptsov

> 3. Saves error-prone parsing work

This might be the key reason? Is it theoretically possible to write a secure parsing code?

Thanks,
Ray

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [edk2-devel] [RFC] Expose HII package list via C variables
  2021-08-26 18:19 ` Ni, Ray
@ 2021-08-26 18:32   ` Marvin Häuser
  2021-08-26 18:38     ` Ni, Ray
  0 siblings, 1 reply; 17+ messages in thread
From: Marvin Häuser @ 2021-08-26 18:32 UTC (permalink / raw)
  To: devel, ray.ni
  Cc: Andrew Fish, leif@nuviainc.com, Kinney, Michael D, Gao, Zhichao,
	Wang, Jian J, Wu, Hao A, Bi, Dandan, Dong, Eric, Bret Barkelew,
	Vitaly Cheptsov

On 26/08/2021 20:19, Ni, Ray wrote:
>> 3. Saves error-prone parsing work
> This might be the key reason? Is it theoretically possible to write a secure parsing code?

Hey Ray,

Yes it is, but the most secure parsing code is that which doesn't exist. 
I'm confident in the one I wrote for the new PE/COFF library, but if 
variables suit the need, I really would like to use them, even if it is 
only by a toggle. The new proposal (please check the chain with Mike) 
was adapted to preserve the old behaviour fully for any platform that 
wants it, permanently. Amaranth will very likely drop support for the 
PE/COFF section, either nicely with an upstream PCD, or not-so-nicely 
with maintaining patches to remove the functionality downstream.

In the same chain I outlined I'm sketching a new terse file format and 
I'd prefer to avoid any unnecessary data or parsing burden. The current 
library I am sketching privately is *much* smaller than both the current 
and the new PE/COFF library. The overall format in an UEFI context (no 
dynamic linking etc.) is at least as powerful as PE/COFF and pretty much 
always smaller than TE. I'd like to keep it that way. :)

Best regards,
Marvin

>
> Thanks,
> Ray
>
>
> 
>
>


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [edk2-devel] [RFC] Expose HII package list via C variables
  2021-08-26 18:32   ` [edk2-devel] " Marvin Häuser
@ 2021-08-26 18:38     ` Ni, Ray
  2021-08-26 19:28       ` Marvin Häuser
  0 siblings, 1 reply; 17+ messages in thread
From: Ni, Ray @ 2021-08-26 18:38 UTC (permalink / raw)
  To: Marvin Häuser, devel@edk2.groups.io
  Cc: Andrew Fish, leif@nuviainc.com, Kinney, Michael D, Gao, Zhichao,
	Wang, Jian J, Wu, Hao A, Bi, Dandan, Dong, Eric, Bret Barkelew,
	Vitaly Cheptsov

Marvin,
If we can keep the secure parsing code inside the PeCoff library, all the backward combability issues because of removing the HII section support don't exist.
The PeCoff library might be very complex but the complexity[1] is hidden inside and abstracted by the PeCoff library API.
The complexity[2] of resolving combability issues after dropping HII section support disappears.

The complexity[1] is a fixed value but [2] is hard to calculate because there are so many platforms that might use this feature.

> -----Original Message-----
> From: Marvin Häuser <mhaeuser@posteo.de>
> Sent: Friday, August 27, 2021 2:33 AM
> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
> Cc: Andrew Fish <afish@apple.com>; leif@nuviainc.com; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Zhichao
> <zhichao.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan
> <dandan.bi@intel.com>; Dong, Eric <eric.dong@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>; Vitaly Cheptsov
> <vit9696@protonmail.com>
> Subject: Re: [edk2-devel] [RFC] Expose HII package list via C variables
> 
> On 26/08/2021 20:19, Ni, Ray wrote:
> >> 3. Saves error-prone parsing work
> > This might be the key reason? Is it theoretically possible to write a secure parsing code?
> 
> Hey Ray,
> 
> Yes it is, but the most secure parsing code is that which doesn't exist.
> I'm confident in the one I wrote for the new PE/COFF library, but if
> variables suit the need, I really would like to use them, even if it is
> only by a toggle. The new proposal (please check the chain with Mike)
> was adapted to preserve the old behaviour fully for any platform that
> wants it, permanently. Amaranth will very likely drop support for the
> PE/COFF section, either nicely with an upstream PCD, or not-so-nicely
> with maintaining patches to remove the functionality downstream.
> 
> In the same chain I outlined I'm sketching a new terse file format and
> I'd prefer to avoid any unnecessary data or parsing burden. The current
> library I am sketching privately is *much* smaller than both the current
> and the new PE/COFF library. The overall format in an UEFI context (no
> dynamic linking etc.) is at least as powerful as PE/COFF and pretty much
> always smaller than TE. I'd like to keep it that way. :)
> 
> Best regards,
> Marvin
> 
> >
> > Thanks,
> > Ray
> >
> >
> > 
> >
> >


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [edk2-devel] [RFC] Expose HII package list via C variables
  2021-08-26 18:38     ` Ni, Ray
@ 2021-08-26 19:28       ` Marvin Häuser
  0 siblings, 0 replies; 17+ messages in thread
From: Marvin Häuser @ 2021-08-26 19:28 UTC (permalink / raw)
  To: Ni, Ray, devel@edk2.groups.io
  Cc: Andrew Fish, leif@nuviainc.com, Kinney, Michael D, Gao, Zhichao,
	Wang, Jian J, Wu, Hao A, Bi, Dandan, Dong, Eric, Bret Barkelew,
	Vitaly Cheptsov

On 26/08/2021 20:38, Ni, Ray wrote:
> Marvin,
> If we can keep the secure parsing code inside the PeCoff library, all the backward combability issues because of removing the HII section support don't exist.

Hey Ray,

As I said, the updated proposal no longer has any impact on 
backwards-compatibility. For the new file format, there is still a big, 
big amount of time to reconsider later. Samer said the usage of this 
feature is declining. For now the comments from the downstreams really 
helped me getting an idea of what I am dealing with.

> The PeCoff library might be very complex but the complexity[1] is hidden inside and abstracted by the PeCoff library API.

Well, bugs hidden behind a nice API are still bugs, it's not just about 
the callers. From the top of my head I can name you this:

"ResourceDirectory", the object the loop is based on [1], is overwritten 
within loop paths that may enter another iteration [2]. With a 
well-formed file this will still work, and with a malformed file it at 
least will not cause dangerous behaviour, but it is still obvious that 
this is not the way this is meant to work.

This is obviously an easy fix and not a bud bug at all, but my point is 
there more code there is, the more slips through review. There are 
general parsing safety issues too.

Another interesting thing, not related to HII but somewhat related to 
the "file format assumptions" point is that there now is an ELF loader 
in PEI phase [3], but despite an attempt at abstraction with the 
LoadFile PPI, there is a clear "PEIM = PE/COFF" assumption in PeiCore 
[4]. Is this a real-world bug? I don't know, I cannot tell because the 
code is too complex. :)

> The complexity[2] of resolving combability issues after dropping HII section support disappears.
>
> The complexity[1] is a fixed value but [2] is hard to calculate because there are so many platforms that might use this feature.

Yes, hence I am happy about all the comments I could gather so far!

Best regards,
Marvin


[1]
https://github.com/tianocore/edk2/blob/6c8dd15c4ae42501438a525ec41299f365f223cb/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L1549

[2]
https://github.com/tianocore/edk2/blob/6c8dd15c4ae42501438a525ec41299f365f223cb/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L1576
https://github.com/tianocore/edk2/blob/6c8dd15c4ae42501438a525ec41299f365f223cb/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L1593

[3]
https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0eab4ac688d5/UefiPayloadPkg/PayloadLoaderPeim/PayloadLoaderPeim.c#L159-L161

[4]
https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0eab4ac688d5/MdeModulePkg/Core/Pei/Image/Image.c#L910
https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0eab4ac688d5/MdeModulePkg/Core/Pei/Image/Image.c#L799-L848

>
>> -----Original Message-----
>> From: Marvin Häuser <mhaeuser@posteo.de>
>> Sent: Friday, August 27, 2021 2:33 AM
>> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
>> Cc: Andrew Fish <afish@apple.com>; leif@nuviainc.com; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Zhichao
>> <zhichao.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan
>> <dandan.bi@intel.com>; Dong, Eric <eric.dong@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>; Vitaly Cheptsov
>> <vit9696@protonmail.com>
>> Subject: Re: [edk2-devel] [RFC] Expose HII package list via C variables
>>
>> On 26/08/2021 20:19, Ni, Ray wrote:
>>>> 3. Saves error-prone parsing work
>>> This might be the key reason? Is it theoretically possible to write a secure parsing code?
>> Hey Ray,
>>
>> Yes it is, but the most secure parsing code is that which doesn't exist.
>> I'm confident in the one I wrote for the new PE/COFF library, but if
>> variables suit the need, I really would like to use them, even if it is
>> only by a toggle. The new proposal (please check the chain with Mike)
>> was adapted to preserve the old behaviour fully for any platform that
>> wants it, permanently. Amaranth will very likely drop support for the
>> PE/COFF section, either nicely with an upstream PCD, or not-so-nicely
>> with maintaining patches to remove the functionality downstream.
>>
>> In the same chain I outlined I'm sketching a new terse file format and
>> I'd prefer to avoid any unnecessary data or parsing burden. The current
>> library I am sketching privately is *much* smaller than both the current
>> and the new PE/COFF library. The overall format in an UEFI context (no
>> dynamic linking etc.) is at least as powerful as PE/COFF and pretty much
>> always smaller than TE. I'd like to keep it that way. :)
>>
>> Best regards,
>> Marvin
>>
>>> Thanks,
>>> Ray
>>>
>>>
>>> 
>>>
>>>


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [edk2-devel] [RFC] Expose HII package list via C variables
  2021-08-26 16:44           ` Michael D Kinney
  2021-08-26 17:29             ` Marvin Häuser
@ 2021-08-26 19:50             ` Andrew Fish
  1 sibling, 0 replies; 17+ messages in thread
From: Andrew Fish @ 2021-08-26 19:50 UTC (permalink / raw)
  To: Mike Kinney
  Cc: Marvin Häuser, tim.lewis@insyde.com, devel@edk2.groups.io,
	leif@nuviainc.com, Ni, Ray, Gao, Zhichao, Wang, Jian J, Wu, Hao A,
	Bi, Dandan, Dong, Eric, Bret Barkelew, Vitaly Cheptsov

[-- Attachment #1: Type: text/plain, Size: 14057 bytes --]

Mike,

That reminds me that my patch to fix this for Xcode is still in limbo. 

Thanks,

Andrew Fish

> On Aug 26, 2021, at 9:44 AM, Kinney, Michael D <michael.d.kinney@intel.com> wrote:
> 
> Marvin,
> 
> The main reason most components in the EDK II repos continue to use the variables is 
> because there are incomplete tools to generate PE/COFF resource sections for all
> the OS/compiler combinations that EDK II supports.
> 
> The preference would be to use PE/COFF resource sections and we would have converted
> all components to that style long ago if the tools existed to be aligned with the
> UEFI Specification instead of an EDK II specific implementation feature.
> 
> Also, it is not a good idea to only look at the open source EDK II repos to
> understand how a feature is used.  There are many downstream consumers of the
> EDK II repos.
> 
> The UEFI Driver Writer's Guide *only* documents the PE/COFF resource section
> method.
> 
> https://tianocore-docs.github.io/edk2-UefiDriverWritersGuide/draft/7_driver_entry_point/74_adding_hii_packages_feature.html#74-adding-hii-packages-feature <https://tianocore-docs.github.io/edk2-UefiDriverWritersGuide/draft/7_driver_entry_point/74_adding_hii_packages_feature.html#74-adding-hii-packages-feature>
> 
> Best regards,
> 
> Mike
> 
>> -----Original Message-----
>> From: Marvin Häuser <mhaeuser@posteo.de <mailto:mhaeuser@posteo.de>>
>> Sent: Thursday, August 26, 2021 9:07 AM
>> To: tim.lewis@insyde.com <mailto:tim.lewis@insyde.com>; devel@edk2.groups.io <mailto:devel@edk2.groups.io>; Kinney, Michael D <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>>
>> Cc: 'Andrew Fish' <afish@apple.com <mailto:afish@apple.com>>; leif@nuviainc.com <mailto:leif@nuviainc.com>; Ni, Ray <ray.ni@intel.com <mailto:ray.ni@intel.com>>; Gao, Zhichao <zhichao.gao@intel.com <mailto:zhichao.gao@intel.com>>;
>> Wang, Jian J <jian.j.wang@intel.com <mailto:jian.j.wang@intel.com>>; Wu, Hao A <hao.a.wu@intel.com <mailto:hao.a.wu@intel.com>>; Bi, Dandan <dandan.bi@intel.com <mailto:dandan.bi@intel.com>>; Dong, Eric
>> <eric.dong@intel.com <mailto:eric.dong@intel.com>>; 'Bret Barkelew' <Bret.Barkelew@microsoft.com <mailto:Bret.Barkelew@microsoft.com>>; 'Vitaly Cheptsov' <vit9696@protonmail.com <mailto:vit9696@protonmail.com>>
>> Subject: Re: [edk2-devel] [RFC] Expose HII package list via C variables
>> 
>> Hey Tim,
>> 
>> Interesting, do you happen to have some tool or code that performs such
>> edits at hand to take a look at? Seems like most modules already use
>> variables and thus cannot be modified in such a way even right now?
>> 
>> That's the kind of information I am looking for, thanks a lot!
>> 
>> Best regards,
>> Marvin
>> 
>> On 26/08/2021 18:01, tim.lewis@insyde.com wrote:
>>> Hi Marvin --
>>> 
>>> I would like to add some historical perspective on this. One of the design requirements back when HII was first
>> introduced into the UEFI specification after Intel's initial contribution was that of binary editability. In order to be
>> able to reliably find, extract and then re-insert the HII data into the binary, it needed to be discoverable and not
>> affect the offsets of the code and data in the binary.
>>> 
>>> While it was possible to put some sort of signature in the read-only data sections of the binary and leave padding for
>> possible future edits, it was felt that using a PE/COFF section similar to the resource sections was better. Resource
>> sections are commonly in use for PE/COFF files (in Windows) and the similar idea existed in the then-extant Mac binary
>> format (resource fork?).
>>> 
>>> Thanks,
>>> 
>>> Tim Lewis
>>> CTO, Insyde Software
>>> www.insyde.com
>>> 
>>> -----Original Message-----
>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael D Kinney
>>> Sent: Thursday, August 26, 2021 7:37 AM
>>> To: devel@edk2.groups.io; mhaeuser@posteo.de; Kinney, Michael D <michael.d.kinney@intel.com>
>>> Cc: Andrew Fish <afish@apple.com>; leif@nuviainc.com; Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>;
>> Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Dong, Eric
>> <eric.dong@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>; Vitaly Cheptsov <vit9696@protonmail.com>
>>> Subject: Re: [edk2-devel] [RFC] Expose HII package list via C variables
>>> 
>>> Marvin,
>>> 
>>> One constraint in this discussion is that the HII content in a PE/COFF resource section is defined in the UEFI
>> Specification, Which means UEFI Apps and UEFI Drivers from media and option ROMs that are not part of the system FW image
>> are allowed to use this feature,  This means the system FW PE/COFF loader must support loading HII content from this
>> PE/COFF resource section to be UEFI conformant.  So we cannot remove this feature from the PE/COFF loader without changes
>> to the UEFI Specification.  Even if changes to the UEFI Specification we made, we would have to continue to support this
>> feature for backward compatibility with existing UEFI Apps/Drivers that may be using this feature.
>>> 
>>> Thanks,
>>> 
>>> Mike
>>> 
>>>> -----Original Message-----
>>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Marvin
>>>> Häuser
>>>> Sent: Thursday, August 26, 2021 1:51 AM
>>>> To: devel@edk2.groups.io; Kinney, Michael D
>>>> <michael.d.kinney@intel.com>
>>>> Cc: devel@edk2.groups.io; Andrew Fish <afish@apple.com>;
>>>> leif@nuviainc.com; Ni, Ray <ray.ni@intel.com>; Gao, Zhichao
>>>> <zhichao.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao
>>>> A <hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Dong, Eric
>>>> <eric.dong@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>;
>>>> Vitaly Cheptsov <vit9696@protonmail.com>
>>>> Subject: Re: [edk2-devel] [RFC] Expose HII package list via C
>>>> variables
>>>> 
>>>> Hey Mike,
>>>> 
>>>> Thanks for your reply!
>>>> 
>>>> Well, this switch is not well-documented. Looking now at both the
>>>> generation code and the emitted code, it does not generate a package
>>>> list like my code does, but separate data variables (strings and
>>>> images) that cannot easily be passed to HiiDatabase as-is. However
>>>> apparently there are drivers that use this functionality successfully
>>>> by composing the package list at runtime [1].
>>>> 
>>>> Looking with this information now at the pattern of using HII C
>>>> variables (which I did not know existed before) vs the PE/COFF HII
>>>> section, most that use latter are Shell applications, which I guess
>>>> means the section has actually been introduced to resolve D.? There
>>>> are exceptions such as LogoDxe [2], which use the PE/COFF section while D.
>>>> is not a problem, hence I got confused, sorry. I think these modules
>>>> should be updated in any case. Do you agree?
>>>> 
>>>> So, for modules that use C variables already, my patch would save some
>>>> runtime generation code and dynamic memory allocation for the HII
>>>> package list. This was not my goal (as I said, I didn't realise HII C
>>>> variables already were a thing in the first place), but the changes
>>>> are small enough that it might be worth considering anyway, in my opinion.
>>>> If a HII package list is generated for both Shell and non-Shell apps,
>>>> this also means code paths can be unified. For example, there could be
>>>> a library class with constructor and destructor to add/remove packages
>>>> from the HII database for all modules that use such, Shell or not. For
>>>> BaseTools it means that there is no real need for separate Python and
>>>> C paths as ideally they just generate the exact same data.
>>>> 
>>>> Now to D., the only usage for this seems to be that Shell can locate
>>>> the help text in the executable without executing it, yet it is fully
>>>> loaded anyway [3]. To be honest, I find it hard to justify loading an
>>>> executable (PE/COFF loading, memory permission application, the full
>>>> process) to retrieve a help text and then unloading it again,
>>>> especially with the HII code being on a core dispatcher level. 1. to
>>>> 7. still hold true in my opinion. Was there any discussion I could
>>>> read through why Shell apps cannot simply support a "--help" or "-?"
>>>> command and output the string themselves? Pushing the burden to the
>>>> Shell apps does preserve the "drawback" that a full loading is
>>>> required (which honestly does not matter for a debugging application
>>>> like Shell), however it does relieve the burden of PE/COFF HII parsing
>>>> from the core dispatcher (which matters a lot in my opinion to keep
>>>> the core simple). It would simply be a normal Shell app execution as
>>>> any other however. If someone wants to avoid the PE/COFF burden
>>>> altogether, they can still provide a .man file.
>>>> 
>>>> As for my points 6. and 7., maybe I should provide some context. Due
>>>> to many issues with TE files, platforms started abandoning them and
>>>> returned to PE/COFF Images. I think a big reason for this is that TE
>>>> is not really a sound and complete format, but a stripped version of
>>>> PE/COFF with none of the necessary fixups applied. I'm currently
>>>> sketching a possible alternative [4], and I would really like to not
>>>> having to specify a HII section type, while still preserving
>>>> compatibility with all of the UEFI Image types and use-cases [4].
>>>> 
>>>> Thanks again!
>>>> 
>>>> Best regards,
>>>> Marvin
>>>> 
>>>> 
>>>> [1]
>>>> https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0ea
>>>> b4ac688d5/MdeModulePkg/Application/BootManagerMenuAp
>>>> p/BootManagerMenu.c#L929-L934
>>>> 
>>>> [2]
>>>> https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0ea
>>>> b4ac688d5/MdeModulePkg/Logo/LogoDxe.inf#L23
>>>> 
>>>> [3]
>>>> 
>> https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0eab4ac688d5/ShellPkg/Application/Shell/ShellManParser.
>>>> c#L646-L671
>>>> 
>>>> [4]
>>>> https://github.com/mhaeuser/edk2/blob/ue_poc/MdePkg/Include/IndustrySt
>>>> andard/UeImage.h
>>>> 
>>>> 26.08.2021 00:34:12 Michael D Kinney <michael.d.kinney@intel.com>:
>>>> 
>>>>> Hi Marvin,
>>>>> 
>>>>> I think this feature is already there and supported.
>>>>> 
>>>>> HII can either be in a global variable or in a PE/COFF resource section.
>>>>> The default is a global variable because HII was implemented before
>>>>> the PE/COFF resource section feature was added to the UEFI Specification.
>>>>> 
>>>>> There is an INF [Defines] section statement to enable the PE/COFF
>>>>> section. See UefiHiiResource in the following link.
>>>>> 
>>>>> https://tianocore-docs.github.io/edk2-InfSpecification/draft/3_edk_i
>>>>> i_inf_file_format/34_[defines]_section.html#34-
>>>> defines-section
>>>>> How is your proposal different than this existing capability?
>>>>> 
>>>>> Thanks,
>>>>> 
>>>>> Mike
>>>>> 
>>>>>> -----Original Message-----
>>>>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
>>>>>> Marvin Häuser
>>>>>> Sent: Wednesday, August 25, 2021 2:21 PM
>>>>>> To: devel@edk2.groups.io
>>>>>> Cc: Andrew Fish <afish@apple.com>; leif@nuviainc.com; Kinney,
>>>>>> Michael D <michael.d.kinney@intel.com>; Ni, Ray <ray.ni@intel.com>;
>>>>>> Gao, Zhichao <zhichao.gao@intel.com>; Wang, Jian J
>>>>>> <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan
>>>>>> <dandan.bi@intel.com>; Dong, Eric <eric.dong@intel.com>; Bret
>>>>>> Barkelew <Bret.Barkelew@microsoft.com>; Vitaly Cheptsov
>>>>>> <vit9696@protonmail.com>
>>>>>> Subject: [edk2-devel] [RFC] Expose HII package list via C variables
>>>>>> 
>>>>>> Good day everyone,
>>>>>> 
>>>>>> Currently, the HII package list is stored in a PE/COFF resource
>>>>>> section [1]. I propose to store it in a C variable (byte array with
>>>>>> a pointer to it and its size exposed) instead. DxeCore would have a
>>>>>> guard to toggle the deprecated support for the automatic protocol
>>>>>> installation. This has the following advantages:
>>>>>> 
>>>>>> 1. Fixes BZ (incl. future toolchains):
>>>>>> https://bugzilla.tianocore.org/show_bug.cgi?id=557
>>>>>> 2. Universal method across all toolchains and output file formats
>>>>>> 3. Saves error-prone parsing work 4. Saves protocol install/locate
>>>>>> work, the data is available right away 5. The omission of a
>>>>>> dedicated section can save space 6. Terse file formats can support
>>>>>> this and remain terse :) 7. Removes a dependency on the PE/COFF
>>>>>> format specifically
>>>>>> 
>>>>>> A *very rough* PoC diff can be found here:
>>>>>> https://github.com/mhaeuser/edk2/compare/master...wip_hii_cvar
>>>>>> If the feedback is positive, I will clean it up of course. OVMF
>>>>>> boots with everything working fine.
>>>>>> 
>>>>>> I'd explicitly like feedback on the following:
>>>>>> A. Is this an acceptable solution to BZ 557 (Andrew?)?
>>>>>> B. Is this an acceptable solution for the "HII workflow" (MdeModule
>>>>>> maintainers?)?
>>>>>> C. Is it acceptable to make support UEFI-side support for the old
>>>>>> mechanism optional (Stewards?)?
>>>>>> D. Can an acceptable alternative be found for the removed ShellPkg
>>>>>> code (Shell maintainers?)?
>>>>>> 
>>>>>> As you can see the BaseTools part also is rough, but that is more a
>>>>>> question of "how" rather than "whether", so I'll postpone asking about it.
>>>>>> 
>>>>>> Thanks for your time and feedback!
>>>>>> 
>>>>>> Best regards,
>>>>>> Marvin
>>>>>> 
>>>>>> 
>>>>>> [1] "Once the image is loaded, LoadImage() installs
>>>>>> EFI_HII_PACKAGE_LIST_PROTOCOL on the handle if the image contains a
>>>>>> custom PE/COFF resource with the type 'HII'."
>>>>>> - UEFI 2.9, 7.4, "EFI_BOOT_SERVICES.LoadImage()"
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>> 
>>>>> 
>>>>> 
>>>>> 
>>>> 
>>>> 
>>>> 
>>> 
>>> 
>>> 


[-- Attachment #2: Type: text/html, Size: 32657 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2021-08-26 19:51 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-08-25 21:21 [RFC] Expose HII package list via C variables Marvin Häuser
2021-08-25 22:33 ` [edk2-devel] " Michael D Kinney
2021-08-26  8:50   ` Marvin Häuser
2021-08-26 14:37     ` Michael D Kinney
2021-08-26 14:47       ` Marvin Häuser
2021-08-26 16:01       ` Tim Lewis
2021-08-26 16:07         ` Marvin Häuser
2021-08-26 16:44           ` Michael D Kinney
2021-08-26 17:29             ` Marvin Häuser
2021-08-26 19:50             ` Andrew Fish
2021-08-26 16:49           ` Samer El-Haj-Mahmoud
2021-08-26 17:02             ` Marvin Häuser
2021-08-26 17:07           ` Tim Lewis
2021-08-26 18:19 ` Ni, Ray
2021-08-26 18:32   ` [edk2-devel] " Marvin Häuser
2021-08-26 18:38     ` Ni, Ray
2021-08-26 19:28       ` Marvin Häuser

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox