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.
> +//
[edk2-devel] [RFC PATCH 1/1] OvmfPkg: add 'initrd' shell command to expose Linux initrd via device path
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(+)
diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
index 10037c938eb8..0570eba6ed0c 100644
--- a/ArmVirtPkg/ArmVirt.dsc.inc
+++ b/ArmVirtPkg/ArmVirt.dsc.inc
@@ -382,6 +382,7 @@ [Components.common]
ShellPkg/Application/Shell/Shell.inf {
ShellCommandLib|ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.inf
+
NULL|OvmfPkg/Library/LinuxInitrdShellCommandLib/LinuxInitrdShellCommandLib.inf
NULL|ShellPkg/Library/UefiShellLevel2CommandsLib/UefiShellLevel2CommandsLib.inf
NULL|ShellPkg/Library/UefiShellLevel1CommandsLib/UefiShellLevel1CommandsLib.inf
NULL|ShellPkg/Library/UefiShellLevel3CommandsLib/UefiShellLevel3CommandsLib.inf
diff --git
a/OvmfPkg/Library/LinuxInitrdShellCommandLib/LinuxInitrdShellCommandLib.c
b/OvmfPkg/Library/LinuxInitrdShellCommandLib/LinuxInitrdShellCommandLib.c
new file mode 100644
index ..cfa4526df6d8
--- /dev/null
+++ b/OvmfPkg/Library/LinuxInitrdShellCommandLib/LinuxInitrdShellCommandLib.c
@@ -0,0 +1,269 @@
+/** @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
+**/
+
+#include
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+
+#pragma pack(1)
+typedef struct {
+ VENDOR_DEVICE_PATH VenMediaNode;
+ EFI_DEVICE_PATH_PROTOCOLEndNode;
+} SINGLE_NODE_VENDOR_MEDIA_DEVPATH;
+#pragma pack()
+
+STATIC CONST CHAR16 mFileName[] = L"";
+STATIC EFI_HII_HANDLE gLinuxInitrdShellCommandHiiHandle;
+STATIC SHELL_FILE_HANDLEmInitrdFileHandle;
+STATIC EFI_HANDLE mInitrdLoadFile2Handle;
+
+STATIC CONST SHELL_PARAM_ITEM ParamList[] = {
+ {L"-u", TypeFlag},
+ {NULL, TypeMax}
+ };
+
+/**
+ Get the filename to get help text from if not using HII.
+
+ @retval The filename.
+**/
+STATIC
+CONST CHAR16*
+EFIAPI
+ShellCommandGetManFileNameInitrd (
+ VOID
+ )
+{
+ return mFileName;
+}
+
+STATIC CONST SINGLE_NODE_VENDOR_MEDIA_DEVPATH mInitrdDevicePath = {
+ {
+{
+ MEDIA_DEVICE_PATH, MEDIA_VENDOR_DP, { sizeof (VENDOR_DEVICE_PATH) }
+},
+{
+ // LINUX_EFI_INITRD_MEDIA_GUID
+ 0x5568e427, 0x68fc, 0x4f3d,
+ { 0xac, 0x74, 0xca, 0x55, 0x52, 0x31, 0xcc, 0x68 }
+}
+ },
+
+ {
+END_DEVICE_PATH_TYPE, END_ENTIRE_DEVICE_PATH_SUBTYPE,
+{ sizeof (EFI_DEVICE_PATH_PROTOCOL) }
+ }
+};
+
+STATIC
+EFI_STATUS
+EFIAPI
+InitrdLoadFile2 (
+ IN EFI_LOAD_FILE2_PROTOCOL *This,
+ IN EFI_DEVICE_PATH_PROTOCOL *FilePath,
+ IN BOOLEAN BootPolicy,
+ IN OUT UINTN*BufferSize,
+ IN VOID *Buffer OPTIONAL
+ )
+{
+ UINTN InitrdSize;
+ EFI_STATUSStatus;
+
+ if (BootPolicy) {
+return EFI_UNSUPPORTED;
+ }
+
+ if (BufferSize == NULL || !IsDevicePathValid (FilePath, 0)) {
+return EFI_INVALID_PARAMETER;
+ }
+
+ if (FilePath->Type != END_DEVICE_PATH_TYPE ||
+ FilePath->SubType != END_ENTIRE_DEVICE_PATH_SUBTYPE ||
+ mInitrdFileHandle == NULL) {
+return EFI_NOT_FOUND;
+ }
+
+ Status = gEfiShellProtocol->GetFileSize (mInitrdFileHandle, );
+ ASSERT_EFI_ERROR(Status);
+
+ if (Buffer == NULL || *BufferSize < InitrdSize) {
+*BufferSize = InitrdSize;
+return EFI_BUFFER_TOO_SMALL;
+ }
+
+ return gEfiShellProtocol->ReadFile (mInitrdFileHandle, BufferSize, Buffer);
+}
+
+STATIC CONST EFI_LOAD_FILE2_PROTOCOL mInitrdLoadFile2 = {
+ InitrdLoadFile2,
+};
+
+/**
+ Function for 'initrd' command.
+
+ @param[in] ImageHandle Handle to the Image (NULL if Internal).
+ @param[in] SystemTable Pointer to the System Table (NULL if Internal).
+**/
+SHELL_STATUS
+EFIAPI
+ShellCommandRunInitrd
