| Message ID | 20221214064518.753432-1-marex@denx.de |
|---|---|
| State | Accepted |
| Commit | 519e6641dbdd46907feac094ccd032743f4174b0 |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [v2] cmd: pxe_utils: Limit fdtcontroladdr usage to non-fitImage | expand |
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))
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.
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>
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
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>
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!
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))
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(-)