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(-)
Currently ffa_dev_ops_get() is the way to fetch the ffa_dev_ops pointer. It checks if the ffa_dev structure pointer is valid before returning the ffa_dev_ops pointer.
Instead, the pointer can be made part of the ffa_dev structure and since the core driver is incharge of creating ffa_device for each identified partition, there is no need to check for the validity explicitly if the pointer is embedded in the structure.
Add the pointer to the ffa_dev_ops in the ffa_dev structure itself and initialise the same as part of creation of the device.
Signed-off-by: Sudeep Holla sudeep.holla@arm.com --- drivers/firmware/arm_ffa/bus.c | 4 +++- drivers/firmware/arm_ffa/driver.c | 2 +- include/linux/arm_ffa.h | 7 +++++-- 3 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/firmware/arm_ffa/bus.c b/drivers/firmware/arm_ffa/bus.c index 641a91819088..69328041fbc3 100644 --- a/drivers/firmware/arm_ffa/bus.c +++ b/drivers/firmware/arm_ffa/bus.c @@ -167,7 +167,8 @@ bool ffa_device_is_valid(struct ffa_device *ffa_dev) return valid; }
-struct ffa_device *ffa_device_register(const uuid_t *uuid, int vm_id) +struct ffa_device *ffa_device_register(const uuid_t *uuid, int vm_id, + const struct ffa_dev_ops *ops) { int ret; struct device *dev; @@ -183,6 +184,7 @@ struct ffa_device *ffa_device_register(const uuid_t *uuid, int vm_id) dev_set_name(&ffa_dev->dev, "arm-ffa-%04x", vm_id);
ffa_dev->vm_id = vm_id; + ffa_dev->ops = ops; uuid_copy(&ffa_dev->uuid, uuid);
ret = device_register(&ffa_dev->dev); diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c index ec731e9e942b..213665e5ad0e 100644 --- a/drivers/firmware/arm_ffa/driver.c +++ b/drivers/firmware/arm_ffa/driver.c @@ -688,7 +688,7 @@ static void ffa_setup_partitions(void) * as part of the discovery API, we need to pass the * discovered UUID here instead. */ - ffa_dev = ffa_device_register(&uuid_null, tpbuf->id); + ffa_dev = ffa_device_register(&uuid_null, 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 e5c76c1ef9ed..91b47e42b73d 100644 --- a/include/linux/arm_ffa.h +++ b/include/linux/arm_ffa.h @@ -17,6 +17,7 @@ struct ffa_device { bool mode_32bit; uuid_t uuid; struct device dev; + const struct ffa_dev_ops *ops; };
#define to_ffa_dev(d) container_of(d, struct ffa_device, dev) @@ -47,7 +48,8 @@ static inline void *ffa_dev_get_drvdata(struct ffa_device *fdev) }
#if IS_REACHABLE(CONFIG_ARM_FFA_TRANSPORT) -struct ffa_device *ffa_device_register(const uuid_t *uuid, int vm_id); +struct ffa_device *ffa_device_register(const uuid_t *uuid, int vm_id, + const struct ffa_dev_ops *ops); void ffa_device_unregister(struct ffa_device *ffa_dev); int ffa_driver_register(struct ffa_driver *driver, struct module *owner, const char *mod_name); @@ -57,7 +59,8 @@ const struct ffa_dev_ops *ffa_dev_ops_get(struct ffa_device *dev);
#else static inline -struct ffa_device *ffa_device_register(const uuid_t *uuid, int vm_id) +struct ffa_device *ffa_device_register(const uuid_t *uuid, int vm_id, + const struct ffa_dev_ops *ops) { return NULL; }
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 sudeep.holla@arm.com --- 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;
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 sudeep.holla@arm.com --- 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) \
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 sudeep.holla@arm.com --- 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;
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 sudeep.holla@arm.com --- 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)
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 sudeep.holla@arm.com --- 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 */
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 sudeep.holla@arm.com --- 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 */
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 sudeep.holla@arm.com --- 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;
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 sudeep.holla@arm.com --- 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 */
On Tue, 30 Aug 2022 at 15:38, Sudeep Holla sudeep.holla@arm.com 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 sudeep.holla@arm.com
drivers/tee/optee/ffa_abi.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
Reviewed-by: Sumit Garg sumit.garg@linaro.org
-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
Hi Sudeep,
On Tue, 30 Aug 2022 at 15:39, Sudeep Holla sudeep.holla@arm.com 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 sudeep.holla@arm.com
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
On Tue, 30 Aug 2022 at 15:40, Sudeep Holla sudeep.holla@arm.com 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 sudeep.holla@arm.com
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
On Wed, Aug 31, 2022 at 04:28:01PM +0530, Sumit Garg wrote:
On Tue, 30 Aug 2022 at 15:40, Sudeep Holla sudeep.holla@arm.com 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 sudeep.holla@arm.com
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.
On Wed, 31 Aug 2022 at 16:57, Sudeep Holla sudeep.holla@arm.com wrote:
On Wed, Aug 31, 2022 at 04:28:01PM +0530, Sumit Garg wrote:
On Tue, 30 Aug 2022 at 15:40, Sudeep Holla sudeep.holla@arm.com 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 sudeep.holla@arm.com
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
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 sudeep.holla@arm.com 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 sudeep.holla@arm.com
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.
On Tue, Aug 30, 2022 at 12:07 PM Sudeep Holla sudeep.holla@arm.com 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 sudeep.holla@arm.com
drivers/tee/optee/ffa_abi.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
Reviewed-by: Jens Wiklander jens.wiklander@linaro.org
On Wed, Aug 31, 2022 at 1:29 PM Sumit Garg sumit.garg@linaro.org wrote:
On Wed, 31 Aug 2022 at 16:57, Sudeep Holla sudeep.holla@arm.com wrote:
On Wed, Aug 31, 2022 at 04:28:01PM +0530, Sumit Garg wrote:
On Tue, 30 Aug 2022 at 15:40, Sudeep Holla sudeep.holla@arm.com 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 sudeep.holla@arm.com
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
On Tue, Aug 30, 2022 at 12:07 PM Sudeep Holla sudeep.holla@arm.com 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 sudeep.holla@arm.com
drivers/firmware/arm_ffa/driver.c | 9 --------- include/linux/arm_ffa.h | 6 ------ 2 files changed, 15 deletions(-)
Reviewed-by: Jens Wiklander jens.wiklander@linaro.org
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
On Tue, Aug 30, 2022 at 12:07 PM Sudeep Holla sudeep.holla@arm.com wrote:
Currently ffa_dev_ops_get() is the way to fetch the ffa_dev_ops pointer. It checks if the ffa_dev structure pointer is valid before returning the ffa_dev_ops pointer.
Instead, the pointer can be made part of the ffa_dev structure and since the core driver is incharge of creating ffa_device for each identified partition, there is no need to check for the validity explicitly if the pointer is embedded in the structure.
Add the pointer to the ffa_dev_ops in the ffa_dev structure itself and initialise the same as part of creation of the device.
Signed-off-by: Sudeep Holla sudeep.holla@arm.com
drivers/firmware/arm_ffa/bus.c | 4 +++- drivers/firmware/arm_ffa/driver.c | 2 +- include/linux/arm_ffa.h | 7 +++++-- 3 files changed, 9 insertions(+), 4 deletions(-)
Reviewed-by: Jens Wiklander jens.wiklander@linaro.org
diff --git a/drivers/firmware/arm_ffa/bus.c b/drivers/firmware/arm_ffa/bus.c index 641a91819088..69328041fbc3 100644 --- a/drivers/firmware/arm_ffa/bus.c +++ b/drivers/firmware/arm_ffa/bus.c @@ -167,7 +167,8 @@ bool ffa_device_is_valid(struct ffa_device *ffa_dev) return valid; }
-struct ffa_device *ffa_device_register(const uuid_t *uuid, int vm_id) +struct ffa_device *ffa_device_register(const uuid_t *uuid, int vm_id,
const struct ffa_dev_ops *ops){ int ret; struct device *dev; @@ -183,6 +184,7 @@ struct ffa_device *ffa_device_register(const uuid_t *uuid, int vm_id) dev_set_name(&ffa_dev->dev, "arm-ffa-%04x", vm_id);
ffa_dev->vm_id = vm_id;
ffa_dev->ops = ops; uuid_copy(&ffa_dev->uuid, uuid); ret = device_register(&ffa_dev->dev);diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c index ec731e9e942b..213665e5ad0e 100644 --- a/drivers/firmware/arm_ffa/driver.c +++ b/drivers/firmware/arm_ffa/driver.c @@ -688,7 +688,7 @@ static void ffa_setup_partitions(void) * as part of the discovery API, we need to pass the * discovered UUID here instead. */
ffa_dev = ffa_device_register(&uuid_null, tpbuf->id);
ffa_dev = ffa_device_register(&uuid_null, 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 e5c76c1ef9ed..91b47e42b73d 100644 --- a/include/linux/arm_ffa.h +++ b/include/linux/arm_ffa.h @@ -17,6 +17,7 @@ struct ffa_device { bool mode_32bit; uuid_t uuid; struct device dev;
const struct ffa_dev_ops *ops;};
#define to_ffa_dev(d) container_of(d, struct ffa_device, dev) @@ -47,7 +48,8 @@ static inline void *ffa_dev_get_drvdata(struct ffa_device *fdev) }
#if IS_REACHABLE(CONFIG_ARM_FFA_TRANSPORT) -struct ffa_device *ffa_device_register(const uuid_t *uuid, int vm_id); +struct ffa_device *ffa_device_register(const uuid_t *uuid, int vm_id,
const struct ffa_dev_ops *ops);void ffa_device_unregister(struct ffa_device *ffa_dev); int ffa_driver_register(struct ffa_driver *driver, struct module *owner, const char *mod_name); @@ -57,7 +59,8 @@ const struct ffa_dev_ops *ffa_dev_ops_get(struct ffa_device *dev);
#else static inline -struct ffa_device *ffa_device_register(const uuid_t *uuid, int vm_id) +struct ffa_device *ffa_device_register(const uuid_t *uuid, int vm_id,
const struct ffa_dev_ops *ops){ return NULL; } -- 2.37.2
On Tue, Aug 30, 2022 at 12:07 PM Sudeep Holla sudeep.holla@arm.com 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 sudeep.holla@arm.com
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
On Tue, Aug 30, 2022 at 12:07 PM Sudeep Holla sudeep.holla@arm.com 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 sudeep.holla@arm.com
drivers/firmware/arm_ffa/driver.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-)
Reviewed-by: Jens Wiklander jens.wiklander@linaro.org
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
On Tue, Aug 30, 2022 at 12:07 PM Sudeep Holla sudeep.holla@arm.com 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 sudeep.holla@arm.com
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)?
elsesize = 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
On Tue, Aug 30, 2022 at 12:07 PM Sudeep Holla sudeep.holla@arm.com 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 sudeep.holla@arm.com
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 jens.wiklander@linaro.org
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
On Thu, Sep 01, 2022 at 10:43:58AM +0200, Jens Wiklander wrote:
On Tue, Aug 30, 2022 at 12:07 PM Sudeep Holla sudeep.holla@arm.com 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 sudeep.holla@arm.com
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.
On Thu, Sep 01, 2022 at 09:56:41AM +0200, Jens Wiklander wrote:
On Tue, Aug 30, 2022 at 12:07 PM Sudeep Holla sudeep.holla@arm.com 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 sudeep.holla@arm.com
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
On Thu, Sep 1, 2022 at 10:58 AM Sudeep Holla sudeep.holla@arm.com 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 sudeep.holla@arm.com 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 sudeep.holla@arm.com
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
On Thu, Sep 1, 2022 at 11:04 AM Sudeep Holla sudeep.holla@arm.com 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 sudeep.holla@arm.com 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 sudeep.holla@arm.com
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
op-tee@lists.trustedfirmware.org
-
Jens Wiklander -
Sudeep Holla -
Sumit Garg