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 D2B72C7EE29 for ; Mon, 29 May 2023 13:47:54 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id E48D88267E; Mon, 29 May 2023 15:47:49 +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="O0WPRGea"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 22B9884791; Mon, 29 May 2023 15:47:48 +0200 (CEST) Received: from mail-wm1-x330.google.com (mail-wm1-x330.google.com [IPv6:2a00:1450:4864:20::330]) (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 344AB8215F for ; Mon, 29 May 2023 15:47:45 +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=rui.silva@linaro.org Received: by mail-wm1-x330.google.com with SMTP id 5b1f17b1804b1-3f68fc6b479so34049465e9.2 for ; Mon, 29 May 2023 06:47:45 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1685368064; x=1687960064; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:from:to:cc:subject:date:message-id:reply-to; bh=W5Yu1zW/X0W+pgywr69eqwJOf/QOUQB6ka0aGg3FEQk=; b=O0WPRGea0lH3Mf4h8ZIqWvY178a4gcIfk1ypEuwh+m7cUlyoObNLfKoqYWQR7/Bu9C 5Ez+qHyXqHollQEJ9o8ezUUWoShBAL3QM+2LCrGTBKCKmpJdK/++L5avH8rxCNFxvyMY qZFqs2sDtKpPi1HIXIjEQ1uAoo4TO2pQ3UvvHwYDE4VizLnZGx4dkIw2ZCHxrcHT381x P0SVx4uBmUi8r68Tym8M7a8ULSDVmXhq4Y4Xi2PXedUNsrwJ3S2rcdBnP/qiPU0ZgVkY S/vlieLTgmP0WlxsUvJcKm7qFJmZcKYwHK+zb2NoRysdLlxIQDuNQTcUm4FUoNu0r6iM ticg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1685368064; x=1687960064; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=W5Yu1zW/X0W+pgywr69eqwJOf/QOUQB6ka0aGg3FEQk=; b=HuoyryIHi9UcZ4k6dMu9dUfjfqRnIu7qHt9o3MbdxuQe2QnN7cfHX1NiIdPUjZ8jog AvBUnjZ4dlAnF+lzTlyUe/azMcwHFke4DwQqCggv/Nqqe8Mb04za7KkRrj+0eLyFj+50 GAPV9Tspz6kfd5D9rvor4/4PAcReCr4IxqztbVRGspKGnf5xFCmsHJX0F3H9WloxvFjT eds0UTH0JbV7wnielpT7SydgJf5ohzaZKc/ps8WNp+/aMMEy7SA8dhRfmDAlTF9jmKXL niVjNAypZwW5oX5j3J0z4Wl2JtjK0Q8T5hANLXw3G/O9RjlYa65i6JXkovLilMaPu6aW pJlw== X-Gm-Message-State: AC+VfDzX1kvOk5kFufY32PgIHcZEsWZj+MS66g3BB3XtFWHONhHkQBwf HxUZHFjkOkvw5+xKOrxFcoJOpA== X-Google-Smtp-Source: ACHHUZ52zwDcoLkGqJfkugxy8Up8DFWVjgdUDx3XdvZGp2ohnbgA2Wt8KKGSenLW0G1fpX5jHO2EVw== X-Received: by 2002:a5d:6d4a:0:b0:30a:f05f:ce46 with SMTP id k10-20020a5d6d4a000000b0030af05fce46mr1150302wri.8.1685368064668; Mon, 29 May 2023 06:47:44 -0700 (PDT) Received: from localhost (a109-49-33-111.cpe.netcabo.pt. [109.49.33.111]) by smtp.gmail.com with ESMTPSA id e10-20020a5d530a000000b002f6176cc6desm13975422wrv.110.2023.05.29.06.47.44 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 29 May 2023 06:47:44 -0700 (PDT) From: Rui Miguel Silva To: Ilias Apalodimas Cc: Heinrich Schuchardt , u-boot@lists.denx.de, Simon Glass , Tom Rini Subject: Re: [PATCH 1/6] fwu_metadata: make sure structures are packed In-Reply-To: References: <20230502131200.2551513-1-rui.silva@linaro.org> <20230502131200.2551513-2-rui.silva@linaro.org> Date: Mon, 29 May 2023 14:47:43 +0100 Message-ID: MIME-Version: 1.0 Content-Type: text/plain 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 Ilias Apalodimas writes: > Hi, > > On Mon, 29 May 2023 at 16:12, Rui Miguel Silva wrote: >> >> Hi Heinrich, >> thanks for the review, >> Heinrich Schuchardt writes: >> >> > On 5/3/23 10:06, Ilias Apalodimas wrote: >> >> On Tue, 2 May 2023 at 16:12, Rui Miguel Silva wrote: >> >> >> >> Hi Rui, >> >>> >> >>> The fwu metadata in the metadata partitions >> >>> should/are packed to guarantee that the info is >> >>> correct in all platforms. Also the size of them >> >>> are used to calculate the crc32 and that is important >> >>> to get it right. >> >>> >> >>> Signed-off-by: Rui Miguel Silva >> >>> --- >> >>> include/fwu_mdata.h | 6 +++--- >> >>> 1 file changed, 3 insertions(+), 3 deletions(-) >> >>> >> >>> diff --git a/include/fwu_mdata.h b/include/fwu_mdata.h >> >>> index 8fda4f4ac225..c61221a91735 100644 >> >>> --- a/include/fwu_mdata.h >> >>> +++ b/include/fwu_mdata.h >> >>> @@ -22,7 +22,7 @@ struct fwu_image_bank_info { >> >>> efi_guid_t image_uuid; >> >>> uint32_t accepted; >> >>> uint32_t reserved; >> >>> -}; >> >>> +} __packed; >> > >> > This structure is naturally packed. Why should adding a __packed >> > attribute make a difference? >> >> Agree. >> >> > >> >>> >> >>> /** >> >>> * struct fwu_image_entry - information for a particular type of image >> >>> @@ -38,7 +38,7 @@ struct fwu_image_entry { >> >>> efi_guid_t image_type_uuid; >> >>> efi_guid_t location_uuid; >> >>> struct fwu_image_bank_info img_bank_info[CONFIG_FWU_NUM_BANKS]; >> >>> -}; >> >>> +} __packed; >> > >> > ditto >> >> Agree. >> >> > >> >>> >> >>> /** >> >>> * struct fwu_mdata - FWU metadata structure for multi-bank updates >> >>> @@ -62,6 +62,6 @@ struct fwu_mdata { >> >>> uint32_t previous_active_index; >> >>> >> >>> struct fwu_image_entry img_entry[CONFIG_FWU_NUM_IMAGES_PER_BANK]; >> >>> -}; >> >>> +} __packed; >> > >> > Depending on the alignment of efi_guid_t __packed might make a >> > difference here. But as efi_guid_t is defined as 4 aligned I can't see >> > an impact here either. >> >> but efi_guid_t is defined as 8 aligned, right? > > No, we have it as 4byte aligned, Sorry, was looking at an relative old branch. (only with this: 1dd705cf99 efi: use 32-bit alignment for efi_guid_t in v2023.04 it changed to 4 aligned) and, please note, that there is a definition of efi_guid_t as aligned 8 in eficapsule tool: https://elixir.bootlin.com/u-boot/latest/source/tools/eficapsule.h#L27 > >> Even though, I think that this type of data written to disk/flash >> without guaranteeing (trusting natural picketing) packed and endianness >> is never a good idea. > > I think we should overall define these as packed regardless. The spec > might change at some point and add a few fields ane even though it > doesn't explicitly requires packing it only mentions offsets. This is > going to make a difference in code generation, but it's in a path > noone cares about speed. Agree. Thanks for the feedback. Cheers, Rui > > Thanks > /Ilias > >> >> Cheers, >> Rui >> >> > >> > Best regards >> > >> > Heinrich >> > >> >>> >> >>> #endif /* _FWU_MDATA_H_ */ >> >>> -- >> >>> 2.40.0 >> >>> >> >> >> >> Yep, that's a good catch. The spec defines 'just' the offsets and not >> >> the C representation, so those should be indeed packed. >> >> >> >> Reviewed-by: Ilias Apalodimas