On Thu, Jan 26, 2023 at 17:04, Safae Ouajih souajih@baylibre.com wrote:

Android introduced boot header version 3 or 4. The header structure change with version 3 and 4 to support the new updates such as:

• Introducing Vendor boot image: with a vendor ramdisk
• Bootconfig feature (v4)

To maintain support for version v0, v1 and v2 while introducing version 3 and 4, this struct name must change to refer to the header structure before v3.

Signed-off-by: Safae Ouajih souajih@baylibre.com

Reviewed-by: Mattijs Korpershoek mkorpershoek@baylibre.com

---

boot/image-android.c | 26 +++++++++++++------------- boot/image-fdt.c | 2 +- cmd/abootimg.c | 4 ++-- drivers/fastboot/fb_mmc.c | 8 ++++---- include/android_image.h | 4 ++-- include/image.h | 18 +++++++++--------- 6 files changed, 31 insertions(+), 31 deletions(-)

diff --git a/boot/image-android.c b/boot/image-android.c index 2628db3741..8c0a304cc0 100644 --- a/boot/image-android.c +++ b/boot/image-android.c @@ -18,7 +18,7 @@

static char andr_tmp_str[ANDR_BOOT_ARGS_SIZE + 1];

-static ulong android_image_get_kernel_addr(const struct andr_img_hdr *hdr) +static ulong android_image_get_kernel_addr(const struct andr_boot_img_hdr_v0 *hdr) { /* * All the Android tools that generate a boot.img use this @@ -59,7 +59,7 @@ static ulong android_image_get_kernel_addr(const struct andr_img_hdr *hdr)

• Return: Zero, os start address and length on success,

• otherwise on failure.
  

*/ -int android_image_get_kernel(const struct andr_img_hdr *hdr, int verify, +int android_image_get_kernel(const struct andr_boot_img_hdr_v0 *hdr, int verify, ulong *os_data, ulong *os_len) { u32 kernel_addr = android_image_get_kernel_addr(hdr); @@ -122,12 +122,12 @@ int android_image_get_kernel(const struct andr_img_hdr *hdr, int verify, return 0; }

-int android_image_check_header(const struct andr_img_hdr *hdr) +int android_image_check_header(const struct andr_boot_img_hdr_v0 *hdr) { return memcmp(ANDR_BOOT_MAGIC, hdr->magic, ANDR_BOOT_MAGIC_SIZE); }

-ulong android_image_get_end(const struct andr_img_hdr *hdr) +ulong android_image_get_end(const struct andr_boot_img_hdr_v0 *hdr) { ulong end;

@@ -150,12 +150,12 @@ ulong android_image_get_end(const struct andr_img_hdr *hdr) return end; }

-ulong android_image_get_kload(const struct andr_img_hdr *hdr) +ulong android_image_get_kload(const struct andr_boot_img_hdr_v0 *hdr) { return android_image_get_kernel_addr(hdr); }

-ulong android_image_get_kcomp(const struct andr_img_hdr *hdr) +ulong android_image_get_kcomp(const struct andr_boot_img_hdr_v0 *hdr) { const void *p = (void *)((uintptr_t)hdr + hdr->page_size);

@@ -167,7 +167,7 @@ ulong android_image_get_kcomp(const struct andr_img_hdr *hdr) return image_decomp_type(p, sizeof(u32)); }

-int android_image_get_ramdisk(const struct andr_img_hdr *hdr, +int android_image_get_ramdisk(const struct andr_boot_img_hdr_v0 *hdr, ulong *rd_data, ulong *rd_len) { if (!hdr->ramdisk_size) { @@ -186,7 +186,7 @@ int android_image_get_ramdisk(const struct andr_img_hdr *hdr, return 0; }

-int android_image_get_second(const struct andr_img_hdr *hdr, +int android_image_get_second(const struct andr_boot_img_hdr_v0 *hdr, ulong *second_data, ulong *second_len) { if (!hdr->second_size) { @@ -226,7 +226,7 @@ int android_image_get_second(const struct andr_img_hdr *hdr, */ bool android_image_get_dtbo(ulong hdr_addr, ulong *addr, u32 *size) {

• const struct andr_img_hdr *hdr;

• const struct andr_boot_img_hdr_v0 *hdr; ulong dtbo_img_addr; bool ret = true;

@@ -275,7 +275,7 @@ exit: */ static bool android_image_get_dtb_img_addr(ulong hdr_addr, ulong *addr) {

• const struct andr_img_hdr *hdr;

• const struct andr_boot_img_hdr_v0 *hdr; ulong dtb_img_addr; bool ret = true;

@@ -328,7 +328,7 @@ exit: bool android_image_get_dtb_by_index(ulong hdr_addr, u32 index, ulong *addr, u32 *size) {

• const struct andr_img_hdr *hdr;

• const struct andr_boot_img_hdr_v0 *hdr; bool res; ulong dtb_img_addr; /* address of DTB part in boot image */ u32 dtb_img_size; /* size of DTB payload in boot image */

@@ -393,7 +393,7 @@ bool android_image_get_dtb_by_index(ulong hdr_addr, u32 index, ulong *addr,

• returns:

• no returned results
  

*/ -void android_print_contents(const struct andr_img_hdr *hdr) +void android_print_contents(const struct andr_boot_img_hdr_v0 *hdr) { const char * const p = IMAGE_INDENT_STRING; /* os_version = ver << 11 | lvl */ @@ -485,7 +485,7 @@ static bool android_image_print_dtb_info(const struct fdt_header *fdt, */ bool android_image_print_dtb_contents(ulong hdr_addr) {

• const struct andr_img_hdr *hdr;

• const struct andr_boot_img_hdr_v0 *hdr; bool res; ulong dtb_img_addr; /* address of DTB part in boot image */ u32 dtb_img_size; /* size of DTB payload in boot image */

diff --git a/boot/image-fdt.c b/boot/image-fdt.c index b830a0ab41..1181cb60f0 100644 --- a/boot/image-fdt.c +++ b/boot/image-fdt.c @@ -529,7 +529,7 @@ int boot_get_fdt(int flag, int argc, char *const argv[], uint8_t arch, } #ifdef CONFIG_ANDROID_BOOT_IMAGE } else if (genimg_get_format(buf) == IMAGE_FORMAT_ANDROID) {

• struct andr_img_hdr *hdr = buf;
  

• struct andr_boot_img_hdr_v0 *hdr = buf;
  

  ulong fdt_data, fdt_len; u32 fdt_size, dtb_idx; /*

diff --git a/cmd/abootimg.c b/cmd/abootimg.c index f48a9dcb02..0262adb1e5 100644 --- a/cmd/abootimg.c +++ b/cmd/abootimg.c @@ -18,7 +18,7 @@ static ulong _abootimg_addr = -1;

static int abootimg_get_ver(int argc, char *const argv[]) {

• const struct andr_img_hdr *hdr;

• const struct andr_boot_img_hdr_v0 *hdr; int res = CMD_RET_SUCCESS;

  if (argc > 1)

@@ -65,7 +65,7 @@ static int abootimg_get_recovery_dtbo(int argc, char *const argv[])

static int abootimg_get_dtb_load_addr(int argc, char *const argv[]) {

• const struct andr_img_hdr *hdr;

• const struct andr_boot_img_hdr_v0 *hdr; int res = CMD_RET_SUCCESS;

  if (argc > 1)

diff --git a/drivers/fastboot/fb_mmc.c b/drivers/fastboot/fb_mmc.c index 033c510bc0..6d0112fdf4 100644 --- a/drivers/fastboot/fb_mmc.c +++ b/drivers/fastboot/fb_mmc.c @@ -287,7 +287,7 @@ static void fb_mmc_boot_ops(struct blk_desc *dev_desc, void *buffer, */ static lbaint_t fb_mmc_get_boot_header(struct blk_desc *dev_desc, struct disk_partition *info,

• 		       struct andr_img_hdr *hdr,
  

• 		       struct andr_boot_img_hdr_v0 *hdr,
  	       char *response)
  

{ ulong sector_size; /* boot partition sector size */ @@ -296,7 +296,7 @@ static lbaint_t fb_mmc_get_boot_header(struct blk_desc *dev_desc,

/* Calculate boot image sectors count */ sector_size = info->blksz;

• hdr_sectors = DIV_ROUND_UP(sizeof(struct andr_img_hdr), sector_size);

• hdr_sectors = DIV_ROUND_UP(sizeof(struct andr_boot_img_hdr_v0), sector_size); if (hdr_sectors == 0) { pr_err("invalid number of boot sectors: 0\n"); fastboot_fail("invalid number of boot sectors: 0", response);

@@ -338,7 +338,7 @@ static int fb_mmc_update_zimage(struct blk_desc *dev_desc, char *response) { uintptr_t hdr_addr; /* boot image header address */

• struct andr_img_hdr *hdr; /* boot image header */

• struct andr_boot_img_hdr_v0 *hdr; /* boot image header */ lbaint_t hdr_sectors; /* boot image header sectors */ u8 *ramdisk_buffer; u32 ramdisk_sector_start;

@@ -361,7 +361,7 @@ static int fb_mmc_update_zimage(struct blk_desc *dev_desc,

/* Put boot image header in fastboot buffer after downloaded zImage */ hdr_addr = (uintptr_t)download_buffer + ALIGN(download_bytes, PAGE_SIZE);

• hdr = (struct andr_img_hdr *)hdr_addr;

• hdr = (struct andr_boot_img_hdr_v0 *)hdr_addr;

  /* Read boot image header */ hdr_sectors = fb_mmc_get_boot_header(dev_desc, &info, hdr, response);

diff --git a/include/android_image.h b/include/android_image.h index 54d25af068..2bdcab9122 100644 --- a/include/android_image.h +++ b/include/android_image.h @@ -20,9 +20,9 @@ #define ANDR_BOOT_ARGS_SIZE 512 #define ANDR_BOOT_EXTRA_ARGS_SIZE 1024

-/* The bootloader expects the structure of andr_img_hdr with header +/* The bootloader expects the structure of andr_boot_img_hdr_v0 with header

• version 0 to be as follows: */

-struct andr_img_hdr { +struct andr_boot_img_hdr_v0 { /* Must be ANDR_BOOT_MAGIC. */ char magic[ANDR_BOOT_MAGIC_SIZE];

diff --git a/include/image.h b/include/image.h index 7717a4c13d..bcb24d92de 100644 --- a/include/image.h +++ b/include/image.h @@ -1734,21 +1734,21 @@ int fit_image_cipher_get_algo(const void *fit, int noffset, char **algo);

struct cipher_algo *image_get_cipher_algo(const char *full_name);

-struct andr_img_hdr; -int android_image_check_header(const struct andr_img_hdr *hdr); -int android_image_get_kernel(const struct andr_img_hdr *hdr, int verify, +struct andr_boot_img_hdr_v0; +int android_image_check_header(const struct andr_boot_img_hdr_v0 *hdr); +int android_image_get_kernel(const struct andr_boot_img_hdr_v0 *hdr, int verify, ulong *os_data, ulong *os_len); -int android_image_get_ramdisk(const struct andr_img_hdr *hdr, +int android_image_get_ramdisk(const struct andr_boot_img_hdr_v0 *hdr, ulong *rd_data, ulong *rd_len); -int android_image_get_second(const struct andr_img_hdr *hdr, +int android_image_get_second(const struct andr_boot_img_hdr_v0 *hdr, ulong *second_data, ulong *second_len); bool android_image_get_dtbo(ulong hdr_addr, ulong *addr, u32 *size); bool android_image_get_dtb_by_index(ulong hdr_addr, u32 index, ulong *addr, u32 *size); -ulong android_image_get_end(const struct andr_img_hdr *hdr); -ulong android_image_get_kload(const struct andr_img_hdr *hdr); -ulong android_image_get_kcomp(const struct andr_img_hdr *hdr); -void android_print_contents(const struct andr_img_hdr *hdr); +ulong android_image_get_end(const struct andr_boot_img_hdr_v0 *hdr); +ulong android_image_get_kload(const struct andr_boot_img_hdr_v0 *hdr); +ulong android_image_get_kcomp(const struct andr_boot_img_hdr_v0 *hdr); +void android_print_contents(const struct andr_boot_img_hdr_v0 *hdr); bool android_image_print_dtb_contents(ulong hdr_addr);

/**

2.34.1

On Thu, Jan 26, 2023 at 17:54, Simon Glass sjg@chromium.org wrote:

On Thu, 26 Jan 2023 at 09:05, Safae Ouajih souajih@baylibre.com wrote:

...

With the new vendor boot image introduced in versions 3 and 4 of boot image header, the header check must be done for both boot image and vendor boot image. Thus, android_image_check_header() is being replaced by is_android_boot_image_header() to only refer to boot image header check.

Signed-off-by: Safae Ouajih souajih@baylibre.com

boot/image-android.c | 8 ++++---- boot/image-board.c | 2 +- cmd/abootimg.c | 4 ++-- drivers/fastboot/fb_mmc.c | 3 +-- include/image.h | 2 +- 5 files changed, 9 insertions(+), 10 deletions(-)

[..]

...

diff --git a/include/image.h b/include/image.h index bcb24d92de..c1594ee169 100644 --- a/include/image.h +++ b/include/image.h @@ -1735,7 +1735,6 @@ int fit_image_cipher_get_algo(const void *fit, int noffset, char **algo); struct cipher_algo *image_get_cipher_algo(const char *full_name);

struct andr_boot_img_hdr_v0; -int android_image_check_header(const struct andr_boot_img_hdr_v0 *hdr); int android_image_get_kernel(const struct andr_boot_img_hdr_v0 *hdr, int verify, ulong *os_data, ulong *os_len); int android_image_get_ramdisk(const struct andr_boot_img_hdr_v0 *hdr, @@ -1750,6 +1749,7 @@ ulong android_image_get_kload(const struct andr_boot_img_hdr_v0 *hdr); ulong android_image_get_kcomp(const struct andr_boot_img_hdr_v0 *hdr); void android_print_contents(const struct andr_boot_img_hdr_v0 *hdr); bool android_image_print_dtb_contents(ulong hdr_addr); +bool is_android_boot_image_header(const struct andr_boot_img_hdr_v0 *hdr);

Please add a full comment

...

/**

• board_fit_config_name_match() - Check for a matching board name

-- 2.34.1

With that:

Reviewed-by: Simon Glass sjg@chromium.org

Regards, SImon

Reviewed-by: Mattijs Korpershoek mkorpershoek@baylibre.com

On Thu, Jan 26, 2023 at 17:54, Simon Glass sjg@chromium.org wrote:

On Thu, 26 Jan 2023 at 09:05, Safae Ouajih souajih@baylibre.com wrote:

...

andr_image_data structure is used as a global representation of boot image header structure. This new structure is introduced to support all boot header versions : v0,v1.v2.v3.v4 and will be used to support v3 and v4 while maitaining support for v0,v1,v2. The need of using andr_image_data comes from the change of header structure in both version 3 and 4.

android_image_get_kcomp() is reworked to support this new struct.

nit: Your commit messages should be in imperative tense. See [1] for some thoughts on this.

...

Signed-off-by: Safae Ouajih souajih@baylibre.com

boot/image-android.c | 75 ++++++++++++++++++++++++++++++++++++++++- include/android_image.h | 27 +++++++++++++++ include/image.h | 2 ++ 3 files changed, 103 insertions(+), 1 deletion(-)

Reviewed-by: Simon Glass sjg@chromium.org

But please comment struct andr_image_data.

[..]

Reviewed-by: Mattijs Korpershoek mkorpershoek@baylibre.com

...

diff --git a/include/android_image.h b/include/android_image.h index 4fce363ff7..5b36f96d7b 100644 --- a/include/android_image.h +++ b/include/android_image.h @@ -317,4 +317,31 @@ struct andr_boot_img_hdr_v0 {

• contained outside boot and vendor boot partitions), otherwise
• jump to kernel_addr

*/

+/* Private struct */ +struct andr_image_data {

•   ulong kernel_ptr;
  

•   u32 kernel_size;
  

•   u32 ramdisk_size;
  

•   u32 boot_ramdisk_size;
  

•   ulong second_ptr;
  

•   u32 second_size;
  

•   ulong dtb_ptr;
  

•   u32 dtb_size;
  

•   ulong recovery_dtbo_ptr;
  

•   u32 recovery_dtbo_size;
  

•   const char *kcmdline;
  

•   const char *kcmdline_extra;
  

•   const char *image_name;
  

•   u32 kernel_addr;
  

•   ulong ramdisk_addr;
  

•   ulong ramdisk_ptr;
  

•   ulong dtb_load_addr;
  

•   ulong tags_addr;
  

•   u32 header_version;
  

•   u32 boot_img_total_size;
  

+};

#endif diff --git a/include/image.h b/include/image.h index c1594ee169..9a0bd9d8f2 100644 --- a/include/image.h +++ b/include/image.h @@ -1733,7 +1733,9 @@ struct cipher_algo { int fit_image_cipher_get_algo(const void *fit, int noffset, char **algo);

struct cipher_algo *image_get_cipher_algo(const char *full_name); +struct andr_image_data;

+bool android_image_get_data(const void *boot_hdr, struct andr_image_data *data); struct andr_boot_img_hdr_v0; int android_image_get_kernel(const struct andr_boot_img_hdr_v0 *hdr, int verify, ulong *os_data, ulong *os_len); -- 2.34.1

[1] https://chromium.googlesource.com/chromiumos/docs/+/HEAD/contributing.md#com...

On Thu, Jan 26, 2023 at 17:54, Simon Glass sjg@chromium.org wrote:

On Thu, 26 Jan 2023 at 09:05, Safae Ouajih souajih@baylibre.com wrote:

...

android_image_get_dtbo() is used to get recovery DTBO via abootimg cmd. This is not supported in boot image header v3 and v4. Thus, we print an error message when v1,v2 header version are not used.

Signed-off-by: Safae Ouajih souajih@baylibre.com

boot/image-android.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Simon Glass sjg@chromium.org

log_err() might be better

Reviewed-by: Mattijs Korpershoek mkorpershoek@baylibre.com

...

diff --git a/boot/image-android.c b/boot/image-android.c index a6e7a7abb0..d797270a32 100644 --- a/boot/image-android.c +++ b/boot/image-android.c @@ -316,8 +316,8 @@ bool android_image_get_dtbo(ulong hdr_addr, ulong *addr, u32 *size) goto exit; }

•   if (hdr->header_version < 1) {
  

•           printf("Error: header_version must be >= 1 to get dtbo\n");
  

•   if (hdr->header_version != 1 && hdr->header_version != 2) {
  

•           printf("Error: header version must be >= 1 and <= 2 to get dtbo\n");
            ret = false;
            goto exit;
    }
  

-- 2.34.1

Hi Safae,

On Thu, 26 Jan 2023 at 09:05, Safae Ouajih souajih@baylibre.com wrote:

This is done to prepare for boot image version 3 and 4 support.

Signed-off-by: Safae Ouajih souajih@baylibre.com

boot/bootm.c | 8 +++---- boot/image-android.c | 54 ++++++++++++++++++++++++++++---------------- boot/image-board.c | 3 +-- boot/image-fdt.c | 3 ++- cmd/abootimg.c | 4 ++-- include/image.h | 22 +++++++++++------- 6 files changed, 58 insertions(+), 36 deletions(-)

diff --git a/include/image.h b/include/image.h index 6264d13e0c..df8a9075fa 100644 --- a/include/image.h +++ b/include/image.h @@ -1735,20 +1735,26 @@ int fit_image_cipher_get_algo(const void *fit, int noffset, char **algo); struct cipher_algo *image_get_cipher_algo(const char *full_name); struct andr_image_data;

-bool android_image_get_data(const void *boot_hdr, struct andr_image_data *data); +bool android_image_get_data(const void *boot_hdr, const void *vendor_boot_hdr,

•                       struct andr_image_data *data);
  

struct andr_boot_img_hdr_v0; -int android_image_get_kernel(const struct andr_boot_img_hdr_v0 *hdr, int verify, +int android_image_get_kernel(const struct andr_boot_img_hdr_v0 *hdr,

•                        const void *vendor_boot_img, int verify,
                         ulong *os_data, ulong *os_len);
  

int android_image_get_ramdisk(const struct andr_boot_img_hdr_v0 *hdr,

•                         ulong *rd_data, ulong *rd_len);
  

•                         const void *vendor_boot_img, ulong *rd_data, ulong *rd_len);
  

int android_image_get_second(const struct andr_boot_img_hdr_v0 *hdr, ulong *second_data, ulong *second_len); bool android_image_get_dtbo(ulong hdr_addr, ulong *addr, u32 *size); -bool android_image_get_dtb_by_index(ulong hdr_addr, u32 index, ulong *addr,

•                               u32 *size);
  

-ulong android_image_get_end(const struct andr_boot_img_hdr_v0 *hdr); -ulong android_image_get_kload(const struct andr_boot_img_hdr_v0 *hdr); -ulong android_image_get_kcomp(const struct andr_boot_img_hdr_v0 *hdr); +bool android_image_get_dtb_by_index(ulong hdr_addr, ulong vendor_boot_img,

•                               u32 index, ulong *addr, u32 *size);
  

+ulong android_image_get_end(const struct andr_boot_img_hdr_v0 *hdr,

•                       const void *vendor_boot_img);
  

+ulong android_image_get_kload(const struct andr_boot_img_hdr_v0 *hdr,

•                         const void *vendor_boot_img);
  

+ulong android_image_get_kcomp(const struct andr_boot_img_hdr_v0 *hdr,

•                         const void *vendor_boot_img);
  

void android_print_contents(const struct andr_boot_img_hdr_v0 *hdr); #ifdef CONFIG_CMD_ABOOTIMG bool android_image_print_dtb_contents(ulong hdr_addr); -- 2.34.1

Please add comments for those that you change. Really all of these should have proper comments.

Regards, Simon

Hi Safae,

On Thu, 26 Jan 2023 at 09:05, Safae Ouajih souajih@baylibre.com wrote:

In version 3 and 4 of boot image header, the vendor specific

vendor-spefcific

command line are located in vendor boot image. Thus, using

use the

extra command line to add those cmd to bootargs.

Signed-off-by: Safae Ouajih souajih@baylibre.com

boot/image-android.c | 11 +++++++++++ 1 file changed, 11 insertions(+)

diff --git a/boot/image-android.c b/boot/image-android.c index 5b270e4417..cb4fc22b00 100644 --- a/boot/image-android.c +++ b/boot/image-android.c @@ -55,6 +55,7 @@ static void android_vendor_boot_image_v3_v4_parse_hdr(const struct andr_vendor_i * The header takes a full page, the remaining components are aligned * on page boundary. */

•   data->kcmdline_extra = hdr->cmdline;
    data->tags_addr = hdr->tags_addr;
    data->image_name = hdr->name;
    data->kernel_addr = hdr->kernel_addr;
  

@@ -233,6 +234,11 @@ int android_image_get_kernel(const struct andr_boot_img_hdr_v0 *hdr, len += strlen(img_data.kcmdline); }

•   if (img_data.kcmdline_extra) {
  

•           printf("Kernel extra command line: %s\n", img_data.kcmdline_extra);
  

•           len += strlen(img_data.kcmdline_extra);
  

•   }
  

•   char *bootargs = env_get("bootargs");
    if (bootargs)
            len += strlen(bootargs);
  

@@ -252,6 +258,11 @@ int android_image_get_kernel(const struct andr_boot_img_hdr_v0 *hdr, if (*img_data.kcmdline) strcat(newbootargs, img_data.kcmdline);

•   if (img_data.kcmdline_extra) {
  

•           strcat(newbootargs, " ");
  

•           strcat(newbootargs, img_data.kcmdline_extra);
  

Do we need to worry about overflow?

•   }
  

•   env_set("bootargs", newbootargs);
  
    if (os_data) {
  

-- 2.34.1

Regards, Simon

On Fri, 27 Jan 2023 at 08:51, Safae Ouajih souajih@baylibre.com wrote:

On 27/01/2023 01:54, Simon Glass wrote:

...

Hi Safae,

On Thu, 26 Jan 2023 at 09:05, Safae Ouajih souajih@baylibre.com wrote:

...

In version 3 and 4 of boot image header, the vendor specific

vendor-spefcific

...

command line are located in vendor boot image. Thus, using

use the

...

extra command line to add those cmd to bootargs.

Signed-off-by: Safae Ouajih souajih@baylibre.com

boot/image-android.c | 11 +++++++++++ 1 file changed, 11 insertions(+)

diff --git a/boot/image-android.c b/boot/image-android.c index 5b270e4417..cb4fc22b00 100644 --- a/boot/image-android.c +++ b/boot/image-android.c @@ -55,6 +55,7 @@ static void android_vendor_boot_image_v3_v4_parse_hdr(const struct andr_vendor_i * The header takes a full page, the remaining components are aligned * on page boundary. */

•   data->kcmdline_extra = hdr->cmdline;
     data->tags_addr = hdr->tags_addr;
     data->image_name = hdr->name;
     data->kernel_addr = hdr->kernel_addr;
  

@@ -233,6 +234,11 @@ int android_image_get_kernel(const struct andr_boot_img_hdr_v0 *hdr, len += strlen(img_data.kcmdline); }

•   if (img_data.kcmdline_extra) {
  

•           printf("Kernel extra command line: %s\n", img_data.kcmdline_extra);
  

•           len += strlen(img_data.kcmdline_extra);
  

•   }
  

•    char *bootargs = env_get("bootargs");
     if (bootargs)
             len += strlen(bootargs);
  

@@ -252,6 +258,11 @@ int android_image_get_kernel(const struct andr_boot_img_hdr_v0 *hdr, if (*img_data.kcmdline) strcat(newbootargs, img_data.kcmdline);

•   if (img_data.kcmdline_extra) {
  

•           strcat(newbootargs, " ");
  

•           strcat(newbootargs, img_data.kcmdline_extra);
  

Do we need to worry about overflow?

Hi Simon,

This line adds kcmdline_extra length to the len variable:

    len += strlen(img_data.kcmdline_extra);

newbootargs is allocated just after in :

     char *newbootargs = malloc(len + 2);

I can not see a possible overflow, please let me know if you

think this is not enough.

OK I see, thank you.

Reviewed-by: Simon Glass sjg@chromium.org

On Thu, Jan 26, 2023 at 17:55, Simon Glass sjg@chromium.org wrote:

Hi Safae,

On Thu, 26 Jan 2023 at 09:05, Safae Ouajih souajih@baylibre.com wrote:

...

This adds support for boot image version 3 and 4 in android_image_get_dtb_img_addr(). Since the dtb is now included in vendor_boot image instead of boot image, the dtb address should be extracted from vendor_boot image when a v3 or v4 is used.

Signed-off-by: Safae Ouajih souajih@baylibre.com

boot/image-android.c | 47 +++++++++++++++++++++++++++++++------------- 1 file changed, 33 insertions(+), 14 deletions(-)

Reviewed-by: Simon Glass sjg@chromium.org

Reviewed-by: Mattijs Korpershoek mkorpershoek@baylibre.com

nits below

...

diff --git a/boot/image-android.c b/boot/image-android.c index cb4fc22b00..edeeaaaee0 100644 --- a/boot/image-android.c +++ b/boot/image-android.c @@ -459,6 +459,7 @@ exit: static bool android_image_get_dtb_img_addr(ulong hdr_addr, ulong vhdr_addr, ulong *addr) { const struct andr_boot_img_hdr_v0 *hdr;

•   const struct andr_vendor_img_hdr *v_hdr;
    ulong dtb_img_addr;
    bool ret = true;
  
  

@@ -475,22 +476,40 @@ static bool android_image_get_dtb_img_addr(ulong hdr_addr, ulong vhdr_addr, ulon goto exit; }

•   if (hdr->dtb_size == 0) {
  

•           printf("Error: dtb_size is 0\n");
  

•           ret = false;
  

•           goto exit;
  

•   if (hdr->header_version == 2) {
  

•           if (hdr->dtb_size == 0) {
  

if (!hdr->dtb_size)

...

•                   printf("Error: dtb_size is 0\n");
  

•                   ret = false;
  

•                   goto exit;
  

•           }
  

•           /* Calculate the address of DTB area in boot image */
  

•           dtb_img_addr = hdr_addr;
  

•           dtb_img_addr += hdr->page_size;
  

•           dtb_img_addr += ALIGN(hdr->kernel_size, hdr->page_size);
  

•           dtb_img_addr += ALIGN(hdr->ramdisk_size, hdr->page_size);
  

•           dtb_img_addr += ALIGN(hdr->second_size, hdr->page_size);
  

•           dtb_img_addr += ALIGN(hdr->recovery_dtbo_size, hdr->page_size);
  

•           *addr = dtb_img_addr;
    }
  
  

•   /* Calculate the address of DTB area in boot image */
  

•   dtb_img_addr = hdr_addr;
  

•   dtb_img_addr += hdr->page_size;
  

•   dtb_img_addr += ALIGN(hdr->kernel_size, hdr->page_size);
  

•   dtb_img_addr += ALIGN(hdr->ramdisk_size, hdr->page_size);
  

•   dtb_img_addr += ALIGN(hdr->second_size, hdr->page_size);
  

•   dtb_img_addr += ALIGN(hdr->recovery_dtbo_size, hdr->page_size);
  

•   *addr = dtb_img_addr;
  

•   if (hdr->header_version > 2) {
  

•           v_hdr = map_sysmem(vhdr_addr, sizeof(*v_hdr));
  

•           if (v_hdr->dtb_size == 0) {
  

same here

...

•                   printf("Error: dtb_size is 0\n");
  

•                   ret = false;
  

•                   unmap_sysmem(v_hdr);
  

•                   goto exit;
  

•           }
  

•           /* Calculate the address of DTB area in boot image */
  

•           dtb_img_addr = vhdr_addr;
  

•           dtb_img_addr += v_hdr->page_size;
  

•           if (v_hdr->vendor_ramdisk_size)
  

•                   dtb_img_addr += ALIGN(v_hdr->vendor_ramdisk_size, v_hdr->page_size);
  

•           *addr = dtb_img_addr;
  

•           unmap_sysmem(v_hdr);
  

•           goto exit;
  

•   }
  

exit: unmap_sysmem(hdr); return ret; -- 2.34.1

Regards, Simon

Hi Safae,

On Thu, 26 Jan 2023 at 09:05, Safae Ouajih souajih@baylibre.com wrote:

Print an error message when the boot image header version is greater than 2 as this is not supported for v3 and v4.

But why is is not supported? Please add this here.

Signed-off-by: Safae Ouajih souajih@baylibre.com

drivers/fastboot/fb_mmc.c | 8 ++++++++ 1 file changed, 8 insertions(+)

diff --git a/drivers/fastboot/fb_mmc.c b/drivers/fastboot/fb_mmc.c index 086e5f7843..9fb2ab3820 100644 --- a/drivers/fastboot/fb_mmc.c +++ b/drivers/fastboot/fb_mmc.c @@ -370,6 +370,14 @@ static int fb_mmc_update_zimage(struct blk_desc *dev_desc, return -1; }

•   /* Check if boot image header version is 2 or less */
  

•   if (hdr->header_version > 2) {
  

•           pr_err("zImage flashing supported only for boot images v2 and less\n");
  

•           fastboot_fail("zImage flashing supported only for boot images v2 and less",
  

•                         response);
  

•           return -1;
  

That is not an error message. Perhaps -ENOTSUPP ?

•   }
  

•   /* Check if boot image has second stage in it (we don't support it) */
    if (hdr->second_size > 0) {
            pr_err("moving second stage is not supported yet\n");
  

-- 2.34.1

Regards, Simon

Hi Safae,

On Thu, 26 Jan 2023 at 09:05, Safae Ouajih souajih@baylibre.com wrote:

This enables the support for boot image header version 3 and 4 using abootimg command.

In order to use version 3 or 4:

1- Vendor boot image address should be given to abootimg cmd.

    abootimg addr $1 $vendor_boot_load_addr

2- "ramdisk_addr_r" env variable (ramdisk address) should be set to host the ramdisk : generic ramdisk + vendor ramdisk

"struct andr_boot_img_hdr_v0*" is replaced by "void *" in some functions since v3 and v4 are now supported as well.

Signed-off-by: Safae Ouajih souajih@baylibre.com

boot/bootm.c | 29 ++++++++++++++++++++++++----- boot/image-android.c | 16 ++++++++++------ boot/image-board.c | 14 +++++++++++--- boot/image-fdt.c | 2 +- cmd/abootimg.c | 24 ++++++++++++++++++++++-- include/image.h | 14 ++++++++------ 6 files changed, 76 insertions(+), 23 deletions(-)

diff --git a/boot/bootm.c b/boot/bootm.c index a58e6f391e..3e130c175c 100644 --- a/boot/bootm.c +++ b/boot/bootm.c @@ -113,6 +113,10 @@ static int bootm_find_os(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) { const void *os_hdr; +#ifdef CONFIG_ANDROID_BOOT_IMAGE

•   const void *vendor_boot_img;
  

•   const void *boot_img;
  

+#endif bool ep_found = false; int ret;

@@ -181,12 +185,17 @@ static int bootm_find_os(struct cmd_tbl *cmdtp, int flag, int argc, #endif #ifdef CONFIG_ANDROID_BOOT_IMAGE case IMAGE_FORMAT_ANDROID:

•           boot_img = os_hdr;
  

•           vendor_boot_img = NULL;
  

•           if (IS_ENABLED(CONFIG_CMD_ABOOTIMG)) {
  

•                   boot_img = (void *)get_abootimg_addr();
  

•                   vendor_boot_img = (void *)get_avendor_bootimg_addr();
  

map_sysmem() so it owrks on sandbox

•           }
            images.os.type = IH_TYPE_KERNEL;
  

•           images.os.comp = android_image_get_kcomp(os_hdr, NULL);
  

•           images.os.comp = android_image_get_kcomp(boot_img, vendor_boot_img);
            images.os.os = IH_OS_LINUX;
  

•           images.os.end = android_image_get_end(os_hdr, NULL);
  

•           images.os.load = android_image_get_kload(os_hdr, NULL);
  

•           images.os.end = android_image_get_end(boot_img, vendor_boot_img);
  

•           images.os.load = android_image_get_kload(boot_img, vendor_boot_img);
            images.ep = images.os.load;
            ep_found = true;
            break;
  

@@ -889,6 +898,10 @@ static const void *boot_get_kernel(struct cmd_tbl *cmdtp, int flag, int argc, int os_noffset; #endif

+#ifdef CONFIG_ANDROID_BOOT_IMAGE

•   const void *boot_img;
  

•   const void *vendor_boot_img;
  

+#endif img_addr = genimg_get_kernel_addr_fit(argc < 1 ? NULL : argv[0], &fit_uname_config, &fit_uname_kernel); @@ -964,8 +977,14 @@ static const void *boot_get_kernel(struct cmd_tbl *cmdtp, int flag, int argc, #endif #ifdef CONFIG_ANDROID_BOOT_IMAGE case IMAGE_FORMAT_ANDROID:

•           boot_img = buf;
  

•           vendor_boot_img = NULL;
  

•           if (IS_ENABLED(CONFIG_CMD_ABOOTIMG)) {
  

•                   boot_img = (void *)get_abootimg_addr();
  

•                   vendor_boot_img = (void *)get_avendor_bootimg_addr();
  

and here

•           }
            printf("## Booting Android Image at 0x%08lx ...\n", img_addr);
  

•           if (android_image_get_kernel(buf, NULL, images->verify,
  

•           if (android_image_get_kernel(boot_img, vendor_boot_img, images->verify,
                                         os_data, os_len))
                    return NULL;
            break;
  

diff --git a/boot/image-android.c b/boot/image-android.c index edeeaaaee0..dd06c09279 100644 --- a/boot/image-android.c +++ b/boot/image-android.c @@ -201,7 +201,7 @@ static ulong android_image_get_kernel_addr(struct andr_image_data *img_data)

• Return: Zero, os start address and length on success,

•         otherwise on failure.
  

*/ -int android_image_get_kernel(const struct andr_boot_img_hdr_v0 *hdr, +int android_image_get_kernel(const void *hdr, const void *vendor_boot_img, int verify, ulong *os_data, ulong *os_len) { @@ -286,7 +286,7 @@ bool is_android_vendor_boot_image_header(const void *vendor_boot_img) return !memcmp(VENDOR_BOOT_MAGIC, vendor_boot_img, ANDR_VENDOR_BOOT_MAGIC_SIZE); }

-bool is_android_boot_image_header(const struct andr_boot_img_hdr_v0 *hdr) +bool is_android_boot_image_header(const void *hdr) { return !memcmp(ANDR_BOOT_MAGIC, hdr, ANDR_BOOT_MAGIC_SIZE); } @@ -305,7 +305,7 @@ ulong android_image_get_end(const struct andr_boot_img_hdr_v0 *hdr, return img_data.boot_img_total_size; }

-ulong android_image_get_kload(const struct andr_boot_img_hdr_v0 *hdr, +ulong android_image_get_kload(const void *hdr, const void *vendor_boot_img) { struct andr_image_data img_data; @@ -316,7 +316,7 @@ ulong android_image_get_kload(const struct andr_boot_img_hdr_v0 *hdr, return android_image_get_kernel_addr(&img_data); }

-ulong android_image_get_kcomp(const struct andr_boot_img_hdr_v0 *hdr, +ulong android_image_get_kcomp(const void *hdr, const void *vendor_boot_img) { struct andr_image_data img_data; @@ -364,14 +364,18 @@ int android_image_get_ramdisk(const void *hdr, const void *vendor_boot_img, return 0; }

-int android_image_get_second(const struct andr_boot_img_hdr_v0 *hdr,

•                         ulong *second_data, ulong *second_len)
  

+int android_image_get_second(const void *hdr, ulong *second_data, ulong *second_len) { struct andr_image_data img_data;

    if (!android_image_get_data(hdr, NULL, &img_data))
            return -EINVAL;

•   if (img_data.header_version > 2) {
  

•           printf("Second stage bootloader is only supported for boot image version <= 2\n");
  

•           return -1;
  

Use proper error number. This is -EPERM which seems wrong. Perhaps -ENOTSUPP ?

•   }
  

•   if (!img_data.second_size) {
            *second_data = *second_len = 0;
            return -1;
  

diff --git a/boot/image-board.c b/boot/image-board.c index b6c9bbe8f5..a20b57900e 100644 --- a/boot/image-board.c +++ b/boot/image-board.c @@ -426,10 +426,18 @@ static int select_ramdisk(struct bootm_headers *images, const char *select, u8 a break; case IMAGE_FORMAT_ANDROID: if (IS_ENABLED(CONFIG_ANDROID_BOOT_IMAGE)) {

•                   void *ptr = map_sysmem(images->os.start, 0);
                    int ret;
  

•                   ret = android_image_get_ramdisk(ptr, NULL, rd_datap, rd_lenp);
  

•                   unmap_sysmem(ptr);
  

•                   if (IS_ENABLED(CONFIG_CMD_ABOOTIMG)) {
  

•                           ret = android_image_get_ramdisk((void *)get_abootimg_addr(),
  

map_sysmem() again. Or perhaps update that function to take an address?

•                                                           (void *)get_avendor_bootimg_addr(),
  

•                                                           rd_datap, rd_lenp);
  

•                   } else {
  

•                           void *ptr = map_sysmem(images->os.start, 0);
  

•                           ret = android_image_get_ramdisk(ptr, NULL, rd_datap, rd_lenp);
  

•                           unmap_sysmem(ptr);
  

•                   }
  

•                   if (ret)
                            return ret;
                    done = true;
  

diff --git a/boot/image-fdt.c b/boot/image-fdt.c index f270d00a5f..5f9cf7b4e6 100644 --- a/boot/image-fdt.c +++ b/boot/image-fdt.c @@ -529,7 +529,7 @@ int boot_get_fdt(int flag, int argc, char *const argv[], uint8_t arch, } #ifdef CONFIG_ANDROID_BOOT_IMAGE } else if (genimg_get_format(buf) == IMAGE_FORMAT_ANDROID) {

•           struct andr_boot_img_hdr_v0 *hdr = buf;
  

•           void *hdr = buf;
            ulong           fdt_data, fdt_len;
            u32                     fdt_size, dtb_idx;
            /*
  

diff --git a/cmd/abootimg.c b/cmd/abootimg.c index 4d6cf0fa3e..ef7130da0b 100644 --- a/cmd/abootimg.c +++ b/cmd/abootimg.c @@ -17,6 +17,16 @@ static ulong _abootimg_addr = -1; static ulong _avendor_bootimg_addr = -1;

+ulong get_abootimg_addr(void) +{

•   return (_abootimg_addr == -1 ? image_load_addr : _abootimg_addr);
  

+}

+ulong get_avendor_bootimg_addr(void) +{

•   return _avendor_bootimg_addr;
  

+}

static int abootimg_get_ver(int argc, char *const argv[]) { const struct andr_boot_img_hdr_v0 *hdr; @@ -70,12 +80,21 @@ static int abootimg_get_dtb_load_addr(int argc, char *const argv[]) return CMD_RET_USAGE; struct andr_image_data img_data = {0}; const struct andr_boot_img_hdr_v0 *hdr;

•   const struct andr_vendor_img_hdr *vhdr;
  
    hdr = map_sysmem(abootimg_addr(), sizeof(*hdr));
  

•   if (!android_image_get_data(hdr, NULL, &img_data)) {
  

•   if (get_avendor_bootimg_addr() != -1)
  

•           vhdr = map_sysmem(get_avendor_bootimg_addr(), sizeof(*vhdr));
  

•   if (!android_image_get_data(hdr, vhdr, &img_data)) {
  

•           if (get_avendor_bootimg_addr() != -1)
  

•                   unmap_sysmem(vhdr);
            unmap_sysmem(hdr);
            return CMD_RET_FAILURE;
    }
  

•   if (get_avendor_bootimg_addr() != -1)
  

•           unmap_sysmem(vhdr);
    unmap_sysmem(hdr);
  
    if (img_data.header_version < 2) {
  

@@ -119,7 +138,8 @@ static int abootimg_get_dtb_by_index(int argc, char *const argv[]) return CMD_RET_FAILURE; }

•   if (!android_image_get_dtb_by_index(abootimg_addr(), 0, num,
  

•   if (!android_image_get_dtb_by_index(abootimg_addr(),
  

•                                       get_avendor_bootimg_addr(), num,
                                        &addr, &size)) {
            return CMD_RET_FAILURE;
    }
  

diff --git a/include/image.h b/include/image.h index ac94ecaa14..5bd4bf5d57 100644 --- a/include/image.h +++ b/include/image.h @@ -1739,29 +1739,31 @@ bool android_image_get_data(const void *boot_hdr, const void *vendor_boot_hdr, struct andr_image_data *data);

struct andr_boot_img_hdr_v0; -int android_image_get_kernel(const struct andr_boot_img_hdr_v0 *hdr, +int android_image_get_kernel(const void *hdr, const void *vendor_boot_img, int verify, ulong *os_data, ulong *os_len); int android_image_get_ramdisk(const void *hdr, const void *vendor_boot_img, ulong *rd_data, ulong *rd_len); -int android_image_get_second(const struct andr_boot_img_hdr_v0 *hdr,

•                         ulong *second_data, ulong *second_len);
  

+int android_image_get_second(const void *hdr, ulong *second_data, ulong *second_len); bool android_image_get_dtbo(ulong hdr_addr, ulong *addr, u32 *size); bool android_image_get_dtb_by_index(ulong hdr_addr, ulong vendor_boot_img, u32 index, ulong *addr, u32 *size); ulong android_image_get_end(const struct andr_boot_img_hdr_v0 *hdr, const void *vendor_boot_img); -ulong android_image_get_kload(const struct andr_boot_img_hdr_v0 *hdr, +ulong android_image_get_kload(const void *hdr, const void *vendor_boot_img); -ulong android_image_get_kcomp(const struct andr_boot_img_hdr_v0 *hdr, +ulong android_image_get_kcomp(const void *hdr, const void *vendor_boot_img); void android_print_contents(const struct andr_boot_img_hdr_v0 *hdr); #ifdef CONFIG_CMD_ABOOTIMG bool android_image_print_dtb_contents(ulong hdr_addr); #endif -bool is_android_boot_image_header(const struct andr_boot_img_hdr_v0 *hdr); +bool is_android_boot_image_header(const void *hdr); bool is_android_vendor_boot_image_header(const void *vendor_boot_img);

+ulong get_abootimg_addr(void); +ulong get_avendor_bootimg_addr(void);

again please add comments to all of these.

/**

• board_fit_config_name_match() - Check for a matching board name

-- 2.34.1

Regards, Simon

On 01/02/2023 09:44, Mattijs Korpershoek wrote:

On Thu, Jan 26, 2023 at 17:55, Simon Glass sjg@chromium.org wrote:

...

Hi Safae,

On Thu, 26 Jan 2023 at 09:05, Safae Ouajih souajih@baylibre.com wrote:

...

This enables the support for boot image header version 3 and 4 using abootimg command.

In order to use version 3 or 4:

1- Vendor boot image address should be given to abootimg cmd.

     abootimg addr $1 $vendor_boot_load_addr

2- "ramdisk_addr_r" env variable (ramdisk address) should be set to host the ramdisk : generic ramdisk + vendor ramdisk

"struct andr_boot_img_hdr_v0*" is replaced by "void *" in some functions since v3 and v4 are now supported as well.

Signed-off-by: Safae Ouajih souajih@baylibre.com

boot/bootm.c | 29 ++++++++++++++++++++++++----- boot/image-android.c | 16 ++++++++++------ boot/image-board.c | 14 +++++++++++--- boot/image-fdt.c | 2 +- cmd/abootimg.c | 24 ++++++++++++++++++++++-- include/image.h | 14 ++++++++------ 6 files changed, 76 insertions(+), 23 deletions(-)

Since this introduces the new formats (v3 and v4), can we also update the doc/android/boot-image.rst files which lists all the boot formats for android?

Hi Mattijs,

Thank you for the review,

I will update the documentation in a v3.

Best Regards,

Safae

...

...

diff --git a/boot/bootm.c b/boot/bootm.c index a58e6f391e..3e130c175c 100644 --- a/boot/bootm.c +++ b/boot/bootm.c @@ -113,6 +113,10 @@ static int bootm_find_os(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) { const void *os_hdr; +#ifdef CONFIG_ANDROID_BOOT_IMAGE

•   const void *vendor_boot_img;
  

•   const void *boot_img;
  

+#endif bool ep_found = false; int ret;

@@ -181,12 +185,17 @@ static int bootm_find_os(struct cmd_tbl *cmdtp, int flag, int argc, #endif #ifdef CONFIG_ANDROID_BOOT_IMAGE case IMAGE_FORMAT_ANDROID:

•           boot_img = os_hdr;
  

•           vendor_boot_img = NULL;
  

•           if (IS_ENABLED(CONFIG_CMD_ABOOTIMG)) {
  

•                   boot_img = (void *)get_abootimg_addr();
  

•                   vendor_boot_img = (void *)get_avendor_bootimg_addr();
  

map_sysmem() so it owrks on sandbox

...

•           }
             images.os.type = IH_TYPE_KERNEL;
  

•           images.os.comp = android_image_get_kcomp(os_hdr, NULL);
  

•           images.os.comp = android_image_get_kcomp(boot_img, vendor_boot_img);
             images.os.os = IH_OS_LINUX;
  

•           images.os.end = android_image_get_end(os_hdr, NULL);
  

•           images.os.load = android_image_get_kload(os_hdr, NULL);
  

•           images.os.end = android_image_get_end(boot_img, vendor_boot_img);
  

•           images.os.load = android_image_get_kload(boot_img, vendor_boot_img);
             images.ep = images.os.load;
             ep_found = true;
             break;
  

@@ -889,6 +898,10 @@ static const void *boot_get_kernel(struct cmd_tbl *cmdtp, int flag, int argc, int os_noffset; #endif

+#ifdef CONFIG_ANDROID_BOOT_IMAGE

•   const void *boot_img;
  

•   const void *vendor_boot_img;
  

+#endif img_addr = genimg_get_kernel_addr_fit(argc < 1 ? NULL : argv[0], &fit_uname_config, &fit_uname_kernel); @@ -964,8 +977,14 @@ static const void *boot_get_kernel(struct cmd_tbl *cmdtp, int flag, int argc, #endif #ifdef CONFIG_ANDROID_BOOT_IMAGE case IMAGE_FORMAT_ANDROID:

•           boot_img = buf;
  

•           vendor_boot_img = NULL;
  

•           if (IS_ENABLED(CONFIG_CMD_ABOOTIMG)) {
  

•                   boot_img = (void *)get_abootimg_addr();
  

•                   vendor_boot_img = (void *)get_avendor_bootimg_addr();
  

and here

...

•           }
             printf("## Booting Android Image at 0x%08lx ...\n", img_addr);
  

•           if (android_image_get_kernel(buf, NULL, images->verify,
  

•           if (android_image_get_kernel(boot_img, vendor_boot_img, images->verify,
                                          os_data, os_len))
                     return NULL;
             break;
  

diff --git a/boot/image-android.c b/boot/image-android.c index edeeaaaee0..dd06c09279 100644 --- a/boot/image-android.c +++ b/boot/image-android.c @@ -201,7 +201,7 @@ static ulong android_image_get_kernel_addr(struct andr_image_data *img_data)

• Return: Zero, os start address and length on success,

•         otherwise on failure.
  

*/ -int android_image_get_kernel(const struct andr_boot_img_hdr_v0 *hdr, +int android_image_get_kernel(const void *hdr, const void *vendor_boot_img, int verify, ulong *os_data, ulong *os_len) { @@ -286,7 +286,7 @@ bool is_android_vendor_boot_image_header(const void *vendor_boot_img) return !memcmp(VENDOR_BOOT_MAGIC, vendor_boot_img, ANDR_VENDOR_BOOT_MAGIC_SIZE); }

-bool is_android_boot_image_header(const struct andr_boot_img_hdr_v0 *hdr) +bool is_android_boot_image_header(const void *hdr) { return !memcmp(ANDR_BOOT_MAGIC, hdr, ANDR_BOOT_MAGIC_SIZE); } @@ -305,7 +305,7 @@ ulong android_image_get_end(const struct andr_boot_img_hdr_v0 *hdr, return img_data.boot_img_total_size; }

-ulong android_image_get_kload(const struct andr_boot_img_hdr_v0 *hdr, +ulong android_image_get_kload(const void *hdr, const void *vendor_boot_img) { struct andr_image_data img_data; @@ -316,7 +316,7 @@ ulong android_image_get_kload(const struct andr_boot_img_hdr_v0 *hdr, return android_image_get_kernel_addr(&img_data); }

-ulong android_image_get_kcomp(const struct andr_boot_img_hdr_v0 *hdr, +ulong android_image_get_kcomp(const void *hdr, const void *vendor_boot_img) { struct andr_image_data img_data; @@ -364,14 +364,18 @@ int android_image_get_ramdisk(const void *hdr, const void *vendor_boot_img, return 0; }

-int android_image_get_second(const struct andr_boot_img_hdr_v0 *hdr,

•                         ulong *second_data, ulong *second_len)
  

+int android_image_get_second(const void *hdr, ulong *second_data, ulong *second_len) { struct andr_image_data img_data;

     if (!android_image_get_data(hdr, NULL, &img_data))
             return -EINVAL;

•   if (img_data.header_version > 2) {
  

•           printf("Second stage bootloader is only supported for boot image version <= 2\n");
  

•           return -1;
  

Use proper error number. This is -EPERM which seems wrong. Perhaps -ENOTSUPP ?

...

•   }
  

•    if (!img_data.second_size) {
             *second_data = *second_len = 0;
             return -1;
  

diff --git a/boot/image-board.c b/boot/image-board.c index b6c9bbe8f5..a20b57900e 100644 --- a/boot/image-board.c +++ b/boot/image-board.c @@ -426,10 +426,18 @@ static int select_ramdisk(struct bootm_headers *images, const char *select, u8 a break; case IMAGE_FORMAT_ANDROID: if (IS_ENABLED(CONFIG_ANDROID_BOOT_IMAGE)) {

•                   void *ptr = map_sysmem(images->os.start, 0);
                     int ret;
  

•                   ret = android_image_get_ramdisk(ptr, NULL, rd_datap, rd_lenp);
  

•                   unmap_sysmem(ptr);
  

•                   if (IS_ENABLED(CONFIG_CMD_ABOOTIMG)) {
  

•                           ret = android_image_get_ramdisk((void *)get_abootimg_addr(),
  

map_sysmem() again. Or perhaps update that function to take an address?

...

•                                                           (void *)get_avendor_bootimg_addr(),
  

•                                                           rd_datap, rd_lenp);
  

•                   } else {
  

•                           void *ptr = map_sysmem(images->os.start, 0);
  

•                           ret = android_image_get_ramdisk(ptr, NULL, rd_datap, rd_lenp);
  

•                           unmap_sysmem(ptr);
  

•                   }
  

•                    if (ret)
                             return ret;
                     done = true;
  

diff --git a/boot/image-fdt.c b/boot/image-fdt.c index f270d00a5f..5f9cf7b4e6 100644 --- a/boot/image-fdt.c +++ b/boot/image-fdt.c @@ -529,7 +529,7 @@ int boot_get_fdt(int flag, int argc, char *const argv[], uint8_t arch, } #ifdef CONFIG_ANDROID_BOOT_IMAGE } else if (genimg_get_format(buf) == IMAGE_FORMAT_ANDROID) {

•           struct andr_boot_img_hdr_v0 *hdr = buf;
  

•           void *hdr = buf;
             ulong           fdt_data, fdt_len;
             u32                     fdt_size, dtb_idx;
             /*
  

diff --git a/cmd/abootimg.c b/cmd/abootimg.c index 4d6cf0fa3e..ef7130da0b 100644 --- a/cmd/abootimg.c +++ b/cmd/abootimg.c @@ -17,6 +17,16 @@ static ulong _abootimg_addr = -1; static ulong _avendor_bootimg_addr = -1;

+ulong get_abootimg_addr(void) +{

•   return (_abootimg_addr == -1 ? image_load_addr : _abootimg_addr);
  

+}

+ulong get_avendor_bootimg_addr(void) +{

•   return _avendor_bootimg_addr;
  

+}

• static int abootimg_get_ver(int argc, char *const argv[]) { const struct andr_boot_img_hdr_v0 *hdr;

@@ -70,12 +80,21 @@ static int abootimg_get_dtb_load_addr(int argc, char *const argv[]) return CMD_RET_USAGE; struct andr_image_data img_data = {0}; const struct andr_boot_img_hdr_v0 *hdr;

•   const struct andr_vendor_img_hdr *vhdr;
  
     hdr = map_sysmem(abootimg_addr(), sizeof(*hdr));
  

•   if (!android_image_get_data(hdr, NULL, &img_data)) {
  

•   if (get_avendor_bootimg_addr() != -1)
  

•           vhdr = map_sysmem(get_avendor_bootimg_addr(), sizeof(*vhdr));
  

•   if (!android_image_get_data(hdr, vhdr, &img_data)) {
  

•           if (get_avendor_bootimg_addr() != -1)
  

•                   unmap_sysmem(vhdr);
             unmap_sysmem(hdr);
             return CMD_RET_FAILURE;
     }
  

•   if (get_avendor_bootimg_addr() != -1)
  

•           unmap_sysmem(vhdr);
     unmap_sysmem(hdr);
  
     if (img_data.header_version < 2) {
  

@@ -119,7 +138,8 @@ static int abootimg_get_dtb_by_index(int argc, char *const argv[]) return CMD_RET_FAILURE; }

•   if (!android_image_get_dtb_by_index(abootimg_addr(), 0, num,
  

•   if (!android_image_get_dtb_by_index(abootimg_addr(),
  

•                                       get_avendor_bootimg_addr(), num,
                                         &addr, &size)) {
             return CMD_RET_FAILURE;
     }
  

diff --git a/include/image.h b/include/image.h index ac94ecaa14..5bd4bf5d57 100644 --- a/include/image.h +++ b/include/image.h @@ -1739,29 +1739,31 @@ bool android_image_get_data(const void *boot_hdr, const void *vendor_boot_hdr, struct andr_image_data *data);

struct andr_boot_img_hdr_v0; -int android_image_get_kernel(const struct andr_boot_img_hdr_v0 *hdr, +int android_image_get_kernel(const void *hdr, const void *vendor_boot_img, int verify, ulong *os_data, ulong *os_len); int android_image_get_ramdisk(const void *hdr, const void *vendor_boot_img, ulong *rd_data, ulong *rd_len); -int android_image_get_second(const struct andr_boot_img_hdr_v0 *hdr,

•                         ulong *second_data, ulong *second_len);
  

+int android_image_get_second(const void *hdr, ulong *second_data, ulong *second_len); bool android_image_get_dtbo(ulong hdr_addr, ulong *addr, u32 *size); bool android_image_get_dtb_by_index(ulong hdr_addr, ulong vendor_boot_img, u32 index, ulong *addr, u32 *size); ulong android_image_get_end(const struct andr_boot_img_hdr_v0 *hdr, const void *vendor_boot_img); -ulong android_image_get_kload(const struct andr_boot_img_hdr_v0 *hdr, +ulong android_image_get_kload(const void *hdr, const void *vendor_boot_img); -ulong android_image_get_kcomp(const struct andr_boot_img_hdr_v0 *hdr, +ulong android_image_get_kcomp(const void *hdr, const void *vendor_boot_img); void android_print_contents(const struct andr_boot_img_hdr_v0 *hdr); #ifdef CONFIG_CMD_ABOOTIMG bool android_image_print_dtb_contents(ulong hdr_addr); #endif -bool is_android_boot_image_header(const struct andr_boot_img_hdr_v0 *hdr); +bool is_android_boot_image_header(const void *hdr); bool is_android_vendor_boot_image_header(const void *vendor_boot_img);

+ulong get_abootimg_addr(void); +ulong get_avendor_bootimg_addr(void);

again please add comments to all of these.

...

• /**
  • board_fit_config_name_match() - Check for a matching board name

-- 2.34.1

Regards, Simon

Hi Safae,

On Thu, 26 Jan 2023 at 09:05, Safae Ouajih souajih@baylibre.com wrote:

This adds support for Bootconfig feature.

• The bootconfig feature replaces the androidboot.* kernel cmdline options.

This was adapted from downstream [1] commit : 7af0a0506d4d ("cuttlefish: support bootconfig parameters").

Link:[1] https://android.googlesource.com/platform/external/u-boot/

Signed-off-by: Safae Ouajih souajih@baylibre.com

boot/image-android.c | 58 +++++++++++++++++++++++++++++++++++++++-- include/android_image.h | 11 ++++++++ 2 files changed, 67 insertions(+), 2 deletions(-)

Reviewed-by: Simon Glass sjg@chromium.org

nit below

[..]

diff --git a/include/android_image.h b/include/android_image.h index 351e17750e..6e1afae4c0 100644 --- a/include/android_image.h +++ b/include/android_image.h @@ -25,6 +25,14 @@ #define ANDR_VENDOR_BOOT_ARGS_SIZE 2048 #define ANDR_VENDOR_BOOT_NAME_SIZE 16

+#define BOOTCONFIG_MAGIC "#BOOTCONFIG\n" +#define BOOTCONFIG_MAGIC_SIZE 12 +#define BOOTCONFIG_SIZE_SIZE 4 +#define BOOTCONFIG_CHECKSUM_SIZE 4 +#define BOOTCONFIG_TRAILER_SIZE BOOTCONFIG_MAGIC_SIZE + \

•                           BOOTCONFIG_SIZE_SIZE + \
  

•                           BOOTCONFIG_CHECKSUM_SIZE
  

struct andr_boot_img_hdr_v3 { u8 magic[ANDR_BOOT_MAGIC_SIZE];

@@ -333,6 +341,9 @@ struct andr_image_data { ulong recovery_dtbo_ptr; u32 recovery_dtbo_size;

•   ulong bootconfig_addr;
  

•   ulong bootconfig_size;
  

•   const char *kcmdline;
    const char *kcmdline_extra;
    const char *image_name;
  

-- 2.34.1

That struct needs a full comment

REgards, Simon

On 27/01/2023 01:55, Simon Glass wrote:

Hi Safae,

On Thu, 26 Jan 2023 at 09:05, Safae Ouajih souajih@baylibre.com wrote:

...

test_abootimg is extended to include the testing of boot images version 4. For this, boot.img and vendor_boot.img have been generated using mkbootimg tool with setting the header version to 4.

This tests:

• Getting the header version using abootimg
• Extracting the load address of the dtb
• Extracting the dtb start address in RAM

Running test: $ ./test/py/test.py --bd sandbox --build -k test_abootimg

Signed-off-by: Safae Ouajih souajih@baylibre.com

test/py/tests/test_android/test_abootimg.py | 135 ++++++++++++++++++-- 1 file changed, 122 insertions(+), 13 deletions(-)

Reviewed-by: Simon Glass sjg@chromium.org

Question below

...

diff --git a/test/py/tests/test_android/test_abootimg.py b/test/py/tests/test_android/test_abootimg.py index 43a7099c46..a5c734b9c5 100644 --- a/test/py/tests/test_android/test_abootimg.py +++ b/test/py/tests/test_android/test_abootimg.py @@ -32,6 +32,23 @@ Now one can obtain original boot.img from this hex dump like this:

  $ xxd -r -p boot.img.gz.hex boot.img.gz
  $ gunzip -9 boot.img.gz

+For boot image header version 4, these tests rely on two images that are generated +using the same steps above :

+1- boot.img :

• $ mkbootimg --kernel ./kernel --ramdisk ./ramdisk.img \

•            --cmdline "cmdline test" --dtb ./dtb.img   \
  

•            --os_version R --os_patch_level 2019-06-05 \
  

•            --header_version 4 --output ./boot.img
  

+2- vendor_boot.img

• $ mkbootimg --kernel ./kernel --ramdisk ./ramdisk.img \

•            --cmdline "cmdline test" --dtb ./dtb.img   \
  

•            --os_version R --os_patch_level 2019-06-05 \
  

•            --pagesize 4096  --vendor_ramdisk ./ramdisk.img \
  

•            --header_version 4 --vendor_boot ./vboot.img \
  

Is it possible for us to run these commands in the test, to avoid hassle when we want to change the disk image? See test_ut.py

Hi Simon,

I think that it is possible to run these commands in the test. However,

this requires adding "mkbootimg" android tool with the last updates

to generate the new vendor boot image and boot image with header version 4.

I think this change may come as an update for the whole test_abootimg later.

...

• """

  # boot.img.gz hex dump

@@ -44,6 +61,24 @@ b7762ffff07d345446c1281805e8a0868d81e117a45e111c0d8dc101b253 9c03c41a0c90f17fe85400986d82452b6c3680198a192a0ce17c3610ae34 d4a9820881a70f3873f35352731892f3730b124b32937252a96bb9119ae5 463a5546f82c1f05a360148c8251300a462e000085bf67f200200000"""

+# boot img v4 hex dump +boot_img_hex = """1f8b080827b0cd630203626f6f742e696d6700edd8bd0d82601885d1d7c4 +58d8c808b88195bd098d8d246e40e42b083f1aa0717be99d003d277916b8 +e5bddc8a7b792d8e8788c896ce9b88d32ebe6c971e7ddd3543cae734cd01 +c0ffc84c0000b0766d1a87d4e5afeadd3dab7a6f10000000f84163d5d7cd +d43a000000000000000060c53e7544995700400000"""

+# vendor boot image v4 hex dump +vboot_img_hex = """1f8b0808baaecd63020376626f6f742e696d6700edd8310b824018c6f1b3 +222a08f41b3436b4280dcdd19c11d16ee9109d18d59042d047ec8b04cd0d +d19d5a4345534bf6ffc173ef29272f38e93b1d0ec67dd79d548462aa1cd2 +d5d20b0000f8438678f90c18d584b8a4bbb3a557991ecb2a0000f80d6b2f +f4179b656be5c532f2fc066f040000000080e23936af2755f62a3d918df1 +db2a7ab67f9ffdeb7df7cda3465ecb79c4ce7e5c577562bb9364b74449a5 +1e467e20c53c0a57de763193c1779b3b4fcd9d4ee27c6a0e00000000c0ff +309ffea7010000000040f1dc004129855400400000"""

[..]

...

-gtdi = None +gtdi1 = None @pytest.fixture(scope='function') def abootimg_disk_image(u_boot_console): """pytest fixture to provide a AbootimgTestDiskImage object to tests. @@ -109,10 +148,36 @@ def abootimg_disk_image(u_boot_console): function-scoped. However, we don't need to actually do any function-scope work, so this simply returns the same object over and over each time."""

• global gtdi
• if not gtdi:

•    gtdi = AbootimgTestDiskImage(u_boot_console)
  

• return gtdi

• global gtdi1
• if not gtdi1:

•    gtdi1 = AbootimgTestDiskImage(u_boot_console, 'boot.img', img_hex)
  

• return gtdi1

+gtdi2 = None +@pytest.fixture(scope='function') +def abootimgv4_disk_image_vboot(u_boot_console):

• """pytest fixture to provide a AbootimgTestDiskImage object to tests.
• This is function-scoped because it uses u_boot_console, which is also
• function-scoped. However, we don't need to actually do any function-scope
• work, so this simply returns the same object over and over each time."""
• global gtdi2
• if not gtdi2:

•    gtdi2 = AbootimgTestDiskImage(u_boot_console, 'vendor_boot.img', vboot_img_hex)
  

• return gtdi2

+gtdi3 = None +@pytest.fixture(scope='function') +def abootimgv4_disk_image_boot(u_boot_console):

• """pytest fixture to provide a AbootimgTestDiskImage object to tests.

blank line here (please fix throughout)

...

• This is function-scoped because it uses u_boot_console, which is also

• function-scoped. However, we don't need to actually do any function-scope

• work, so this simply returns the same object over and over each time."""

• global gtdi3

• if not gtdi3:

•    gtdi3 = AbootimgTestDiskImage(u_boot_console, 'bootv4.img', boot_img_hex)
  

• return gtdi3

  @pytest.mark.boardspec('sandbox') @pytest.mark.buildconfigspec('android_boot_image')

@@ -157,3 +222,47 @@ def test_abootimg(abootimg_disk_image, u_boot_console): u_boot_console.run_command('fdt get value v / model') response = u_boot_console.run_command('env print v') assert response == 'v=x2'

+@pytest.mark.boardspec('sandbox') +@pytest.mark.buildconfigspec('android_boot_image') +@pytest.mark.buildconfigspec('cmd_abootimg') +@pytest.mark.buildconfigspec('cmd_fdt') +@pytest.mark.requiredtool('xxd') +@pytest.mark.requiredtool('gunzip') +def test_abootimgv4(abootimgv4_disk_image_vboot, abootimgv4_disk_image_boot, u_boot_console):

• """Test the 'abootimg' command with boot image header v4."""
• u_boot_console.log.action('Loading disk image to RAM...')

Can you put u_boot_console in a var like 'cons' to make it less verbose?

I will try to add this change and all the remarks/fixes

you have pointed out earlier in a v3.

Thank you for the review,

--safae

...

• u_boot_console.run_command('setenv loadaddr 0x%x' % (loadaddr))
• u_boot_console.run_command('setenv vloadaddr 0x%x' % (vloadaddr))
• u_boot_console.run_command('host load hostfs - 0x%x %s' % (vloadaddr,

•   abootimgv4_disk_image_vboot.path))
  

• u_boot_console.run_command('host load hostfs - 0x%x %s' % (loadaddr,

•    abootimgv4_disk_image_boot.path))
  

• u_boot_console.run_command('abootimg addr 0x%x 0x%x' % (loadaddr, vloadaddr))
• u_boot_console.log.action('Testing 'abootimg get ver'...')
• response = u_boot_console.run_command('abootimg get ver')
• assert response == "4"
• u_boot_console.run_command('abootimg get ver v')
• response = u_boot_console.run_command('env print v')
• assert response == 'v=4'
• u_boot_console.log.action('Testing 'abootimg get recovery_dtbo'...')
• response = u_boot_console.run_command('abootimg get recovery_dtbo a')
• assert response == 'Error: header version must be >= 1 and <= 2 to get dtbo'
• u_boot_console.log.action('Testing 'abootimg get dtb_load_addr'...')
• u_boot_console.run_command('abootimg get dtb_load_addr a')
• response = u_boot_console.run_command('env print a')
• assert response == 'a=11f00000'
• u_boot_console.log.action('Testing 'abootimg get dtb --index'...')
• u_boot_console.run_command('abootimg get dtb --index=1 dtb2_start')
• response = u_boot_console.run_command('env print dtb2_start')
• correct_str = "dtb2_start=%x" % (dtb2_addr)
• assert response == correct_str
• u_boot_console.run_command('fdt addr $dtb2_start')
• u_boot_console.run_command('fdt get value v / model')
• response = u_boot_console.run_command('env print v')
• assert response == 'v=x2'

-- 2.34.1

Regards, Simon

On 27/01/2023 21:37, Tom Rini wrote:

On Fri, Jan 27, 2023 at 04:51:41PM +0100, Safae Ouajih wrote:

...

On 27/01/2023 01:55, Simon Glass wrote:

...

Hi Safae,

On Thu, 26 Jan 2023 at 09:05, Safae Ouajih souajih@baylibre.com wrote:

...

test_abootimg is extended to include the testing of boot images version 4. For this, boot.img and vendor_boot.img have been generated using mkbootimg tool with setting the header version to 4.

This tests:

• Getting the header version using abootimg
• Extracting the load address of the dtb
• Extracting the dtb start address in RAM

Running test: $ ./test/py/test.py --bd sandbox --build -k test_abootimg

Signed-off-by: Safae Ouajih souajih@baylibre.com

test/py/tests/test_android/test_abootimg.py | 135 ++++++++++++++++++-- 1 file changed, 122 insertions(+), 13 deletions(-)

Reviewed-by: Simon Glass sjg@chromium.org

Question below

...

diff --git a/test/py/tests/test_android/test_abootimg.py b/test/py/tests/test_android/test_abootimg.py index 43a7099c46..a5c734b9c5 100644 --- a/test/py/tests/test_android/test_abootimg.py +++ b/test/py/tests/test_android/test_abootimg.py @@ -32,6 +32,23 @@ Now one can obtain original boot.img from this hex dump like this:

   $ xxd -r -p boot.img.gz.hex boot.img.gz
   $ gunzip -9 boot.img.gz

+For boot image header version 4, these tests rely on two images that are generated +using the same steps above :

+1- boot.img :

• $ mkbootimg --kernel ./kernel --ramdisk ./ramdisk.img \

•            --cmdline "cmdline test" --dtb ./dtb.img   \
  

•            --os_version R --os_patch_level 2019-06-05 \
  

•            --header_version 4 --output ./boot.img
  

+2- vendor_boot.img

• $ mkbootimg --kernel ./kernel --ramdisk ./ramdisk.img \

•            --cmdline "cmdline test" --dtb ./dtb.img   \
  

•            --os_version R --os_patch_level 2019-06-05 \
  

•            --pagesize 4096  --vendor_ramdisk ./ramdisk.img \
  

•            --header_version 4 --vendor_boot ./vboot.img \
  

Is it possible for us to run these commands in the test, to avoid hassle when we want to change the disk image? See test_ut.py

Hi Simon,

I think that it is possible to run these commands in the test. However,

this requires adding "mkbootimg" android tool with the last updates

Please update tools/docker/Dockerfile to include this tool then, so we can run it in CI (or locally using the images).

Hello Tom,

I will add a patch to add that tool in a v3.

Thank you,

Best regards,

Safae