Re: [PATCH v7 03/10] arm_ffa: introduce Arm FF-A low-level driver

[Jens Wiklander <jens.wiklander@linaro.org>]

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 <abdellatif.elkhlifi@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.

Cheers,
Jens