From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 6D083C61DA4 for ; Thu, 9 Feb 2023 14:26:40 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id A4CD285CEE; Thu, 9 Feb 2023 15:26:37 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=baylibre.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=baylibre-com.20210112.gappssmtp.com header.i=@baylibre-com.20210112.gappssmtp.com header.b="vUtZili0"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id DCA8485CC8; Thu, 9 Feb 2023 15:26:33 +0100 (CET) Received: from mail-wr1-x436.google.com (mail-wr1-x436.google.com [IPv6:2a00:1450:4864:20::436]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 4350885EED for ; Thu, 9 Feb 2023 15:26:29 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=baylibre.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=mkorpershoek@baylibre.com Received: by mail-wr1-x436.google.com with SMTP id az16so1112195wrb.1 for ; Thu, 09 Feb 2023 06:26:29 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=baylibre-com.20210112.gappssmtp.com; s=20210112; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:from:to:cc:subject:date:message-id:reply-to; bh=l7GnEWMZ8uJAJa2Mriy+WFIAmoJNAN+D+TrygSGchGk=; b=vUtZili0OqJqP9hQZeTWlm7HXMkyUYMR0UFz7p7fKprkUQeg0JGpnLSFNx8W4GmVCm mhf6cA84TrJEKcej56rUiaq5afUI95xnAWLpr+X/6grtE9gPTP4FDJWNeUq8phozrUue qgRF/s+1qgwcKz9HdsZiw//0g6QE0jo9Pouo6+WYuuzOQV+bkw14iLqRA/U2eWi6eOvI IUvjQeFIPbm218wNKxDMvqseqQrZcJqLfuvwFu3l7nXAGzbgbk+BkE7sdF8os8x0zHjn elbHBE1nAYb6NVlu2zGrbFTo6yZ9XD+JR4C4dtivlwceJlIK7JsaaeoFEFOSTqm2c1hp zW8g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=l7GnEWMZ8uJAJa2Mriy+WFIAmoJNAN+D+TrygSGchGk=; b=E50UEy3De/nYrU1oXim1wJA0M1SarWaUfkLMRplla11f6xsUZVNDyiNV8AL4ZWH01y 1GapDwNGVJ2+CUZJZuZ0BfsmicfqyEHKbWxdi+9Bu8NYYzBXlBWMQefyDL8UhExjjfhr /961FMbw8kR9PjLdDvSfG5EragqFB13PWW7ASn0sjpgURQPIQOkKPFS2Zdqd8P4nTUm6 FnXvz9+MWjvzdHKTosGSrhfdlgpbxVIuPB5XPHf6CFrbD0EDEq5uVlbqsObicWS5U4rV 5zvP3smjdomJqNKMrVa39Cr02uvD2+WgDjQ32wMYr8YayLxSBM5h3X3Po6HtCIB2kHyo SkwA== X-Gm-Message-State: AO0yUKVRvthSHbOz1Ebkw6IlsSwn41BNrNzneoECt28rwkOYuaSoO0rf sHYdrdoaIoWLOKJlc7ZkE0MiPQ== X-Google-Smtp-Source: AK7set9TxtyHLazb0+CmMP7aJvU76Q2Z0yv2PVobGPhDbnS6i3Tnsvtnztm4uk+FMb5k64LZr47daA== X-Received: by 2002:adf:dfc5:0:b0:2c4:60d:4e13 with SMTP id q5-20020adfdfc5000000b002c4060d4e13mr5614070wrn.14.1675952788654; Thu, 09 Feb 2023 06:26:28 -0800 (PST) Received: from localhost ([82.66.159.240]) by smtp.gmail.com with ESMTPSA id q7-20020adfdfc7000000b002c3daaef051sm1341635wrn.82.2023.02.09.06.26.27 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 09 Feb 2023 06:26:28 -0800 (PST) From: Mattijs Korpershoek To: Safae Ouajih , sjg@chromium.org Cc: u-boot@lists.denx.de, sean.anderson@seco.com, r.stratiienko@gmail.com, glaroque@baylibre.com, khilman@baylibre.com Subject: Re: [PATCH v3 06/19] android: boot: move to andr_image_data structure In-Reply-To: <20230205235021.355410-7-souajih@baylibre.com> References: <20230205235021.355410-1-souajih@baylibre.com> <20230205235021.355410-7-souajih@baylibre.com> Date: Thu, 09 Feb 2023 15:26:27 +0100 Message-ID: <87a61mkj8s.fsf@baylibre.com> MIME-Version: 1.0 Content-Type: text/plain X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.6 at phobos.denx.de X-Virus-Status: Clean On Mon, Feb 06, 2023 at 00:50, Safae Ouajih wrote: > Move from andr_boot_img_hdr_v0 to andr_image_data > structure to prepare for boot image header > version 3 and 4. > > Signed-off-by: Safae Ouajih > --- > boot/image-android.c | 121 +++++++++++++++++++++++-------------------- > cmd/abootimg.c | 31 +++++------ > 2 files changed, 81 insertions(+), 71 deletions(-) > > diff --git a/boot/image-android.c b/boot/image-android.c > index ea05c1814f..15a735e230 100644 > --- a/boot/image-android.c > +++ b/boot/image-android.c > @@ -86,7 +86,7 @@ bool android_image_get_data(const void *boot_hdr, struct andr_image_data *data) > return true; > } > > -static ulong android_image_get_kernel_addr(const struct andr_boot_img_hdr_v0 *hdr) > +static ulong android_image_get_kernel_addr(struct andr_image_data *img_data) > { > /* > * All the Android tools that generate a boot.img use this > @@ -99,17 +99,17 @@ static ulong android_image_get_kernel_addr(const struct andr_boot_img_hdr_v0 *hd > * > * Otherwise, we will return the actual value set by the user. > */ > - if (hdr->kernel_addr == ANDROID_IMAGE_DEFAULT_KERNEL_ADDR) > - return (ulong)hdr + hdr->page_size; > + if (img_data->kernel_addr == ANDROID_IMAGE_DEFAULT_KERNEL_ADDR) > + return img_data->kernel_ptr; > > /* > * abootimg creates images where all load addresses are 0 > * and we need to fix them. > */ > - if (hdr->kernel_addr == 0 && hdr->ramdisk_addr == 0) > + if (img_data->kernel_addr == 0 && img_data->ramdisk_addr == 0) > return env_get_ulong("kernel_addr_r", 16, 0); > > - return hdr->kernel_addr; > + return img_data->kernel_addr; > } > > /** > @@ -130,27 +130,33 @@ static ulong android_image_get_kernel_addr(const struct andr_boot_img_hdr_v0 *hd > 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); > - const struct legacy_img_hdr *ihdr = (const struct legacy_img_hdr *) > - ((uintptr_t)hdr + hdr->page_size); > + struct andr_image_data img_data = {0}; > + u32 kernel_addr; > + const struct legacy_img_hdr *ihdr; > + > + if (!android_image_get_data(hdr, &img_data)) > + return -EINVAL; > + > + kernel_addr = android_image_get_kernel_addr(&img_data); > + ihdr = (const struct legacy_img_hdr *)img_data.kernel_ptr; > > /* > * Not all Android tools use the id field for signing the image with > * sha1 (or anything) so we don't check it. It is not obvious that the > * string is null terminated so we take care of this. > */ > - strncpy(andr_tmp_str, hdr->name, ANDR_BOOT_NAME_SIZE); > + strlcpy(andr_tmp_str, img_data.image_name, ANDR_BOOT_NAME_SIZE); While strlcpy is probably better than strncpy here, it has nothing to do with this change. Either mention in the commit message that this is intentional or drop this change. Note that strncpy was used in v2 of this patch. Why change it to strlcpy here? > andr_tmp_str[ANDR_BOOT_NAME_SIZE] = '\0'; > if (strlen(andr_tmp_str)) > printf("Android's image name: %s\n", andr_tmp_str); > > printf("Kernel load addr 0x%08x size %u KiB\n", > - kernel_addr, DIV_ROUND_UP(hdr->kernel_size, 1024)); > + kernel_addr, DIV_ROUND_UP(img_data.kernel_size, 1024)); > > int len = 0; > - if (*hdr->cmdline) { > - printf("Kernel command line: %s\n", hdr->cmdline); > - len += strlen(hdr->cmdline); > + if (*img_data.kcmdline) { > + printf("Kernel command line: %s\n", img_data.kcmdline); > + len += strlen(img_data.kcmdline); > } > > char *bootargs = env_get("bootargs"); > @@ -168,8 +174,9 @@ int android_image_get_kernel(const struct andr_boot_img_hdr_v0 *hdr, int verify, > strcpy(newbootargs, bootargs); > strcat(newbootargs, " "); > } > - if (*hdr->cmdline) > - strcat(newbootargs, hdr->cmdline); > + > + if (*img_data.kcmdline) > + strcat(newbootargs, img_data.kcmdline); > > env_set("bootargs", newbootargs); > > @@ -177,15 +184,14 @@ int android_image_get_kernel(const struct andr_boot_img_hdr_v0 *hdr, int verify, > if (image_get_magic(ihdr) == IH_MAGIC) { > *os_data = image_get_data(ihdr); > } else { > - *os_data = (ulong)hdr; > - *os_data += hdr->page_size; > + *os_data = img_data.kernel_ptr; > } > } > if (os_len) { > if (image_get_magic(ihdr) == IH_MAGIC) > *os_len = image_get_data_size(ihdr); > else > - *os_len = hdr->kernel_size; > + *os_len = img_data.kernel_size; > } > return 0; > } > @@ -197,30 +203,25 @@ bool is_android_boot_image_header(const struct andr_boot_img_hdr_v0 *hdr) > > ulong android_image_get_end(const struct andr_boot_img_hdr_v0 *hdr) > { > - ulong end; > - > - /* > - * The header takes a full page, the remaining components are aligned > - * on page boundary > - */ > - end = (ulong)hdr; > - end += hdr->page_size; > - end += ALIGN(hdr->kernel_size, hdr->page_size); > - end += ALIGN(hdr->ramdisk_size, hdr->page_size); > - end += ALIGN(hdr->second_size, hdr->page_size); > + struct andr_image_data img_data; > > - if (hdr->header_version >= 1) > - end += ALIGN(hdr->recovery_dtbo_size, hdr->page_size); > + if (!android_image_get_data(hdr, &img_data)) > + return -EINVAL; > > - if (hdr->header_version >= 2) > - end += ALIGN(hdr->dtb_size, hdr->page_size); > + if (img_data.header_version > 2) > + return 0; > > - return end; > + return img_data.boot_img_total_size; > } > > ulong android_image_get_kload(const struct andr_boot_img_hdr_v0 *hdr) > { > - return android_image_get_kernel_addr(hdr); > + struct andr_image_data img_data; > + > + if (!android_image_get_data(hdr, &img_data)) > + return -EINVAL; > + > + return android_image_get_kernel_addr(&img_data); > } > > ulong android_image_get_kcomp(const struct andr_boot_img_hdr_v0 *hdr) > @@ -243,38 +244,43 @@ ulong android_image_get_kcomp(const struct andr_boot_img_hdr_v0 *hdr) > int android_image_get_ramdisk(const struct andr_boot_img_hdr_v0 *hdr, > ulong *rd_data, ulong *rd_len) > { > - if (!hdr->ramdisk_size) { > + struct andr_image_data img_data = {0}; > + > + if (!android_image_get_data(hdr, &img_data)) > + return -EINVAL; > + > + if (!img_data.ramdisk_size) { > *rd_data = *rd_len = 0; > return -1; > } > > - printf("RAM disk load addr 0x%08x size %u KiB\n", > - hdr->ramdisk_addr, DIV_ROUND_UP(hdr->ramdisk_size, 1024)); > + printf("RAM disk load addr 0x%08lx size %u KiB\n", > + img_data.ramdisk_ptr, DIV_ROUND_UP(img_data.ramdisk_size, 1024)); > > - *rd_data = (unsigned long)hdr; > - *rd_data += hdr->page_size; > - *rd_data += ALIGN(hdr->kernel_size, hdr->page_size); > + *rd_data = img_data.ramdisk_ptr; > > - *rd_len = hdr->ramdisk_size; > + *rd_len = img_data.ramdisk_size; > return 0; > } > > int android_image_get_second(const struct andr_boot_img_hdr_v0 *hdr, > ulong *second_data, ulong *second_len) > { > - if (!hdr->second_size) { > + struct andr_image_data img_data; > + > + if (!android_image_get_data(hdr, &img_data)) > + return -EINVAL; > + > + if (!img_data.second_size) { > *second_data = *second_len = 0; > return -1; > } > > - *second_data = (unsigned long)hdr; > - *second_data += hdr->page_size; > - *second_data += ALIGN(hdr->kernel_size, hdr->page_size); > - *second_data += ALIGN(hdr->ramdisk_size, hdr->page_size); > + *second_data = img_data.second_ptr; > > printf("second address is 0x%lx\n",*second_data); > > - *second_len = hdr->second_size; > + *second_len = img_data.second_size; > return 0; > } > > @@ -401,17 +407,22 @@ exit: > bool android_image_get_dtb_by_index(ulong hdr_addr, u32 index, ulong *addr, > u32 *size) > { > + struct andr_image_data img_data; > const struct andr_boot_img_hdr_v0 *hdr; > - bool res; > + > + hdr = map_sysmem(hdr_addr, sizeof(*hdr)); > + if (!android_image_get_data(hdr, &img_data)) { > + unmap_sysmem(hdr); > + return false; > + } > + unmap_sysmem(hdr); > + > ulong dtb_img_addr; /* address of DTB part in boot image */ > u32 dtb_img_size; /* size of DTB payload in boot image */ > ulong dtb_addr; /* address of DTB blob with specified index */ > u32 i; /* index iterator */ > > - res = android_image_get_dtb_img_addr(hdr_addr, &dtb_img_addr); > - if (!res) > - return false; > - > + android_image_get_dtb_img_addr(hdr_addr, &dtb_img_addr); Why drop the error handling here? Shouldn't we bail out early if this fails? > /* Check if DTB area of boot image is in DTBO format */ > if (android_dt_check_header(dtb_img_addr)) { > return android_dt_get_fdt_by_index(dtb_img_addr, index, addr, > @@ -419,9 +430,7 @@ bool android_image_get_dtb_by_index(ulong hdr_addr, u32 index, ulong *addr, > } > > /* Find out the address of DTB with specified index in concat blobs */ > - hdr = map_sysmem(hdr_addr, sizeof(*hdr)); > - dtb_img_size = hdr->dtb_size; > - unmap_sysmem(hdr); > + dtb_img_size = img_data.dtb_size; > i = 0; > dtb_addr = dtb_img_addr; > while (dtb_addr < dtb_img_addr + dtb_img_size) { > diff --git a/cmd/abootimg.c b/cmd/abootimg.c > index b5cfb141ef..f04a7c7c8e 100644 > --- a/cmd/abootimg.c > +++ b/cmd/abootimg.c > @@ -66,33 +66,34 @@ 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_boot_img_hdr_v0 *hdr; > - int res = CMD_RET_SUCCESS; > - > if (argc > 1) > return CMD_RET_USAGE; > + struct andr_image_data img_data = {0}; > + const struct andr_boot_img_hdr_v0 *hdr; Don't move this around. keep the variable declarations at the beginning of the function as done in the other functions. > > hdr = map_sysmem(abootimg_addr(), sizeof(*hdr)); > - if (!is_android_boot_image_header(hdr)) { > - printf("Error: Boot Image header is incorrect\n"); > - res = CMD_RET_FAILURE; > - goto exit; > + if (!android_image_get_data(hdr, &img_data)) { > + unmap_sysmem(hdr); > + return CMD_RET_FAILURE; > } > + unmap_sysmem(hdr); > > - if (hdr->header_version < 2) { > + if (img_data.header_version < 2) { > printf("Error: header_version must be >= 2 for this\n"); > - res = CMD_RET_FAILURE; > - goto exit; > + return CMD_RET_FAILURE; > + } > + > + if (!img_data.dtb_load_addr) { > + printf("Error: failed to read dtb_load_addr\n"); > + return CMD_RET_FAILURE; > } > > if (argc == 0) > - printf("%lx\n", (ulong)hdr->dtb_addr); > + printf("%lx\n", (ulong)img_data.dtb_load_addr); > else > - env_set_hex(argv[0], (ulong)hdr->dtb_addr); > + env_set_hex(argv[0], (ulong)img_data.dtb_load_addr); > > -exit: > - unmap_sysmem(hdr); > - return res; > + return CMD_RET_SUCCESS; > } > > static int abootimg_get_dtb_by_index(int argc, char *const argv[]) > -- > 2.34.1