[PATCH v2] cmd: pxe_utils: Limit fdtcontroladdr usage to non-fitImage

[PATCH v2] cmd: pxe_utils: Limit fdtcontroladdr usage to non-fitImage

[Marek Vasut]

Commit d5ba6188dfb ("cmd: pxe_utils: Check fdtcontroladdr in label_boot")
forces '$fdtcontroladdr' DT address as a third parameter of bootm command
even if the PXE transfer pulls in a fitImage which contains configuration
node with its own DT that is preferrable to be passed to Linux. Limit the
$fdtcontroladdr fallback utilization to non-fitImages, since it is highly
likely a fitImage would come with its own DT, while single-file images do
need a separate DT.

Fixes: d5ba6188dfb ("cmd: pxe_utils: Check fdtcontroladdr in label_boot")
Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Peter Hoyes <Peter.Hoyes@arm.com>
Cc: Ramon Fried <rfried.dev@gmail.com>
Cc: Simon Glass <sjg@chromium.org>
---
V1: Map the kernel buffer before testing image type
V2: Update code comment to reflect the change
---
 boot/pxe_utils.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/boot/pxe_utils.c b/boot/pxe_utils.c
index 8133006875f..099aa2f4bc7 100644
--- a/boot/pxe_utils.c
+++ b/boot/pxe_utils.c
@@ -617,7 +617,7 @@ static int label_boot(struct pxe_context *ctx, struct pxe_label *label)
 	 * bootm, and adjust argc appropriately.
 	 *
 	 * Scenario 3: If there is an fdtcontroladdr specified, pass it along to
-	 * bootm, and adjust argc appropriately.
+	 * bootm, and adjust argc appropriately, unless the image type is fitImage.
 	 *
 	 * Scenario 4: fdt blob is not available.
 	 */
@@ -724,7 +724,10 @@ static int label_boot(struct pxe_context *ctx, struct pxe_label *label)
 	if (!bootm_argv[3])
 		bootm_argv[3] = env_get("fdt_addr");
 
-	if (!bootm_argv[3])
+	kernel_addr_r = genimg_get_kernel_addr(kernel_addr);
+	buf = map_sysmem(kernel_addr_r, 0);
+
+	if (!bootm_argv[3] && genimg_get_format(buf) != IMAGE_FORMAT_FIT)
 		bootm_argv[3] = env_get("fdtcontroladdr");
 
 	if (bootm_argv[3]) {
@@ -733,8 +736,6 @@ static int label_boot(struct pxe_context *ctx, struct pxe_label *label)
 		bootm_argc = 4;
 	}
 
-	kernel_addr_r = genimg_get_kernel_addr(kernel_addr);
-	buf = map_sysmem(kernel_addr_r, 0);
 	/* Try bootm for legacy and FIT format image */
 	if (genimg_get_format(buf) != IMAGE_FORMAT_INVALID &&
             IS_ENABLED(CONFIG_CMD_BOOTM))
-- 
2.35.1

Re: [PATCH v2] cmd: pxe_utils: Limit fdtcontroladdr usage to non-fitImage

[Quentin Schulz]

Hi Marek,

On 12/14/22 07:45, Marek Vasut wrote:
> Commit d5ba6188dfb ("cmd: pxe_utils: Check fdtcontroladdr in label_boot")
> forces '$fdtcontroladdr' DT address as a third parameter of bootm command
> even if the PXE transfer pulls in a fitImage which contains configuration
> node with its own DT that is preferrable to be passed to Linux. Limit the
> $fdtcontroladdr fallback utilization to non-fitImages, since it is highly
> likely a fitImage would come with its own DT, while single-file images do
> need a separate DT.
> 

Reviewed-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
Tested-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>

Tested on top of an almost vanilla v2022.10 on Ringneck PX30, the FDT 
from the selected fitimage configuration (via #conf in kernel field in 
extlinux.conf) is taken into account.

Not sure overriding the DT gotten from the fit image wasn't a use-case 
Peter wanted to support though.

Cheers,
Quentin

> Fixes: d5ba6188dfb ("cmd: pxe_utils: Check fdtcontroladdr in label_boot")
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Peter Hoyes <Peter.Hoyes@arm.com>
> Cc: Ramon Fried <rfried.dev@gmail.com>
> Cc: Simon Glass <sjg@chromium.org>
> ---
> V1: Map the kernel buffer before testing image type
> V2: Update code comment to reflect the change
> ---
>   boot/pxe_utils.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/boot/pxe_utils.c b/boot/pxe_utils.c
> index 8133006875f..099aa2f4bc7 100644
> --- a/boot/pxe_utils.c
> +++ b/boot/pxe_utils.c
> @@ -617,7 +617,7 @@ static int label_boot(struct pxe_context *ctx, struct pxe_label *label)
>   	 * bootm, and adjust argc appropriately.
>   	 *
>   	 * Scenario 3: If there is an fdtcontroladdr specified, pass it along to
> -	 * bootm, and adjust argc appropriately.
> +	 * bootm, and adjust argc appropriately, unless the image type is fitImage.
>   	 *
>   	 * Scenario 4: fdt blob is not available.
>   	 */
> @@ -724,7 +724,10 @@ static int label_boot(struct pxe_context *ctx, struct pxe_label *label)
>   	if (!bootm_argv[3])
>   		bootm_argv[3] = env_get("fdt_addr");
>   
> -	if (!bootm_argv[3])
> +	kernel_addr_r = genimg_get_kernel_addr(kernel_addr);
> +	buf = map_sysmem(kernel_addr_r, 0);
> +
> +	if (!bootm_argv[3] && genimg_get_format(buf) != IMAGE_FORMAT_FIT)
>   		bootm_argv[3] = env_get("fdtcontroladdr");
>   
>   	if (bootm_argv[3]) {
> @@ -733,8 +736,6 @@ static int label_boot(struct pxe_context *ctx, struct pxe_label *label)
>   		bootm_argc = 4;
>   	}
>   
> -	kernel_addr_r = genimg_get_kernel_addr(kernel_addr);
> -	buf = map_sysmem(kernel_addr_r, 0);
>   	/* Try bootm for legacy and FIT format image */
>   	if (genimg_get_format(buf) != IMAGE_FORMAT_INVALID &&
>               IS_ENABLED(CONFIG_CMD_BOOTM))

Re: [PATCH v2] cmd: pxe_utils: Limit fdtcontroladdr usage to non-fitImage

[Marek Vasut]

On 12/14/22 16:23, Quentin Schulz wrote:
> Hi Marek,

Hi,

> On 12/14/22 07:45, Marek Vasut wrote:
>> Commit d5ba6188dfb ("cmd: pxe_utils: Check fdtcontroladdr in label_boot")
>> forces '$fdtcontroladdr' DT address as a third parameter of bootm command
>> even if the PXE transfer pulls in a fitImage which contains configuration
>> node with its own DT that is preferrable to be passed to Linux. Limit the
>> $fdtcontroladdr fallback utilization to non-fitImages, since it is highly
>> likely a fitImage would come with its own DT, while single-file images do
>> need a separate DT.
>>
> 
> Reviewed-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> Tested-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> 
> Tested on top of an almost vanilla v2022.10 on Ringneck PX30, the FDT 
> from the selected fitimage configuration (via #conf in kernel field in 
> extlinux.conf) is taken into account.
> 
> Not sure overriding the DT gotten from the fit image wasn't a use-case 
> Peter wanted to support though.

I am hoping to get feedback from Peter, but that kind of behavior would 
be rather odd. If user wants to use fdtcontroladdr DT, then just don't 
add DT fdt property into the configuration node entry in the fitImage.

Re: [PATCH v2] cmd: pxe_utils: Limit fdtcontroladdr usage to non-fitImage

[Simon Glass]

On Tue, 13 Dec 2022 at 22:45, Marek Vasut <marex@denx.de> wrote:
>
> Commit d5ba6188dfb ("cmd: pxe_utils: Check fdtcontroladdr in label_boot")
> forces '$fdtcontroladdr' DT address as a third parameter of bootm command
> even if the PXE transfer pulls in a fitImage which contains configuration
> node with its own DT that is preferrable to be passed to Linux. Limit the
> $fdtcontroladdr fallback utilization to non-fitImages, since it is highly
> likely a fitImage would come with its own DT, while single-file images do
> need a separate DT.
>
> Fixes: d5ba6188dfb ("cmd: pxe_utils: Check fdtcontroladdr in label_boot")
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Peter Hoyes <Peter.Hoyes@arm.com>
> Cc: Ramon Fried <rfried.dev@gmail.com>
> Cc: Simon Glass <sjg@chromium.org>
> ---
> V1: Map the kernel buffer before testing image type
> V2: Update code comment to reflect the change
> ---
>  boot/pxe_utils.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/boot/pxe_utils.c b/boot/pxe_utils.c
> index 8133006875f..099aa2f4bc7 100644
> --- a/boot/pxe_utils.c
> +++ b/boot/pxe_utils.c
> @@ -617,7 +617,7 @@ static int label_boot(struct pxe_context *ctx, struct pxe_label *label)
>          * bootm, and adjust argc appropriately.
>          *
>          * Scenario 3: If there is an fdtcontroladdr specified, pass it along to
> -        * bootm, and adjust argc appropriately.
> +        * bootm, and adjust argc appropriately, unless the image type is fitImage.
>          *
>          * Scenario 4: fdt blob is not available.
>          */
> @@ -724,7 +724,10 @@ static int label_boot(struct pxe_context *ctx, struct pxe_label *label)
>         if (!bootm_argv[3])
>                 bootm_argv[3] = env_get("fdt_addr");
>
> -       if (!bootm_argv[3])
> +       kernel_addr_r = genimg_get_kernel_addr(kernel_addr);
> +       buf = map_sysmem(kernel_addr_r, 0);
> +
> +       if (!bootm_argv[3] && genimg_get_format(buf) != IMAGE_FORMAT_FIT)
>                 bootm_argv[3] = env_get("fdtcontroladdr");
>
>         if (bootm_argv[3]) {
> @@ -733,8 +736,6 @@ static int label_boot(struct pxe_context *ctx, struct pxe_label *label)
>                 bootm_argc = 4;
>         }
>
> -       kernel_addr_r = genimg_get_kernel_addr(kernel_addr);
> -       buf = map_sysmem(kernel_addr_r, 0);
>         /* Try bootm for legacy and FIT format image */
>         if (genimg_get_format(buf) != IMAGE_FORMAT_INVALID &&
>              IS_ENABLED(CONFIG_CMD_BOOTM))
> --
> 2.35.1
>

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH v2] cmd: pxe_utils: Limit fdtcontroladdr usage to non-fitImage

[Peter Hoyes]

On 14/12/2022 15:25, Marek Vasut wrote:
> On 12/14/22 16:23, Quentin Schulz wrote:
>> Hi Marek,
>
> Hi,
>
>> On 12/14/22 07:45, Marek Vasut wrote:
>>> Commit d5ba6188dfb ("cmd: pxe_utils: Check fdtcontroladdr in 
>>> label_boot")
>>> forces '$fdtcontroladdr' DT address as a third parameter of bootm 
>>> command
>>> even if the PXE transfer pulls in a fitImage which contains 
>>> configuration
>>> node with its own DT that is preferrable to be passed to Linux. 
>>> Limit the
>>> $fdtcontroladdr fallback utilization to non-fitImages, since it is 
>>> highly
>>> likely a fitImage would come with its own DT, while single-file 
>>> images do
>>> need a separate DT.
>>>
>>
>> Reviewed-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>> Tested-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>>
>> Tested on top of an almost vanilla v2022.10 on Ringneck PX30, the FDT 
>> from the selected fitimage configuration (via #conf in kernel field 
>> in extlinux.conf) is taken into account.
>>
>> Not sure overriding the DT gotten from the fit image wasn't a 
>> use-case Peter wanted to support though.
>
> I am hoping to get feedback from Peter, but that kind of behavior 
> would be rather odd. If user wants to use fdtcontroladdr DT, then just 
> don't add DT fdt property into the configuration node entry in the 
> fitImage.

Reviewed-by: Peter Hoyes <peter.hoyes@arm.com>
Tested-by: Peter Hoyes <peter.hoyes@arm.com>

Tested with an fdtcontroladdr provided by a prior boot stage (TF-A) on 
an Arm FVP.

The implemented behavior works for me. We do not have any need to 
override the DT from a FIT image.

Thanks,

Peter

Re: [PATCH v2] cmd: pxe_utils: Limit fdtcontroladdr usage to non-fitImage

[Neil Armstrong]

On 14/12/2022 07:45, Marek Vasut wrote:
> Commit d5ba6188dfb ("cmd: pxe_utils: Check fdtcontroladdr in label_boot")
> forces '$fdtcontroladdr' DT address as a third parameter of bootm command
> even if the PXE transfer pulls in a fitImage which contains configuration
> node with its own DT that is preferrable to be passed to Linux. Limit the
> $fdtcontroladdr fallback utilization to non-fitImages, since it is highly
> likely a fitImage would come with its own DT, while single-file images do
> need a separate DT.
> 
> Fixes: d5ba6188dfb ("cmd: pxe_utils: Check fdtcontroladdr in label_boot")
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Peter Hoyes <Peter.Hoyes@arm.com>
> Cc: Ramon Fried <rfried.dev@gmail.com>
> Cc: Simon Glass <sjg@chromium.org>
> ---
> V1: Map the kernel buffer before testing image type
> V2: Update code comment to reflect the change
> ---
>   boot/pxe_utils.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/boot/pxe_utils.c b/boot/pxe_utils.c
> index 8133006875f..099aa2f4bc7 100644
> --- a/boot/pxe_utils.c
> +++ b/boot/pxe_utils.c
> @@ -617,7 +617,7 @@ static int label_boot(struct pxe_context *ctx, struct pxe_label *label)
>   	 * bootm, and adjust argc appropriately.
>   	 *
>   	 * Scenario 3: If there is an fdtcontroladdr specified, pass it along to
> -	 * bootm, and adjust argc appropriately.
> +	 * bootm, and adjust argc appropriately, unless the image type is fitImage.
>   	 *
>   	 * Scenario 4: fdt blob is not available.
>   	 */
> @@ -724,7 +724,10 @@ static int label_boot(struct pxe_context *ctx, struct pxe_label *label)
>   	if (!bootm_argv[3])
>   		bootm_argv[3] = env_get("fdt_addr");
>   
> -	if (!bootm_argv[3])
> +	kernel_addr_r = genimg_get_kernel_addr(kernel_addr);
> +	buf = map_sysmem(kernel_addr_r, 0);
> +
> +	if (!bootm_argv[3] && genimg_get_format(buf) != IMAGE_FORMAT_FIT)
>   		bootm_argv[3] = env_get("fdtcontroladdr");
>   
>   	if (bootm_argv[3]) {
> @@ -733,8 +736,6 @@ static int label_boot(struct pxe_context *ctx, struct pxe_label *label)
>   		bootm_argc = 4;
>   	}
>   
> -	kernel_addr_r = genimg_get_kernel_addr(kernel_addr);
> -	buf = map_sysmem(kernel_addr_r, 0);
>   	/* Try bootm for legacy and FIT format image */
>   	if (genimg_get_format(buf) != IMAGE_FORMAT_INVALID &&
>               IS_ENABLED(CONFIG_CMD_BOOTM))



Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

Re: [PATCH v2] cmd: pxe_utils: Limit fdtcontroladdr usage to non-fitImage

[Tom Rini]

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

On Wed, Dec 14, 2022 at 07:45:18AM +0100, Marek Vasut wrote:

> Commit d5ba6188dfb ("cmd: pxe_utils: Check fdtcontroladdr in label_boot")
> forces '$fdtcontroladdr' DT address as a third parameter of bootm command
> even if the PXE transfer pulls in a fitImage which contains configuration
> node with its own DT that is preferrable to be passed to Linux. Limit the
> $fdtcontroladdr fallback utilization to non-fitImages, since it is highly
> likely a fitImage would come with its own DT, while single-file images do
> need a separate DT.
> 
> Fixes: d5ba6188dfb ("cmd: pxe_utils: Check fdtcontroladdr in label_boot")
> Signed-off-by: Marek Vasut <marex@denx.de>
> Reviewed-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> Tested-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> Reviewed-by: Peter Hoyes <peter.hoyes@arm.com>
> Tested-by: Peter Hoyes <peter.hoyes@arm.com>
> Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]