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 C2CECC433F5 for ; Thu, 17 Feb 2022 08:22:57 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 03BE383A64; Thu, 17 Feb 2022 09:22:55 +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="cIrfNZrU"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id BE05D83A65; Thu, 17 Feb 2022 09:22:52 +0100 (CET) Received: from mail-wm1-x335.google.com (mail-wm1-x335.google.com [IPv6:2a00:1450:4864:20::335]) (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 797A0837B6 for ; Thu, 17 Feb 2022 09:22:48 +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=ilias.apalodimas@linaro.org Received: by mail-wm1-x335.google.com with SMTP id d14-20020a05600c34ce00b0037bf4d14dc7so3386180wmq.3 for ; Thu, 17 Feb 2022 00:22:48 -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:references:mime-version :content-disposition:in-reply-to; bh=Vex72jksq/eNSMDInjYQs+/Zf61zmsj900rmya6sTtk=; b=cIrfNZrUVVRL/NGzAJoykKlkv1NAxlvGvx/ciQNXFMqw2kkZK+ZJB2ODG0cLuS5CwQ BPdQagvPPZsJ/P1xjkVmDaLGMUC2X6G4NRhbHrbw2hXwrFMI2QqYuR/L0YYeRmC7m/RJ 3V3kKtafeDwfWXnPO/9XqeGhbNvE1ge+ZiPO9IO96ZmESuhXOTkyhlNoFKAPRkPo2LGQ BvnnaPmBgt2g7So/Ab2y7JB+y/3DysyFuk/tWLVUoXnXr6zyIXuL4d2PsHfuGyYEHWCt Pl3KJCKzx+kpkjm4kbPOVWDuPoKbMTOsYfE+2uSBiT/UI5ytEthmjRkAXjSW5348cReJ Ez1g== 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:references :mime-version:content-disposition:in-reply-to; bh=Vex72jksq/eNSMDInjYQs+/Zf61zmsj900rmya6sTtk=; b=WVT4pb77n3Ii7i0dmxvqdbpq5P5torEVYVZughGZPtO4yBqpWWF4KKbe3e8rLzh9RX viWi+hmvsmm8DaZXcF0vsDbsaG/KcvkZZitQ0FS1p+W25oBAHMip7JUIrlOvL/RU+sXs OsKtoWQoNq5lXVBhliNpsN/Mm9azXsg5HvbMWAB4mcWnsUYL6DXGCmtW/XEPJIgbQcgJ BoZhsxirGNRPG9DuvAYfXLfCYwODf1A/c5lA5afju3qCxfyzjCvQIAQki85LhRc4iLpj R+I9nFJkdEhrB8bOqIRVHM874T4AyUx0L4SFqusOCVT0mjqJS3df7msQPrOQtfhAhIHk xO1A== X-Gm-Message-State: AOAM530h3hhuy0OCJaj/3UVpItYcthygtz7qtO8dFkrwIr7QabFyfnCm T41zToq3BnbCJmuZf7Qr6o4KuQ== X-Google-Smtp-Source: ABdhPJykH0tRlJPx7mPGVZWdmgyk233r81BlvkFLyroxkv29K3ETA/2YidqgcvaizvvAk+7bQBS7zw== X-Received: by 2002:a7b:c041:0:b0:34d:5ca8:54d1 with SMTP id u1-20020a7bc041000000b0034d5ca854d1mr1693262wmc.94.1645086167902; Thu, 17 Feb 2022 00:22:47 -0800 (PST) Received: from hades (athedsl-4461669.home.otenet.gr. [94.71.4.85]) by smtp.gmail.com with ESMTPSA id h7sm19048594wru.41.2022.02.17.00.22.46 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 17 Feb 2022 00:22:47 -0800 (PST) Date: Thu, 17 Feb 2022 10:22:44 +0200 From: Ilias Apalodimas To: Sughosh Ganu Cc: AKASHI Takahiro , u-boot@lists.denx.de, Heinrich Schuchardt , Masami Hiramatsu , Patrick Delaunay , Patrice Chotard , Alexander Graf , Simon Glass , Bin Meng , Jose Marinho , Grant Likely , Tom Rini , Etienne Carriere Subject: Re: [PATCH v4 05/11] EFI: FMP: Add provision to update image's ImageTypeId in image descriptor Message-ID: References: <20220210024828.GB12412@laputa> <20220210075808.GI12412@laputa> <20220214032434.GG39639@laputa> <20220215015109.GB38476@laputa> 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 > > > > > [...] > > > > > 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. 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? > > 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