Message ID | 20230125031151.69027-1-sjg@chromium.org |
---|---|
State | Changes Requested |
Delegated to: | Heinrich Schuchardt |
Headers | show |
Series | bootstd: Replicate the dtb-filename quirks of distroboot | expand |
On 1/25/23 04:11, Simon Glass wrote: > For EFI, the distro boot scripts search in three different directories > for the .dtb file. The SOC-based filename fallback is supported only for > 32-bit ARM. > > Adjust the code to mirror this behaviour. > > Signed-off-by: Simon Glass <sjg@chromium.org> > Suggested-by: Mark Kettenis <kettenis@openbsd.org> > --- > > boot/bootmeth_efi.c | 63 ++++++++++++++++++++++++++++++++++++++------- > 1 file changed, 54 insertions(+), 9 deletions(-) > > diff --git a/boot/bootmeth_efi.c b/boot/bootmeth_efi.c > index 67c972e3fe4..f9544846e68 100644 > --- a/boot/bootmeth_efi.c > +++ b/boot/bootmeth_efi.c > @@ -147,25 +147,57 @@ static int distro_efi_check(struct udevice *dev, struct bootflow_iter *iter) > return 0; > } > > -static void distro_efi_get_fdt_name(char *fname, int size) > +/** > + * distro_efi_get_fdt_name() - Get the filename for reading the .dtb file > + * > + * @fname: Place to put filename > + * @size: Max size of filename > + * @seq: Sequence number, to cycle through options (0=first) > + * Returns: 0 on success, -ENOENT if the "fdtfile" env var does not exist, > + * -EINVAL if there are no more options > + */ > +static int distro_efi_get_fdt_name(char *fname, int size, int seq) > { > const char *fdt_fname; > + const char *prefix; > + > + /* select the prefix */ > + switch (seq) { > + case 0: > + /* this is the default */ > + prefix = "/dtb"; > + break; > + case 1: > + prefix = ""; > + break; > + case 2: > + prefix = "/dtb/current"; > + break; > + default: > + return log_msg_ret("pref", -EINVAL); > + } > > fdt_fname = env_get("fdtfile"); > if (fdt_fname) { > - snprintf(fname, size, "dtb/%s", fdt_fname); > + snprintf(fname, size, "%s/%s", prefix, fdt_fname); > log_debug("Using device tree: %s\n", fname); > - } else { > + > + /* Use this fallback only for 32-bit ARM */ > + } else if (IS_ENABLED(CONFIG_ARM) && !IS_ENABLED(CONFIG_ARM64)) { > const char *soc = env_get("soc"); > const char *board = env_get("board"); > const char *boardver = env_get("boardver"); > > /* cf the code in label_boot() which seems very complex */ > - snprintf(fname, size, "dtb/%s%s%s%s.dtb", > + snprintf(fname, size, "%s/%s%s%s%s.dtb", prefix, > soc ? soc : "", soc ? "-" : "", board ? board : "", > boardver ? boardver : ""); > log_debug("Using default device tree: %s\n", fname); > + } else { > + return log_msg_ret("env", -ENOENT); > } > + > + return 0; > } > > static int distro_efi_read_bootflow_file(struct udevice *dev, > @@ -174,7 +206,7 @@ static int distro_efi_read_bootflow_file(struct udevice *dev, > struct blk_desc *desc = NULL; > ulong fdt_addr, size; > char fname[256]; > - int ret; > + int ret, seq; > > /* We require a partition table */ > if (!bflow->part) > @@ -196,13 +228,22 @@ static int distro_efi_read_bootflow_file(struct udevice *dev, > if (ret) > return log_msg_ret("read", -EINVAL); > > - distro_efi_get_fdt_name(fname, sizeof(fname)); > + fdt_addr = env_get_hex("fdt_addr_r", 0); > + > + /* try the various available names */ > + ret = -ENOENT; > + for (seq = 0; ret; seq++) There are boards that don't have a dtb file available and that is ok. Don't loop past seq = 2. { > + ret = distro_efi_get_fdt_name(fname, sizeof(fname), seq); > + if (ret) > + return log_msg_ret("nam", ret); > + ret = bootmeth_common_read_file(dev, bflow, fname, fdt_addr, > + &size); > + } > + > bflow->fdt_fname = strdup(fname); > if (!bflow->fdt_fname) > return log_msg_ret("fil", -ENOMEM); This should not be an error. The Distroboot fallback is the device-tree at $fdtcontroladdr. Best regards Heinrich > > - fdt_addr = env_get_hex("fdt_addr_r", 0); > - ret = bootmeth_common_read_file(dev, bflow, fname, fdt_addr, &size); > if (!ret) { > bflow->fdt_size = size; > bflow->fdt_addr = fdt_addr; > @@ -277,7 +318,11 @@ static int distro_efi_read_bootflow_net(struct bootflow *bflow) > fdt_addr = hextoul(fdt_addr_str, NULL); > sprintf(file_addr, "%lx", fdt_addr); > > - distro_efi_get_fdt_name(fname, sizeof(fname)); > + /* We only allow the first prefix with PXE */ > + ret = distro_efi_get_fdt_name(fname, sizeof(fname), 0); > + if (ret) > + return log_msg_ret("nam", ret); > + > bflow->fdt_fname = strdup(fname); > if (!bflow->fdt_fname) > return log_msg_ret("fil", -ENOMEM);
On Wed, Jan 25, 2023 at 04:38:53PM +0100, Heinrich Schuchardt wrote: > On 1/25/23 04:11, Simon Glass wrote: > > For EFI, the distro boot scripts search in three different directories > > for the .dtb file. The SOC-based filename fallback is supported only for > > 32-bit ARM. > > > > Adjust the code to mirror this behaviour. > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > Suggested-by: Mark Kettenis <kettenis@openbsd.org> > > --- > > > > boot/bootmeth_efi.c | 63 ++++++++++++++++++++++++++++++++++++++------- > > 1 file changed, 54 insertions(+), 9 deletions(-) > > > > diff --git a/boot/bootmeth_efi.c b/boot/bootmeth_efi.c > > index 67c972e3fe4..f9544846e68 100644 > > --- a/boot/bootmeth_efi.c > > +++ b/boot/bootmeth_efi.c > > @@ -147,25 +147,57 @@ static int distro_efi_check(struct udevice *dev, struct bootflow_iter *iter) > > return 0; > > } > > -static void distro_efi_get_fdt_name(char *fname, int size) > > +/** > > + * distro_efi_get_fdt_name() - Get the filename for reading the .dtb file > > + * > > + * @fname: Place to put filename > > + * @size: Max size of filename > > + * @seq: Sequence number, to cycle through options (0=first) > > + * Returns: 0 on success, -ENOENT if the "fdtfile" env var does not exist, > > + * -EINVAL if there are no more options > > + */ > > +static int distro_efi_get_fdt_name(char *fname, int size, int seq) > > { > > const char *fdt_fname; > > + const char *prefix; > > + > > + /* select the prefix */ > > + switch (seq) { > > + case 0: > > + /* this is the default */ > > + prefix = "/dtb"; > > + break; > > + case 1: > > + prefix = ""; > > + break; > > + case 2: > > + prefix = "/dtb/current"; > > + break; > > + default: > > + return log_msg_ret("pref", -EINVAL); > > + } > > fdt_fname = env_get("fdtfile"); > > if (fdt_fname) { > > - snprintf(fname, size, "dtb/%s", fdt_fname); > > + snprintf(fname, size, "%s/%s", prefix, fdt_fname); > > log_debug("Using device tree: %s\n", fname); > > - } else { > > + > > + /* Use this fallback only for 32-bit ARM */ > > + } else if (IS_ENABLED(CONFIG_ARM) && !IS_ENABLED(CONFIG_ARM64)) { > > const char *soc = env_get("soc"); > > const char *board = env_get("board"); > > const char *boardver = env_get("boardver"); > > /* cf the code in label_boot() which seems very complex */ > > - snprintf(fname, size, "dtb/%s%s%s%s.dtb", > > + snprintf(fname, size, "%s/%s%s%s%s.dtb", prefix, > > soc ? soc : "", soc ? "-" : "", board ? board : "", > > boardver ? boardver : ""); > > log_debug("Using default device tree: %s\n", fname); > > + } else { > > + return log_msg_ret("env", -ENOENT); > > } > > + > > + return 0; > > } > > static int distro_efi_read_bootflow_file(struct udevice *dev, > > @@ -174,7 +206,7 @@ static int distro_efi_read_bootflow_file(struct udevice *dev, > > struct blk_desc *desc = NULL; > > ulong fdt_addr, size; > > char fname[256]; > > - int ret; > > + int ret, seq; > > /* We require a partition table */ > > if (!bflow->part) > > @@ -196,13 +228,22 @@ static int distro_efi_read_bootflow_file(struct udevice *dev, > > if (ret) > > return log_msg_ret("read", -EINVAL); > > - distro_efi_get_fdt_name(fname, sizeof(fname)); > > + fdt_addr = env_get_hex("fdt_addr_r", 0); > > + > > + /* try the various available names */ > > + ret = -ENOENT; > > + for (seq = 0; ret; seq++) > > There are boards that don't have a dtb file available and that is ok. Don't > loop past seq = 2. > > { > > + ret = distro_efi_get_fdt_name(fname, sizeof(fname), seq); > > + if (ret) > > + return log_msg_ret("nam", ret); > > + ret = bootmeth_common_read_file(dev, bflow, fname, fdt_addr, > > + &size); > > + } > > + > > bflow->fdt_fname = strdup(fname); > > if (!bflow->fdt_fname) > > return log_msg_ret("fil", -ENOMEM); > > This should not be an error. The Distroboot fallback is the device-tree at > $fdtcontroladdr. But it should note that it's using that because that's still quite often an unexpected feature to people.
On 1/25/23 17:15, Tom Rini wrote: > On Wed, Jan 25, 2023 at 04:38:53PM +0100, Heinrich Schuchardt wrote: >> On 1/25/23 04:11, Simon Glass wrote: >>> For EFI, the distro boot scripts search in three different directories >>> for the .dtb file. The SOC-based filename fallback is supported only for >>> 32-bit ARM. >>> >>> Adjust the code to mirror this behaviour. >>> >>> Signed-off-by: Simon Glass <sjg@chromium.org> >>> Suggested-by: Mark Kettenis <kettenis@openbsd.org> >>> --- >>> >>> boot/bootmeth_efi.c | 63 ++++++++++++++++++++++++++++++++++++++------- >>> 1 file changed, 54 insertions(+), 9 deletions(-) >>> >>> diff --git a/boot/bootmeth_efi.c b/boot/bootmeth_efi.c >>> index 67c972e3fe4..f9544846e68 100644 >>> --- a/boot/bootmeth_efi.c >>> +++ b/boot/bootmeth_efi.c >>> @@ -147,25 +147,57 @@ static int distro_efi_check(struct udevice *dev, struct bootflow_iter *iter) >>> return 0; >>> } >>> -static void distro_efi_get_fdt_name(char *fname, int size) >>> +/** >>> + * distro_efi_get_fdt_name() - Get the filename for reading the .dtb file >>> + * >>> + * @fname: Place to put filename >>> + * @size: Max size of filename >>> + * @seq: Sequence number, to cycle through options (0=first) >>> + * Returns: 0 on success, -ENOENT if the "fdtfile" env var does not exist, >>> + * -EINVAL if there are no more options >>> + */ >>> +static int distro_efi_get_fdt_name(char *fname, int size, int seq) >>> { >>> const char *fdt_fname; >>> + const char *prefix; >>> + >>> + /* select the prefix */ >>> + switch (seq) { >>> + case 0: >>> + /* this is the default */ >>> + prefix = "/dtb"; >>> + break; >>> + case 1: >>> + prefix = ""; >>> + break; >>> + case 2: >>> + prefix = "/dtb/current"; >>> + break; >>> + default: >>> + return log_msg_ret("pref", -EINVAL); >>> + } >>> fdt_fname = env_get("fdtfile"); >>> if (fdt_fname) { >>> - snprintf(fname, size, "dtb/%s", fdt_fname); >>> + snprintf(fname, size, "%s/%s", prefix, fdt_fname); >>> log_debug("Using device tree: %s\n", fname); >>> - } else { >>> + >>> + /* Use this fallback only for 32-bit ARM */ >>> + } else if (IS_ENABLED(CONFIG_ARM) && !IS_ENABLED(CONFIG_ARM64)) { >>> const char *soc = env_get("soc"); >>> const char *board = env_get("board"); >>> const char *boardver = env_get("boardver"); >>> /* cf the code in label_boot() which seems very complex */ >>> - snprintf(fname, size, "dtb/%s%s%s%s.dtb", >>> + snprintf(fname, size, "%s/%s%s%s%s.dtb", prefix, >>> soc ? soc : "", soc ? "-" : "", board ? board : "", >>> boardver ? boardver : ""); >>> log_debug("Using default device tree: %s\n", fname); >>> + } else { >>> + return log_msg_ret("env", -ENOENT); >>> } >>> + >>> + return 0; >>> } >>> static int distro_efi_read_bootflow_file(struct udevice *dev, >>> @@ -174,7 +206,7 @@ static int distro_efi_read_bootflow_file(struct udevice *dev, >>> struct blk_desc *desc = NULL; >>> ulong fdt_addr, size; >>> char fname[256]; >>> - int ret; >>> + int ret, seq; >>> /* We require a partition table */ >>> if (!bflow->part) >>> @@ -196,13 +228,22 @@ static int distro_efi_read_bootflow_file(struct udevice *dev, >>> if (ret) >>> return log_msg_ret("read", -EINVAL); >>> - distro_efi_get_fdt_name(fname, sizeof(fname)); >>> + fdt_addr = env_get_hex("fdt_addr_r", 0); >>> + >>> + /* try the various available names */ >>> + ret = -ENOENT; >>> + for (seq = 0; ret; seq++) >> >> There are boards that don't have a dtb file available and that is ok. Don't >> loop past seq = 2. >> >> { >>> + ret = distro_efi_get_fdt_name(fname, sizeof(fname), seq); >>> + if (ret) >>> + return log_msg_ret("nam", ret); >>> + ret = bootmeth_common_read_file(dev, bflow, fname, fdt_addr, >>> + &size); >>> + } >>> + >>> bflow->fdt_fname = strdup(fname); >>> if (!bflow->fdt_fname) >>> return log_msg_ret("fil", -ENOMEM); >> >> This should not be an error. The Distroboot fallback is the device-tree at >> $fdtcontroladdr. > > But it should note that it's using that because that's still quite often > an unexpected feature to people. > On QEMU it is just what is needed. Other boards supply the device-tree via TF-A or OpenSBI. We should not start breaking boards. A message "fil: returning err= -12" signals not caring about users. Best regards Heinrich
On Wed, Jan 25, 2023 at 06:04:56PM +0100, Heinrich Schuchardt wrote: > On 1/25/23 17:15, Tom Rini wrote: > > On Wed, Jan 25, 2023 at 04:38:53PM +0100, Heinrich Schuchardt wrote: > > > On 1/25/23 04:11, Simon Glass wrote: > > > > For EFI, the distro boot scripts search in three different directories > > > > for the .dtb file. The SOC-based filename fallback is supported only for > > > > 32-bit ARM. > > > > > > > > Adjust the code to mirror this behaviour. > > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > > > Suggested-by: Mark Kettenis <kettenis@openbsd.org> > > > > --- > > > > > > > > boot/bootmeth_efi.c | 63 ++++++++++++++++++++++++++++++++++++++------- > > > > 1 file changed, 54 insertions(+), 9 deletions(-) > > > > > > > > diff --git a/boot/bootmeth_efi.c b/boot/bootmeth_efi.c > > > > index 67c972e3fe4..f9544846e68 100644 > > > > --- a/boot/bootmeth_efi.c > > > > +++ b/boot/bootmeth_efi.c > > > > @@ -147,25 +147,57 @@ static int distro_efi_check(struct udevice *dev, struct bootflow_iter *iter) > > > > return 0; > > > > } > > > > -static void distro_efi_get_fdt_name(char *fname, int size) > > > > +/** > > > > + * distro_efi_get_fdt_name() - Get the filename for reading the .dtb file > > > > + * > > > > + * @fname: Place to put filename > > > > + * @size: Max size of filename > > > > + * @seq: Sequence number, to cycle through options (0=first) > > > > + * Returns: 0 on success, -ENOENT if the "fdtfile" env var does not exist, > > > > + * -EINVAL if there are no more options > > > > + */ > > > > +static int distro_efi_get_fdt_name(char *fname, int size, int seq) > > > > { > > > > const char *fdt_fname; > > > > + const char *prefix; > > > > + > > > > + /* select the prefix */ > > > > + switch (seq) { > > > > + case 0: > > > > + /* this is the default */ > > > > + prefix = "/dtb"; > > > > + break; > > > > + case 1: > > > > + prefix = ""; > > > > + break; > > > > + case 2: > > > > + prefix = "/dtb/current"; > > > > + break; > > > > + default: > > > > + return log_msg_ret("pref", -EINVAL); > > > > + } > > > > fdt_fname = env_get("fdtfile"); > > > > if (fdt_fname) { > > > > - snprintf(fname, size, "dtb/%s", fdt_fname); > > > > + snprintf(fname, size, "%s/%s", prefix, fdt_fname); > > > > log_debug("Using device tree: %s\n", fname); > > > > - } else { > > > > + > > > > + /* Use this fallback only for 32-bit ARM */ > > > > + } else if (IS_ENABLED(CONFIG_ARM) && !IS_ENABLED(CONFIG_ARM64)) { > > > > const char *soc = env_get("soc"); > > > > const char *board = env_get("board"); > > > > const char *boardver = env_get("boardver"); > > > > /* cf the code in label_boot() which seems very complex */ > > > > - snprintf(fname, size, "dtb/%s%s%s%s.dtb", > > > > + snprintf(fname, size, "%s/%s%s%s%s.dtb", prefix, > > > > soc ? soc : "", soc ? "-" : "", board ? board : "", > > > > boardver ? boardver : ""); > > > > log_debug("Using default device tree: %s\n", fname); > > > > + } else { > > > > + return log_msg_ret("env", -ENOENT); > > > > } > > > > + > > > > + return 0; > > > > } > > > > static int distro_efi_read_bootflow_file(struct udevice *dev, > > > > @@ -174,7 +206,7 @@ static int distro_efi_read_bootflow_file(struct udevice *dev, > > > > struct blk_desc *desc = NULL; > > > > ulong fdt_addr, size; > > > > char fname[256]; > > > > - int ret; > > > > + int ret, seq; > > > > /* We require a partition table */ > > > > if (!bflow->part) > > > > @@ -196,13 +228,22 @@ static int distro_efi_read_bootflow_file(struct udevice *dev, > > > > if (ret) > > > > return log_msg_ret("read", -EINVAL); > > > > - distro_efi_get_fdt_name(fname, sizeof(fname)); > > > > + fdt_addr = env_get_hex("fdt_addr_r", 0); > > > > + > > > > + /* try the various available names */ > > > > + ret = -ENOENT; > > > > + for (seq = 0; ret; seq++) > > > > > > There are boards that don't have a dtb file available and that is ok. Don't > > > loop past seq = 2. > > > > > > { > > > > + ret = distro_efi_get_fdt_name(fname, sizeof(fname), seq); > > > > + if (ret) > > > > + return log_msg_ret("nam", ret); > > > > + ret = bootmeth_common_read_file(dev, bflow, fname, fdt_addr, > > > > + &size); > > > > + } > > > > + > > > > bflow->fdt_fname = strdup(fname); > > > > if (!bflow->fdt_fname) > > > > return log_msg_ret("fil", -ENOMEM); > > > > > > This should not be an error. The Distroboot fallback is the device-tree at > > > $fdtcontroladdr. > > > > But it should note that it's using that because that's still quite often > > an unexpected feature to people. > > > > On QEMU it is just what is needed. Other boards supply the device-tree via > TF-A or OpenSBI. We should not start breaking boards. > > A message "fil: returning err= -12" signals not caring about users. Yes, I'm saying we need a positive message when using the in memory device tree.
diff --git a/boot/bootmeth_efi.c b/boot/bootmeth_efi.c index 67c972e3fe4..f9544846e68 100644 --- a/boot/bootmeth_efi.c +++ b/boot/bootmeth_efi.c @@ -147,25 +147,57 @@ static int distro_efi_check(struct udevice *dev, struct bootflow_iter *iter) return 0; } -static void distro_efi_get_fdt_name(char *fname, int size) +/** + * distro_efi_get_fdt_name() - Get the filename for reading the .dtb file + * + * @fname: Place to put filename + * @size: Max size of filename + * @seq: Sequence number, to cycle through options (0=first) + * Returns: 0 on success, -ENOENT if the "fdtfile" env var does not exist, + * -EINVAL if there are no more options + */ +static int distro_efi_get_fdt_name(char *fname, int size, int seq) { const char *fdt_fname; + const char *prefix; + + /* select the prefix */ + switch (seq) { + case 0: + /* this is the default */ + prefix = "/dtb"; + break; + case 1: + prefix = ""; + break; + case 2: + prefix = "/dtb/current"; + break; + default: + return log_msg_ret("pref", -EINVAL); + } fdt_fname = env_get("fdtfile"); if (fdt_fname) { - snprintf(fname, size, "dtb/%s", fdt_fname); + snprintf(fname, size, "%s/%s", prefix, fdt_fname); log_debug("Using device tree: %s\n", fname); - } else { + + /* Use this fallback only for 32-bit ARM */ + } else if (IS_ENABLED(CONFIG_ARM) && !IS_ENABLED(CONFIG_ARM64)) { const char *soc = env_get("soc"); const char *board = env_get("board"); const char *boardver = env_get("boardver"); /* cf the code in label_boot() which seems very complex */ - snprintf(fname, size, "dtb/%s%s%s%s.dtb", + snprintf(fname, size, "%s/%s%s%s%s.dtb", prefix, soc ? soc : "", soc ? "-" : "", board ? board : "", boardver ? boardver : ""); log_debug("Using default device tree: %s\n", fname); + } else { + return log_msg_ret("env", -ENOENT); } + + return 0; } static int distro_efi_read_bootflow_file(struct udevice *dev, @@ -174,7 +206,7 @@ static int distro_efi_read_bootflow_file(struct udevice *dev, struct blk_desc *desc = NULL; ulong fdt_addr, size; char fname[256]; - int ret; + int ret, seq; /* We require a partition table */ if (!bflow->part) @@ -196,13 +228,22 @@ static int distro_efi_read_bootflow_file(struct udevice *dev, if (ret) return log_msg_ret("read", -EINVAL); - distro_efi_get_fdt_name(fname, sizeof(fname)); + fdt_addr = env_get_hex("fdt_addr_r", 0); + + /* try the various available names */ + ret = -ENOENT; + for (seq = 0; ret; seq++) { + ret = distro_efi_get_fdt_name(fname, sizeof(fname), seq); + if (ret) + return log_msg_ret("nam", ret); + ret = bootmeth_common_read_file(dev, bflow, fname, fdt_addr, + &size); + } + bflow->fdt_fname = strdup(fname); if (!bflow->fdt_fname) return log_msg_ret("fil", -ENOMEM); - fdt_addr = env_get_hex("fdt_addr_r", 0); - ret = bootmeth_common_read_file(dev, bflow, fname, fdt_addr, &size); if (!ret) { bflow->fdt_size = size; bflow->fdt_addr = fdt_addr; @@ -277,7 +318,11 @@ static int distro_efi_read_bootflow_net(struct bootflow *bflow) fdt_addr = hextoul(fdt_addr_str, NULL); sprintf(file_addr, "%lx", fdt_addr); - distro_efi_get_fdt_name(fname, sizeof(fname)); + /* We only allow the first prefix with PXE */ + ret = distro_efi_get_fdt_name(fname, sizeof(fname), 0); + if (ret) + return log_msg_ret("nam", ret); + bflow->fdt_fname = strdup(fname); if (!bflow->fdt_fname) return log_msg_ret("fil", -ENOMEM);
For EFI, the distro boot scripts search in three different directories for the .dtb file. The SOC-based filename fallback is supported only for 32-bit ARM. Adjust the code to mirror this behaviour. Signed-off-by: Simon Glass <sjg@chromium.org> Suggested-by: Mark Kettenis <kettenis@openbsd.org> --- boot/bootmeth_efi.c | 63 ++++++++++++++++++++++++++++++++++++++------- 1 file changed, 54 insertions(+), 9 deletions(-)