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
v1[1]->v2:
- Merged dropping of ffa_ops in optee_ffa structure and using
ffa_dev->ops into single patch
- Added separate patch(didn't fit any patch strictly to fit in)
to rename ffa_dev_ops as ffa_ops as suggested by Sumit
- Fixed some minor comments, handling size > structure size in
partition_info_get and added extra parameter to ffa_features
to get both possible output/interface properties.
[1] https://lore.kernel.org/all/[email protected]/
Sudeep Holla (10):
firmware: arm_ffa: Add pointer to the ffa_dev_ops in struct ffa_dev
tee: optee: Drop ffa_ops in optee_ffa structure using 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: Rename ffa_dev_ops as ffa_ops
firmware: arm_ffa: Add v1.1 get_partition_info support
firmware: arm_ffa: Set up 32bit execution mode flag using partiion property
firmware: arm_ffa: Split up ffa_ops into info, message and memory operations
drivers/firmware/arm_ffa/bus.c | 4 +-
drivers/firmware/arm_ffa/driver.c | 130 +++++++++++++++++++++++-------
drivers/tee/optee/ffa_abi.c | 46 +++++------
drivers/tee/optee/optee_private.h | 1 -
include/linux/arm_ffa.h | 36 ++++++---
5 files changed, 150 insertions(+), 67 deletions(-)
--
2.37.3
FF-A v1.1 adds support to discovery the UUIDs of the partitions that was
missing in v1.0 and which the driver workarounds by using UUIDs supplied
by the ffa_drivers.
Add the v1.1 get_partition_info support and disable the workaround if
the detected FF-A version is greater than v1.0.
Signed-off-by: Sudeep Holla <[email protected]>
---
drivers/firmware/arm_ffa/driver.c | 42 +++++++++++++++++++++++++------
include/linux/arm_ffa.h | 1 +
2 files changed, 35 insertions(+), 8 deletions(-)
diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
index 2532e0f16cc9..dd6ab2f81580 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,18 @@ __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;
+ if (size > sizeof(*buffer))
+ size = sizeof(*buffer);
+ } 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();
@@ -681,6 +697,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;
@@ -694,6 +718,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;
@@ -704,14 +729,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 if the UUID will be uuid_null, 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 4c4b06783035..09567ffd1f49 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.3
FF-A v1.1 adds a flag in the partition properties to indicate if the
partition runs in the AArch32 or AArch64 execution state. Use the same
to set-up the 32-bit execution flag mode in the ffa_dev automatically
and ignore any requests to do the same from the ffa_driver.
Signed-off-by: Sudeep Holla <[email protected]>
---
drivers/firmware/arm_ffa/driver.c | 13 ++++++++++++-
include/linux/arm_ffa.h | 2 ++
2 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
index dd6ab2f81580..dbbe15592173 100644
--- a/drivers/firmware/arm_ffa/driver.c
+++ b/drivers/firmware/arm_ffa/driver.c
@@ -647,11 +647,19 @@ static int ffa_partition_info_get(const char *uuid_str,
return 0;
}
-static void ffa_mode_32bit_set(struct ffa_device *dev)
+static void _ffa_mode_32bit_set(struct ffa_device *dev)
{
dev->mode_32bit = true;
}
+static void ffa_mode_32bit_set(struct ffa_device *dev)
+{
+ if (drv_info->version > FFA_VERSION_1_0)
+ return;
+
+ _ffa_mode_32bit_set(dev);
+}
+
static int ffa_sync_send_receive(struct ffa_device *dev,
struct ffa_send_direct_data *data)
{
@@ -743,6 +751,9 @@ static void ffa_setup_partitions(void)
__func__, tpbuf->id);
continue;
}
+
+ if (tpbuf->properties & FFA_PARTITION_AARCH64_EXEC)
+ _ffa_mode_32bit_set(ffa_dev);
}
kfree(pbuf);
}
diff --git a/include/linux/arm_ffa.h b/include/linux/arm_ffa.h
index 09567ffd1f49..5964b6104996 100644
--- a/include/linux/arm_ffa.h
+++ b/include/linux/arm_ffa.h
@@ -106,6 +106,8 @@ struct ffa_partition_info {
#define FFA_PARTITION_DIRECT_SEND BIT(1)
/* partition can send and receive indirect messages. */
#define FFA_PARTITION_INDIRECT_MSG BIT(2)
+/* partition runs in the AArch64 execution state. */
+#define FFA_PARTITION_AARCH64_EXEC BIT(8)
u32 properties;
u32 uuid[4];
};
--
2.37.3
Hi Jens,
On Fri, Sep 02, 2022 at 01:40:22PM +0100, Sudeep Holla wrote:
> 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.
>
Thanks for the review of v1. There are some trivial changes that touches
optee files in v2 that needs official ack/review from you to take it
via my tree for v6.1. Please provide the same if you are OK especially
for patch 06,07/10 and even others if possible.
--
Regards,
Sudeep
On Fri, Sep 2, 2022 at 2:40 PM Sudeep Holla <[email protected]> wrote:
>
> FF-A v1.1 adds support to discovery the UUIDs of the partitions that was
> missing in v1.0 and which the driver workarounds by using UUIDs supplied
> by the ffa_drivers.
>
> Add the v1.1 get_partition_info support and disable the workaround if
> the detected FF-A version is greater than v1.0.
>
> Signed-off-by: Sudeep Holla <[email protected]>
> ---
> drivers/firmware/arm_ffa/driver.c | 42 +++++++++++++++++++++++++------
> include/linux/arm_ffa.h | 1 +
> 2 files changed, 35 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
> index 2532e0f16cc9..dd6ab2f81580 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,18 @@ __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;
> + if (size > sizeof(*buffer))
> + size = sizeof(*buffer);
Below when calculating the address in the source buffer with
"drv_info->rx_buffer + idx * size" you should use the size from
partition_info.a3 even if it's larger than sizeof(*buffer). The amount
of bytes to copy looks correct though. Does that make sense?
Cheers,
Jens
> + } 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();
>
> @@ -681,6 +697,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;
> @@ -694,6 +718,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;
>
> @@ -704,14 +729,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 if the UUID will be uuid_null, 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 4c4b06783035..09567ffd1f49 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.3
>
On Fri, Sep 2, 2022 at 2:40 PM Sudeep Holla <[email protected]> wrote:
>
> FF-A v1.1 adds a flag in the partition properties to indicate if the
> partition runs in the AArch32 or AArch64 execution state. Use the same
> to set-up the 32-bit execution flag mode in the ffa_dev automatically
> and ignore any requests to do the same from the ffa_driver.
>
> Signed-off-by: Sudeep Holla <[email protected]>
> ---
> drivers/firmware/arm_ffa/driver.c | 13 ++++++++++++-
> include/linux/arm_ffa.h | 2 ++
> 2 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
> index dd6ab2f81580..dbbe15592173 100644
> --- a/drivers/firmware/arm_ffa/driver.c
> +++ b/drivers/firmware/arm_ffa/driver.c
> @@ -647,11 +647,19 @@ static int ffa_partition_info_get(const char *uuid_str,
> return 0;
> }
>
> -static void ffa_mode_32bit_set(struct ffa_device *dev)
> +static void _ffa_mode_32bit_set(struct ffa_device *dev)
> {
> dev->mode_32bit = true;
> }
>
> +static void ffa_mode_32bit_set(struct ffa_device *dev)
> +{
> + if (drv_info->version > FFA_VERSION_1_0)
> + return;
> +
> + _ffa_mode_32bit_set(dev);
> +}
> +
> static int ffa_sync_send_receive(struct ffa_device *dev,
> struct ffa_send_direct_data *data)
> {
> @@ -743,6 +751,9 @@ static void ffa_setup_partitions(void)
> __func__, tpbuf->id);
> continue;
> }
> +
> + if (tpbuf->properties & FFA_PARTITION_AARCH64_EXEC)
Shouldn't this be !(tpbuf->properties & FFA_PARTITION_AARCH64_EXEC)?
Before this, we used 64-bit calling convention by default for FF-A
v1.0. I suppose we would like to keep that behaviour.
Cheers,
Jens
> + _ffa_mode_32bit_set(ffa_dev);
> }
> kfree(pbuf);
> }
> diff --git a/include/linux/arm_ffa.h b/include/linux/arm_ffa.h
> index 09567ffd1f49..5964b6104996 100644
> --- a/include/linux/arm_ffa.h
> +++ b/include/linux/arm_ffa.h
> @@ -106,6 +106,8 @@ struct ffa_partition_info {
> #define FFA_PARTITION_DIRECT_SEND BIT(1)
> /* partition can send and receive indirect messages. */
> #define FFA_PARTITION_INDIRECT_MSG BIT(2)
> +/* partition runs in the AArch64 execution state. */
> +#define FFA_PARTITION_AARCH64_EXEC BIT(8)
> u32 properties;
> u32 uuid[4];
> };
> --
> 2.37.3
>
On Wed, Sep 07, 2022 at 11:01:27AM +0200, Jens Wiklander wrote:
> On Fri, Sep 2, 2022 at 2:40 PM Sudeep Holla <[email protected]> wrote:
> >
> > FF-A v1.1 adds a flag in the partition properties to indicate if the
> > partition runs in the AArch32 or AArch64 execution state. Use the same
> > to set-up the 32-bit execution flag mode in the ffa_dev automatically
> > and ignore any requests to do the same from the ffa_driver.
> >
> > Signed-off-by: Sudeep Holla <[email protected]>
> > ---
> > drivers/firmware/arm_ffa/driver.c | 13 ++++++++++++-
> > include/linux/arm_ffa.h | 2 ++
> > 2 files changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
> > index dd6ab2f81580..dbbe15592173 100644
> > --- a/drivers/firmware/arm_ffa/driver.c
> > +++ b/drivers/firmware/arm_ffa/driver.c
> > @@ -647,11 +647,19 @@ static int ffa_partition_info_get(const char *uuid_str,
> > return 0;
> > }
> >
> > -static void ffa_mode_32bit_set(struct ffa_device *dev)
> > +static void _ffa_mode_32bit_set(struct ffa_device *dev)
> > {
> > dev->mode_32bit = true;
> > }
> >
> > +static void ffa_mode_32bit_set(struct ffa_device *dev)
> > +{
> > + if (drv_info->version > FFA_VERSION_1_0)
> > + return;
> > +
> > + _ffa_mode_32bit_set(dev);
> > +}
> > +
> > static int ffa_sync_send_receive(struct ffa_device *dev,
> > struct ffa_send_direct_data *data)
> > {
> > @@ -743,6 +751,9 @@ static void ffa_setup_partitions(void)
> > __func__, tpbuf->id);
> > continue;
> > }
> > +
> > + if (tpbuf->properties & FFA_PARTITION_AARCH64_EXEC)
>
> Shouldn't this be !(tpbuf->properties & FFA_PARTITION_AARCH64_EXEC)?
>
> Before this, we used 64-bit calling convention by default for FF-A
> v1.0. I suppose we would like to keep that behaviour.
>
Correct, it is stupid mistake on my part. I did notice this but somehow
messed up not to add the change I made to test with OPTEE. Thanks for
noticing this.
--
Regards,
Sudeep
On Wed, Sep 07, 2022 at 10:38:05AM +0200, Jens Wiklander wrote:
> On Fri, Sep 2, 2022 at 2:40 PM Sudeep Holla <[email protected]> wrote:
> >
> > FF-A v1.1 adds support to discovery the UUIDs of the partitions that was
> > missing in v1.0 and which the driver workarounds by using UUIDs supplied
> > by the ffa_drivers.
> >
> > Add the v1.1 get_partition_info support and disable the workaround if
> > the detected FF-A version is greater than v1.0.
> >
> > Signed-off-by: Sudeep Holla <[email protected]>
> > ---
> > drivers/firmware/arm_ffa/driver.c | 42 +++++++++++++++++++++++++------
> > include/linux/arm_ffa.h | 1 +
> > 2 files changed, 35 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
> > index 2532e0f16cc9..dd6ab2f81580 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,18 @@ __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;
> > + if (size > sizeof(*buffer))
> > + size = sizeof(*buffer);
>
> Below when calculating the address in the source buffer with
> "drv_info->rx_buffer + idx * size" you should use the size from
> partition_info.a3 even if it's larger than sizeof(*buffer). The amount
> of bytes to copy looks correct though. Does that make sense?
>
Good spot, I missed to see that. It makes complete sense. I will fix it.
--
Regards,
Sudeep