| Message ID | 20230104025330.2648640-1-sandeep.gundlupet-raju@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] kernel-fitimage: Adjust order of dtb/dtbo files | expand |
Hi Sandeep, On 1/4/23 03:53, Sandeep Gundlupet Raju via lists.openembedded.org wrote: > The dtb files must be before the dtbo files, otherwise the overlays may > not be applied correctly. > > From Bruce Ashfield: > > We can split between dtbs and dtbos, they just need to be sorted > for reproducibility reasons. Two loops versus one, would be > fine, with not too much duplicated code. > > Of course, this was only working by luck previously (before the > sort), since it has always been gathering dtbs and dtbo's with > find, depending on filesystem ordering for the order in the > fitimage). > > Signed-off-by: Sandeep Gundlupet Raju <sandeep.gundlupet-raju@amd.com> > --- > meta/classes-recipe/kernel-fitimage.bbclass | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/meta/classes-recipe/kernel-fitimage.bbclass b/meta/classes-recipe/kernel-fitimage.bbclass > index 7980910aa8..cb6635a673 100644 > --- a/meta/classes-recipe/kernel-fitimage.bbclass > +++ b/meta/classes-recipe/kernel-fitimage.bbclass > @@ -590,7 +590,7 @@ fitimage_assemble() { > > if [ -n "${EXTERNAL_KERNEL_DEVICETREE}" ]; then > dtbcount=1 > - for DTB in $(find "${EXTERNAL_KERNEL_DEVICETREE}" \( -name '*.dtb' -o -name '*.dtbo' \) -printf '%P\n' | sort); do > + for DTB in $(find "${EXTERNAL_KERNEL_DEVICETREE}" -name '*.dtb' -printf '%P\n' | sort); do I believe you could have for DTB in $(find "${EXTERNAL_KERNEL_DEVICETREE}" -name '*.dtb' -printf '%P\n' | sort) $(find "${EXTERNAL_KERNEL_DEVICETREE}" -name '*.dtbo' -printf '%P\n' | sort); do would make sure dtbs appear before dtbos in the fitimage sections but still use the same logic within the loop without going for code duplication. I believe the issue is the content of the DTBS variable, which might start with a dtbo file. When this happens, the default configuration for the fitimage takes the dtbo only and I guess this is your bug. We could also decide to have the original loop but have two variables: DTBS for dtbs only, matched on file extension and DTBOS for dtbos only Then you'd iterate over DTBS for fitimage_emit_section_config first and then DTBOS. I'm also not sure we need to create fit configuration nodes for dtbos only? but maybe there's something specific to security here? Cheers, Quentin
Hi Quentin, Thanks for the review. On 1/4/2023 4:36 AM, Quentin Schulz wrote: > Hi Sandeep, > > On 1/4/23 03:53, Sandeep Gundlupet Raju via lists.openembedded.org wrote: >> The dtb files must be before the dtbo files, otherwise the overlays may >> not be applied correctly. >> >> From Bruce Ashfield: >> >> We can split between dtbs and dtbos, they just need to be sorted >> for reproducibility reasons. Two loops versus one, would be >> fine, with not too much duplicated code. >> >> Of course, this was only working by luck previously (before the >> sort), since it has always been gathering dtbs and dtbo's with >> find, depending on filesystem ordering for the order in the >> fitimage). >> >> Signed-off-by: Sandeep Gundlupet Raju <sandeep.gundlupet-raju@amd.com> >> --- >> meta/classes-recipe/kernel-fitimage.bbclass | 14 +++++++++++++- >> 1 file changed, 13 insertions(+), 1 deletion(-) >> >> diff --git a/meta/classes-recipe/kernel-fitimage.bbclass b/meta/classes-recipe/kernel-fitimage.bbclass >> index 7980910aa8..cb6635a673 100644 >> --- a/meta/classes-recipe/kernel-fitimage.bbclass >> +++ b/meta/classes-recipe/kernel-fitimage.bbclass >> @@ -590,7 +590,7 @@ fitimage_assemble() { >> >> if [ -n "${EXTERNAL_KERNEL_DEVICETREE}" ]; then >> dtbcount=1 >> - for DTB in $(find "${EXTERNAL_KERNEL_DEVICETREE}" \( -name '*.dtb' -o -name '*.dtbo' \) -printf '%P\n' | sort); do >> + for DTB in $(find "${EXTERNAL_KERNEL_DEVICETREE}" -name '*.dtb' -printf '%P\n' | sort); do > I believe you could have > > for DTB in $(find "${EXTERNAL_KERNEL_DEVICETREE}" -name '*.dtb' -printf > '%P\n' | sort) $(find "${EXTERNAL_KERNEL_DEVICETREE}" -name '*.dtbo' > -printf '%P\n' | sort); do > > would make sure dtbs appear before dtbos in the fitimage sections but > still use the same logic within the loop without going for code duplication. [Sandeep]: Good thought I echo you on this one, I'll send a v2 patch with changes. > > I believe the issue is the content of the DTBS variable, which might > start with a dtbo file. > When this happens, the default configuration for the fitimage takes the > dtbo only and I guess this is your bug. [Sandeep]: Yes that correct, it fitimage picked the dtb based on sorting files in alphabetical order and in our case it looks like below, Where it picked up default.dtbo as default which caused boot issue. -rw-r--r-- 8 sandeepg boulder 480 Jan 3 11:33 default.dtbo -rw-r--r-- 8 sandeepg boulder 29K Jan 3 11:33 system-default.dtb -rw-r--r-- 8 sandeepg boulder 33K Jan 3 11:33 system-top.dtb -rw-r--r-- 8 sandeepg boulder 29K Jan 3 11:33 system-vck190-rev1.dtb -rw-r--r-- 8 sandeepg boulder 435 Jan 3 11:33 vck190-rev1.dtbo > > We could also decide to have the original loop but have two variables: > DTBS for dtbs only, matched on file extension > and DTBOS for dtbos only > > Then you'd iterate over DTBS for fitimage_emit_section_config first and > then DTBOS. > > I'm also not sure we need to create fit configuration nodes for dtbos > only? but maybe there's something specific to security here? [Sandeep]: I will defer this question to Bruce Ashfield and Mark Hatle. > > Cheers, > Quentin
diff --git a/meta/classes-recipe/kernel-fitimage.bbclass b/meta/classes-recipe/kernel-fitimage.bbclass index 7980910aa8..cb6635a673 100644 --- a/meta/classes-recipe/kernel-fitimage.bbclass +++ b/meta/classes-recipe/kernel-fitimage.bbclass @@ -590,7 +590,7 @@ fitimage_assemble() { if [ -n "${EXTERNAL_KERNEL_DEVICETREE}" ]; then dtbcount=1 - for DTB in $(find "${EXTERNAL_KERNEL_DEVICETREE}" \( -name '*.dtb' -o -name '*.dtbo' \) -printf '%P\n' | sort); do + for DTB in $(find "${EXTERNAL_KERNEL_DEVICETREE}" -name '*.dtb' -printf '%P\n' | sort); do # Skip DTB if we've picked it up previously echo "$DTBS" | tr ' ' '\n' | grep -xq "$DTB" && continue @@ -599,6 +599,18 @@ fitimage_assemble() { # Also skip if a symlink. We'll later have each config section point at it [ $(symlink_points_below $DTB "${EXTERNAL_KERNEL_DEVICETREE}") ] && continue + DTB=$(echo $DTB | tr '/' '_') + fitimage_emit_section_dtb $1 $DTB "${EXTERNAL_KERNEL_DEVICETREE}/$DTB" + done + for DTB in $(find "${EXTERNAL_KERNEL_DEVICETREE}" -name '*.dtbo' -printf '%P\n' | sort); do + # Skip DTBO if we've picked it up previously + echo "$DTBS" | tr ' ' '\n' | grep -xq "$DTB" && continue + + DTBS="$DTBS $DTB" + + # Also skip if a symlink. We'll later have each config section point at it + [ $(symlink_points_below $DTB "${EXTERNAL_KERNEL_DEVICETREE}") ] && continue + DTB=$(echo $DTB | tr '/' '_') fitimage_emit_section_dtb $1 $DTB "${EXTERNAL_KERNEL_DEVICETREE}/$DTB" done
The dtb files must be before the dtbo files, otherwise the overlays may not be applied correctly. From Bruce Ashfield: We can split between dtbs and dtbos, they just need to be sorted for reproducibility reasons. Two loops versus one, would be fine, with not too much duplicated code. Of course, this was only working by luck previously (before the sort), since it has always been gathering dtbs and dtbo's with find, depending on filesystem ordering for the order in the fitimage). Signed-off-by: Sandeep Gundlupet Raju <sandeep.gundlupet-raju@amd.com> --- meta/classes-recipe/kernel-fitimage.bbclass | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)