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 5F406C4332F for ; Tue, 15 Nov 2022 10:33:04 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 904A184F7C; Tue, 15 Nov 2022 11:32:59 +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="HxOhxNpW"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 03D4884F11; Tue, 15 Nov 2022 11:32:56 +0100 (CET) Received: from mail-lj1-x22d.google.com (mail-lj1-x22d.google.com [IPv6:2a00:1450:4864:20::22d]) (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 852BD80A46 for ; Tue, 15 Nov 2022 11:32:52 +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=jens.wiklander@linaro.org Received: by mail-lj1-x22d.google.com with SMTP id c25so16934653ljr.8 for ; Tue, 15 Nov 2022 02:32:52 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:from:to :cc:subject:date:message-id:reply-to; bh=0MZvlQFMUfXLGkqXpmhaDOBlT+LiKGr2Mdn5Dq6V3cg=; b=HxOhxNpWYGQzoJiVb0hZArkkQEXfBMfKv8j608eFp+vgVcHFuzU/S2rP24Vck9q5x7 +CFN6TV7LEjcfFL2NjT3R5dIJ0Oy3JQ3uIjOVlqf4qMvewqp5kLreGe7kZut8NDU6GZ4 JTJiCymJgbmiEqcSC9zqEfVW7DawlrlZi8UL9STql6hQVVoYax3rYXmF1P73rnRFJJ1n PJOlBxGZWcaLqo6rsl9zHQSHKFcPqMPFpFf0eRLZ3RpUwLSIUXPooJgm7MNyU//Knk3e AY5HkSw5op/qriglTm2neAsN2gcrUiyjvloR58DUK5VIsolmSNhPYbWlBN/ePFQs+uV2 /4Rg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=0MZvlQFMUfXLGkqXpmhaDOBlT+LiKGr2Mdn5Dq6V3cg=; b=Avsyk8KpUjwg0lCGGL5nVr4MYxX6c11CDNahVrCPja2LOisqhdcvHyK/yEBuSKZMKU MLIjqIphQDtHYCkJY1I2b2uBM/HAgMzhnf52QRZtRuRASQY/LaJ8Ghhd8/KJf0WESD+i UU/En0I0ckRXy/oNYehMgLj/ihNIMJPvUPWL4MVXAW65HB4TVJ51EMHRb2I/MFp5tftf 309UewvoNVeSXAD/3uxjdbXMVIPBroEa2cP7uehGyAB0o+MKBehx4s/1SWGhDNobhNHb 2+j/MI0SMWOQaYNb4Ey4Kduewxn3LWmsFP+BJ45z2gUM9a8U1L28+3GwvM1KSdl0RobQ c66g== X-Gm-Message-State: ANoB5pkI9ic4rEJoRWAyIP7015O7pDUCGLxHgftCDrakAeLBshgUU4Rs jR6gts3Qt/dydRTGmal/yWWnvQ== X-Google-Smtp-Source: AA0mqf6hN0GqvIpgujVLLzEIzqvVKNllsCMDty5TWlNYvi7N4V7EKHJZAJ6lpGM5PipE8U8nlLAjeA== X-Received: by 2002:a2e:b705:0:b0:277:1c8f:7e8c with SMTP id j5-20020a2eb705000000b002771c8f7e8cmr6057154ljo.296.1668508371741; Tue, 15 Nov 2022 02:32:51 -0800 (PST) Received: from jade ([85.235.10.72]) by smtp.gmail.com with ESMTPSA id a3-20020a05651c030300b002772414817esm2446744ljp.1.2022.11.15.02.32.50 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 15 Nov 2022 02:32:51 -0800 (PST) Date: Tue, 15 Nov 2022 11:32:49 +0100 From: Jens Wiklander To: Abdellatif El Khlifi Cc: u-boot@lists.denx.de, nd@arm.com Subject: Re: [PATCH v7 03/10] arm_ffa: introduce Arm FF-A low-level driver Message-ID: References: <20221013103857.614-1-abdellatif.elkhlifi@arm.com> <20221107192055.21669-1-abdellatif.elkhlifi@arm.com> <20221107192055.21669-4-abdellatif.elkhlifi@arm.com> <20221111143611.GA7516@e121910.cambridge.arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20221111143611.GA7516@e121910.cambridge.arm.com> 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 On Fri, Nov 11, 2022 at 02:36:11PM +0000, Abdellatif El Khlifi wrote: > On Wed, Nov 09, 2022 at 12:51:26PM +0100, Jens Wiklander wrote: > > On Mon, Nov 07, 2022 at 07:20:48PM +0000, Abdellatif El Khlifi wrote: [snip] > > > +/** > > > + * ffa_msg_send_direct_req - FFA_MSG_SEND_DIRECT_{REQ,RESP} handler function > > > + * @dst_part_id: destination partition ID > > > + * @msg: pointer to the message data preallocated by the client (in/out) > > > + * @is_smc64: select 64-bit or 32-bit FF-A ABI > > > + * > > > + * This function implements FFA_MSG_SEND_DIRECT_{REQ,RESP} > > > + * FF-A functions. > > > + * > > > + * FFA_MSG_SEND_DIRECT_REQ is used to send the data to the secure partition. > > > + * The response from the secure partition is handled by reading the > > > + * FFA_MSG_SEND_DIRECT_RESP arguments. > > > + * > > > + * The maximum size of the data that can be exchanged is 40 bytes which is > > > + * sizeof(struct ffa_send_direct_data) as defined by the FF-A specification 1.0 > > > + * in the section relevant to FFA_MSG_SEND_DIRECT_{REQ,RESP} > > > + * > > > + * Return: > > > + * > > > + * 0 on success. Otherwise, failure > > > + */ > > > +static int ffa_msg_send_direct_req(u16 dst_part_id, struct ffa_send_direct_data *msg, bool is_smc64) > > > +{ > > > + ffa_value_t res = {0}; > > > + int ffa_errno; > > > + u64 req_mode, resp_mode; > > > + > > > + if (!ffa_priv_data || !ffa_priv_data->invoke_ffa_fn) > > > + return -EINVAL; > > > + > > > + /* No partition installed */ > > > + if (!ffa_priv_data->partitions.count || !ffa_priv_data->partitions.descs) > > > + return -ENODEV; > > > > This check isn't needed. What if the partition ID is known by other > > means? > > I'm happy to remove this check. I'd like to add a comment explaining > what the other means are. Could you please explain what are they ? In some systems perhaps you have well known partition ids reserved for certain services. > > > > > > + > > > + if (is_smc64) { > > > + req_mode = FFA_SMC_64(FFA_MSG_SEND_DIRECT_REQ); > > > + resp_mode = FFA_SMC_64(FFA_MSG_SEND_DIRECT_RESP); > > > + } else { > > > + req_mode = FFA_SMC_32(FFA_MSG_SEND_DIRECT_REQ); > > > + resp_mode = FFA_SMC_32(FFA_MSG_SEND_DIRECT_RESP); > > > + } > > > + > > > + ffa_priv_data->invoke_ffa_fn((ffa_value_t){ > > > + .a0 = req_mode, > > > + .a1 = PREP_SELF_ENDPOINT_ID(ffa_priv_data->id) | > > > + PREP_PART_ENDPOINT_ID(dst_part_id), > > > + .a2 = 0, > > > + .a3 = msg->data0, > > > + .a4 = msg->data1, > > > + .a5 = msg->data2, > > > + .a6 = msg->data3, > > > + .a7 = msg->data4, > > > + }, &res); > > > + > > > + while (res.a0 == FFA_SMC_32(FFA_INTERRUPT)) > > > + ffa_priv_data->invoke_ffa_fn((ffa_value_t){ > > > + .a0 = FFA_SMC_32(FFA_RUN), > > > + .a1 = res.a1, > > > + }, &res); > > > + > > > + if (res.a0 == FFA_SMC_32(FFA_SUCCESS)) { > > > + /* Message sent with no response */ > > > + return 0; > > > + } > > > + > > > + if (res.a0 == resp_mode) { > > > + /* > > > + * Message sent with response > > > + * extract the return data > > > + */ > > > + msg->data0 = res.a3; > > > + msg->data1 = res.a4; > > > + msg->data2 = res.a5; > > > + msg->data3 = res.a6; > > > + msg->data4 = res.a7; > > > + > > > + return 0; > > > + } > > > + > > > + ffa_errno = res.a2; > > > + return ffa_to_std_errno(ffa_errno); > > > +} > > > + [snip] > > > +/** > > > + * ffa_bus_prvdata_get - bus driver private data getter > > > + * > > > + * Return: > > > + * This function returns a pointer to the main private data structure > > > + */ > > > +struct ffa_prvdata **ffa_bus_prvdata_get(void) > > > > Why a pointer to a pointer, isn't "struct ffa_prvdata *" enough? > > Because ffa_priv_data is a pointer. ffa_bus_prvdata_get() returns an > address of a pointer so the returned type should be struct ffa_prvdata > ** > > Otherwise, a compiler warning: > > drivers/firmware/arm-ffa/core.c: In function ‘ffa_bus_prvdata_get’: > drivers/firmware/arm-ffa/core.c:1278:9: warning: return from incompatible pointer type [-Wincompatible-pointer-types] > return &ffa_priv_data; > ^~~~~~~~~~~~~~ Why not return ffa_priv_data instead? > > > > > > +{ > > > + return &ffa_priv_data; > > > +} [snip] > > +++ b/include/arm_ffa.h > > > @@ -0,0 +1,93 @@ > > > +/* SPDX-License-Identifier: GPL-2.0+ */ > > > +/* > > > + * (C) Copyright 2022 ARM Limited > > > + * Abdellatif El Khlifi > > > + */ > > > + > > > +#ifndef __ARM_FFA_H > > > +#define __ARM_FFA_H > > > + > > > +#include > > > + > > > +/* > > > + * This header is public. It can be used by clients to access > > > + * data structures and definitions they need > > > + */ > > > + > > > +/* > > > + * Macros for displaying logs > > > + */ > > > + > > > +#define ffa_info(fmt, ...) pr_info("[FFA] " fmt "\n", ##__VA_ARGS__) > > > +#define ffa_err(fmt, ...) pr_err("[FFA] " fmt "\n", ##__VA_ARGS__) > > > + > > > +/* > > > + * struct ffa_partition_info - Partition information descriptor > > > + * @id: Partition ID > > > + * @exec_ctxt: Execution context count > > > + * @properties: Partition properties > > > + * > > > + * Data structure containing information about partitions instantiated in the system > > > + * This structure is filled with the data queried by FFA_PARTITION_INFO_GET > > > + */ > > > +struct __packed ffa_partition_info { > > > + u16 id; > > > + u16 exec_ctxt; > > > +/* partition supports receipt of direct requests */ > > > +#define FFA_PARTITION_DIRECT_RECV BIT(0) > > > +/* partition can send direct requests. */ > > > +#define FFA_PARTITION_DIRECT_SEND BIT(1) > > > +/* partition can send and receive indirect messages. */ > > > +#define FFA_PARTITION_INDIRECT_MSG BIT(2) > > > + u32 properties; > > > +}; > > > > Perhaps this has been discussed before. Why is this packed? Is it to allow > > unaligned access or to be sure that there is not implicitly added padding? > > The Linux kernel does seem to need it. > > When not using __packed the compiler will add paddings. > ffa_partition_info structure is used for reading SP information > from the secure world. > > The issue arises when the non secure world and the secure world > have different architectures (Aarch64 vs Aarch32) or different > endianess. In these cases, the data will be corrupted. > > I think we need to use __packed for all the comms structures. > > I'm aware ffa_partition_info in the kernel is not packed. I'm happy > to remove __packed from here to align it with Linux. > > But please share your thoughts about the padding issues when > working with different architecures/endianess. __packed doesn't help with endianess, besides if I remember correctly these are always expected to be little endian. But I guess that could be a problem for another day. I believe the layout of these structs are designed in a way that a reasonable compiler wouldn't add padding on AArch32 or AArch64. Note that packed does more than just force generation of inefficient code on Arm (unaligned access), it also makes it invalid to take the address of a member inside such a struct. Cheers, Jens