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 04E02C433F5 for ; Tue, 8 Mar 2022 13:14:10 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 32AF880F6F; Tue, 8 Mar 2022 14:14:07 +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=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="mX8q+Xs9"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id F40798002A; Tue, 8 Mar 2022 14:14:01 +0100 (CET) Received: from mail-pj1-x1030.google.com (mail-pj1-x1030.google.com [IPv6:2607:f8b0:4864:20::1030]) (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 BD91380F6F for ; Tue, 8 Mar 2022 14:13:54 +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=takahiro.akashi@linaro.org Received: by mail-pj1-x1030.google.com with SMTP id 15-20020a17090a098f00b001bef0376d5cso2354388pjo.5 for ; Tue, 08 Mar 2022 05:13:54 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:mail-followup-to:references :mime-version:content-disposition:in-reply-to; bh=e7Vu+84em2vDD9Kdn8JqvAQQ9MYgJcdUfnnoYfuR64Q=; b=mX8q+Xs9es3iW+OcFrzoERUWX++dxdsud6yyeunuQT7Z+mC1ZLvGYYwM3isFh4IveF f/1YajJUhLAVfND51iXUTKqfZ1dHch0ACLAYpSN+MjILIGYeGMYLDU5OaIJSCY+tALvJ InDcTLHi72OOS19xorzK7wogtRUyGz3ThUoOalBgmMEUss6NT167GG92AfuZZgHQ8jqV 5jxLBvBCG+2/t6QNoULIwlSjNVnK4DAc7L4mktPAzJiv1giwNO9LCSAUYP1LUDsJmXfF ODePUO4wFYSJyTvljv4HI6cRCDdXvD5WVHoWiY1SKLhsukMejY2nESgqHozvCkMU5QZf Ri0g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id :mail-followup-to:references:mime-version:content-disposition :in-reply-to; bh=e7Vu+84em2vDD9Kdn8JqvAQQ9MYgJcdUfnnoYfuR64Q=; b=68VH+9sY7SFQuI3d+L/ECzVitiFWi/DLLHIlb3bpz+tBN0Ujsj5BSZS6EPw5fTKo2/ d/3VgApWNpO8nyH2ZO+OPZsQqkOuvEx+U1iYtP04TRDsnwf6QbUcBlDqlHuUUpD/QQtj LzSzM5zsp+tMzAyyv270JX9yq4TaD3+EQSiRtBrn1kZsShwIiTN5Q3An4KeV55rhM8ao oo2iL6FsNlpF3iFdVWnsOzq6lTSBektFDna+myC5fUUrRsspJAGdb0dADgNIMzY1RcqD TXSNfqooiRB4hm0ltTU/9ygvRMSdZDG4h+7uK4WiVQe8j5Af9mXMvedfD+M52nUYoz45 EqDg== X-Gm-Message-State: AOAM53336LnRAp4yS+L8rNCq87yjnSSZeJexBVC9RKEwGmlxuIf4M4gV AzvJKnWjdLanPvfFpZ6hkxanSw== X-Google-Smtp-Source: ABdhPJzDW2znli1zTLLsNjuVHAOC9QnBjGs3/xNcd9LsOHt7eZqks5+WbGStAxKRRKN7KeZOzM9+ug== X-Received: by 2002:a17:90b:1801:b0:1bf:2854:dd46 with SMTP id lw1-20020a17090b180100b001bf2854dd46mr4542830pjb.230.1646745233005; Tue, 08 Mar 2022 05:13:53 -0800 (PST) Received: from laputa ([2400:4050:c3e1:100:b94a:f47a:3d51:185c]) by smtp.gmail.com with ESMTPSA id q8-20020aa78428000000b004f7281cda23sm3350037pfn.201.2022.03.08.05.13.48 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 08 Mar 2022 05:13:52 -0800 (PST) Date: Tue, 8 Mar 2022 22:13:47 +0900 From: AKASHI Takahiro To: Sughosh Ganu Cc: Heinrich Schuchardt , u-boot@lists.denx.de, Masami Hiramatsu , Patrick Delaunay , Patrice Chotard , Alexander Graf , Simon Glass , Bin Meng , Jose Marinho , Grant Likely , Tom Rini , Etienne Carriere , Ilias Apalodimas Subject: Re: [PATCH v4 05/11] EFI: FMP: Add provision to update image's ImageTypeId in image descriptor Message-ID: <20220308131347.GB228584@laputa> Mail-Followup-To: AKASHI Takahiro , Sughosh Ganu , Heinrich Schuchardt , u-boot@lists.denx.de, Masami Hiramatsu , Patrick Delaunay , Patrice Chotard , Alexander Graf , Simon Glass , Bin Meng , Jose Marinho , Grant Likely , Tom Rini , Etienne Carriere , Ilias Apalodimas References: <20220214032434.GG39639@laputa> <20220215015109.GB38476@laputa> <48e5dddb-f8c9-f3e6-db99-ed11578498bc@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.5 at phobos.denx.de X-Virus-Status: Clean On Sat, Feb 26, 2022 at 03:24:10PM +0530, Sughosh Ganu wrote: > hello Heinrich, > > On Sat, 26 Feb 2022 at 12:24, Heinrich Schuchardt wrote: > > > > On 2/17/22 11:10, Sughosh Ganu wrote: > > > On Thu, 17 Feb 2022 at 13:52, Ilias Apalodimas > > > wrote: > > >> > > >>>>>>> > > >> > > >> [...] > > >> > > >>>>>>> Yes, we can use --index when we know the index value corresponding to > > >>>>>>> the firmware image that we need to update. But like I mentioned in my > > >>>>>>> earlier reply, for A/B updates, we do not know what the index value is > > >>>>>>> going to be. That is going to be determined at runtime. > > >>>>>> > > >>>>>> I don't think so. See below for alternative approach. > > >>>>>> > > >>>>>>> Also, the point I was making is that we can have a capsule which is > > >>>>>>> consumed by an FMP protocol which has more than one image, and those > > >>>>>>> images have different ImageTypeId/UpdateImageTypeId. > > >>>>>> > > >>>>>> Yes, but it is a design choice in my first implementation. > > >>>>>> I didn't think that we need to "have a capsule which is consumed > > >>>>>> by an FMP protocol which has more than one image" as long as we > > >>>>>> use DFU framework (and FIT as standard format of aggregation on U-Boot). > > >>>>> > > >>>>> But this design can be extended without any hassle, and more > > >>>>> importantly without any regression, no? What kind of a problem does it > > >>>>> create if the FMP can handle more than one image type. > > >>>>> > > >>>>> Even as per the UEFI spec, we have the EFI_FIRMWARE_MANAGEMENT_CAPSULE > > >>>>> header for all images to be managed by the FMP protocol which has > > >>>>> multiple images with different UpdateImageTypeId. > > >>>>> > > >>>>>> > > >>>>>>>> > > >>>>>>>>> Please check the > > >>>>>>>>> GenerateCapsule script in EDK2. In case of a multi payload based > > >>>>>>>>> capsule, individual parameters like the UpdateImageTypeId are passed > > >>>>>>>>> through the json file, where each of the UpdateImageTypeId has a > > >>>>>>>>> different value per payload. > > >>>>>>>>> > > >>>>>>>>>> > > >>>>>>>>>>>> 2) Each firmware object handled by a given FMP driver can further be > > >>>>>>>>>>>> identified by ImageIndex. > > >>>>>>>>>>>> > > >>>>>>>>>>>> My implementation of efi_fmp_find() does (1) and Raw FMP driver does > > >>>>>>>>>>>> (2) in efi_firmware_raw_set_image() which takes "image_index" as > > >>>>>>>>>>>> a parameter. > > >>>>>>>>>>>> > > >>>>>>>>>>>> Using ImageTypeId as an identifier is simply wrong in my opinion and > > >>>>>>>>>>>> doesn't meet the UEFI specification. > > >>>>>>>>>>> > > >>>>>>>>>>> So, as per what you are stating, all payloads under a given > > >>>>>>>>>>> EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER should have the same > > >>>>>>>>>>> ImageTypeId, either EFI_FIRMWARE_IMAGE_TYPE_UBOOT_FIT_GUID or > > >>>>>>>>>>> EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID. Same applies for all images in > > >>>>>>>>>>> the EFI_FIRMWARE_IMAGE_DESCRIPTOR. Because, without one of the two > > >>>>>>>>>>> values, > the check in efi_fmp_find to compare the UpdateImageTypeId > > >>>>>>>>>>> with the ImageTypeId retrieved from the image descriptor would simply > > >>>>>>>>>>> fail. > > >>>>>>>>>> > > >>>>>>>>>> I don't follow your point. > > >>>>>>>>>> Please elaborate a bit more. > > >>>>>>>>> > > >>>>>>>>> The current implementation of GetImageInfo, passes either of > > >>>>>>>>> EFI_FIRMWARE_IMAGE_TYPE_UBOOT_FIT_GUID or > > >>>>>>>>> EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID for all the images of the image > > >>>>>>>>> descriptor array. So, in case the capsule is generated with a '--guid' > > >>>>>>>>> value which is different from these two values, the check in > > >>>>>>>>> efi_fmp_find on line 204 will fail. > > >>>>>>>> > > >>>>>>>> That is an expected behavior, isn't it? > > >>>>>>> > > >>>>>>> Yes it is. Do not contest that. > > >>>>>>> > > >>>>>>>> If you want to use a different FMP driver (with another GUID), > > >>>>>>>> you naturally need to add your own FMP driver. > > >>>>>>> > > >>>>>>> This is where I differ. We can use the same FMP protocol instance for > > >>>>>>> any type of ImageTypeId. I do not see why we need to define a > > >>>>>>> different FMP protocol instance for a GUID value other than what has > > >>>>>>> been defined for u-boot raw and u-boot FIT GUIDs. > > >>>>>> > > >>>>>> I do understand part of your concern a bit. > > >>>>>> I thought of using the same ImageType GUID, say IMAGE_TYPE_DFU_GUID, at first > > >>>>>> but we needed different GUIDs here simply because we need to determine > > >>>>>> the format of payload, FIT format or raw binary. > > >>>>>> > > >>>>>>> The platform can give us the image descriptor array, and with that, > > >>>>>>> the same FMP instance can be used for any type of image(ImageTypeId). > > >>>>>> > > >>>>>> "any type of image"? Really? > > >>>>> > > >>>>> The raw FMP instance can certainly handle any type of binary payloads > > >>>>> right. There is no restriction on what type of payload it is as long > > >>>>> as it is all going as a single entity to a given dfu partition. > > >>>>> > > >>>>>>> > > >>>>>>>> > > >>>>>>>> > > >>>>>>>>> This means that unless the --guid > > >>>>>>>>> value passed to the capsule generation is either of u-boot FIT or > > >>>>>>>>> u-boot raw, the current FMP protocol for raw devices cannot be used. > > >>>>>>>>> Why do we need that restriction. It should be possible to use the raw > > >>>>>>>>> FMP protocol for any other type of image types as well. > > >>>>>>>>> > > >>>>>>>>> > > >>>>>>>>>> > > >>>>>>>>>>> I think this interpretation of the UEFI spec is incorrect, since the > > >>>>>>>>>>> spec states that the ImageTypeId and the UpdateImageTypeId are fields > > >>>>>>>>>>> used to identify the firmware component targeted for the update. If > > >>>>>>>>>>> all values in the image descriptor array and the UpdateImageTypeId are > > >>>>>>>>>>> the same, why have this field in the first place for individual > > >>>>>>>>>>> images. > > >>>>>>>>>> > > >>>>>>>>>> As I said, ImageIndex is for that purpose. > > >>>>>>>>> > > >>>>>>>>> Yes, that is one possible way in the scenario where the ImageIndex is > > >>>>>>>>> determined at the capsule generation time. But, for the A/B update > > >>>>>>>>> scenario, we do not know the ImageIndex at build time > > >>>>>>>> > > >>>>>>>> "Build time" of what? > > >>>>>>> > > >>>>>>> Of the capsule. > > >>>>>>> > > >>>>>>>> I think that users should know how "dfu_alt_info" is defined > > >>>>>>>> (in other words, where the firmware be located on the target system) > > >>>>>>>> when capsule files are created. > > >>>>>>> > > >>>>>>> That is true for a non A/B scenario. And that is how it works in the > > >>>>>>> non A/B updates case. But for A/B updates, since the determination of > > >>>>>>> the "location" where the firmware image has to be written will be done > > >>>>>>> only at runtime, we cannot use the --index to differentiate. > > >>>>>> > > >>>>>> Yes, we can :) > > >>>>> > > >>>>> You know what I mean -- if we could use the same logic, I would not > > >>>>> have added all that code :) > > >>>>> > > >>>>>> > > >>>>>> First of all, my essential assumption in either FIT or RAW FMP driver > > >>>>>> is that U-Boot has (somehow conceptually) single firmware blob represented > > >>>>>> by DFU or dfu_alt_info. As I said, each object or location in > > >>>>>> dfu_alt_info can be further identified by index or "UpdateImageIndex". > > >>>>>> > > >>>>>> Let's assume that we have two locations of firmware, fw1 and fw2, and > > >>>>>> that we have two bank A and B. > > >>>>>> Then we will define dfu_alt_info as follows: > > >>>>>> ;;;; > > >>>>>> |<--- 1st set --->|<--- 2nd set --->| > > >>>>>> > > >>>>>> When you want to update bank A, we can use the first set of dfu_alt_info, > > >>>>>> and use the second set of dfu_alt_info for bank B. > > >>>>>> At runtime, you should know which bank you're working on, and therefore > > >>>>>> you should know the exact physical location from dfu_alt_info. > > >>>>>> > > >>>>>> Please note that you don't have to change the syntax of dfu_alt_info > > >>>>>> at all. Simply offset the location with 0 for bank A and with 2 for bank B. > > >>>> > > >>>> I'll try digging a bit more, but I think the current approach is not > > >>>> working as it was intended wrt to the EFI spec. My reading of the spec > > >>>> and specifically section 23.3.2 is that a Capsule consists of an > > >>>> EFI capsule header and a payload. The payload now has an > > >>>> EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER which in it's turn can host multiple > > >>>> firmware images of different type described in EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER. > > >>>> > > >>>> An FMP implementation should read the UpdateImageTypeId's used to identify > > >>>> the image you are updating and from that derive the UpdateImageIndex > > >>>> which SetImage will use. That would give you the ability to update the > > >>>> all the firmware components with a single capsule. > > >>>> > > >>>> Sughosh what about the ESRT table generation? If you use different UpdateImageTypeId > > >>>> those should be reflected on the ESRT tables from the OS > > >>> > > >>> That would depend on the values populated in the > > >>> EFI_FIRMWARE_IMAGE_DESCRIPTOR array by the GetImageInfo function. The > > >>> image descriptor structure has an ImageTypeId field. The value of > > >>> ImageTypeId is what will be reflected in the ESRT table. > > >>> > > >>> In the current implementation, all the images in the ESRT table will > > >>> show the same ImageTypeId value, either > > >>> EFI_FIRMWARE_IMAGE_TYPE_UBOOT_FIT_GUID or > > >>> EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID. > > > > This is wrong. According to the the UEFI 2.9 specification the > > UpdateImageTypeId is used to "identify (the) device firmware targeted by > > this update". It does not identify the format in which the firmware is > > delivered. > > > > So this needs to be fixed in the next revision of this patch series. > > This patch series is actually adding that platform function which > populates the image descriptor array with the image GUID's -- patch 6 > of this series[1] actually does that for the ST DK2 platform. This > discussion was because Takahiro wanted to use the same image > GUID(u-boot raw/FIT) for all the images, and use the image index for > identifying where the image is to be written. The discussion depends on what the *firmware* means. With my FMP drivers (either FIT or raw), I intended a *set of firmware* managed with a *single* "dfu_alt_info", which may include various *images* for different target *components* as the original DFU framework does. -Takahiro Akashi > I guess with what you are stating, along with Ilias's opinion on this, > I will send the next version with the same approach, i.e. using a > platform function to populate the image GUIDs in the firmware image > descriptor array. With this, each firmware image will have a different > GUID which can be used to identify the image. > > -sughosh > > [1] - https://lists.denx.de/pipermail/u-boot/2022-February/474434.html > > > > > For each firmware part that can be updated provide a unique GUID. > > > > Best regards > > > > Heinrich > > > > >> > > >> Yea I was mostly asking wrt to A/B updates. Would the correct UUID be > > >> shown in the ESRT instead of EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID? > > > > > > Yes, the platform would have to define the fill_image_type_guid_array > > > function which would populate the ImageTypeId values in the > > > EFI_FIRMWARE_IMAGE_DESCRIPTOR array, and the image GUID values would > > > then show up as part of the ESRT table. > > > > > > As part of this patchset, I have added this function for the STM32MP1 DK2 board. > > > > > > -sughosh > > > > > >> > > >>> > > >>> The UpdateImageTypeId value from the capsule is used to compare with > > >>> the ImageTypeId values returned by the GetImageInfo function to check > > >>> if the given FMP protocol can be used for the update. > > >>> > > >>> -sughosh > > >>> > > >> [...] > > >> > > >> > > >> Regards > > >> /Ilias > >