Re: [edk2-devel] [RFC PATCH 1/1] OvmfPkg: add 'initrd' shell command to expose Linux initrd via device path
On Fri, 14 Feb 2020 at 15:17, Laszlo Ersek wrote:
>
> On 02/14/20 01:55, Ni, Ray wrote:
> >
> >
> >> -Original Message-
> >> From: Laszlo Ersek
> >> Sent: Friday, February 14, 2020 7:15 AM
> >> To: Ni, Ray ; devel@edk2.groups.io; Ard Biesheuvel
> >>
> >> Cc: l...@nuviainc.com; phi...@redhat.com; Gao, Zhichao
> >>
> >> Subject: Re: [edk2-devel] [RFC PATCH 1/1] OvmfPkg: add 'initrd' shell
> >> command to expose Linux initrd via device path
> >>
> >> On 02/12/20 15:21, Ni, Ray wrote:
> >>>> (3) However: I think this should be added as a Dynamic Command instead.
> >>>> I'm basing this on the message of commit 0961002352e9 ("ShellPkg/tftp:
> >>>> Convert from NULL class library to Dynamic Command", 2017-11-28),
> >> which
> >>>> is the first commit in edk2 ever to introduce a Dynamic Command.
> >>>>
> >>>> And the commit message there says:
> >>>>
> >>>> The guideline is:
> >>>> 1. Only use NULL class library for Shell spec defined commands.
> >>>> 2. New commands can be provided as not only a standalone application
> >>>>but also a dynamic command. So it can be used either as an
> >>>>internal command, but also as a standalone application.
> >>>>
> >>>> I'm not asking for the command to be usable as a separate application,
> >>>> but I think we might want to follow the first guideline.
> >>>>
> >>>> (I've checked the UEFI Shell 2.2 spec. While it talks about dynamic
> >>>> commands, it does not seem to spell out guideline#1. So I think it's
> >>>> rather an edk2-specific guideline than a standard one. Nonetheless we
> >>>> might want to adhere to it.)
> >>>
> >>> Laszlo, thanks for the comments.
> >>> I didn't remember that I said these guideline publicly.
> >>> The reason behind that is we can have the same shell binary everywhere
> >>> and new non-spec commands can be added through dynamic command
> >> without
> >>> impacting the shell binary.
> >>
> >> Thanks for the explanation -- this means that the NULL class lib
> >> approach is good for OvmfPkg after all. I'm putting the remaining parts
> >> of this patch back on my review queue (it will take a while).
> >
> > Please don't misunderstand my points.
>
> OK. From your response, I thought that the guidelines you captured in
> the commit message in question were only for internal shell builds.
>
> > I still prefer to use dynamic commands
> > for all non-spec defined shell internal commands.
> > Sorry for the confusion caused by my previous mail.
>
> It's OK, I understand better now. So I guess I'll de-queue the review of
> the rest of this patch once again, and wait for the next version (with
> the dynamic command implementation).
>
Thanks for the review and the clarification. I will change this into a
dynamic command for v2, but it may be a while before I get back to it,
since this feature is still under discussion on the Linux side as
well.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#54451): https://edk2.groups.io/g/devel/message/54451
Mute This Topic: https://groups.io/mt/71177416/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [RFC PATCH 1/1] OvmfPkg: add 'initrd' shell command to expose Linux initrd via device path
> -Original Message-
> From: Laszlo Ersek
> Sent: Friday, February 14, 2020 7:15 AM
> To: Ni, Ray ; devel@edk2.groups.io; Ard Biesheuvel
>
> Cc: l...@nuviainc.com; phi...@redhat.com; Gao, Zhichao
>
> Subject: Re: [edk2-devel] [RFC PATCH 1/1] OvmfPkg: add 'initrd' shell
> command to expose Linux initrd via device path
>
> On 02/12/20 15:21, Ni, Ray wrote:
> >> (3) However: I think this should be added as a Dynamic Command instead.
> >> I'm basing this on the message of commit 0961002352e9 ("ShellPkg/tftp:
> >> Convert from NULL class library to Dynamic Command", 2017-11-28),
> which
> >> is the first commit in edk2 ever to introduce a Dynamic Command.
> >>
> >> And the commit message there says:
> >>
> >> The guideline is:
> >> 1. Only use NULL class library for Shell spec defined commands.
> >> 2. New commands can be provided as not only a standalone application
> >>but also a dynamic command. So it can be used either as an
> >>internal command, but also as a standalone application.
> >>
> >> I'm not asking for the command to be usable as a separate application,
> >> but I think we might want to follow the first guideline.
> >>
> >> (I've checked the UEFI Shell 2.2 spec. While it talks about dynamic
> >> commands, it does not seem to spell out guideline#1. So I think it's
> >> rather an edk2-specific guideline than a standard one. Nonetheless we
> >> might want to adhere to it.)
> >
> > Laszlo, thanks for the comments.
> > I didn't remember that I said these guideline publicly.
> > The reason behind that is we can have the same shell binary everywhere
> > and new non-spec commands can be added through dynamic command
> without
> > impacting the shell binary.
>
> Thanks for the explanation -- this means that the NULL class lib
> approach is good for OvmfPkg after all. I'm putting the remaining parts
> of this patch back on my review queue (it will take a while).
Please don't misunderstand my points. I still prefer to use dynamic commands
for all non-spec defined shell internal commands.
Sorry for the confusion caused by my previous mail.
>
> Thanks
> Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#54399): https://edk2.groups.io/g/devel/message/54399
Mute This Topic: https://groups.io/mt/71177416/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [RFC PATCH 1/1] OvmfPkg: add 'initrd' shell command to expose Linux initrd via device path
On 02/12/20 15:21, Ni, Ray wrote:
>> (3) However: I think this should be added as a Dynamic Command instead.
>> I'm basing this on the message of commit 0961002352e9 ("ShellPkg/tftp:
>> Convert from NULL class library to Dynamic Command", 2017-11-28), which
>> is the first commit in edk2 ever to introduce a Dynamic Command.
>>
>> And the commit message there says:
>>
>> The guideline is:
>> 1. Only use NULL class library for Shell spec defined commands.
>> 2. New commands can be provided as not only a standalone application
>>but also a dynamic command. So it can be used either as an
>>internal command, but also as a standalone application.
>>
>> I'm not asking for the command to be usable as a separate application,
>> but I think we might want to follow the first guideline.
>>
>> (I've checked the UEFI Shell 2.2 spec. While it talks about dynamic
>> commands, it does not seem to spell out guideline#1. So I think it's
>> rather an edk2-specific guideline than a standard one. Nonetheless we
>> might want to adhere to it.)
>
> Laszlo, thanks for the comments.
> I didn't remember that I said these guideline publicly.
> The reason behind that is we can have the same shell binary everywhere
> and new non-spec commands can be added through dynamic command without
> impacting the shell binary.
Thanks for the explanation -- this means that the NULL class lib
approach is good for OvmfPkg after all. I'm putting the remaining parts
of this patch back on my review queue (it will take a while).
Thanks
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#54388): https://edk2.groups.io/g/devel/message/54388
Mute This Topic: https://groups.io/mt/71177416/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [RFC PATCH 1/1] OvmfPkg: add 'initrd' shell command to expose Linux initrd via device path
> (3) However: I think this should be added as a Dynamic Command instead.
> I'm basing this on the message of commit 0961002352e9 ("ShellPkg/tftp:
> Convert from NULL class library to Dynamic Command", 2017-11-28), which
> is the first commit in edk2 ever to introduce a Dynamic Command.
>
> And the commit message there says:
>
> The guideline is:
> 1. Only use NULL class library for Shell spec defined commands.
> 2. New commands can be provided as not only a standalone application
>but also a dynamic command. So it can be used either as an
>internal command, but also as a standalone application.
>
> I'm not asking for the command to be usable as a separate application,
> but I think we might want to follow the first guideline.
>
> (I've checked the UEFI Shell 2.2 spec. While it talks about dynamic
> commands, it does not seem to spell out guideline#1. So I think it's
> rather an edk2-specific guideline than a standard one. Nonetheless we
> might want to adhere to it.)
Laszlo, thanks for the comments.
I didn't remember that I said these guideline publicly.
The reason behind that is we can have the same shell binary everywhere
and new non-spec commands can be added through dynamic command without
impacting the shell binary.
Thanks,
Ray
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#54300): https://edk2.groups.io/g/devel/message/54300
Mute This Topic: https://groups.io/mt/71177416/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [RFC PATCH 1/1] OvmfPkg: add 'initrd' shell command to expose Linux initrd via device path
Adding Ray and Zhichao; comments below
On 02/11/20 19:03, Ard Biesheuvel wrote:
> Add a new 'initrd' command to the UEFI Shell that allows any file that is
> accessible to the shell to be registered as the initrd that is returned
> when Linux's EFI stub loader invokes the LoadFile2 protocol on its special
> vendor media device path.
>
> Signed-off-by: Ard Biesheuvel
> ---
> Note that the vendor media device path initrd loading method is still
> under review on the Linux side, so this is for discussion only for now.
>
> ArmVirtPkg/ArmVirt.dsc.inc|
> 1 +
> OvmfPkg/Library/LinuxInitrdShellCommandLib/LinuxInitrdShellCommandLib.c |
> 269
> OvmfPkg/Library/LinuxInitrdShellCommandLib/LinuxInitrdShellCommandLib.inf |
> 49
> OvmfPkg/Library/LinuxInitrdShellCommandLib/LinuxInitrdShellCommandLib.uni |
> 37 +++
> OvmfPkg/OvmfPkg.dec |
> 1 +
> OvmfPkg/OvmfPkgIa32.dsc |
> 1 +
> OvmfPkg/OvmfPkgIa32X64.dsc|
> 1 +
> OvmfPkg/OvmfPkgX64.dsc|
> 1 +
> 8 files changed, 360 insertions(+)
(1) As usual (and I'm sure you're aware -- this is just an RFC), please
split the ArmVirtPkg change to a different patch.
(2) "ShellPkg/Application/Shell/Shell.inf" is part of
"OvmfPkg/OvmfXen.dsc" too, so I'd suggest enabling the new shell command
there as well.
> diff --git
> a/OvmfPkg/Library/LinuxInitrdShellCommandLib/LinuxInitrdShellCommandLib.inf
> b/OvmfPkg/Library/LinuxInitrdShellCommandLib/LinuxInitrdShellCommandLib.inf
> new file mode 100644
> index ..ed8a0ca85e85
> --- /dev/null
> +++
> b/OvmfPkg/Library/LinuxInitrdShellCommandLib/LinuxInitrdShellCommandLib.inf
> @@ -0,0 +1,49 @@
> +## @file
> +# Provides initrd command to load a Linux initrd via its GUIDed vendor media
> +# path
> +#
> +# Copyright (c) 2020, Arm, Ltd. All rights reserved.
> +#
> +# SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +#
> +##
> +
> +[Defines]
> + INF_VERSION= 1.27
> + BASE_NAME = LinuxInitrdShellCommandLib
> + FILE_GUID = 2f30da26-f51b-4b6f-85c4-31873c281bca
> + MODULE_TYPE= UEFI_APPLICATION
> + VERSION_STRING = 1.0
> + LIBRARY_CLASS = NULL|UEFI_APPLICATION UEFI_DRIVER
> + CONSTRUCTOR= LinuxInitrdShellCommandLibConstructor
> + DESTRUCTOR = LinuxInitrdShellCommandLibDestructor
> +
> +#
> +# VALID_ARCHITECTURES = IA32 X64 ARM AARCH64
> +#
> +
> +[Sources.common]
> + LinuxInitrdShellCommandLib.c
> + LinuxInitrdShellCommandLib.uni
> +
> +[Packages]
> + MdePkg/MdePkg.dec
> + ShellPkg/ShellPkg.dec
> + MdeModulePkg/MdeModulePkg.dec
> + OvmfPkg/OvmfPkg.dec
> +
> +[LibraryClasses]
> + DebugLib
> + DevicePathLib
> + HiiLib
> + MemoryAllocationLib
> + ShellCommandLib
> + ShellLib
> + UefiBootServicesTableLib
> +
> +[Protocols]
> + gEfiLoadFile2ProtocolGuid ## SOMETIMES_PRODUCES
> +
> +[Guids]
> + gShellInitrdHiiGuid ## SOMETIMES_CONSUMES ##
> HII
Seems reasonable, and to match e.g.
"ShellPkg/Library/UefiShellLevel1CommandsLib/UefiShellLevel1CommandsLib.inf".
(3) However: I think this should be added as a Dynamic Command instead.
I'm basing this on the message of commit 0961002352e9 ("ShellPkg/tftp:
Convert from NULL class library to Dynamic Command", 2017-11-28), which
is the first commit in edk2 ever to introduce a Dynamic Command.
And the commit message there says:
The guideline is:
1. Only use NULL class library for Shell spec defined commands.
2. New commands can be provided as not only a standalone application
but also a dynamic command. So it can be used either as an
internal command, but also as a standalone application.
I'm not asking for the command to be usable as a separate application,
but I think we might want to follow the first guideline.
(I've checked the UEFI Shell 2.2 spec. While it talks about dynamic
commands, it does not seem to spell out guideline#1. So I think it's
rather an edk2-specific guideline than a standard one. Nonetheless we
might want to adhere to it.)
Implementing the feature as a dynamic command would imply
MODULE_TYPE=DXE_DRIVER, and using ENTRY_POINT/UNLOAD_IMAGE rathern than
CONSTRUCTOR/DESTRUCTOR; I think.
> diff --git
> a/OvmfPkg/Library/LinuxInitrdShellCommandLib/LinuxInitrdShellCommandLib.uni
> b/OvmfPkg/Library/LinuxInitrdShellCommandLib/LinuxInitrdShellCommandLib.uni
> new file mode 100644
> index ..d4fe798b1ea2
> --- /dev/null
> +++
> b/OvmfPkg/Library/LinuxInitrdShellCommandLib/LinuxInitrdShellCommandLib.uni
> @@ -0,0 +1,37 @@
> +// /**
> +//
> +// Copyright (c) 2020, Arm, Ltd. All rights reserved.
> +//
