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

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

[Sudeep Holla]

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

[PATCH 1/9] firmware: arm_ffa: Add pointer to the ffa_dev_ops in struct ffa_dev

[Sudeep Holla]

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;
 }
-- 
2.37.2

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

[Sudeep Holla]

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;
-- 
2.37.2

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

[Sudeep Holla]

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) \
-- 
2.37.2

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

[Sudeep Holla]

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;
-- 
2.37.2

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

[Sudeep Holla]

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)
-- 
2.37.2

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

[Sudeep Holla]

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 */
-- 
2.37.2

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

[Sudeep Holla]

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 */
-- 
2.37.2

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

[Sudeep Holla]

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;
-- 
2.37.2

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

[Sudeep Holla]

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 */
-- 
2.37.2

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

[Sumit Garg]

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
>

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

[Sumit Garg]

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
>

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

[Sumit Garg]

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
>

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

[Sudeep Holla]

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.

-- 
Regards,
Sudeep

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

[Sumit Garg]

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

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

[Sudeep Holla]

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.

-- 
Regards,
Sudeep

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

[Jens Wiklander]

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>

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

[Jens Wiklander]

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

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

[Jens Wiklander]

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
>

Re: [PATCH 1/9] firmware: arm_ffa: Add pointer to the ffa_dev_ops in struct ffa_dev

[Jens Wiklander]

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
>

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

[Jens Wiklander]

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
>

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

[Jens Wiklander]

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
>

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

[Jens Wiklander]

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)?

> +       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
>

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

[Jens Wiklander]

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
>

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

[Sudeep Holla]

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.

-- 
Regards,
Sudeep

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

[Sudeep Holla]

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

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

[Jens Wiklander]

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

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

[Jens Wiklander]

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