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 576D9C433EF for ; Mon, 29 Nov 2021 11:39:13 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id CF5BA82F70; Mon, 29 Nov 2021 12:39:10 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=fail (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=linaro.org header.i=@linaro.org header.b="RPAzn7PD"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 530AF82F8B; Mon, 29 Nov 2021 12:39:08 +0100 (CET) Received: from mail-io1-xd34.google.com (mail-io1-xd34.google.com [IPv6:2607:f8b0:4864:20::d34]) (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 8EE74801B2 for ; Mon, 29 Nov 2021 12:38:59 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=sughosh.ganu@linaro.org Received: by mail-io1-xd34.google.com with SMTP id k21so21035490ioh.4 for ; Mon, 29 Nov 2021 03:38:59 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=h4UPJC4bHllDuUU5JyuHbqqOOofCmGlFZvZf6GWrsLQ=; b=RPAzn7PDYC4tmBeDiwmFrBwT6wqLNiEcCnJC9EwcEpUQ6GoyTwk9wGkx1xB/ngTo81 M0nuNS9GZQAPUFyaNKBDRzTim6YP8TIm+wQvOGo5mE44UE+CAwDpHPcAQi6St/GYfIEM vMfkFYgOKnNd6+HHG8Cd0WbpaHcXr/OUd5MWyQsNk2pwFbJLZxa+wxl1TUe9N+ZSQSOl JKEOCUYOCiOeOqTtkyoRbJbrZobw4c/+JCGc2gH+r5pr8UqxrNJUwlD+XME9Zcv7N9ex 9eqrA8Tbv5Bbmg5H4frYUHAFqmftH1vfoGIfAvS6sx7EXntVITUIOUhMMcjLv7T8OX2R 4qWw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=h4UPJC4bHllDuUU5JyuHbqqOOofCmGlFZvZf6GWrsLQ=; b=8NElduME37qYAf0VHRJ7bBFy1cWaltBTs+2gVaT2aqh4tfbMd1rT59ADk47ReIWNUd TsGOMixIsE43ZOOBHuzJGzKvV8Kqoqm0smM9kfLxo2Ch6ptIiizPT3jl/ma4hUghUYdj Eh50IhuURaSu3Rmkff+TA8Qmuj+W/swFT/JnTZ7VPgUlsjXdzuR/yRNASx0KFQd693LL aPWE88hTmqIN4OvJbaQhzsUNu7R/6WcLBB1eBvNW8DYptA7lv8vd0laWpxwyzCjbEICr LAqqDfSYSbWi4tEiOaQkexsdsBm6RfKFQNniC2xrXP/fKxsu3x+d60mExObb4Qac750S Vgdw== X-Gm-Message-State: AOAM533pnQUAFFcnL2Bp+XeG8Cin18W9mzpLaAsbErJxewl1PVXkWL0c bl1VRzfHkiQOJN+TDjGjQYaxvF47wrH6vLNu66ib3g== X-Google-Smtp-Source: ABdhPJwUyu6/nhdqZdGKikCy71tDTbY9Xxc00+Vc4HRq2NYaIRX4J7oRaQQJkhBG/Dp0f2rZ2RA2QSsBmu/K7IPj0Kw= X-Received: by 2002:a6b:ba05:: with SMTP id k5mr52310021iof.194.1638185937916; Mon, 29 Nov 2021 03:38:57 -0800 (PST) MIME-Version: 1.0 References: <20211125071302.3644-1-sughosh.ganu@linaro.org> <20211125071302.3644-8-sughosh.ganu@linaro.org> In-Reply-To: From: Sughosh Ganu Date: Mon, 29 Nov 2021 17:08:47 +0530 Message-ID: Subject: Re: [RESEND RFC PATCH 07/10] EFI: FMP: Add provision to update image's ImageTypeId in image descriptor To: Heinrich Schuchardt Cc: Patrick Delaunay , Patrice Chotard , Alexander Graf , Simon Glass , Bin Meng , Peng Fan , AKASHI Takahiro , Ilias Apalodimas , Jose Marinho , Grant Likely , Jason Liu , u-boot@lists.denx.de Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.37 X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.37 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.2 at phobos.denx.de X-Virus-Status: Clean hi Heinrich, Thanks for taking up the review. On Fri, 26 Nov 2021 at 18:18, Heinrich Schuchardt wrote: > On 11/25/21 08:12, Sughosh Ganu wrote: > > The FWU Multi Banks Update feature allows updating different types of > > updatable firmware images on the platform. These image types are > > identified using the ImageTypeId GUID value. Add support in the > > GetImageInfo function of the FMP protocol to get the GUID values for > > the individual images and populate these in the image descriptor for > > the corresponding images. > > > > Signed-off-by: Sughosh Ganu > > --- > > lib/efi_loader/efi_firmware.c | 76 ++++++++++++++++++++++++++++++++--- > > 1 file changed, 71 insertions(+), 5 deletions(-) > > > > diff --git a/lib/efi_loader/efi_firmware.c > b/lib/efi_loader/efi_firmware.c > > index a1b88dbfc2..a2b639b448 100644 > > --- a/lib/efi_loader/efi_firmware.c > > +++ b/lib/efi_loader/efi_firmware.c > > @@ -10,6 +10,7 @@ > > #include > > #include > > #include > > +#include > > #include > > #include > > > > @@ -106,7 +107,8 @@ efi_status_t EFIAPI > efi_firmware_set_package_info_unsupported( > > * @descriptor_size: Pointer to descriptor size > > * package_version: Package version > > * package_version_name: Package version's name > > - * image_type: Image type GUID > > + * guid_array: Image type GUID array > > + * nparts: Number of partions on the storage device > > * > > * Return information bout the current firmware image in @image_info. > > * @image_info will consist of a number of descriptors. > > @@ -122,7 +124,7 @@ static efi_status_t efi_get_dfu_info( > > efi_uintn_t *descriptor_size, > > u32 *package_version, > > u16 **package_version_name, > > - const efi_guid_t *image_type) > > + const efi_guid_t *guid_array, u32 nparts) > > { > > struct dfu_entity *dfu; > > size_t names_len, total_size; > > @@ -145,6 +147,19 @@ static efi_status_t efi_get_dfu_info( > > return EFI_SUCCESS; > > } > > > > + if (IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) { > > The concept of multiple banks is not related to the concept of multiple > GUIDs. So this if statement should be removed. Okay. Will make the change. > > > + /* > > + * For FWU multi bank updates, the number of partitions > > + * should at least be same as dfu partitions or less > > + */ > > + if (nparts > dfu_num) { > > + log_err("Number of dfu alt no's less than > partitions\n"); > > + dfu_free_entities(); > > + > > + return EFI_INVALID_PARAMETER; > > + } > > + } > > + > > total_size = sizeof(*image_info) * dfu_num + names_len; > > /* > > * we will assume that sizeof(*image_info) * dfu_name > > @@ -172,7 +187,11 @@ static efi_status_t efi_get_dfu_info( > > next = name; > > list_for_each_entry(dfu, &dfu_list, list) { > > image_info[i].image_index = dfu->alt + 1; > > - image_info[i].image_type_id = *image_type; > > + if (IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) > > The number of GUIDs is not related to the number of banks. > Please, remove this test. > Okay, will make it common, as you suggest. > > > + image_info[i].image_type_id = guid_array[i]; > > The sequence of GUIDs in a capsule should not influence the update. The > design lacks a configuration defining which GUID maps to which DFU part. > > Please, get rid of all DFU environment variables and describe the update > in a configuration file that you add to the capsule. > Like we discussed, the information on the update, like the device, partition number etc cannot be stored as part of the capsule header currently. So, for now, we have to make do with using the dfu variables for this. > > + else > > + image_info[i].image_type_id = *guid_array; > > Do you want to write the same image to multiple places? Why? > I was keeping the logic consistent with the way it is currently. This will go away as the explicit check for CONFIG_FWU_MULTI_BANK_UPDATE is removed based on your comment above. -sughosh > Best regards > > Heirnich > > > + > > image_info[i].image_id = dfu->alt; > > > > /* copy the DFU entity name */ > > @@ -249,7 +268,9 @@ efi_status_t EFIAPI efi_firmware_fit_get_image_info( > > u32 *package_version, > > u16 **package_version_name) > > { > > + u32 nparts; > > efi_status_t ret; > > + efi_guid_t *part_guid_arr; > > > > EFI_ENTRY("%p %p %p %p %p %p %p %p\n", this, > > image_info_size, image_info, > > @@ -264,12 +285,24 @@ efi_status_t EFIAPI > efi_firmware_fit_get_image_info( > > !descriptor_size || !package_version || > !package_version_name)) > > return EFI_EXIT(EFI_INVALID_PARAMETER); > > > > + part_guid_arr = malloc(sizeof(efi_guid_t)); > > + if (!part_guid_arr) { > > + log_err("Unable to allocate memory for guid array\n"); > > + ret = EFI_OUT_OF_RESOURCES; > > + goto out; > > + } > > + > > + guidcpy(part_guid_arr, &efi_firmware_image_type_uboot_fit); > > + nparts = 1; > > + > > ret = efi_get_dfu_info(image_info_size, image_info, > > descriptor_version, descriptor_count, > > descriptor_size, > > package_version, package_version_name, > > - &efi_firmware_image_type_uboot_fit); > > + part_guid_arr, nparts); > > > > +out: > > + free(part_guid_arr); > > return EFI_EXIT(ret); > > } > > > > @@ -358,7 +391,10 @@ efi_status_t EFIAPI efi_firmware_raw_get_image_info( > > u32 *package_version, > > u16 **package_version_name) > > { > > + u32 nparts; > > + int status; > > efi_status_t ret = EFI_SUCCESS; > > + efi_guid_t *part_guid_arr; > > > > EFI_ENTRY("%p %p %p %p %p %p %p %p\n", this, > > image_info_size, image_info, > > @@ -373,12 +409,42 @@ efi_status_t EFIAPI > efi_firmware_raw_get_image_info( > > !descriptor_size || !package_version || > !package_version_name)) > > return EFI_EXIT(EFI_INVALID_PARAMETER); > > > > + if (IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) { > > + /* > > + * Read the ImageType GUID values. Populate the guid array > > + * with thesevalues. These are the values to be used in the > > + * capsule for ImageTypeId. > > + */ > > + status = fwu_fill_partition_guid_array(&part_guid_arr, > > + &nparts); > > + if (status < 0) { > > + log_err("Unable to get partiion guid's\n"); > > + if (status == -EIO) > > + ret = EFI_DEVICE_ERROR; > > + else if (status == -ENOMEM) > > + ret = EFI_OUT_OF_RESOURCES; > > + goto out; > > + } > > + } else { > > + part_guid_arr = malloc(sizeof(efi_guid_t)); > > + if (!part_guid_arr) { > > + log_err("Unable to allocate memory for guid > array\n"); > > + ret = EFI_OUT_OF_RESOURCES; > > + goto out; > > + } > > + > > + guidcpy(part_guid_arr, &efi_firmware_image_type_uboot_raw); > > + nparts = 1; > > + } > > + > > ret = efi_get_dfu_info(image_info_size, image_info, > > descriptor_version, descriptor_count, > > descriptor_size, > > package_version, package_version_name, > > - &efi_firmware_image_type_uboot_raw); > > + part_guid_arr, nparts); > > > > +out: > > + free(part_guid_arr); > > return EFI_EXIT(ret); > > } > > > > > >