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 82759C54EE9 for ; Wed, 7 Sep 2022 13:34:28 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 44F3284A71; Wed, 7 Sep 2022 15:34:26 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (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=pass (2048-bit key; unprotected) header.d=linaro.org header.i=@linaro.org header.b="wCVTpc8U"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 86DAE84A9C; Wed, 7 Sep 2022 15:34:24 +0200 (CEST) Received: from mail-ej1-x632.google.com (mail-ej1-x632.google.com [IPv6:2a00:1450:4864:20::632]) (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 6A69184A1B for ; Wed, 7 Sep 2022 15:34:21 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=ilias.apalodimas@linaro.org Received: by mail-ej1-x632.google.com with SMTP id fg1so8309591ejc.2 for ; Wed, 07 Sep 2022 06:34:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date; bh=JSU0ThmDf57Y5dUP9X3D6N9g061JxXEzT/bCbe4M95g=; b=wCVTpc8U+pBcgEsLndPAuVhlK4uSo80VZ4NgjBV1k0nZGJPooOvp8hZYIIqztBzszA ktvTLkgeMgmI4gDRdCTIio54ytK0n8DRSl+hkjmWtHNqyuVe1rvw8QnMK5K14xOuEDld dSh+wal8X2wVpQHbbNTnsMmxDdF6nV+xkW+bm9hQxKIxqeg4HSGjqZcpb3Jly3YLSQYh yU8vJ3tpunVZV6vGa8TCoCT3I23/ZmStku1GtWaFtMm8WeI9OsgSAyf2vKfyxgdrQCNw 2VPb9mNy95jLgaifx/iPJGcOhggPvRvovXKGea0obKNmcwE2mkhQXxCEyVvBq2EamWNO PHRg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date; bh=JSU0ThmDf57Y5dUP9X3D6N9g061JxXEzT/bCbe4M95g=; b=JWTbLWmhnNDurtS+Lx8EbOMtmMjD4pvTOFH5s++866c6+0UGmZus6kaF6LCIqJdSi6 h321Yp95LPb/qNjMwWXiqOD3t94+ZD5n8+bq6pzuyBB8en3kjuWSbiP8chLWXXVoEAPG lMStyGB2aYXfAxEkVUyZhXNSIxVmf0meKvRCLA4L2wk3dQHgssp+p69wg9xm2BFf/kkF PXIQSq5EMETimUr/qKbdlSOIDp/6XgejyMe9zo8UklY7fyafGUKHVxosbH29HKX2pulM 05j5p8iuoq2HMs3JSlEkMbcURW72W2sCyU5ARqOPNkZ1QDHyvPGBwNWm6ZJCLemU4Qtn e4DQ== X-Gm-Message-State: ACgBeo10wnaiMsz/sohOXv8Kx0u0o5stzWJuNVqdO/yWjmQ0VYxOdIdG dVGwgrZEWYJyAeu3UZUR74eSPg== X-Google-Smtp-Source: AA6agR4woW+yLV2IvZwfrRKLx9ODCfP9W3ExgEYisax8iUcd8hfDnjiBsFbQmGPyHoJB6T4jik21gg== X-Received: by 2002:a17:907:762b:b0:771:5755:82b7 with SMTP id jy11-20020a170907762b00b00771575582b7mr440938ejc.684.1662557660973; Wed, 07 Sep 2022 06:34:20 -0700 (PDT) Received: from hera ([46.103.15.185]) by smtp.gmail.com with ESMTPSA id p25-20020aa7d319000000b0044ee2869ef7sm3071925edq.4.2022.09.07.06.34.19 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 07 Sep 2022 06:34:20 -0700 (PDT) Date: Wed, 7 Sep 2022 16:34:17 +0300 From: Ilias Apalodimas To: Sughosh Ganu Cc: u-boot@lists.denx.de, Heinrich Schuchardt , Takahiro Akashi , Patrick Delaunay , Patrice Chotard , Simon Glass , Bin Meng , Tom Rini , Etienne Carriere , Michal Simek , Jassi Brar Subject: Re: [PATCH v9 10/15] FWU: Add support for the FWU Multi Bank Update feature Message-ID: References: <20220826095716.1676150-1-sughosh.ganu@linaro.org> <20220826095716.1676150-11-sughosh.ganu@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220826095716.1676150-11-sughosh.ganu@linaro.org> 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 Hi Sughosh, Overall, I think the code is covering all the cases for A/B updates. Irrelevant to this patchset, but in the future I think we shopuld look into having efi_launch_capsules() call efi_update_capsule() would be cleaner. [...] I think you can get rid all of these > + > +__maybe_unused static u32 update_index; This can be calculated at runtime > +__maybe_unused static bool capsule_update; Dittio, you just need a call to figure out if it's an empty capsule or not > +__maybe_unused static bool fw_accept_os; Same for this one > +static bool image_index_check = true; Can't this just be an ifdef in the if statement that uses it? > > #ifdef CONFIG_EFI_CAPSULE_ON_DISK > /* for file system access */ > @@ -205,7 +217,8 @@ efi_fmp_find(efi_guid_t *image_type, u8 image_index, u64 instance, > log_debug("+++ desc[%d] index: %d, name: %ls\n", > j, desc->image_index, desc->image_id_name); > if (!guidcmp(&desc->image_type_id, image_type) && > - (desc->image_index == image_index) && > + (!image_index_check || > + desc->image_index == image_index) && > (!instance || > !desc->hardware_instance || > desc->hardware_instance == instance)) > @@ -388,6 +401,83 @@ efi_status_t efi_capsule_authenticate(const void *capsule, efi_uintn_t capsule_s > } > #endif /* CONFIG_EFI_CAPSULE_AUTHENTICATE */ > > +static bool fwu_empty_capsule(struct efi_capsule_header *capsule) > +{ > + return !guidcmp(&capsule->capsule_guid, > + &fwu_guid_os_request_fw_revert) || > + !guidcmp(&capsule->capsule_guid, > + &fwu_guid_os_request_fw_accept); > +} > + > +static efi_status_t fwu_to_efi_error(int err) > +{ > + efi_status_t ret; > + > + switch(err) { > + case 0: > + ret = EFI_SUCCESS; > + break; > + case -ENODEV: > + case -ERANGE: > + case -EIO: > + ret = EFI_DEVICE_ERROR; > + break; > + case -EINVAL: > + ret = EFI_INVALID_PARAMETER; > + break; > + default: > + ret = EFI_OUT_OF_RESOURCES; > + } > + > + return ret; > +} > + > +static efi_status_t fwu_empty_capsule_process( > + struct efi_capsule_header *capsule) > +{ > + int status; > + u32 active_idx; > + efi_status_t ret; > + efi_guid_t *image_guid; > + > + if (!guidcmp(&capsule->capsule_guid, > + &fwu_guid_os_request_fw_revert)) { > + /* > + * One of the previously updated image has > + * failed the OS acceptance test. OS has > + * requested to revert back to the earlier > + * boot index > + */ > + status = fwu_revert_boot_index(); > + ret = fwu_to_efi_error(status); > + if (ret == EFI_SUCCESS) > + log_info("Reverted the FWU active_index. Recommend rebooting the system\n"); > + else > + log_err("Failed to revert the FWU boot index\n"); > + } else { > + /* > + * Image accepted by the OS. Set the acceptance > + * status for the image. > + */ > + image_guid = (void *)(char *)capsule + > + capsule->header_size; > + > + status = fwu_get_active_index(&active_idx); > + ret = fwu_to_efi_error(status); > + if (ret != EFI_SUCCESS) { > + log_err("Unable to get the active_index from the FWU metadata\n"); > + return ret; > + } > + > + status = fwu_accept_image(image_guid, active_idx); > + ret = fwu_to_efi_error(status); > + if (ret != EFI_SUCCESS) > + log_err("Unable to set the Accept bit for the image %pUs\n", > + image_guid); > + } > + > + return ret; > +} > > /** > * efi_capsule_update_firmware - update firmware from capsule > @@ -407,10 +497,42 @@ static efi_status_t efi_capsule_update_firmware( > void *image_binary, *vendor_code; > efi_handle_t *handles; > efi_uintn_t no_handles; > - int item; > + int item, alt_no; > struct efi_firmware_management_protocol *fmp; > u16 *abort_reason; > + efi_guid_t image_type_id; > efi_status_t ret = EFI_SUCCESS; > + int status; > + u8 image_index; > + > + if (IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) { > + if (!fwu_empty_capsule(capsule_data) && > + !fwu_update_checks_pass()) { > + log_err("FWU checks failed. Cannot start update\n"); > + return EFI_INVALID_PARAMETER; > + } > + > + if (fwu_empty_capsule(capsule_data)) { > + capsule_update = false; > + return fwu_empty_capsule_process(capsule_data); > + } else { > + capsule_update = true; > + } > + > + /* Obtain the update_index from the platform */ > + status = fwu_plat_get_update_index(&update_index); > + if (status < 0) { > + log_err("Failed to get the FWU update_index value\n"); > + return EFI_DEVICE_ERROR; > + } > + > + fw_accept_os = capsule_data->flags & FW_ACCEPT_OS ? 0x1 : 0x0; > + /* > + * For Multi Bank updates, the image index is determined at > + * runtime based on the value of the update bank. > + */ > + image_index_check = false; > + } > > /* sanity check */ > if (capsule_data->header_size < sizeof(*capsule) || > @@ -485,8 +607,31 @@ static efi_status_t efi_capsule_update_firmware( > goto out; > } > > + if (IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) { > + /* > + * Based on the value of update_image_type_id, > + * derive the alt number value. This will be > + * passed as update_image_index to the > + * set_image function. > + */ > + image_type_id = image->update_image_type_id; > + status = fwu_get_image_alt_num(&image_type_id, > + update_index, > + &alt_no); > + ret = fwu_to_efi_error(status); > + if (ret != EFI_SUCCESS) { > + log_err("Unable to get the alt no for the image type %pUs\n", > + &image_type_id); > + goto out; > + } > + log_debug("alt_no %u for Image Type Id %pUs\n", > + alt_no, &image_type_id); > + image_index = alt_no + 1; > + } else { > + image_index = image->update_image_index; > + } > abort_reason = NULL; > - ret = EFI_CALL(fmp->set_image(fmp, image->update_image_index, > + ret = EFI_CALL(fmp->set_image(fmp, image_index, > image_binary, > image_binary_size, > vendor_code, NULL, > @@ -497,6 +642,33 @@ static efi_status_t efi_capsule_update_firmware( > efi_free_pool(abort_reason); > goto out; > } > + > + if (IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) { > + if (!fw_accept_os) { > + /* > + * The OS will not be accepting the firmware > + * images. Set the accept bit of all the > + * images contained in this capsule. > + */ > + status = fwu_accept_image(&image_type_id, > + update_index); > + } else { > + status = fwu_clear_accept_image(&image_type_id, > + update_index); > + } > + ret = fwu_to_efi_error(status); > + if (ret != EFI_SUCCESS) { > + log_err("Unable to %s the accept bit for the image %pUs\n", > + fw_accept_os ? "clear" : "set", > + &image_type_id); > + goto out; > + } > + > + log_debug("%s the accepted bit for Image %pUs\n", > + fw_accept_os ? "Cleared" : "Set", > + &image_type_id); > + } > + > } > > out: > @@ -1102,8 +1274,10 @@ efi_status_t efi_launch_capsules(void) > { > struct efi_capsule_header *capsule = NULL; > u16 **files; > + int status; > unsigned int nfiles, index, i; > efi_status_t ret; > + bool update_status = true; > > if (check_run_capsules() != EFI_SUCCESS) > return EFI_SUCCESS; > @@ -1131,12 +1305,14 @@ efi_status_t efi_launch_capsules(void) > ret = efi_capsule_read_file(files[i], &capsule); > if (ret == EFI_SUCCESS) { > ret = efi_capsule_update_firmware(capsule); > - if (ret != EFI_SUCCESS) > + if (ret != EFI_SUCCESS) { > log_err("Applying capsule %ls failed.\n", > files[i]); > - else > + update_status = false; > + } else { > log_info("Applying capsule %ls succeeded.\n", > files[i]); > + } > > /* create CapsuleXXXX */ > set_capsule_result(index, capsule, ret); > @@ -1144,6 +1320,7 @@ efi_status_t efi_launch_capsules(void) > free(capsule); > } else { > log_err("Reading capsule %ls failed\n", files[i]); > + update_status = false; > } > /* delete a capsule either in case of success or failure */ > ret = efi_capsule_delete_file(files[i]); > @@ -1151,7 +1328,33 @@ efi_status_t efi_launch_capsules(void) > log_err("Deleting capsule %ls failed\n", > files[i]); > } > + > efi_capsule_scan_done(); > + if (IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) { > + if (update_status == true && capsule_update == true) { > + /* > + * All the capsules have been updated successfully, > + * update the FWU metadata. > + */ > + log_debug("Update Complete. Now updating active_index to %u\n", > + update_index); > + status = fwu_update_active_index(update_index); > + ret = fwu_to_efi_error(status); > + if (ret != EFI_SUCCESS) { > + log_err("Failed to update FWU metadata index values\n"); > + } else { > + log_debug("Successfully updated the active_index\n"); > + ret = EFI_SUCCESS; > + if (fw_accept_os) { > + status = fwu_trial_state_ctr_start(); > + if (status < 0) > + ret = EFI_DEVICE_ERROR; > + } > + } > + } else if (capsule_update == true && update_status == false) { > + log_err("All capsules were not updated. Not updating FWU metadata\n"); > + } > + } > > for (i = 0; i < nfiles; i++) > free(files[i]); > diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c > index 23dc7a0710..60012e873e 100644 > --- a/lib/efi_loader/efi_setup.c > +++ b/lib/efi_loader/efi_setup.c > @@ -351,7 +351,8 @@ efi_status_t efi_init_obj_list(void) > goto out; > > /* Execute capsules after reboot */ > - if (IS_ENABLED(CONFIG_EFI_CAPSULE_ON_DISK) && > + if (!IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE) && > + IS_ENABLED(CONFIG_EFI_CAPSULE_ON_DISK) && > !IS_ENABLED(CONFIG_EFI_CAPSULE_ON_DISK_EARLY)) > ret = efi_launch_capsules(); > out: > diff --git a/lib/fwu_updates/Kconfig b/lib/fwu_updates/Kconfig > new file mode 100644 > index 0000000000..e33085f1d3 > --- /dev/null > +++ b/lib/fwu_updates/Kconfig > @@ -0,0 +1,32 @@ > +config FWU_MULTI_BANK_UPDATE > + bool "Enable FWU Multi Bank Update Feature" > + depends on EFI_HAVE_CAPSULE_SUPPORT > + select PARTITION_TYPE_GUID > + select EFI_SETUP_EARLY > + select EVENT > + help > + Feature for updating firmware images on platforms having > + multiple banks(copies) of the firmware images. One of the > + bank is selected for updating all the firmware components > + > +config FWU_NUM_BANKS > + int "Number of Banks defined by the platform" > + depends on FWU_MULTI_BANK_UPDATE > + help > + Define the number of banks of firmware images on a platform > + > +config FWU_NUM_IMAGES_PER_BANK > + int "Number of firmware images per bank" > + depends on FWU_MULTI_BANK_UPDATE > + help > + Define the number of firmware images per bank. This value > + should be the same for all the banks. > + > +config FWU_TRIAL_STATE_CNT > + int "Number of times system boots in Trial State" > + depends on FWU_MULTI_BANK_UPDATE > + default 3 > + help > + With FWU Multi Bank Update feature enabled, number of times > + the platform is allowed to boot in Trial State after an > + update. > diff --git a/lib/fwu_updates/Makefile b/lib/fwu_updates/Makefile > new file mode 100644 > index 0000000000..1993088e5b > --- /dev/null > +++ b/lib/fwu_updates/Makefile > @@ -0,0 +1,7 @@ > +# SPDX-License-Identifier: GPL-2.0-or-later > +# > +# Copyright (c) 2022, Linaro Limited > +# > + > +obj-$(CONFIG_FWU_MULTI_BANK_UPDATE) += fwu.o > +obj-$(CONFIG_FWU_MDATA_GPT_BLK) += fwu_gpt.o > diff --git a/lib/fwu_updates/fwu.c b/lib/fwu_updates/fwu.c > index 27f48855af..de2412785d 100644 > --- a/lib/fwu_updates/fwu.c > +++ b/lib/fwu_updates/fwu.c > @@ -513,7 +513,30 @@ u8 fwu_update_checks_pass(void) > return !trial_state && boottime_check; > } > > +/** > + * fwu_trial_state_ctr_start() - Start the Trial State counter > + * > + * Start the counter to identify the platform booting in the > + * Trial State. The counter is implemented as an EFI variable. > + * > + * Return: 0 if OK, -ve on error > + * > + */ > +int fwu_trial_state_ctr_start(void) > +{ > + int ret; > + u16 trial_state_ctr; > + > + trial_state_ctr = 0; > + ret = trial_counter_update(&trial_state_ctr); > + if (ret) > + log_err("Unable to initialise TrialStateCtr\n"); > + > + return ret; > +} > + > static int fwu_boottime_checks(void *ctx, struct event *event) > + > { > int ret; > struct udevice *dev; > -- > 2.34.1 > Thanks /Ilias