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 AB859C77B7C for ; Wed, 10 May 2023 00:28:18 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 4B0A9846C6; Wed, 10 May 2023 02:28:16 +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="gZsbQ6L0"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 097D782708; Wed, 10 May 2023 02:28:15 +0200 (CEST) Received: from mail-pl1-x62b.google.com (mail-pl1-x62b.google.com [IPv6:2607:f8b0:4864:20::62b]) (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 992C6847D2 for ; Wed, 10 May 2023 02:28:11 +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=takahiro.akashi@linaro.org Received: by mail-pl1-x62b.google.com with SMTP id d9443c01a7336-1aaf0bc8a07so13765555ad.0 for ; Tue, 09 May 2023 17:28:11 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1683678490; x=1686270490; h=in-reply-to:content-disposition:mime-version:references :mail-followup-to:message-id:subject:cc:to:from:date:from:to:cc :subject:date:message-id:reply-to; bh=iXZT/kvDFiWqwxvVRekFQqtHf+DfyYnVY07tq8VE4Sc=; b=gZsbQ6L0wdYhHlCwYdKVqyG8KIJtI4SNpmpBgaDwfJvPFTrlCPmogYKTFU8b3JJn26 9NMw5tfmegTm5BfuB7SzgdQWJL6At2rhSsVVqvoKM4Jn4b6nzFJgZog2tDOyy+WSI9c9 lqh/4SOY46gCSe9KiY4wcJcW/7NBxM1aynvnsO8lAU0s0jUb4MinzifcWpmVBf1XHhV5 UzR5mrtucI/P92SaAKOnvekWerKoaxzQHk1hnHFB1/KcSVVUgeKfovADN5cwFIZh/mN3 lG2dOEid+dYt9s4wUuL8Ip3UMkuvq20IbUg6s7LGfgZRWkKemHhfSBLZDrjqHg/osfle WNpQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1683678490; x=1686270490; h=in-reply-to:content-disposition:mime-version:references :mail-followup-to:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=iXZT/kvDFiWqwxvVRekFQqtHf+DfyYnVY07tq8VE4Sc=; b=bFCAZYpBLa/TWC2FeP/NwbvmLYYGggCi72lkxU6Nb8WM24qhIRSYuDQJhmSeVifbSp ENwNRXchfMtwX+dTy5yWAilH+JkA7UH8uDEyoDslHGfB+HkDW6Ll5Hqql83rdqZusZ4W vczdEg7qb/i4dVQbD2gDckzq+8ktY1HBHPz5Cjas2PPKjOLkj4z1Vc2IRmEusWHsxX4U Plg9fbXu+DRJio4o6d+ZDw+VoOx7HwaNOqkpnMjKK64w7lnH/6Cce6ZjhIZKuoo10R/y lj3pAvOxXO6myFhCmuQ4haOthHHERETHEIkmKGLN4Yg4SH+k4kBPu/c6p+KnBSSaeezf BFKw== X-Gm-Message-State: AC+VfDz24nk3Rl/QKvTNj1UYP9KieSeYF9extTUyAGi3uearIVjRHClU eHfwuxNfGwyCMSu+7JzZdARgaQ== X-Google-Smtp-Source: ACHHUZ55/MZMSYyaq4UknM01zcpP1dcHcsBKw5g/wNksXWSpUEzMSrgKxPXjvI6F2yrAdLIxF7OQaA== X-Received: by 2002:a17:902:d4c4:b0:1ac:40f7:8b5a with SMTP id o4-20020a170902d4c400b001ac40f78b5amr18905413plg.3.1683678489818; Tue, 09 May 2023 17:28:09 -0700 (PDT) Received: from laputa ([2400:4050:c3e1:100:4aec:13b6:1b75:ada5]) by smtp.gmail.com with ESMTPSA id bb5-20020a170902bc8500b001ab05aaae2fsm2250756plb.107.2023.05.09.17.28.07 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 09 May 2023 17:28:09 -0700 (PDT) Date: Wed, 10 May 2023 09:28:05 +0900 From: Takahiro Akashi To: Masahisa Kojima Cc: Heinrich Schuchardt , Ilias Apalodimas , u-boot@lists.denx.de Subject: Re: [PATCH v5 1/4] efi_loader: get version information from device tree Message-ID: <20230510002805.GA64854@laputa> Mail-Followup-To: Takahiro Akashi , Masahisa Kojima , Heinrich Schuchardt , Ilias Apalodimas , u-boot@lists.denx.de References: <20230410090732.1676-1-masahisa.kojima@linaro.org> <20230410090732.1676-2-masahisa.kojima@linaro.org> <6e02854a-61be-be74-067c-2aee770ac7fc@gmx.de> <23d7a8c1-019b-9731-81d3-ff7bdef61373@gmx.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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.8 at phobos.denx.de X-Virus-Status: Clean On Tue, May 09, 2023 at 06:57:19PM +0900, Masahisa Kojima wrote: > On Mon, 8 May 2023 at 18:44, Heinrich Schuchardt wrote: > > > > On 5/8/23 10:15, Masahisa Kojima wrote: > > > Hi Heinrich, > > > > > > On Thu, 27 Apr 2023 at 15:09, Heinrich Schuchardt wrote: > > >> > > >> On 4/10/23 11:07, Masahisa Kojima wrote: > > >>> Current FMP->GetImageInfo() always return 0 for the firmware > > >>> version, user can not identify which firmware version is currently > > >>> running through the EFI interface. > > >>> > > >>> This commit gets the version information from device tree, > > >>> then fills the firmware version, lowest supported version > > >>> in FMP->GetImageInfo(). > > >>> > > >>> Now FMP->GetImageInfo() and ESRT have the meaningful version number. > > >>> > > >>> Signed-off-by: Masahisa Kojima > > >>> --- > > >>> Changes in v5: > > >>> - newly implement a device tree based versioning > > >>> > > >>> .../firmware/firmware-version.txt | 25 ++++++++ > > >>> lib/efi_loader/efi_firmware.c | 63 +++++++++++++++++-- > > >>> 2 files changed, 84 insertions(+), 4 deletions(-) > > >>> create mode 100644 doc/device-tree-bindings/firmware/firmware-version.txt > > >>> > > >>> diff --git a/doc/device-tree-bindings/firmware/firmware-version.txt b/doc/device-tree-bindings/firmware/firmware-version.txt > > >>> new file mode 100644 > > >>> index 0000000000..6112de4a1d > > >>> --- /dev/null > > >>> +++ b/doc/device-tree-bindings/firmware/firmware-version.txt > > >>> @@ -0,0 +1,25 @@ > > >>> +firmware-version bindings > > >>> +------------------------------- > > >>> + > > >>> +Required properties: > > >>> +- image-type-id : guid for image blob type > > >>> +- image-index : image index > > >>> +- fw-version : firmware version > > >>> +- lowest-supported-version : lowest supported version > > >>> + > > >>> +Example: > > >>> + > > >>> + firmware-version { > > >>> + image1 { > > >>> + image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8"; > > >>> + image-index = <1>; > > >>> + fw-version = <5>; > > >>> + lowest-supported-version = <3>; > > >>> + }; > > >>> + image2 { > > >>> + image-type-id = "5A7021F5-FEF2-48B4-AABA-832E777418C0"; > > >>> + image-index = <2>; > > >>> + fw-version = <10>; > > >>> + lowest-supported-version = <7>; > > >>> + }; > > >>> + }; > > >>> diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c > > >>> index 93e2b01c07..1c6ef468bf 100644 > > >>> --- a/lib/efi_loader/efi_firmware.c > > >>> +++ b/lib/efi_loader/efi_firmware.c > > >>> @@ -102,6 +102,56 @@ efi_status_t EFIAPI efi_firmware_set_package_info_unsupported( > > >>> return EFI_EXIT(EFI_UNSUPPORTED); > > >>> } > > >>> > > >>> +/** > > >>> + * efi_firmware_get_firmware_version - get firmware version from dtb > > >>> + * @image_index: Image index > > >>> + * @image_type_id: Image type id > > >>> + * @fw_version: Pointer to store the version number > > >>> + * @lsv: Pointer to store the lowest supported version > > >>> + * > > >>> + * Authenticate the capsule if authentication is enabled. > > >>> + * The image pointer and the image size are updated in case of success. > > >>> + */ > > >>> +void efi_firmware_get_firmware_version(u8 image_index, > > >>> + efi_guid_t *image_type_id, > > >>> + u32 *fw_version, u32 *lsv) > > >>> +{ > > >>> + const void *fdt = gd->fdt_blob; > > >>> + const fdt32_t *val; > > >>> + const char *guid_str; > > >>> + int len, offset, index; > > >>> + int parent; > > >>> + > > >>> + parent = fdt_subnode_offset(fdt, 0, "firmware-version"); > > >>> + if (parent < 0) > > >>> + return; > > >>> + > > >>> + fdt_for_each_subnode(offset, fdt, parent) { > > >>> + efi_guid_t guid; > > >>> + > > >>> + guid_str = fdt_getprop(fdt, offset, "image-type-id", &len); > > >>> + if (!guid_str) > > >>> + continue; > > >>> + uuid_str_to_bin(guid_str, guid.b, UUID_STR_FORMAT_GUID); > > >>> + > > >>> + val = fdt_getprop(fdt, offset, "image-index", &len); > > >>> + if (!val) > > >>> + continue; > > >>> + index = fdt32_to_cpu(*val); > > >>> + > > >>> + if (!guidcmp(&guid, image_type_id) && index == image_index) { > > >>> + val = fdt_getprop(fdt, offset, "fw-version", &len); > > >>> + if (val) > > >>> + *fw_version = fdt32_to_cpu(*val); > > >>> + > > >>> + val = fdt_getprop(fdt, offset, > > >>> + "lowest-supported-version", &len); > > >>> + if (val) > > >>> + *lsv = fdt32_to_cpu(*val); > > >>> + } > > >>> + } > > >>> +} > > >>> + > > >>> /** > > >>> * efi_fill_image_desc_array - populate image descriptor array > > >>> * @image_info_size: Size of @image_info > > >>> @@ -148,13 +198,19 @@ static efi_status_t efi_fill_image_desc_array( > > >>> *package_version_name = NULL; /* not supported */ > > >>> > > >>> for (i = 0; i < num_image_type_guids; i++) { > > >> > > >> Currently we define num_image_type_guids per board in a C file. Using > > >> the same line of code once per board makes no sense to me. Please, move > > >> the definition of that variable to lib/efi_loader/efi_firmware.c. > > > > > > Sorry for the late reply. > > > > > > num_image_type_guids is calculated with "ARRAY_SIZE(fw_images)", > > > fw_images[] array is also defined in each board file, > > > so we can not simply move num_image_type_guids into > > > lib/efi_loader/efi_firmware.c. > > > > Why can't we have > > > > int num_image_type_guids = ARRAY_SIZE(fw_images); > > > > in lib/efi_loader/efi_firmware.c? > > At first thought, I thought it was a matter of abstraction. > > But there is a compilation error when we expose fw_images[]. > fw_images[] array is initialized in each board file, > and sizeof() of the external fw_images[] array in lib/efi_loader/efi_firmware.c > will cause compilation failure. > We need to specify the array size when fw_images is exposed, > for example: > extern struct efi_fw_image fw_images[2]; > > but currently there is no method to pre-define the fw_images[] array size, > it is board specific. > > We can define the macro to indicate the array size or having > sentinel in the fw_images[] array, but I think the current I simply wonder if the value should be embedded in "struct efi_capsule_update_info". struct efi_capsule_update_info { const char *dfu_string; int num_images; <- added struct efi_fw_image *images; }; This is the best place because the value must match not only "images" but also (entries in) "dfu_string". Even now, efi_fill_image_desc_array() tries to access "fw_images[]" via the exposed update_info variable. Beautiful, isn't it? One more comment: uefi.rst doesn't mention anything about num_image_type_guids. -Takahiro Akashi > implementation is simpler, > I would like to keep the current implementation. > Correct me if I'm wrong. > > Thanks, > Masahisa Kojima > > > > > Best regards > > > > Heinrich > > > > > > > > And fw_images[] array is abstracted by struct efi_capsule_update_info, > > > so I think > > > we should not extract the fw_images[] array. > > > > > >> > > >>> + u32 fw_version = 0; > > >>> + u32 lowest_supported_version = 0; > > >> > > >> These assignments should be in efi_firmware_get_firmware_version. > > > > > > OK. > > > > > >> > > >>> + > > >>> image_info[i].image_index = fw_array[i].image_index; > > >> > > >> Why did we ever introduce the field image_index? Please, eliminate it it > > >> as the GUID is always sufficient to identify an image. > > > > > > This is derived from the UEFI specification. > > > UEFI specification "23.1.2 > > > EFI_FIRMWARE_MANAGEMENT_PROTOCOL.GetImageInfo()" requires ImageIndex > > > and ImageTypeId(guid). > > > > > > ImageIndex: A unique number identifying the firmware image within the > > > device. The number is between 1 and > > > DescriptorCount. > > > > > >> > > >>> image_info[i].image_type_id = fw_array[i].image_type_id; > > >>> image_info[i].image_id = fw_array[i].image_index; > > >>> > > >>> image_info[i].image_id_name = fw_array[i].fw_name; > > >>> - > > >>> - image_info[i].version = 0; /* not supported */ > > >>> + efi_firmware_get_firmware_version(fw_array[i].image_index, > > >>> + &fw_array[i].image_type_id, > > >>> + &fw_version, > > >>> + &lowest_supported_version); > > >> > > >> This interface makes no sense to me. We expect images with specific > > >> GUIDs and should not care about images with other GUIDs that may > > >> additionally exist in the capsule. > > >> > > >> So you must pass the expected GUID as input variable here. > > > > > > I don't clearly understand this comment, but the expected GUID is > > > fw_array[i].image_type_id. > > > > > >> > > >>> + image_info[i].version = fw_version; > > >>> image_info[i].version_name = NULL; /* not supported */ > > >> > > >> Please, add the missing functionality to > > >> efi_firmware_get_firmware_version(). > > > > > > Does it mean we need to support version_name? > > > I can add a version_name in dtb. > > > > > >> > > >> Please, pass *image_info[i] to efi_firmware_get_firmware_version. That > > >> will simplify the code. > > > > > > OK. > > > > > > Thank you for your review. > > > > > > Regards, > > > Masahisa Kojima > > > > > >> > > >> Best regards > > >> > > >> Heinrich > > >> > > >>> image_info[i].size = 0; > > >>> image_info[i].attributes_supported = > > >>> @@ -168,7 +224,7 @@ static efi_status_t efi_fill_image_desc_array( > > >>> image_info[0].attributes_setting |= > > >>> IMAGE_ATTRIBUTE_AUTHENTICATION_REQUIRED; > > >>> > > >>> - image_info[i].lowest_supported_image_version = 0; > > >>> + image_info[i].lowest_supported_image_version = lowest_supported_version; > > >>> image_info[i].last_attempt_version = 0; > > >>> image_info[i].last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS; > > >>> image_info[i].hardware_instance = 1; > > >>> @@ -290,7 +346,6 @@ efi_status_t EFIAPI efi_firmware_get_image_info( > > >>> descriptor_version, descriptor_count, > > >>> descriptor_size, package_version, > > >>> package_version_name); > > >>> - > > >>> return EFI_EXIT(ret); > > >>> } > > >>> > > >> > >