Re: [edk2-devel] [PATCH v2 2/2] EmbeddedPkg: Allow longer android kernel command line

["Chang, Abner via groups.io" <abner.chang=amd.com@groups.io>]

[AMD Official Use Only - General]

Ok, I got it. Thanks.

Reviewed-by: Abner Chang <abner.chang@amd.com>

Abner


From: Ashish Singhal <ashishsingha@nvidia.com>
Sent: Tuesday, November 21, 2023 1:18 AM
To: Chang, Abner <Abner.Chang@amd.com>; devel@edk2.groups.io; quic_llindhol@quicinc.com; ardb+tianocore@kernel.org; git@danielschaefer.me; Jeff Brasen <jbrasen@nvidia.com>
Subject: Re: [PATCH v2 2/2] EmbeddedPkg: Allow longer android kernel command line

[AMD Official Use Only - General]


Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.



________________________________________
From: Chang, Abner <mailto:Abner.Chang@amd.com>
Sent: Monday, November 20, 2023 5:30 AM
To: Ashish Singhal <mailto:ashishsingha@nvidia.com>; mailto:devel@edk2.groups.io <mailto:devel@edk2.groups.io>; mailto:quic_llindhol@quicinc.com <mailto:quic_llindhol@quicinc.com>; mailto:ardb+tianocore@kernel.org <mailto:ardb+tianocore@kernel.org>; mailto:git@danielschaefer.me <mailto:git@danielschaefer.me>; Jeff Brasen <mailto:jbrasen@nvidia.com>
Subject: RE: [PATCH v2 2/2] EmbeddedPkg: Allow longer android kernel command line

External email: Use caution opening links or attachments


[AMD Official Use Only - General]

> -----Original Message-----
> From: Ashish Singhal <mailto:ashishsingha@nvidia.com>
> Sent: Wednesday, November 15, 2023 11:12 AM
> To: mailto:devel@edk2.groups.io; mailto:quic_llindhol@quicinc.com;
> mailto:ardb+tianocore@kernel.org; Chang, Abner <mailto:Abner.Chang@amd.com>;
> mailto:git@danielschaefer.me; mailto:jbrasen@nvidia.com
> Cc: Ashish Singhal <mailto:ashishsingha@nvidia.com>
> Subject: [PATCH v2 2/2] EmbeddedPkg: Allow longer android kernel command
> line
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> AndroidBootImgLib allows for platforms to append to kernel command
> line but does not allow for the overall kernel command line to go
> beyond the limit set by the image header. Address this limitation
> by adding a pcd where platform can tell how many extra characters
> they expect on their platform in addition to what the image header
> specifies.
>
> Signed-off-by: Ashish Singhal <mailto:ashishsingha@nvidia.com>
> ---
>  EmbeddedPkg/EmbeddedPkg.dec                                 | 5 +++++
>  EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c   | 2 +-
>  EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf | 3 ++-
>  3 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/EmbeddedPkg/EmbeddedPkg.dec
> b/EmbeddedPkg/EmbeddedPkg.dec
> index 341ef5e6a6..94dc3c9b76 100644
> --- a/EmbeddedPkg/EmbeddedPkg.dec
> +++ b/EmbeddedPkg/EmbeddedPkg.dec
> @@ -183,3 +183,8 @@
>    # Selection between DT and ACPI as a default
>    #
>    gEmbeddedTokenSpaceGuid.PcdDefaultDtPref|TRUE|BOOLEAN|0x0000059
> +
> +  #
> +  # Expected Overflow Android Kernel Command Line Characters
> +  #
> +
> gEmbeddedTokenSpaceGuid.PcdAndroidKernelCommandLineOverflow|0|UI
> NT32|0x000005C
> diff --git a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> index f63648e60d..d16929f2bb 100644
> --- a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> +++ b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> @@ -335,7 +335,7 @@ AndroidBootImgUpdateArgs (
>      return Status;
>    }
>
> -  NewKernelArgSize = ANDROID_BOOTIMG_KERNEL_ARGS_SIZE;
> +  NewKernelArgSize = ANDROID_BOOTIMG_KERNEL_ARGS_SIZE + PcdGet32
> (PcdAndroidKernelCommandLineOverflow);

I don't know the history of ANDROID_BOOTIMG_KERNEL_ARGS_SIZE, however, I am a little bit confused.
Is this PCD introduced for the case the string size may greater than ANDROID_BOOTIMG_KERNEL_ARGS_SIZE? If yes, then why don't we just use StrSize (ImageKernelArgs) * sizeof(UINT16) as the buffer size?

Regards,
Abner
Android style image header can never have more than ANDROID_BOOTIMG_KERNEL_ARGS_SIZE bytes. However, a platform may need to add new kernel command line arguments at boot time that may need more buffer space than ANDROID_BOOTIMG_KERNEL_ARGS_SIZE bytes. To allow for that, we can have a PCD that by default is set to 0 but a user can override that in their platform dsc if they expect more than ANDROID_BOOTIMG_KERNEL_ARGS_SIZE characters. This is not a bugfix but a limitation that is being addressed in this patch.

Thanks
Ashish

>    *KernelArgs      = AllocateZeroPool (sizeof (CHAR16) * NewKernelArgSize);
>    if (*KernelArgs == NULL) {
>      DEBUG ((DEBUG_ERROR, "Fail to allocate memory\n"));
> diff --git a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf
> b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf
> index 8eefeef4f9..9754664df5 100644
> --- a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf
> +++ b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf
> @@ -45,5 +45,6 @@
>    gEfiAcpiTableGuid
>    gFdtTableGuid
>
> -[FeaturePcd]
> +[Pcd]
>    gEmbeddedTokenSpaceGuid.PcdAndroidBootLoadFile2
> +  gEmbeddedTokenSpaceGuid.PcdAndroidKernelCommandLineOverflow
> --
> 2.17.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111494): https://edk2.groups.io/g/devel/message/111494
Mute This Topic: https://groups.io/mt/102598723/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-