2022-08-30 10:20:45

by Sudeep Holla

[permalink] [raw]
Subject: [PATCH 0/9] firmware: arm_ffa: Refactoring and initial/minor v1.1 update

Hi All,

This series is just some refactoring in preparation to add FF-A v1.1 support.
It doesn't have any memory layout or notification changes supported in v1.1
yet.

Regards,
Sudeep

Sudeep Holla (9):
firmware: arm_ffa: Add pointer to the ffa_dev_ops in struct ffa_dev
tee: optee: Use ffa_dev->ops directly
firmware: arm_ffa: Remove ffa_dev_ops_get()
firmware: arm_ffa: Add support for querying FF-A features
firmware: arm_ffa: Use FFA_FEATURES to detect if native versions are supported
firmware: arm_ffa: Make memory apis ffa_device independent
firmware: arm_ffa: Add v1.1 get_partition_info support
tee: optee: Drop ffa_ops in optee_ffa structure
firmware: arm_ffa: Split up ffa_dev_ops into info, message and memory operations

drivers/firmware/arm_ffa/bus.c | 4 +-
drivers/firmware/arm_ffa/driver.c | 111 ++++++++++++++++++++++--------
drivers/tee/optee/ffa_abi.c | 40 +++++------
drivers/tee/optee/optee_private.h | 1 -
include/linux/arm_ffa.h | 34 +++++----
5 files changed, 127 insertions(+), 63 deletions(-)

--
2.37.2


2022-08-30 10:21:27

by Sudeep Holla

[permalink] [raw]
Subject: [PATCH 2/9] tee: optee: Use ffa_dev->ops directly

Now that the ffa_device structure holds the pointer to ffa_dev_ops,
there is no need to obtain the same through ffa_dev_ops_get().

Just use the ffa_dev->ops directly.

Signed-off-by: Sudeep Holla <[email protected]>
---
drivers/tee/optee/ffa_abi.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/tee/optee/ffa_abi.c b/drivers/tee/optee/ffa_abi.c
index 7ab31740cff8..4c3b5d0008dd 100644
--- a/drivers/tee/optee/ffa_abi.c
+++ b/drivers/tee/optee/ffa_abi.c
@@ -793,11 +793,7 @@ static int optee_ffa_probe(struct ffa_device *ffa_dev)
u32 sec_caps;
int rc;

- ffa_ops = ffa_dev_ops_get(ffa_dev);
- if (!ffa_ops) {
- pr_warn("failed \"method\" init: ffa\n");
- return -ENOENT;
- }
+ ffa_ops = ffa_dev->ops;

if (!optee_ffa_api_is_compatbile(ffa_dev, ffa_ops))
return -EINVAL;
--
2.37.2

2022-08-30 10:21:31

by Sudeep Holla

[permalink] [raw]
Subject: [PATCH 8/9] tee: optee: Drop ffa_ops in optee_ffa structure

Since the ffa_device itself carries ffa_dev_ops now, there is no need
to keep a copy in optee_ffa structure.

Drop ffa_ops in the optee_ffa structure as it is not needed anymore.

Signed-off-by: Sudeep Holla <[email protected]>
---
drivers/tee/optee/ffa_abi.c | 9 ++++-----
drivers/tee/optee/optee_private.h | 1 -
2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/tee/optee/ffa_abi.c b/drivers/tee/optee/ffa_abi.c
index 7ec0a2f9a63b..7257b42d0545 100644
--- a/drivers/tee/optee/ffa_abi.c
+++ b/drivers/tee/optee/ffa_abi.c
@@ -271,8 +271,8 @@ static int optee_ffa_shm_register(struct tee_context *ctx, struct tee_shm *shm,
unsigned long start)
{
struct optee *optee = tee_get_drvdata(ctx->teedev);
- const struct ffa_dev_ops *ffa_ops = optee->ffa.ffa_ops;
struct ffa_device *ffa_dev = optee->ffa.ffa_dev;
+ const struct ffa_dev_ops *ffa_ops = ffa_dev->ops;
struct ffa_mem_region_attributes mem_attr = {
.receiver = ffa_dev->vm_id,
.attrs = FFA_MEM_RW,
@@ -314,8 +314,8 @@ static int optee_ffa_shm_unregister(struct tee_context *ctx,
struct tee_shm *shm)
{
struct optee *optee = tee_get_drvdata(ctx->teedev);
- const struct ffa_dev_ops *ffa_ops = optee->ffa.ffa_ops;
struct ffa_device *ffa_dev = optee->ffa.ffa_dev;
+ const struct ffa_dev_ops *ffa_ops = ffa_dev->ops;
u64 global_handle = shm->sec_world_id;
struct ffa_send_direct_data data = {
.data0 = OPTEE_FFA_UNREGISTER_SHM,
@@ -342,7 +342,7 @@ static int optee_ffa_shm_unregister_supp(struct tee_context *ctx,
struct tee_shm *shm)
{
struct optee *optee = tee_get_drvdata(ctx->teedev);
- const struct ffa_dev_ops *ffa_ops = optee->ffa.ffa_ops;
+ const struct ffa_dev_ops *ffa_ops = optee->ffa.ffa_dev->ops;
u64 global_handle = shm->sec_world_id;
int rc;

@@ -529,8 +529,8 @@ static int optee_ffa_yielding_call(struct tee_context *ctx,
struct optee_msg_arg *rpc_arg)
{
struct optee *optee = tee_get_drvdata(ctx->teedev);
- const struct ffa_dev_ops *ffa_ops = optee->ffa.ffa_ops;
struct ffa_device *ffa_dev = optee->ffa.ffa_dev;
+ const struct ffa_dev_ops *ffa_ops = ffa_dev->ops;
struct optee_call_waiter w;
u32 cmd = data->data0;
u32 w4 = data->data1;
@@ -817,7 +817,6 @@ static int optee_ffa_probe(struct ffa_device *ffa_dev)

optee->ops = &optee_ffa_ops;
optee->ffa.ffa_dev = ffa_dev;
- optee->ffa.ffa_ops = ffa_ops;
optee->rpc_param_count = rpc_param_count;

teedev = tee_device_alloc(&optee_ffa_clnt_desc, NULL, optee->pool,
diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
index a33d98d17cfd..04ae58892608 100644
--- a/drivers/tee/optee/optee_private.h
+++ b/drivers/tee/optee/optee_private.h
@@ -111,7 +111,6 @@ struct optee_smc {
*/
struct optee_ffa {
struct ffa_device *ffa_dev;
- const struct ffa_dev_ops *ffa_ops;
/* Serializes access to @global_ids */
struct mutex mutex;
struct rhashtable global_ids;
--
2.37.2

2022-08-30 10:21:31

by Sudeep Holla

[permalink] [raw]
Subject: [PATCH 7/9] firmware: arm_ffa: Add v1.1 get_partition_info support

FF-A v1.1 adds support to discovery the UUIDs of the partitions that was
missing in v1.0 and which the driver workarounds by using UUIDs supplied
by the ffa_drivers.

Add the v1.1 get_partition_info support and disable the workaround if
the detected FF-A version is greater than v1.0.

Signed-off-by: Sudeep Holla <[email protected]>
---
drivers/firmware/arm_ffa/driver.c | 39 ++++++++++++++++++++++++-------
include/linux/arm_ffa.h | 1 +
2 files changed, 32 insertions(+), 8 deletions(-)

diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
index 5c8484b05c50..6822241168d6 100644
--- a/drivers/firmware/arm_ffa/driver.c
+++ b/drivers/firmware/arm_ffa/driver.c
@@ -264,18 +264,24 @@ static int ffa_rxtx_unmap(u16 vm_id)
return 0;
}

+#define PARTITION_INFO_GET_RETURN_COUNT_ONLY BIT(0)
+
/* buffer must be sizeof(struct ffa_partition_info) * num_partitions */
static int
__ffa_partition_info_get(u32 uuid0, u32 uuid1, u32 uuid2, u32 uuid3,
struct ffa_partition_info *buffer, int num_partitions)
{
- int count;
+ int idx, count, flags = 0, size;
ffa_value_t partition_info;

+ if (!buffer || !num_partitions) /* Just get the count for now */
+ flags = PARTITION_INFO_GET_RETURN_COUNT_ONLY;
+
mutex_lock(&drv_info->rx_lock);
invoke_ffa_fn((ffa_value_t){
.a0 = FFA_PARTITION_INFO_GET,
.a1 = uuid0, .a2 = uuid1, .a3 = uuid2, .a4 = uuid3,
+ .a5 = flags,
}, &partition_info);

if (partition_info.a0 == FFA_ERROR) {
@@ -285,8 +291,15 @@ __ffa_partition_info_get(u32 uuid0, u32 uuid1, u32 uuid2, u32 uuid3,

count = partition_info.a2;

+ if (drv_info->version > FFA_VERSION_1_0)
+ size = partition_info.a3;
+ else
+ size = 8; /* FFA_VERSION_1_0 lacks size in the response */
+
if (buffer && count <= num_partitions)
- memcpy(buffer, drv_info->rx_buffer, sizeof(*buffer) * count);
+ for (idx = 0; idx < count; idx++)
+ memcpy(buffer + idx, drv_info->rx_buffer + idx * size,
+ size);

ffa_rx_release();

@@ -678,6 +691,14 @@ void ffa_device_match_uuid(struct ffa_device *ffa_dev, const uuid_t *uuid)
int count, idx;
struct ffa_partition_info *pbuf, *tpbuf;

+ /*
+ * FF-A v1.1 provides UUID for each partition as part of the discovery
+ * API, the discovered UUID must be populated in the device's UUID and
+ * there is no need to copy the same from the driver table.
+ */
+ if (drv_info->version > FFA_VERSION_1_0)
+ return;
+
count = ffa_partition_probe(uuid, &pbuf);
if (count <= 0)
return;
@@ -691,6 +712,7 @@ void ffa_device_match_uuid(struct ffa_device *ffa_dev, const uuid_t *uuid)
static void ffa_setup_partitions(void)
{
int count, idx;
+ uuid_t uuid;
struct ffa_device *ffa_dev;
struct ffa_partition_info *pbuf, *tpbuf;

@@ -701,14 +723,15 @@ static void ffa_setup_partitions(void)
}

for (idx = 0, tpbuf = pbuf; idx < count; idx++, tpbuf++) {
- /* Note that the &uuid_null parameter will require
+ import_uuid(&uuid, (u8 *)tpbuf->uuid);
+
+ /* Note that the UUID will be uuid_null and that will require
* ffa_device_match() to find the UUID of this partition id
- * with help of ffa_device_match_uuid(). Once the FF-A spec
- * is updated to provide correct UUID here for each partition
- * as part of the discovery API, we need to pass the
- * discovered UUID here instead.
+ * with help of ffa_device_match_uuid(). FF-A v1.1 and above
+ * provides UUID here for each partition as part of the
+ * discovery API and the same is passed.
*/
- ffa_dev = ffa_device_register(&uuid_null, tpbuf->id, &ffa_ops);
+ ffa_dev = ffa_device_register(&uuid, tpbuf->id, &ffa_ops);
if (!ffa_dev) {
pr_err("%s: failed to register partition ID 0x%x\n",
__func__, tpbuf->id);
diff --git a/include/linux/arm_ffa.h b/include/linux/arm_ffa.h
index eafab07c9f58..b40afa7933dc 100644
--- a/include/linux/arm_ffa.h
+++ b/include/linux/arm_ffa.h
@@ -107,6 +107,7 @@ struct ffa_partition_info {
/* partition can send and receive indirect messages. */
#define FFA_PARTITION_INDIRECT_MSG BIT(2)
u32 properties;
+ u32 uuid[4];
};

/* For use with FFA_MSG_SEND_DIRECT_{REQ,RESP} which pass data via registers */
--
2.37.2

2022-08-30 10:21:33

by Sudeep Holla

[permalink] [raw]
Subject: [PATCH 9/9] firmware: arm_ffa: Split up ffa_dev_ops into info, message and memory operations

In preparation to make memory operations accessible for a non
ffa_driver/device, it is better to split the ffa_dev_ops into different
categories of operations: info, message and memory. The info and memory
are ffa_device independent and can be used without any associated
ffa_device from a non ffa_driver.

However, we don't export these info and memory APIs yet without the user.
The first users of these APIs can export them.

Signed-off-by: Sudeep Holla <[email protected]>
---
drivers/firmware/arm_ffa/driver.c | 14 ++++++++++++-
drivers/tee/optee/ffa_abi.c | 33 +++++++++++++++++--------------
include/linux/arm_ffa.h | 14 ++++++++++++-
3 files changed, 44 insertions(+), 17 deletions(-)

diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
index 6822241168d6..d7a90c49fdcf 100644
--- a/drivers/firmware/arm_ffa/driver.c
+++ b/drivers/firmware/arm_ffa/driver.c
@@ -676,16 +676,28 @@ static int ffa_memory_lend(struct ffa_mem_ops_args *args)
return ffa_memory_ops(FFA_MEM_LEND, args);
}

-static const struct ffa_dev_ops ffa_ops = {
+static const struct ffa_dev_info_ops ffa_info_ops = {
.api_version_get = ffa_api_version_get,
.partition_info_get = ffa_partition_info_get,
+};
+
+static const struct ffa_dev_msg_ops ffa_msg_ops = {
.mode_32bit_set = ffa_mode_32bit_set,
.sync_send_receive = ffa_sync_send_receive,
+};
+
+static const struct ffa_dev_mem_ops ffa_mem_ops = {
.memory_reclaim = ffa_memory_reclaim,
.memory_share = ffa_memory_share,
.memory_lend = ffa_memory_lend,
};

+static const struct ffa_dev_ops ffa_ops = {
+ .info_ops = &ffa_info_ops,
+ .msg_ops = &ffa_msg_ops,
+ .mem_ops = &ffa_mem_ops,
+};
+
void ffa_device_match_uuid(struct ffa_device *ffa_dev, const uuid_t *uuid)
{
int count, idx;
diff --git a/drivers/tee/optee/ffa_abi.c b/drivers/tee/optee/ffa_abi.c
index 7257b42d0545..bbc63fd145a1 100644
--- a/drivers/tee/optee/ffa_abi.c
+++ b/drivers/tee/optee/ffa_abi.c
@@ -272,7 +272,7 @@ static int optee_ffa_shm_register(struct tee_context *ctx, struct tee_shm *shm,
{
struct optee *optee = tee_get_drvdata(ctx->teedev);
struct ffa_device *ffa_dev = optee->ffa.ffa_dev;
- const struct ffa_dev_ops *ffa_ops = ffa_dev->ops;
+ const struct ffa_dev_mem_ops *ffa_mem_ops = ffa_dev->ops->mem_ops;
struct ffa_mem_region_attributes mem_attr = {
.receiver = ffa_dev->vm_id,
.attrs = FFA_MEM_RW,
@@ -294,14 +294,14 @@ static int optee_ffa_shm_register(struct tee_context *ctx, struct tee_shm *shm,
if (rc)
return rc;
args.sg = sgt.sgl;
- rc = ffa_ops->memory_share(&args);
+ rc = ffa_mem_ops->memory_share(&args);
sg_free_table(&sgt);
if (rc)
return rc;

rc = optee_shm_add_ffa_handle(optee, shm, args.g_handle);
if (rc) {
- ffa_ops->memory_reclaim(args.g_handle, 0);
+ ffa_mem_ops->memory_reclaim(args.g_handle, 0);
return rc;
}

@@ -315,7 +315,8 @@ static int optee_ffa_shm_unregister(struct tee_context *ctx,
{
struct optee *optee = tee_get_drvdata(ctx->teedev);
struct ffa_device *ffa_dev = optee->ffa.ffa_dev;
- const struct ffa_dev_ops *ffa_ops = ffa_dev->ops;
+ const struct ffa_dev_msg_ops *ffa_msg_ops = ffa_dev->ops->msg_ops;
+ const struct ffa_dev_mem_ops *ffa_mem_ops = ffa_dev->ops->mem_ops;
u64 global_handle = shm->sec_world_id;
struct ffa_send_direct_data data = {
.data0 = OPTEE_FFA_UNREGISTER_SHM,
@@ -327,11 +328,11 @@ static int optee_ffa_shm_unregister(struct tee_context *ctx,
optee_shm_rem_ffa_handle(optee, global_handle);
shm->sec_world_id = 0;

- rc = ffa_ops->sync_send_receive(ffa_dev, &data);
+ rc = ffa_msg_ops->sync_send_receive(ffa_dev, &data);
if (rc)
pr_err("Unregister SHM id 0x%llx rc %d\n", global_handle, rc);

- rc = ffa_ops->memory_reclaim(global_handle, 0);
+ rc = ffa_mem_ops->memory_reclaim(global_handle, 0);
if (rc)
pr_err("mem_reclaim: 0x%llx %d", global_handle, rc);

@@ -342,7 +343,7 @@ static int optee_ffa_shm_unregister_supp(struct tee_context *ctx,
struct tee_shm *shm)
{
struct optee *optee = tee_get_drvdata(ctx->teedev);
- const struct ffa_dev_ops *ffa_ops = optee->ffa.ffa_dev->ops;
+ const struct ffa_dev_mem_ops *ffa_mem_ops;
u64 global_handle = shm->sec_world_id;
int rc;

@@ -353,7 +354,8 @@ static int optee_ffa_shm_unregister_supp(struct tee_context *ctx,
*/

optee_shm_rem_ffa_handle(optee, global_handle);
- rc = ffa_ops->memory_reclaim(global_handle, 0);
+ ffa_mem_ops = optee->ffa.ffa_dev->ops->mem_ops;
+ rc = ffa_mem_ops->memory_reclaim(global_handle, 0);
if (rc)
pr_err("mem_reclaim: 0x%llx %d", global_handle, rc);

@@ -530,7 +532,7 @@ static int optee_ffa_yielding_call(struct tee_context *ctx,
{
struct optee *optee = tee_get_drvdata(ctx->teedev);
struct ffa_device *ffa_dev = optee->ffa.ffa_dev;
- const struct ffa_dev_ops *ffa_ops = ffa_dev->ops;
+ const struct ffa_dev_msg_ops *ffa_msg_ops = ffa_dev->ops->msg_ops;
struct optee_call_waiter w;
u32 cmd = data->data0;
u32 w4 = data->data1;
@@ -541,7 +543,7 @@ static int optee_ffa_yielding_call(struct tee_context *ctx,
/* Initialize waiter */
optee_cq_wait_init(&optee->call_queue, &w);
while (true) {
- rc = ffa_ops->sync_send_receive(ffa_dev, data);
+ rc = ffa_msg_ops->sync_send_receive(ffa_dev, data);
if (rc)
goto done;

@@ -576,7 +578,7 @@ static int optee_ffa_yielding_call(struct tee_context *ctx,
* OP-TEE has returned with a RPC request.
*
* Note that data->data4 (passed in register w7) is already
- * filled in by ffa_ops->sync_send_receive() returning
+ * filled in by ffa_mem_ops->sync_send_receive() returning
* above.
*/
cond_resched();
@@ -654,12 +656,13 @@ static int optee_ffa_do_call_with_arg(struct tee_context *ctx,
static bool optee_ffa_api_is_compatbile(struct ffa_device *ffa_dev,
const struct ffa_dev_ops *ops)
{
+ const struct ffa_dev_msg_ops *ffa_msg_ops = ops->msg_ops;
struct ffa_send_direct_data data = { OPTEE_FFA_GET_API_VERSION };
int rc;

- ops->mode_32bit_set(ffa_dev);
+ ffa_msg_ops->mode_32bit_set(ffa_dev);

- rc = ops->sync_send_receive(ffa_dev, &data);
+ rc = ffa_msg_ops->sync_send_receive(ffa_dev, &data);
if (rc) {
pr_err("Unexpected error %d\n", rc);
return false;
@@ -672,7 +675,7 @@ static bool optee_ffa_api_is_compatbile(struct ffa_device *ffa_dev,
}

data = (struct ffa_send_direct_data){ OPTEE_FFA_GET_OS_VERSION };
- rc = ops->sync_send_receive(ffa_dev, &data);
+ rc = ffa_msg_ops->sync_send_receive(ffa_dev, &data);
if (rc) {
pr_err("Unexpected error %d\n", rc);
return false;
@@ -694,7 +697,7 @@ static bool optee_ffa_exchange_caps(struct ffa_device *ffa_dev,
struct ffa_send_direct_data data = { OPTEE_FFA_EXCHANGE_CAPABILITIES };
int rc;

- rc = ops->sync_send_receive(ffa_dev, &data);
+ rc = ops->msg_ops->sync_send_receive(ffa_dev, &data);
if (rc) {
pr_err("Unexpected error %d", rc);
return false;
diff --git a/include/linux/arm_ffa.h b/include/linux/arm_ffa.h
index b40afa7933dc..45e2b83d2364 100644
--- a/include/linux/arm_ffa.h
+++ b/include/linux/arm_ffa.h
@@ -255,16 +255,28 @@ struct ffa_mem_ops_args {
struct ffa_mem_region_attributes *attrs;
};

-struct ffa_dev_ops {
+struct ffa_dev_info_ops {
u32 (*api_version_get)(void);
int (*partition_info_get)(const char *uuid_str,
struct ffa_partition_info *buffer);
+};
+
+struct ffa_dev_msg_ops {
void (*mode_32bit_set)(struct ffa_device *dev);
int (*sync_send_receive)(struct ffa_device *dev,
struct ffa_send_direct_data *data);
+};
+
+struct ffa_dev_mem_ops {
int (*memory_reclaim)(u64 g_handle, u32 flags);
int (*memory_share)(struct ffa_mem_ops_args *args);
int (*memory_lend)(struct ffa_mem_ops_args *args);
};

+struct ffa_dev_ops {
+ const struct ffa_dev_info_ops *info_ops;
+ const struct ffa_dev_msg_ops *msg_ops;
+ const struct ffa_dev_mem_ops *mem_ops;
+};
+
#endif /* _LINUX_ARM_FFA_H */
--
2.37.2

2022-08-30 10:35:01

by Sudeep Holla

[permalink] [raw]
Subject: [PATCH 5/9] firmware: arm_ffa: Use FFA_FEATURES to detect if native versions are supported

Currently, the ffa_dev->mode_32bit is use to detect if the native 64-bit
or 32-bit versions of FF-A ABI needs to be used. However for the FF-A
memory ABIs, it is not dependent on the ffa_device(i.e. the partition)
itself, but the partition manager(SPM).

So, the FFA_FEATURES can be use to detect if the native 64bit ABIs are
supported or not and appropriate calls can be made based on that.

Use FFA_FEATURES to detect if native versions of MEM_LEND or MEM_SHARE
are implemented and make of the same to use native memory ABIs later on.

Signed-off-by: Sudeep Holla <[email protected]>
---
drivers/firmware/arm_ffa/driver.c | 22 ++++++++++++++++------
1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
index de94073f4109..5f02b670e964 100644
--- a/drivers/firmware/arm_ffa/driver.c
+++ b/drivers/firmware/arm_ffa/driver.c
@@ -163,6 +163,7 @@ struct ffa_drv_info {
struct mutex tx_lock; /* lock to protect Tx buffer */
void *rx_buffer;
void *tx_buffer;
+ bool mem_ops_native;
};

static struct ffa_drv_info *drv_info;
@@ -594,6 +595,13 @@ static int ffa_features(u32 func_feat_id, u32 input_props, u32 *if_props)
return 0;
}

+static void ffa_set_up_mem_ops_native_flag(void)
+{
+ if (!ffa_features(FFA_FN_NATIVE(MEM_LEND), 0, NULL) ||
+ !ffa_features(FFA_FN_NATIVE(MEM_SHARE), 0, NULL))
+ drv_info->mem_ops_native = true;
+}
+
static u32 ffa_api_version_get(void)
{
return drv_info->version;
@@ -635,10 +643,10 @@ static int ffa_sync_send_receive(struct ffa_device *dev,
static int
ffa_memory_share(struct ffa_device *dev, struct ffa_mem_ops_args *args)
{
- if (dev->mode_32bit)
- return ffa_memory_ops(FFA_MEM_SHARE, args);
+ if (drv_info->mem_ops_native)
+ return ffa_memory_ops(FFA_FN_NATIVE(MEM_SHARE), args);

- return ffa_memory_ops(FFA_FN_NATIVE(MEM_SHARE), args);
+ return ffa_memory_ops(FFA_MEM_SHARE, args);
}

static int
@@ -651,10 +659,10 @@ ffa_memory_lend(struct ffa_device *dev, struct ffa_mem_ops_args *args)
* however on systems without a hypervisor the responsibility
* falls to the calling kernel driver to prevent access.
*/
- if (dev->mode_32bit)
- return ffa_memory_ops(FFA_MEM_LEND, args);
+ if (drv_info->mem_ops_native)
+ return ffa_memory_ops(FFA_FN_NATIVE(MEM_LEND), args);

- return ffa_memory_ops(FFA_FN_NATIVE(MEM_LEND), args);
+ return ffa_memory_ops(FFA_MEM_LEND, args);
}

static const struct ffa_dev_ops ffa_ops = {
@@ -765,6 +773,8 @@ static int __init ffa_init(void)

ffa_setup_partitions();

+ ffa_set_up_mem_ops_native_flag();
+
return 0;
free_pages:
if (drv_info->tx_buffer)
--
2.37.2

2022-08-30 10:35:31

by Sudeep Holla

[permalink] [raw]
Subject: [PATCH 3/9] firmware: arm_ffa: Remove ffa_dev_ops_get()

The only user of this exported ffa_dev_ops_get() was OPTEE driver which
now uses ffa_dev->ops directly, there are no other users for this.

Also, since any ffa driver can use ffa_dev->ops directly, there will be
no need for ffa_dev_ops_get(), so just remove ffa_dev_ops_get().

Signed-off-by: Sudeep Holla <[email protected]>
---
drivers/firmware/arm_ffa/driver.c | 9 ---------
include/linux/arm_ffa.h | 6 ------
2 files changed, 15 deletions(-)

diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
index 213665e5ad0e..04e7cbb1b9aa 100644
--- a/drivers/firmware/arm_ffa/driver.c
+++ b/drivers/firmware/arm_ffa/driver.c
@@ -644,15 +644,6 @@ static const struct ffa_dev_ops ffa_ops = {
.memory_lend = ffa_memory_lend,
};

-const struct ffa_dev_ops *ffa_dev_ops_get(struct ffa_device *dev)
-{
- if (ffa_device_is_valid(dev))
- return &ffa_ops;
-
- return NULL;
-}
-EXPORT_SYMBOL_GPL(ffa_dev_ops_get);
-
void ffa_device_match_uuid(struct ffa_device *ffa_dev, const uuid_t *uuid)
{
int count, idx;
diff --git a/include/linux/arm_ffa.h b/include/linux/arm_ffa.h
index 91b47e42b73d..556f50f27fb1 100644
--- a/include/linux/arm_ffa.h
+++ b/include/linux/arm_ffa.h
@@ -55,7 +55,6 @@ int ffa_driver_register(struct ffa_driver *driver, struct module *owner,
const char *mod_name);
void ffa_driver_unregister(struct ffa_driver *driver);
bool ffa_device_is_valid(struct ffa_device *ffa_dev);
-const struct ffa_dev_ops *ffa_dev_ops_get(struct ffa_device *dev);

#else
static inline
@@ -79,11 +78,6 @@ static inline void ffa_driver_unregister(struct ffa_driver *driver) {}
static inline
bool ffa_device_is_valid(struct ffa_device *ffa_dev) { return false; }

-static inline
-const struct ffa_dev_ops *ffa_dev_ops_get(struct ffa_device *dev)
-{
- return NULL;
-}
#endif /* CONFIG_ARM_FFA_TRANSPORT */

#define ffa_register(driver) \
--
2.37.2

2022-08-30 11:14:12

by Sudeep Holla

[permalink] [raw]
Subject: [PATCH 4/9] firmware: arm_ffa: Add support for querying FF-A features

Add support for FFA_FEATURES to discover properties supported at the
FF-A interface. This interface can be used to query:
- If an FF-A interface is implemented by the component at the higher EL,
- If an implemented FF-A interface also implements any optional features
described in its interface definition, and
- Any implementation details exported by an implemented FF-A interface
as described in its interface definition.

Signed-off-by: Sudeep Holla <[email protected]>
---
drivers/firmware/arm_ffa/driver.c | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)

diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
index 04e7cbb1b9aa..de94073f4109 100644
--- a/drivers/firmware/arm_ffa/driver.c
+++ b/drivers/firmware/arm_ffa/driver.c
@@ -571,6 +571,29 @@ static int ffa_memory_reclaim(u64 g_handle, u32 flags)
return 0;
}

+static int ffa_features(u32 func_feat_id, u32 input_props, u32 *if_props)
+{
+ ffa_value_t id;
+
+ if (!ARM_SMCCC_IS_FAST_CALL(func_feat_id) && input_props) {
+ pr_err("%s: Invalid Parameters: %x, %x", __func__,
+ func_feat_id, input_props);
+ return ffa_to_linux_errno(FFA_RET_INVALID_PARAMETERS);
+ }
+
+ invoke_ffa_fn((ffa_value_t){
+ .a0 = FFA_FEATURES, .a1 = func_feat_id, .a2 = input_props,
+ }, &id);
+
+ if (id.a0 == FFA_ERROR)
+ return ffa_to_linux_errno((int)id.a2);
+
+ if (if_props)
+ *if_props = id.a2;
+
+ return 0;
+}
+
static u32 ffa_api_version_get(void)
{
return drv_info->version;
--
2.37.2

2022-08-30 11:14:28

by Sudeep Holla

[permalink] [raw]
Subject: [PATCH 6/9] firmware: arm_ffa: Make memory apis ffa_device independent

There is a requirement to make memory APIs independent of the ffa_device.
One of the use-case is to have a common memory driver that manages the
memory for all the ffa_devices. That commom memory driver won't be a
ffa_driver or won't have any ffa_device associated with it. So having
these memory APIs accessible without a ffa_device is needed and should
be possible as most of these are handled by the partition manager(SPM
or hypervisor).

Drop the ffa_device argument to the memory APIs and make them ffa_device
independent.

Signed-off-by: Sudeep Holla <[email protected]>
---
drivers/firmware/arm_ffa/driver.c | 6 ++----
drivers/tee/optee/ffa_abi.c | 2 +-
include/linux/arm_ffa.h | 6 ++----
3 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
index 5f02b670e964..5c8484b05c50 100644
--- a/drivers/firmware/arm_ffa/driver.c
+++ b/drivers/firmware/arm_ffa/driver.c
@@ -640,8 +640,7 @@ static int ffa_sync_send_receive(struct ffa_device *dev,
dev->mode_32bit, data);
}

-static int
-ffa_memory_share(struct ffa_device *dev, struct ffa_mem_ops_args *args)
+static int ffa_memory_share(struct ffa_mem_ops_args *args)
{
if (drv_info->mem_ops_native)
return ffa_memory_ops(FFA_FN_NATIVE(MEM_SHARE), args);
@@ -649,8 +648,7 @@ ffa_memory_share(struct ffa_device *dev, struct ffa_mem_ops_args *args)
return ffa_memory_ops(FFA_MEM_SHARE, args);
}

-static int
-ffa_memory_lend(struct ffa_device *dev, struct ffa_mem_ops_args *args)
+static int ffa_memory_lend(struct ffa_mem_ops_args *args)
{
/* Note that upon a successful MEM_LEND request the caller
* must ensure that the memory region specified is not accessed
diff --git a/drivers/tee/optee/ffa_abi.c b/drivers/tee/optee/ffa_abi.c
index 4c3b5d0008dd..7ec0a2f9a63b 100644
--- a/drivers/tee/optee/ffa_abi.c
+++ b/drivers/tee/optee/ffa_abi.c
@@ -294,7 +294,7 @@ static int optee_ffa_shm_register(struct tee_context *ctx, struct tee_shm *shm,
if (rc)
return rc;
args.sg = sgt.sgl;
- rc = ffa_ops->memory_share(ffa_dev, &args);
+ rc = ffa_ops->memory_share(&args);
sg_free_table(&sgt);
if (rc)
return rc;
diff --git a/include/linux/arm_ffa.h b/include/linux/arm_ffa.h
index 556f50f27fb1..eafab07c9f58 100644
--- a/include/linux/arm_ffa.h
+++ b/include/linux/arm_ffa.h
@@ -262,10 +262,8 @@ struct ffa_dev_ops {
int (*sync_send_receive)(struct ffa_device *dev,
struct ffa_send_direct_data *data);
int (*memory_reclaim)(u64 g_handle, u32 flags);
- int (*memory_share)(struct ffa_device *dev,
- struct ffa_mem_ops_args *args);
- int (*memory_lend)(struct ffa_device *dev,
- struct ffa_mem_ops_args *args);
+ int (*memory_share)(struct ffa_mem_ops_args *args);
+ int (*memory_lend)(struct ffa_mem_ops_args *args);
};

#endif /* _LINUX_ARM_FFA_H */
--
2.37.2

2022-08-31 08:06:08

by Sumit Garg

[permalink] [raw]
Subject: Re: [PATCH 2/9] tee: optee: Use ffa_dev->ops directly

On Tue, 30 Aug 2022 at 15:38, Sudeep Holla <[email protected]> wrote:
>
> Now that the ffa_device structure holds the pointer to ffa_dev_ops,
> there is no need to obtain the same through ffa_dev_ops_get().
>
> Just use the ffa_dev->ops directly.
>
> Signed-off-by: Sudeep Holla <[email protected]>
> ---
> drivers/tee/optee/ffa_abi.c | 6 +-----
> 1 file changed, 1 insertion(+), 5 deletions(-)
>

Reviewed-by: Sumit Garg <[email protected]>

-Sumit

> diff --git a/drivers/tee/optee/ffa_abi.c b/drivers/tee/optee/ffa_abi.c
> index 7ab31740cff8..4c3b5d0008dd 100644
> --- a/drivers/tee/optee/ffa_abi.c
> +++ b/drivers/tee/optee/ffa_abi.c
> @@ -793,11 +793,7 @@ static int optee_ffa_probe(struct ffa_device *ffa_dev)
> u32 sec_caps;
> int rc;
>
> - ffa_ops = ffa_dev_ops_get(ffa_dev);
> - if (!ffa_ops) {
> - pr_warn("failed \"method\" init: ffa\n");
> - return -ENOENT;
> - }
> + ffa_ops = ffa_dev->ops;
>
> if (!optee_ffa_api_is_compatbile(ffa_dev, ffa_ops))
> return -EINVAL;
> --
> 2.37.2
>

2022-08-31 11:03:57

by Sumit Garg

[permalink] [raw]
Subject: Re: [PATCH 6/9] firmware: arm_ffa: Make memory apis ffa_device independent

Hi Sudeep,

On Tue, 30 Aug 2022 at 15:39, Sudeep Holla <[email protected]> wrote:
>
> There is a requirement to make memory APIs independent of the ffa_device.
> One of the use-case is to have a common memory driver that manages the
> memory for all the ffa_devices. That commom memory driver won't be a

s/commom/common/

> ffa_driver or won't have any ffa_device associated with it. So having
> these memory APIs accessible without a ffa_device is needed and should
> be possible as most of these are handled by the partition manager(SPM
> or hypervisor).
>
> Drop the ffa_device argument to the memory APIs and make them ffa_device
> independent.
>
> Signed-off-by: Sudeep Holla <[email protected]>
> ---
> drivers/firmware/arm_ffa/driver.c | 6 ++----
> drivers/tee/optee/ffa_abi.c | 2 +-
> include/linux/arm_ffa.h | 6 ++----
> 3 files changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
> index 5f02b670e964..5c8484b05c50 100644
> --- a/drivers/firmware/arm_ffa/driver.c
> +++ b/drivers/firmware/arm_ffa/driver.c
> @@ -640,8 +640,7 @@ static int ffa_sync_send_receive(struct ffa_device *dev,
> dev->mode_32bit, data);
> }
>
> -static int
> -ffa_memory_share(struct ffa_device *dev, struct ffa_mem_ops_args *args)
> +static int ffa_memory_share(struct ffa_mem_ops_args *args)
> {
> if (drv_info->mem_ops_native)
> return ffa_memory_ops(FFA_FN_NATIVE(MEM_SHARE), args);
> @@ -649,8 +648,7 @@ ffa_memory_share(struct ffa_device *dev, struct ffa_mem_ops_args *args)
> return ffa_memory_ops(FFA_MEM_SHARE, args);
> }
>
> -static int
> -ffa_memory_lend(struct ffa_device *dev, struct ffa_mem_ops_args *args)
> +static int ffa_memory_lend(struct ffa_mem_ops_args *args)
> {
> /* Note that upon a successful MEM_LEND request the caller
> * must ensure that the memory region specified is not accessed
> diff --git a/drivers/tee/optee/ffa_abi.c b/drivers/tee/optee/ffa_abi.c
> index 4c3b5d0008dd..7ec0a2f9a63b 100644
> --- a/drivers/tee/optee/ffa_abi.c
> +++ b/drivers/tee/optee/ffa_abi.c
> @@ -294,7 +294,7 @@ static int optee_ffa_shm_register(struct tee_context *ctx, struct tee_shm *shm,
> if (rc)
> return rc;
> args.sg = sgt.sgl;
> - rc = ffa_ops->memory_share(ffa_dev, &args);
> + rc = ffa_ops->memory_share(&args);
> sg_free_table(&sgt);
> if (rc)
> return rc;
> diff --git a/include/linux/arm_ffa.h b/include/linux/arm_ffa.h
> index 556f50f27fb1..eafab07c9f58 100644
> --- a/include/linux/arm_ffa.h
> +++ b/include/linux/arm_ffa.h
> @@ -262,10 +262,8 @@ struct ffa_dev_ops {
> int (*sync_send_receive)(struct ffa_device *dev,
> struct ffa_send_direct_data *data);
> int (*memory_reclaim)(u64 g_handle, u32 flags);
> - int (*memory_share)(struct ffa_device *dev,
> - struct ffa_mem_ops_args *args);
> - int (*memory_lend)(struct ffa_device *dev,
> - struct ffa_mem_ops_args *args);
> + int (*memory_share)(struct ffa_mem_ops_args *args);
> + int (*memory_lend)(struct ffa_mem_ops_args *args);
> };
>

Since these are included under "struct ffa_dev_ops", wouldn't it be
better to rename the struct (ffa_ops?) as well?

-Sumit

> #endif /* _LINUX_ARM_FFA_H */
> --
> 2.37.2
>

2022-08-31 11:09:58

by Sumit Garg

[permalink] [raw]
Subject: Re: [PATCH 8/9] tee: optee: Drop ffa_ops in optee_ffa structure

On Tue, 30 Aug 2022 at 15:40, Sudeep Holla <[email protected]> wrote:
>
> Since the ffa_device itself carries ffa_dev_ops now, there is no need
> to keep a copy in optee_ffa structure.
>
> Drop ffa_ops in the optee_ffa structure as it is not needed anymore.
>
> Signed-off-by: Sudeep Holla <[email protected]>
> ---
> drivers/tee/optee/ffa_abi.c | 9 ++++-----
> drivers/tee/optee/optee_private.h | 1 -
> 2 files changed, 4 insertions(+), 6 deletions(-)
>

Is this patch doing anything different from patch #2? If not then I
think both should be squashed.

-Sumit

> diff --git a/drivers/tee/optee/ffa_abi.c b/drivers/tee/optee/ffa_abi.c
> index 7ec0a2f9a63b..7257b42d0545 100644
> --- a/drivers/tee/optee/ffa_abi.c
> +++ b/drivers/tee/optee/ffa_abi.c
> @@ -271,8 +271,8 @@ static int optee_ffa_shm_register(struct tee_context *ctx, struct tee_shm *shm,
> unsigned long start)
> {
> struct optee *optee = tee_get_drvdata(ctx->teedev);
> - const struct ffa_dev_ops *ffa_ops = optee->ffa.ffa_ops;
> struct ffa_device *ffa_dev = optee->ffa.ffa_dev;
> + const struct ffa_dev_ops *ffa_ops = ffa_dev->ops;
> struct ffa_mem_region_attributes mem_attr = {
> .receiver = ffa_dev->vm_id,
> .attrs = FFA_MEM_RW,
> @@ -314,8 +314,8 @@ static int optee_ffa_shm_unregister(struct tee_context *ctx,
> struct tee_shm *shm)
> {
> struct optee *optee = tee_get_drvdata(ctx->teedev);
> - const struct ffa_dev_ops *ffa_ops = optee->ffa.ffa_ops;
> struct ffa_device *ffa_dev = optee->ffa.ffa_dev;
> + const struct ffa_dev_ops *ffa_ops = ffa_dev->ops;
> u64 global_handle = shm->sec_world_id;
> struct ffa_send_direct_data data = {
> .data0 = OPTEE_FFA_UNREGISTER_SHM,
> @@ -342,7 +342,7 @@ static int optee_ffa_shm_unregister_supp(struct tee_context *ctx,
> struct tee_shm *shm)
> {
> struct optee *optee = tee_get_drvdata(ctx->teedev);
> - const struct ffa_dev_ops *ffa_ops = optee->ffa.ffa_ops;
> + const struct ffa_dev_ops *ffa_ops = optee->ffa.ffa_dev->ops;
> u64 global_handle = shm->sec_world_id;
> int rc;
>
> @@ -529,8 +529,8 @@ static int optee_ffa_yielding_call(struct tee_context *ctx,
> struct optee_msg_arg *rpc_arg)
> {
> struct optee *optee = tee_get_drvdata(ctx->teedev);
> - const struct ffa_dev_ops *ffa_ops = optee->ffa.ffa_ops;
> struct ffa_device *ffa_dev = optee->ffa.ffa_dev;
> + const struct ffa_dev_ops *ffa_ops = ffa_dev->ops;
> struct optee_call_waiter w;
> u32 cmd = data->data0;
> u32 w4 = data->data1;
> @@ -817,7 +817,6 @@ static int optee_ffa_probe(struct ffa_device *ffa_dev)
>
> optee->ops = &optee_ffa_ops;
> optee->ffa.ffa_dev = ffa_dev;
> - optee->ffa.ffa_ops = ffa_ops;
> optee->rpc_param_count = rpc_param_count;
>
> teedev = tee_device_alloc(&optee_ffa_clnt_desc, NULL, optee->pool,
> diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
> index a33d98d17cfd..04ae58892608 100644
> --- a/drivers/tee/optee/optee_private.h
> +++ b/drivers/tee/optee/optee_private.h
> @@ -111,7 +111,6 @@ struct optee_smc {
> */
> struct optee_ffa {
> struct ffa_device *ffa_dev;
> - const struct ffa_dev_ops *ffa_ops;
> /* Serializes access to @global_ids */
> struct mutex mutex;
> struct rhashtable global_ids;
> --
> 2.37.2
>

2022-08-31 11:38:21

by Sumit Garg

[permalink] [raw]
Subject: Re: [PATCH 8/9] tee: optee: Drop ffa_ops in optee_ffa structure

On Wed, 31 Aug 2022 at 16:57, Sudeep Holla <[email protected]> wrote:
>
> On Wed, Aug 31, 2022 at 04:28:01PM +0530, Sumit Garg wrote:
> > On Tue, 30 Aug 2022 at 15:40, Sudeep Holla <[email protected]> wrote:
> > >
> > > Since the ffa_device itself carries ffa_dev_ops now, there is no need
> > > to keep a copy in optee_ffa structure.
> > >
> > > Drop ffa_ops in the optee_ffa structure as it is not needed anymore.
> > >
> > > Signed-off-by: Sudeep Holla <[email protected]>
> > > ---
> > > drivers/tee/optee/ffa_abi.c | 9 ++++-----
> > > drivers/tee/optee/optee_private.h | 1 -
> > > 2 files changed, 4 insertions(+), 6 deletions(-)
> > >
> >
> > Is this patch doing anything different from patch #2? If not then I
> > think both should be squashed.
> >
>
> Indeed, I thought about squashing them and forgot before getting there.
> Does the review tag still apply for 2/9 after I squash this into it.
>

Yeah, feel free to apply my review tag.

-Sumit

> Thanks for the review.
>
> --
> Regards,
> Sudeep

2022-08-31 11:38:19

by Sudeep Holla

[permalink] [raw]
Subject: Re: [PATCH 8/9] tee: optee: Drop ffa_ops in optee_ffa structure

On Wed, Aug 31, 2022 at 04:28:01PM +0530, Sumit Garg wrote:
> On Tue, 30 Aug 2022 at 15:40, Sudeep Holla <[email protected]> wrote:
> >
> > Since the ffa_device itself carries ffa_dev_ops now, there is no need
> > to keep a copy in optee_ffa structure.
> >
> > Drop ffa_ops in the optee_ffa structure as it is not needed anymore.
> >
> > Signed-off-by: Sudeep Holla <[email protected]>
> > ---
> > drivers/tee/optee/ffa_abi.c | 9 ++++-----
> > drivers/tee/optee/optee_private.h | 1 -
> > 2 files changed, 4 insertions(+), 6 deletions(-)
> >
>
> Is this patch doing anything different from patch #2? If not then I
> think both should be squashed.
>

Indeed, I thought about squashing them and forgot before getting there.
Does the review tag still apply for 2/9 after I squash this into it.

Thanks for the review.

--
Regards,
Sudeep

2022-08-31 13:01:14

by Sudeep Holla

[permalink] [raw]
Subject: Re: [PATCH 6/9] firmware: arm_ffa: Make memory apis ffa_device independent

On Wed, Aug 31, 2022 at 04:16:09PM +0530, Sumit Garg wrote:
> Hi Sudeep,
>
> On Tue, 30 Aug 2022 at 15:39, Sudeep Holla <[email protected]> wrote:
> >
> > There is a requirement to make memory APIs independent of the ffa_device.
> > One of the use-case is to have a common memory driver that manages the
> > memory for all the ffa_devices. That commom memory driver won't be a
>
> s/commom/common/
>
> > ffa_driver or won't have any ffa_device associated with it. So having
> > these memory APIs accessible without a ffa_device is needed and should
> > be possible as most of these are handled by the partition manager(SPM
> > or hypervisor).
> >
> > Drop the ffa_device argument to the memory APIs and make them ffa_device
> > independent.
> >
> > Signed-off-by: Sudeep Holla <[email protected]>
> > ---
> > drivers/firmware/arm_ffa/driver.c | 6 ++----
> > drivers/tee/optee/ffa_abi.c | 2 +-
> > include/linux/arm_ffa.h | 6 ++----
> > 3 files changed, 5 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
> > index 5f02b670e964..5c8484b05c50 100644
> > --- a/drivers/firmware/arm_ffa/driver.c
> > +++ b/drivers/firmware/arm_ffa/driver.c
> > @@ -640,8 +640,7 @@ static int ffa_sync_send_receive(struct ffa_device *dev,
> > dev->mode_32bit, data);
> > }
> >
> > -static int
> > -ffa_memory_share(struct ffa_device *dev, struct ffa_mem_ops_args *args)
> > +static int ffa_memory_share(struct ffa_mem_ops_args *args)
> > {
> > if (drv_info->mem_ops_native)
> > return ffa_memory_ops(FFA_FN_NATIVE(MEM_SHARE), args);
> > @@ -649,8 +648,7 @@ ffa_memory_share(struct ffa_device *dev, struct ffa_mem_ops_args *args)
> > return ffa_memory_ops(FFA_MEM_SHARE, args);
> > }
> >
> > -static int
> > -ffa_memory_lend(struct ffa_device *dev, struct ffa_mem_ops_args *args)
> > +static int ffa_memory_lend(struct ffa_mem_ops_args *args)
> > {
> > /* Note that upon a successful MEM_LEND request the caller
> > * must ensure that the memory region specified is not accessed
> > diff --git a/drivers/tee/optee/ffa_abi.c b/drivers/tee/optee/ffa_abi.c
> > index 4c3b5d0008dd..7ec0a2f9a63b 100644
> > --- a/drivers/tee/optee/ffa_abi.c
> > +++ b/drivers/tee/optee/ffa_abi.c
> > @@ -294,7 +294,7 @@ static int optee_ffa_shm_register(struct tee_context *ctx, struct tee_shm *shm,
> > if (rc)
> > return rc;
> > args.sg = sgt.sgl;
> > - rc = ffa_ops->memory_share(ffa_dev, &args);
> > + rc = ffa_ops->memory_share(&args);
> > sg_free_table(&sgt);
> > if (rc)
> > return rc;
> > diff --git a/include/linux/arm_ffa.h b/include/linux/arm_ffa.h
> > index 556f50f27fb1..eafab07c9f58 100644
> > --- a/include/linux/arm_ffa.h
> > +++ b/include/linux/arm_ffa.h
> > @@ -262,10 +262,8 @@ struct ffa_dev_ops {
> > int (*sync_send_receive)(struct ffa_device *dev,
> > struct ffa_send_direct_data *data);
> > int (*memory_reclaim)(u64 g_handle, u32 flags);
> > - int (*memory_share)(struct ffa_device *dev,
> > - struct ffa_mem_ops_args *args);
> > - int (*memory_lend)(struct ffa_device *dev,
> > - struct ffa_mem_ops_args *args);
> > + int (*memory_share)(struct ffa_mem_ops_args *args);
> > + int (*memory_lend)(struct ffa_mem_ops_args *args);
> > };
> >
>
> Since these are included under "struct ffa_dev_ops", wouldn't it be
> better to rename the struct (ffa_ops?) as well?
>

Makes sense, I just avoided churn. But now I think there is some churn
anyways, so I am happy to rename.

--
Regards,
Sudeep

2022-09-01 07:39:30

by Jens Wiklander

[permalink] [raw]
Subject: Re: [PATCH 2/9] tee: optee: Use ffa_dev->ops directly

On Tue, Aug 30, 2022 at 12:07 PM Sudeep Holla <[email protected]> wrote:
>
> Now that the ffa_device structure holds the pointer to ffa_dev_ops,
> there is no need to obtain the same through ffa_dev_ops_get().
>
> Just use the ffa_dev->ops directly.
>
> Signed-off-by: Sudeep Holla <[email protected]>
> ---
> drivers/tee/optee/ffa_abi.c | 6 +-----
> 1 file changed, 1 insertion(+), 5 deletions(-)

Reviewed-by: Jens Wiklander <[email protected]>

2022-09-01 07:54:18

by Jens Wiklander

[permalink] [raw]
Subject: Re: [PATCH 8/9] tee: optee: Drop ffa_ops in optee_ffa structure

On Wed, Aug 31, 2022 at 1:29 PM Sumit Garg <[email protected]> wrote:
>
> On Wed, 31 Aug 2022 at 16:57, Sudeep Holla <[email protected]> wrote:
> >
> > On Wed, Aug 31, 2022 at 04:28:01PM +0530, Sumit Garg wrote:
> > > On Tue, 30 Aug 2022 at 15:40, Sudeep Holla <[email protected]> wrote:
> > > >
> > > > Since the ffa_device itself carries ffa_dev_ops now, there is no need
> > > > to keep a copy in optee_ffa structure.
> > > >
> > > > Drop ffa_ops in the optee_ffa structure as it is not needed anymore.
> > > >
> > > > Signed-off-by: Sudeep Holla <[email protected]>
> > > > ---
> > > > drivers/tee/optee/ffa_abi.c | 9 ++++-----
> > > > drivers/tee/optee/optee_private.h | 1 -
> > > > 2 files changed, 4 insertions(+), 6 deletions(-)
> > > >
> > >
> > > Is this patch doing anything different from patch #2? If not then I
> > > think both should be squashed.
> > >
> >
> > Indeed, I thought about squashing them and forgot before getting there.
> > Does the review tag still apply for 2/9 after I squash this into it.
> >
>
> Yeah, feel free to apply my review tag.

Same for me.

Cheers,
Jens

>
> -Sumit
>
> > Thanks for the review.
> >
> > --
> > Regards,
> > Sudeep

2022-09-01 08:09:42

by Jens Wiklander

[permalink] [raw]
Subject: Re: [PATCH 4/9] firmware: arm_ffa: Add support for querying FF-A features

On Tue, Aug 30, 2022 at 12:07 PM Sudeep Holla <[email protected]> wrote:
>
> Add support for FFA_FEATURES to discover properties supported at the
> FF-A interface. This interface can be used to query:
> - If an FF-A interface is implemented by the component at the higher EL,
> - If an implemented FF-A interface also implements any optional features
> described in its interface definition, and
> - Any implementation details exported by an implemented FF-A interface
> as described in its interface definition.
>
> Signed-off-by: Sudeep Holla <[email protected]>
> ---
> drivers/firmware/arm_ffa/driver.c | 23 +++++++++++++++++++++++
> 1 file changed, 23 insertions(+)
>
> diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
> index 04e7cbb1b9aa..de94073f4109 100644
> --- a/drivers/firmware/arm_ffa/driver.c
> +++ b/drivers/firmware/arm_ffa/driver.c
> @@ -571,6 +571,29 @@ static int ffa_memory_reclaim(u64 g_handle, u32 flags)
> return 0;
> }
>
> +static int ffa_features(u32 func_feat_id, u32 input_props, u32 *if_props)
> +{
> + ffa_value_t id;
> +
> + if (!ARM_SMCCC_IS_FAST_CALL(func_feat_id) && input_props) {
> + pr_err("%s: Invalid Parameters: %x, %x", __func__,
> + func_feat_id, input_props);
> + return ffa_to_linux_errno(FFA_RET_INVALID_PARAMETERS);
> + }
> +
> + invoke_ffa_fn((ffa_value_t){
> + .a0 = FFA_FEATURES, .a1 = func_feat_id, .a2 = input_props,
> + }, &id);
> +
> + if (id.a0 == FFA_ERROR)
> + return ffa_to_linux_errno((int)id.a2);
> +
> + if (if_props)
> + *if_props = id.a2;

w3 (id.a3) also contains a value when querying for
FFA_MEM_RETRIEVE_REQ. I see that in "[PATCH 5/9] firmware: arm_ffa:
Use FFA_FEATURES to detect if native versions are supported" you're
using this function with if_props = NULL. So I guess that at the
moment we have more than needed, but in case you need to add another
parameter to this function you'll need to update all the call sites
too.

Cheers,
Jens

> +
> + return 0;
> +}
> +
> static u32 ffa_api_version_get(void)
> {
> return drv_info->version;
> --
> 2.37.2
>

2022-09-01 08:34:51

by Jens Wiklander

[permalink] [raw]
Subject: Re: [PATCH 3/9] firmware: arm_ffa: Remove ffa_dev_ops_get()

On Tue, Aug 30, 2022 at 12:07 PM Sudeep Holla <[email protected]> wrote:
>
> The only user of this exported ffa_dev_ops_get() was OPTEE driver which
> now uses ffa_dev->ops directly, there are no other users for this.
>
> Also, since any ffa driver can use ffa_dev->ops directly, there will be
> no need for ffa_dev_ops_get(), so just remove ffa_dev_ops_get().
>
> Signed-off-by: Sudeep Holla <[email protected]>
> ---
> drivers/firmware/arm_ffa/driver.c | 9 ---------
> include/linux/arm_ffa.h | 6 ------
> 2 files changed, 15 deletions(-)

Reviewed-by: Jens Wiklander <[email protected]>

>
> diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
> index 213665e5ad0e..04e7cbb1b9aa 100644
> --- a/drivers/firmware/arm_ffa/driver.c
> +++ b/drivers/firmware/arm_ffa/driver.c
> @@ -644,15 +644,6 @@ static const struct ffa_dev_ops ffa_ops = {
> .memory_lend = ffa_memory_lend,
> };
>
> -const struct ffa_dev_ops *ffa_dev_ops_get(struct ffa_device *dev)
> -{
> - if (ffa_device_is_valid(dev))
> - return &ffa_ops;
> -
> - return NULL;
> -}
> -EXPORT_SYMBOL_GPL(ffa_dev_ops_get);
> -
> void ffa_device_match_uuid(struct ffa_device *ffa_dev, const uuid_t *uuid)
> {
> int count, idx;
> diff --git a/include/linux/arm_ffa.h b/include/linux/arm_ffa.h
> index 91b47e42b73d..556f50f27fb1 100644
> --- a/include/linux/arm_ffa.h
> +++ b/include/linux/arm_ffa.h
> @@ -55,7 +55,6 @@ int ffa_driver_register(struct ffa_driver *driver, struct module *owner,
> const char *mod_name);
> void ffa_driver_unregister(struct ffa_driver *driver);
> bool ffa_device_is_valid(struct ffa_device *ffa_dev);
> -const struct ffa_dev_ops *ffa_dev_ops_get(struct ffa_device *dev);
>
> #else
> static inline
> @@ -79,11 +78,6 @@ static inline void ffa_driver_unregister(struct ffa_driver *driver) {}
> static inline
> bool ffa_device_is_valid(struct ffa_device *ffa_dev) { return false; }
>
> -static inline
> -const struct ffa_dev_ops *ffa_dev_ops_get(struct ffa_device *dev)
> -{
> - return NULL;
> -}
> #endif /* CONFIG_ARM_FFA_TRANSPORT */
>
> #define ffa_register(driver) \
> --
> 2.37.2
>

2022-09-01 08:46:49

by Jens Wiklander

[permalink] [raw]
Subject: Re: [PATCH 5/9] firmware: arm_ffa: Use FFA_FEATURES to detect if native versions are supported

On Tue, Aug 30, 2022 at 12:07 PM Sudeep Holla <[email protected]> wrote:
>
> Currently, the ffa_dev->mode_32bit is use to detect if the native 64-bit
> or 32-bit versions of FF-A ABI needs to be used. However for the FF-A
> memory ABIs, it is not dependent on the ffa_device(i.e. the partition)
> itself, but the partition manager(SPM).
>
> So, the FFA_FEATURES can be use to detect if the native 64bit ABIs are
> supported or not and appropriate calls can be made based on that.
>
> Use FFA_FEATURES to detect if native versions of MEM_LEND or MEM_SHARE
> are implemented and make of the same to use native memory ABIs later on.
>
> Signed-off-by: Sudeep Holla <[email protected]>
> ---
> drivers/firmware/arm_ffa/driver.c | 22 ++++++++++++++++------
> 1 file changed, 16 insertions(+), 6 deletions(-)

Reviewed-by: Jens Wiklander <[email protected]>

>
> diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
> index de94073f4109..5f02b670e964 100644
> --- a/drivers/firmware/arm_ffa/driver.c
> +++ b/drivers/firmware/arm_ffa/driver.c
> @@ -163,6 +163,7 @@ struct ffa_drv_info {
> struct mutex tx_lock; /* lock to protect Tx buffer */
> void *rx_buffer;
> void *tx_buffer;
> + bool mem_ops_native;
> };
>
> static struct ffa_drv_info *drv_info;
> @@ -594,6 +595,13 @@ static int ffa_features(u32 func_feat_id, u32 input_props, u32 *if_props)
> return 0;
> }
>
> +static void ffa_set_up_mem_ops_native_flag(void)
> +{
> + if (!ffa_features(FFA_FN_NATIVE(MEM_LEND), 0, NULL) ||
> + !ffa_features(FFA_FN_NATIVE(MEM_SHARE), 0, NULL))
> + drv_info->mem_ops_native = true;
> +}
> +
> static u32 ffa_api_version_get(void)
> {
> return drv_info->version;
> @@ -635,10 +643,10 @@ static int ffa_sync_send_receive(struct ffa_device *dev,
> static int
> ffa_memory_share(struct ffa_device *dev, struct ffa_mem_ops_args *args)
> {
> - if (dev->mode_32bit)
> - return ffa_memory_ops(FFA_MEM_SHARE, args);
> + if (drv_info->mem_ops_native)
> + return ffa_memory_ops(FFA_FN_NATIVE(MEM_SHARE), args);
>
> - return ffa_memory_ops(FFA_FN_NATIVE(MEM_SHARE), args);
> + return ffa_memory_ops(FFA_MEM_SHARE, args);
> }
>
> static int
> @@ -651,10 +659,10 @@ ffa_memory_lend(struct ffa_device *dev, struct ffa_mem_ops_args *args)
> * however on systems without a hypervisor the responsibility
> * falls to the calling kernel driver to prevent access.
> */
> - if (dev->mode_32bit)
> - return ffa_memory_ops(FFA_MEM_LEND, args);
> + if (drv_info->mem_ops_native)
> + return ffa_memory_ops(FFA_FN_NATIVE(MEM_LEND), args);
>
> - return ffa_memory_ops(FFA_FN_NATIVE(MEM_LEND), args);
> + return ffa_memory_ops(FFA_MEM_LEND, args);
> }
>
> static const struct ffa_dev_ops ffa_ops = {
> @@ -765,6 +773,8 @@ static int __init ffa_init(void)
>
> ffa_setup_partitions();
>
> + ffa_set_up_mem_ops_native_flag();
> +
> return 0;
> free_pages:
> if (drv_info->tx_buffer)
> --
> 2.37.2
>

2022-09-01 08:58:27

by Jens Wiklander

[permalink] [raw]
Subject: Re: [PATCH 9/9] firmware: arm_ffa: Split up ffa_dev_ops into info, message and memory operations

On Tue, Aug 30, 2022 at 12:07 PM Sudeep Holla <[email protected]> wrote:
>
> In preparation to make memory operations accessible for a non
> ffa_driver/device, it is better to split the ffa_dev_ops into different
> categories of operations: info, message and memory. The info and memory
> are ffa_device independent and can be used without any associated
> ffa_device from a non ffa_driver.
>
> However, we don't export these info and memory APIs yet without the user.
> The first users of these APIs can export them.
>
> Signed-off-by: Sudeep Holla <[email protected]>
> ---
> drivers/firmware/arm_ffa/driver.c | 14 ++++++++++++-
> drivers/tee/optee/ffa_abi.c | 33 +++++++++++++++++--------------
> include/linux/arm_ffa.h | 14 ++++++++++++-
> 3 files changed, 44 insertions(+), 17 deletions(-)

Reviewed-by: Jens Wiklander <[email protected]>

>
> diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
> index 6822241168d6..d7a90c49fdcf 100644
> --- a/drivers/firmware/arm_ffa/driver.c
> +++ b/drivers/firmware/arm_ffa/driver.c
> @@ -676,16 +676,28 @@ static int ffa_memory_lend(struct ffa_mem_ops_args *args)
> return ffa_memory_ops(FFA_MEM_LEND, args);
> }
>
> -static const struct ffa_dev_ops ffa_ops = {
> +static const struct ffa_dev_info_ops ffa_info_ops = {
> .api_version_get = ffa_api_version_get,
> .partition_info_get = ffa_partition_info_get,
> +};
> +
> +static const struct ffa_dev_msg_ops ffa_msg_ops = {
> .mode_32bit_set = ffa_mode_32bit_set,
> .sync_send_receive = ffa_sync_send_receive,
> +};
> +
> +static const struct ffa_dev_mem_ops ffa_mem_ops = {
> .memory_reclaim = ffa_memory_reclaim,
> .memory_share = ffa_memory_share,
> .memory_lend = ffa_memory_lend,
> };
>
> +static const struct ffa_dev_ops ffa_ops = {
> + .info_ops = &ffa_info_ops,
> + .msg_ops = &ffa_msg_ops,
> + .mem_ops = &ffa_mem_ops,
> +};
> +
> void ffa_device_match_uuid(struct ffa_device *ffa_dev, const uuid_t *uuid)
> {
> int count, idx;
> diff --git a/drivers/tee/optee/ffa_abi.c b/drivers/tee/optee/ffa_abi.c
> index 7257b42d0545..bbc63fd145a1 100644
> --- a/drivers/tee/optee/ffa_abi.c
> +++ b/drivers/tee/optee/ffa_abi.c
> @@ -272,7 +272,7 @@ static int optee_ffa_shm_register(struct tee_context *ctx, struct tee_shm *shm,
> {
> struct optee *optee = tee_get_drvdata(ctx->teedev);
> struct ffa_device *ffa_dev = optee->ffa.ffa_dev;
> - const struct ffa_dev_ops *ffa_ops = ffa_dev->ops;
> + const struct ffa_dev_mem_ops *ffa_mem_ops = ffa_dev->ops->mem_ops;
> struct ffa_mem_region_attributes mem_attr = {
> .receiver = ffa_dev->vm_id,
> .attrs = FFA_MEM_RW,
> @@ -294,14 +294,14 @@ static int optee_ffa_shm_register(struct tee_context *ctx, struct tee_shm *shm,
> if (rc)
> return rc;
> args.sg = sgt.sgl;
> - rc = ffa_ops->memory_share(&args);
> + rc = ffa_mem_ops->memory_share(&args);
> sg_free_table(&sgt);
> if (rc)
> return rc;
>
> rc = optee_shm_add_ffa_handle(optee, shm, args.g_handle);
> if (rc) {
> - ffa_ops->memory_reclaim(args.g_handle, 0);
> + ffa_mem_ops->memory_reclaim(args.g_handle, 0);
> return rc;
> }
>
> @@ -315,7 +315,8 @@ static int optee_ffa_shm_unregister(struct tee_context *ctx,
> {
> struct optee *optee = tee_get_drvdata(ctx->teedev);
> struct ffa_device *ffa_dev = optee->ffa.ffa_dev;
> - const struct ffa_dev_ops *ffa_ops = ffa_dev->ops;
> + const struct ffa_dev_msg_ops *ffa_msg_ops = ffa_dev->ops->msg_ops;
> + const struct ffa_dev_mem_ops *ffa_mem_ops = ffa_dev->ops->mem_ops;
> u64 global_handle = shm->sec_world_id;
> struct ffa_send_direct_data data = {
> .data0 = OPTEE_FFA_UNREGISTER_SHM,
> @@ -327,11 +328,11 @@ static int optee_ffa_shm_unregister(struct tee_context *ctx,
> optee_shm_rem_ffa_handle(optee, global_handle);
> shm->sec_world_id = 0;
>
> - rc = ffa_ops->sync_send_receive(ffa_dev, &data);
> + rc = ffa_msg_ops->sync_send_receive(ffa_dev, &data);
> if (rc)
> pr_err("Unregister SHM id 0x%llx rc %d\n", global_handle, rc);
>
> - rc = ffa_ops->memory_reclaim(global_handle, 0);
> + rc = ffa_mem_ops->memory_reclaim(global_handle, 0);
> if (rc)
> pr_err("mem_reclaim: 0x%llx %d", global_handle, rc);
>
> @@ -342,7 +343,7 @@ static int optee_ffa_shm_unregister_supp(struct tee_context *ctx,
> struct tee_shm *shm)
> {
> struct optee *optee = tee_get_drvdata(ctx->teedev);
> - const struct ffa_dev_ops *ffa_ops = optee->ffa.ffa_dev->ops;
> + const struct ffa_dev_mem_ops *ffa_mem_ops;
> u64 global_handle = shm->sec_world_id;
> int rc;
>
> @@ -353,7 +354,8 @@ static int optee_ffa_shm_unregister_supp(struct tee_context *ctx,
> */
>
> optee_shm_rem_ffa_handle(optee, global_handle);
> - rc = ffa_ops->memory_reclaim(global_handle, 0);
> + ffa_mem_ops = optee->ffa.ffa_dev->ops->mem_ops;
> + rc = ffa_mem_ops->memory_reclaim(global_handle, 0);
> if (rc)
> pr_err("mem_reclaim: 0x%llx %d", global_handle, rc);
>
> @@ -530,7 +532,7 @@ static int optee_ffa_yielding_call(struct tee_context *ctx,
> {
> struct optee *optee = tee_get_drvdata(ctx->teedev);
> struct ffa_device *ffa_dev = optee->ffa.ffa_dev;
> - const struct ffa_dev_ops *ffa_ops = ffa_dev->ops;
> + const struct ffa_dev_msg_ops *ffa_msg_ops = ffa_dev->ops->msg_ops;
> struct optee_call_waiter w;
> u32 cmd = data->data0;
> u32 w4 = data->data1;
> @@ -541,7 +543,7 @@ static int optee_ffa_yielding_call(struct tee_context *ctx,
> /* Initialize waiter */
> optee_cq_wait_init(&optee->call_queue, &w);
> while (true) {
> - rc = ffa_ops->sync_send_receive(ffa_dev, data);
> + rc = ffa_msg_ops->sync_send_receive(ffa_dev, data);
> if (rc)
> goto done;
>
> @@ -576,7 +578,7 @@ static int optee_ffa_yielding_call(struct tee_context *ctx,
> * OP-TEE has returned with a RPC request.
> *
> * Note that data->data4 (passed in register w7) is already
> - * filled in by ffa_ops->sync_send_receive() returning
> + * filled in by ffa_mem_ops->sync_send_receive() returning
> * above.
> */
> cond_resched();
> @@ -654,12 +656,13 @@ static int optee_ffa_do_call_with_arg(struct tee_context *ctx,
> static bool optee_ffa_api_is_compatbile(struct ffa_device *ffa_dev,
> const struct ffa_dev_ops *ops)
> {
> + const struct ffa_dev_msg_ops *ffa_msg_ops = ops->msg_ops;
> struct ffa_send_direct_data data = { OPTEE_FFA_GET_API_VERSION };
> int rc;
>
> - ops->mode_32bit_set(ffa_dev);
> + ffa_msg_ops->mode_32bit_set(ffa_dev);
>
> - rc = ops->sync_send_receive(ffa_dev, &data);
> + rc = ffa_msg_ops->sync_send_receive(ffa_dev, &data);
> if (rc) {
> pr_err("Unexpected error %d\n", rc);
> return false;
> @@ -672,7 +675,7 @@ static bool optee_ffa_api_is_compatbile(struct ffa_device *ffa_dev,
> }
>
> data = (struct ffa_send_direct_data){ OPTEE_FFA_GET_OS_VERSION };
> - rc = ops->sync_send_receive(ffa_dev, &data);
> + rc = ffa_msg_ops->sync_send_receive(ffa_dev, &data);
> if (rc) {
> pr_err("Unexpected error %d\n", rc);
> return false;
> @@ -694,7 +697,7 @@ static bool optee_ffa_exchange_caps(struct ffa_device *ffa_dev,
> struct ffa_send_direct_data data = { OPTEE_FFA_EXCHANGE_CAPABILITIES };
> int rc;
>
> - rc = ops->sync_send_receive(ffa_dev, &data);
> + rc = ops->msg_ops->sync_send_receive(ffa_dev, &data);
> if (rc) {
> pr_err("Unexpected error %d", rc);
> return false;
> diff --git a/include/linux/arm_ffa.h b/include/linux/arm_ffa.h
> index b40afa7933dc..45e2b83d2364 100644
> --- a/include/linux/arm_ffa.h
> +++ b/include/linux/arm_ffa.h
> @@ -255,16 +255,28 @@ struct ffa_mem_ops_args {
> struct ffa_mem_region_attributes *attrs;
> };
>
> -struct ffa_dev_ops {
> +struct ffa_dev_info_ops {
> u32 (*api_version_get)(void);
> int (*partition_info_get)(const char *uuid_str,
> struct ffa_partition_info *buffer);
> +};
> +
> +struct ffa_dev_msg_ops {
> void (*mode_32bit_set)(struct ffa_device *dev);
> int (*sync_send_receive)(struct ffa_device *dev,
> struct ffa_send_direct_data *data);
> +};
> +
> +struct ffa_dev_mem_ops {
> int (*memory_reclaim)(u64 g_handle, u32 flags);
> int (*memory_share)(struct ffa_mem_ops_args *args);
> int (*memory_lend)(struct ffa_mem_ops_args *args);
> };
>
> +struct ffa_dev_ops {
> + const struct ffa_dev_info_ops *info_ops;
> + const struct ffa_dev_msg_ops *msg_ops;
> + const struct ffa_dev_mem_ops *mem_ops;
> +};
> +
> #endif /* _LINUX_ARM_FFA_H */
> --
> 2.37.2
>

2022-09-01 09:35:19

by Sudeep Holla

[permalink] [raw]
Subject: Re: [PATCH 7/9] firmware: arm_ffa: Add v1.1 get_partition_info support

On Thu, Sep 01, 2022 at 10:43:58AM +0200, Jens Wiklander wrote:
> On Tue, Aug 30, 2022 at 12:07 PM Sudeep Holla <[email protected]> wrote:
> >
> > FF-A v1.1 adds support to discovery the UUIDs of the partitions that was
> > missing in v1.0 and which the driver workarounds by using UUIDs supplied
> > by the ffa_drivers.
> >
> > Add the v1.1 get_partition_info support and disable the workaround if
> > the detected FF-A version is greater than v1.0.
> >
> > Signed-off-by: Sudeep Holla <[email protected]>
> > ---
> > drivers/firmware/arm_ffa/driver.c | 39 ++++++++++++++++++++++++-------
> > include/linux/arm_ffa.h | 1 +
> > 2 files changed, 32 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
> > index 5c8484b05c50..6822241168d6 100644
> > --- a/drivers/firmware/arm_ffa/driver.c
> > +++ b/drivers/firmware/arm_ffa/driver.c
> > @@ -264,18 +264,24 @@ static int ffa_rxtx_unmap(u16 vm_id)
> > return 0;
> > }
> >
> > +#define PARTITION_INFO_GET_RETURN_COUNT_ONLY BIT(0)
> > +
> > /* buffer must be sizeof(struct ffa_partition_info) * num_partitions */
> > static int
> > __ffa_partition_info_get(u32 uuid0, u32 uuid1, u32 uuid2, u32 uuid3,
> > struct ffa_partition_info *buffer, int num_partitions)
> > {
> > - int count;
> > + int idx, count, flags = 0, size;
> > ffa_value_t partition_info;
> >
> > + if (!buffer || !num_partitions) /* Just get the count for now */
> > + flags = PARTITION_INFO_GET_RETURN_COUNT_ONLY;
> > +
> > mutex_lock(&drv_info->rx_lock);
> > invoke_ffa_fn((ffa_value_t){
> > .a0 = FFA_PARTITION_INFO_GET,
> > .a1 = uuid0, .a2 = uuid1, .a3 = uuid2, .a4 = uuid3,
> > + .a5 = flags,
> > }, &partition_info);
> >
> > if (partition_info.a0 == FFA_ERROR) {
> > @@ -285,8 +291,15 @@ __ffa_partition_info_get(u32 uuid0, u32 uuid1, u32 uuid2, u32 uuid3,
> >
> > count = partition_info.a2;
> >
> > + if (drv_info->version > FFA_VERSION_1_0)
> > + size = partition_info.a3;
>
> What happens if this value is larger than sizeof(struct ffa_partition_info)?
>

Right, I had the check for both 0 size and size > struct size. I removed
both at once instead of just dropping 0 size. I assume 0 size is non
compliant or do you prefer to keep that check as well.

Thanks a lot for the reviews.

--
Regards,
Sudeep

2022-09-01 09:39:31

by Sudeep Holla

[permalink] [raw]
Subject: Re: [PATCH 4/9] firmware: arm_ffa: Add support for querying FF-A features

On Thu, Sep 01, 2022 at 09:56:41AM +0200, Jens Wiklander wrote:
> On Tue, Aug 30, 2022 at 12:07 PM Sudeep Holla <[email protected]> wrote:
> >
> > Add support for FFA_FEATURES to discover properties supported at the
> > FF-A interface. This interface can be used to query:
> > - If an FF-A interface is implemented by the component at the higher EL,
> > - If an implemented FF-A interface also implements any optional features
> > described in its interface definition, and
> > - Any implementation details exported by an implemented FF-A interface
> > as described in its interface definition.
> >
> > Signed-off-by: Sudeep Holla <[email protected]>
> > ---
> > drivers/firmware/arm_ffa/driver.c | 23 +++++++++++++++++++++++
> > 1 file changed, 23 insertions(+)
> >
> > diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
> > index 04e7cbb1b9aa..de94073f4109 100644
> > --- a/drivers/firmware/arm_ffa/driver.c
> > +++ b/drivers/firmware/arm_ffa/driver.c
> > @@ -571,6 +571,29 @@ static int ffa_memory_reclaim(u64 g_handle, u32 flags)
> > return 0;
> > }
> >
> > +static int ffa_features(u32 func_feat_id, u32 input_props, u32 *if_props)
> > +{
> > + ffa_value_t id;
> > +
> > + if (!ARM_SMCCC_IS_FAST_CALL(func_feat_id) && input_props) {
> > + pr_err("%s: Invalid Parameters: %x, %x", __func__,
> > + func_feat_id, input_props);
> > + return ffa_to_linux_errno(FFA_RET_INVALID_PARAMETERS);
> > + }
> > +
> > + invoke_ffa_fn((ffa_value_t){
> > + .a0 = FFA_FEATURES, .a1 = func_feat_id, .a2 = input_props,
> > + }, &id);
> > +
> > + if (id.a0 == FFA_ERROR)
> > + return ffa_to_linux_errno((int)id.a2);
> > +
> > + if (if_props)
> > + *if_props = id.a2;
>
> w3 (id.a3) also contains a value when querying for
> FFA_MEM_RETRIEVE_REQ. I see that in "[PATCH 5/9] firmware: arm_ffa:
> Use FFA_FEATURES to detect if native versions are supported" you're
> using this function with if_props = NULL. So I guess that at the
> moment we have more than needed, but in case you need to add another
> parameter to this function you'll need to update all the call sites
> too.
>

Right, I clearly missed to observe that. I am fine either way. Do you
have any preference ? As long as the callers are with this driver, it
shouldn't be much of an issue to change all, but happy to update to
accommodate w3 as well in v2.

--
Regards,
Sudeep

2022-09-01 09:39:55

by Jens Wiklander

[permalink] [raw]
Subject: Re: [PATCH 7/9] firmware: arm_ffa: Add v1.1 get_partition_info support

On Tue, Aug 30, 2022 at 12:07 PM Sudeep Holla <[email protected]> wrote:
>
> FF-A v1.1 adds support to discovery the UUIDs of the partitions that was
> missing in v1.0 and which the driver workarounds by using UUIDs supplied
> by the ffa_drivers.
>
> Add the v1.1 get_partition_info support and disable the workaround if
> the detected FF-A version is greater than v1.0.
>
> Signed-off-by: Sudeep Holla <[email protected]>
> ---
> drivers/firmware/arm_ffa/driver.c | 39 ++++++++++++++++++++++++-------
> include/linux/arm_ffa.h | 1 +
> 2 files changed, 32 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
> index 5c8484b05c50..6822241168d6 100644
> --- a/drivers/firmware/arm_ffa/driver.c
> +++ b/drivers/firmware/arm_ffa/driver.c
> @@ -264,18 +264,24 @@ static int ffa_rxtx_unmap(u16 vm_id)
> return 0;
> }
>
> +#define PARTITION_INFO_GET_RETURN_COUNT_ONLY BIT(0)
> +
> /* buffer must be sizeof(struct ffa_partition_info) * num_partitions */
> static int
> __ffa_partition_info_get(u32 uuid0, u32 uuid1, u32 uuid2, u32 uuid3,
> struct ffa_partition_info *buffer, int num_partitions)
> {
> - int count;
> + int idx, count, flags = 0, size;
> ffa_value_t partition_info;
>
> + if (!buffer || !num_partitions) /* Just get the count for now */
> + flags = PARTITION_INFO_GET_RETURN_COUNT_ONLY;
> +
> mutex_lock(&drv_info->rx_lock);
> invoke_ffa_fn((ffa_value_t){
> .a0 = FFA_PARTITION_INFO_GET,
> .a1 = uuid0, .a2 = uuid1, .a3 = uuid2, .a4 = uuid3,
> + .a5 = flags,
> }, &partition_info);
>
> if (partition_info.a0 == FFA_ERROR) {
> @@ -285,8 +291,15 @@ __ffa_partition_info_get(u32 uuid0, u32 uuid1, u32 uuid2, u32 uuid3,
>
> count = partition_info.a2;
>
> + if (drv_info->version > FFA_VERSION_1_0)
> + size = partition_info.a3;

What happens if this value is larger than sizeof(struct ffa_partition_info)?

> + else
> + size = 8; /* FFA_VERSION_1_0 lacks size in the response */
> +
> if (buffer && count <= num_partitions)
> - memcpy(buffer, drv_info->rx_buffer, sizeof(*buffer) * count);
> + for (idx = 0; idx < count; idx++)
> + memcpy(buffer + idx, drv_info->rx_buffer + idx * size,
> + size);
>
> ffa_rx_release();
>
> @@ -678,6 +691,14 @@ void ffa_device_match_uuid(struct ffa_device *ffa_dev, const uuid_t *uuid)
> int count, idx;
> struct ffa_partition_info *pbuf, *tpbuf;
>
> + /*
> + * FF-A v1.1 provides UUID for each partition as part of the discovery
> + * API, the discovered UUID must be populated in the device's UUID and
> + * there is no need to copy the same from the driver table.
> + */
> + if (drv_info->version > FFA_VERSION_1_0)
> + return;
> +
> count = ffa_partition_probe(uuid, &pbuf);
> if (count <= 0)
> return;
> @@ -691,6 +712,7 @@ void ffa_device_match_uuid(struct ffa_device *ffa_dev, const uuid_t *uuid)
> static void ffa_setup_partitions(void)
> {
> int count, idx;
> + uuid_t uuid;
> struct ffa_device *ffa_dev;
> struct ffa_partition_info *pbuf, *tpbuf;
>
> @@ -701,14 +723,15 @@ static void ffa_setup_partitions(void)
> }
>
> for (idx = 0, tpbuf = pbuf; idx < count; idx++, tpbuf++) {
> - /* Note that the &uuid_null parameter will require
> + import_uuid(&uuid, (u8 *)tpbuf->uuid);
> +
> + /* Note that the UUID will be uuid_null and that will require

Note that if the UUID is uuid_null, that will require ...

Cheers,
Jen s

> * ffa_device_match() to find the UUID of this partition id
> - * with help of ffa_device_match_uuid(). Once the FF-A spec
> - * is updated to provide correct UUID here for each partition
> - * as part of the discovery API, we need to pass the
> - * discovered UUID here instead.
> + * with help of ffa_device_match_uuid(). FF-A v1.1 and above
> + * provides UUID here for each partition as part of the
> + * discovery API and the same is passed.
> */
> - ffa_dev = ffa_device_register(&uuid_null, tpbuf->id, &ffa_ops);
> + ffa_dev = ffa_device_register(&uuid, tpbuf->id, &ffa_ops);
> if (!ffa_dev) {
> pr_err("%s: failed to register partition ID 0x%x\n",
> __func__, tpbuf->id);
> diff --git a/include/linux/arm_ffa.h b/include/linux/arm_ffa.h
> index eafab07c9f58..b40afa7933dc 100644
> --- a/include/linux/arm_ffa.h
> +++ b/include/linux/arm_ffa.h
> @@ -107,6 +107,7 @@ struct ffa_partition_info {
> /* partition can send and receive indirect messages. */
> #define FFA_PARTITION_INDIRECT_MSG BIT(2)
> u32 properties;
> + u32 uuid[4];
> };
>
> /* For use with FFA_MSG_SEND_DIRECT_{REQ,RESP} which pass data via registers */
> --
> 2.37.2
>

2022-09-01 10:15:35

by Jens Wiklander

[permalink] [raw]
Subject: Re: [PATCH 7/9] firmware: arm_ffa: Add v1.1 get_partition_info support

On Thu, Sep 1, 2022 at 10:58 AM Sudeep Holla <[email protected]> wrote:
>
> On Thu, Sep 01, 2022 at 10:43:58AM +0200, Jens Wiklander wrote:
> > On Tue, Aug 30, 2022 at 12:07 PM Sudeep Holla <[email protected]> wrote:
> > >
> > > FF-A v1.1 adds support to discovery the UUIDs of the partitions that was
> > > missing in v1.0 and which the driver workarounds by using UUIDs supplied
> > > by the ffa_drivers.
> > >
> > > Add the v1.1 get_partition_info support and disable the workaround if
> > > the detected FF-A version is greater than v1.0.
> > >
> > > Signed-off-by: Sudeep Holla <[email protected]>
> > > ---
> > > drivers/firmware/arm_ffa/driver.c | 39 ++++++++++++++++++++++++-------
> > > include/linux/arm_ffa.h | 1 +
> > > 2 files changed, 32 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
> > > index 5c8484b05c50..6822241168d6 100644
> > > --- a/drivers/firmware/arm_ffa/driver.c
> > > +++ b/drivers/firmware/arm_ffa/driver.c
> > > @@ -264,18 +264,24 @@ static int ffa_rxtx_unmap(u16 vm_id)
> > > return 0;
> > > }
> > >
> > > +#define PARTITION_INFO_GET_RETURN_COUNT_ONLY BIT(0)
> > > +
> > > /* buffer must be sizeof(struct ffa_partition_info) * num_partitions */
> > > static int
> > > __ffa_partition_info_get(u32 uuid0, u32 uuid1, u32 uuid2, u32 uuid3,
> > > struct ffa_partition_info *buffer, int num_partitions)
> > > {
> > > - int count;
> > > + int idx, count, flags = 0, size;
> > > ffa_value_t partition_info;
> > >
> > > + if (!buffer || !num_partitions) /* Just get the count for now */
> > > + flags = PARTITION_INFO_GET_RETURN_COUNT_ONLY;
> > > +
> > > mutex_lock(&drv_info->rx_lock);
> > > invoke_ffa_fn((ffa_value_t){
> > > .a0 = FFA_PARTITION_INFO_GET,
> > > .a1 = uuid0, .a2 = uuid1, .a3 = uuid2, .a4 = uuid3,
> > > + .a5 = flags,
> > > }, &partition_info);
> > >
> > > if (partition_info.a0 == FFA_ERROR) {
> > > @@ -285,8 +291,15 @@ __ffa_partition_info_get(u32 uuid0, u32 uuid1, u32 uuid2, u32 uuid3,
> > >
> > > count = partition_info.a2;
> > >
> > > + if (drv_info->version > FFA_VERSION_1_0)
> > > + size = partition_info.a3;
> >
> > What happens if this value is larger than sizeof(struct ffa_partition_info)?
> >
>
> Right, I had the check for both 0 size and size > struct size. I removed
> both at once instead of just dropping 0 size. I assume 0 size is non
> compliant or do you prefer to keep that check as well.

The purpose with this size field is to be able to read the known part
of each element in the array.
So I believe that memcpy() below should copy MIN(size, sizeof(struct
ffa_partition_info)), assuming that we update struct
ffa_partition_info if we need to when bumping our reported FF-A
version.

I'm not sure if we need more checks for compliance or not.

Cheers,
Jens

>
> Thanks a lot for the reviews.
>
> --
> Regards,
> Sudeep

2022-09-01 12:28:07

by Jens Wiklander

[permalink] [raw]
Subject: Re: [PATCH 4/9] firmware: arm_ffa: Add support for querying FF-A features

On Thu, Sep 1, 2022 at 11:04 AM Sudeep Holla <[email protected]> wrote:
>
> On Thu, Sep 01, 2022 at 09:56:41AM +0200, Jens Wiklander wrote:
> > On Tue, Aug 30, 2022 at 12:07 PM Sudeep Holla <[email protected]> wrote:
> > >
> > > Add support for FFA_FEATURES to discover properties supported at the
> > > FF-A interface. This interface can be used to query:
> > > - If an FF-A interface is implemented by the component at the higher EL,
> > > - If an implemented FF-A interface also implements any optional features
> > > described in its interface definition, and
> > > - Any implementation details exported by an implemented FF-A interface
> > > as described in its interface definition.
> > >
> > > Signed-off-by: Sudeep Holla <[email protected]>
> > > ---
> > > drivers/firmware/arm_ffa/driver.c | 23 +++++++++++++++++++++++
> > > 1 file changed, 23 insertions(+)
> > >
> > > diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
> > > index 04e7cbb1b9aa..de94073f4109 100644
> > > --- a/drivers/firmware/arm_ffa/driver.c
> > > +++ b/drivers/firmware/arm_ffa/driver.c
> > > @@ -571,6 +571,29 @@ static int ffa_memory_reclaim(u64 g_handle, u32 flags)
> > > return 0;
> > > }
> > >
> > > +static int ffa_features(u32 func_feat_id, u32 input_props, u32 *if_props)
> > > +{
> > > + ffa_value_t id;
> > > +
> > > + if (!ARM_SMCCC_IS_FAST_CALL(func_feat_id) && input_props) {
> > > + pr_err("%s: Invalid Parameters: %x, %x", __func__,
> > > + func_feat_id, input_props);
> > > + return ffa_to_linux_errno(FFA_RET_INVALID_PARAMETERS);
> > > + }
> > > +
> > > + invoke_ffa_fn((ffa_value_t){
> > > + .a0 = FFA_FEATURES, .a1 = func_feat_id, .a2 = input_props,
> > > + }, &id);
> > > +
> > > + if (id.a0 == FFA_ERROR)
> > > + return ffa_to_linux_errno((int)id.a2);
> > > +
> > > + if (if_props)
> > > + *if_props = id.a2;
> >
> > w3 (id.a3) also contains a value when querying for
> > FFA_MEM_RETRIEVE_REQ. I see that in "[PATCH 5/9] firmware: arm_ffa:
> > Use FFA_FEATURES to detect if native versions are supported" you're
> > using this function with if_props = NULL. So I guess that at the
> > moment we have more than needed, but in case you need to add another
> > parameter to this function you'll need to update all the call sites
> > too.
> >
>
> Right, I clearly missed to observe that. I am fine either way. Do you
> have any preference ? As long as the callers are with this driver, it
> shouldn't be much of an issue to change all, but happy to update to
> accommodate w3 as well in v2.

Either way is fine, it was more of an observation.

Cheers,
Jens