On Tue, Nov 15, 2022 at 11:32:49AM +0100, Jens Wiklander wrote:
> 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.
I double checked with Achin about this.
FFA_PARTITION_INFO_GET provides the partitions information for all
partitions including the reserved ones. The driver will cache the
information of all the partitions. Not finding any partition cached
before a direct request means: there is no partition in the system.
I think we need to keep the check.
>
> >
> > >
> > > > +
> > > > + 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?
Thanks, addressed in v8 patchset.
>
> >
> > >
> > > > +{
> > > > + 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 <abdellatif.elkhl...@arm.com>
> > > > + */
> > > > +
> > > > +#ifndef __ARM_FFA_H
> > > > +#define __ARM_FFA_H
> > > > +
> > > > +#include <linux/printk.h>
> > > > +
> > > > +/*
> > > > + * 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.
Packing removed from ffa_partition_info and ffa_send_direct_data structures
in v8 patchset.
>
> Cheers,
> Jens
