[PATCH 0/4] firmware: arm_ffa: Support for MSG_SEND2 and minor harderning checks

[PATCH 0/4] firmware: arm_ffa: Support for MSG_SEND2 and minor harderning checks

[Sudeep Holla]

The series mainly adds support for FFA_MSG_SEND2. When trying to add
the support for the same, it was found that currently we don't check
the properties of the partitions before sending any direct messages
or registering notification callbacks.

Support for checking if the partition can receive notifications and
direct requests before sending the same can be added by stashing the
discovered partition properties for later query purposes.

Rx buffer full notification support is not yet added as the details of
the usecase are still be thought through.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
Sudeep Holla (4):
      firmware: arm_ffa: Stash the partition properties for query purposes
      firmware: arm_ffa: Check if receiving direct requests are supported before sending
      firmware: arm_ffa: Check if receiving notifications are supported before sending
      firmware: arm_ffa: Add support for FFA_MSG_SEND2

 drivers/firmware/arm_ffa/driver.c | 66 ++++++++++++++++++++++++++++++++++++++-
 include/linux/arm_ffa.h           | 11 +++++++
 2 files changed, 76 insertions(+), 1 deletion(-)
---
base-commit: 0bbac3facb5d6cc0171c45c9873a2dc96bea9680
change-id: 20240410-ffa_msg2_support-0a9767b399c0

Best regards,
-- 
Regards,
Sudeep


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/4] firmware: arm_ffa: Stash the partition properties for query purposes

[Sudeep Holla]

The properies obtained from the partition information descriptor as
part of initial partitions discovery is useful as it contain info
if the partition
- Runs in AArch64 or AArch32 execution state
- Can send and/or receive direct requests
- Can send and receive indirect message
- Does support receipt of notifications.

These can be used for querying before attempting to do any of the
above operations.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_ffa/driver.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
index 9bc2e10381af..d258d04c1eda 100644
--- a/drivers/firmware/arm_ffa/driver.c
+++ b/drivers/firmware/arm_ffa/driver.c
@@ -721,6 +721,7 @@ static int ffa_notification_get(u32 flags, struct ffa_notify_bitmaps *notify)
 }
 
 struct ffa_dev_part_info {
+	u32 properties;
 	ffa_sched_recv_cb callback;
 	void *cb_data;
 	rwlock_t rw_lock;
@@ -863,6 +864,17 @@ static void ffa_mode_32bit_set(struct ffa_device *dev)
 	dev->mode_32bit = true;
 }
 
+static bool ffa_partition_check_property(struct ffa_device *dev, u32 property)
+{
+	struct ffa_dev_part_info *partition;
+
+	partition = xa_load(&drv_info->partition_info, dev->vm_id);
+	if (!partition)
+		return false;
+
+	return partition->properties & property;
+}
+
 static int ffa_sync_send_receive(struct ffa_device *dev,
 				 struct ffa_send_direct_data *data)
 {
@@ -1236,6 +1248,8 @@ static int ffa_setup_partitions(void)
 			ffa_device_unregister(ffa_dev);
 			continue;
 		}
+
+		info->properties = tpbuf->properties;
 		rwlock_init(&info->rw_lock);
 		ret = xa_insert(&drv_info->partition_info, tpbuf->id,
 				info, GFP_KERNEL);

-- 
2.43.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/4] firmware: arm_ffa: Check if receiving direct requests are supported before sending

[Sudeep Holla]

Add check to see if the target partition can receive the direct requests
before sending any message to the partition.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_ffa/driver.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
index d258d04c1eda..52379885a403 100644
--- a/drivers/firmware/arm_ffa/driver.c
+++ b/drivers/firmware/arm_ffa/driver.c
@@ -875,9 +875,15 @@ static bool ffa_partition_check_property(struct ffa_device *dev, u32 property)
 	return partition->properties & property;
 }
 
+#define ffa_partition_supports_direct_recv(dev)	\
+	ffa_partition_check_property(dev, FFA_PARTITION_DIRECT_RECV)
+
 static int ffa_sync_send_receive(struct ffa_device *dev,
 				 struct ffa_send_direct_data *data)
 {
+	if (!ffa_partition_supports_direct_recv(dev))
+		return -EOPNOTSUPP;
+
 	return ffa_msg_send_direct_req(drv_info->vm_id, dev->vm_id,
 				       dev->mode_32bit, data);
 }

-- 
2.43.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/4] firmware: arm_ffa: Check if receiving direct requests are supported before sending

[Jens Wiklander]

On Mon, Apr 15, 2024 at 6:05 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> Add check to see if the target partition can receive the direct requests
> before sending any message to the partition.
>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/firmware/arm_ffa/driver.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
> index d258d04c1eda..52379885a403 100644
> --- a/drivers/firmware/arm_ffa/driver.c
> +++ b/drivers/firmware/arm_ffa/driver.c
> @@ -875,9 +875,15 @@ static bool ffa_partition_check_property(struct ffa_device *dev, u32 property)
>         return partition->properties & property;
>  }
>
> +#define ffa_partition_supports_direct_recv(dev)        \
> +       ffa_partition_check_property(dev, FFA_PARTITION_DIRECT_RECV)
> +
>  static int ffa_sync_send_receive(struct ffa_device *dev,
>                                  struct ffa_send_direct_data *data)
>  {
> +       if (!ffa_partition_supports_direct_recv(dev))
> +               return -EOPNOTSUPP;

This is done in the critical path of a direct request. Checking that a
partition supports direct requests makes sense before using it, but
doing it for each call in the critical path is excessive. There are
checks in the other layers to catch unsupported requests. Can we let
the caller do this check if desired? Omitting the check shouldn't lead
to any bad things.

Cheers,
Jens

> +
>         return ffa_msg_send_direct_req(drv_info->vm_id, dev->vm_id,
>                                        dev->mode_32bit, data);
>  }
>
> --
> 2.43.2
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/4] firmware: arm_ffa: Check if receiving notifications are supported before sending

[Sudeep Holla]

Add check to see if the target partition can receive the notifications
before sending any notifications to the partition.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_ffa/driver.c | 6 +++++-
 include/linux/arm_ffa.h           | 2 ++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
index 52379885a403..d5087e4f6d35 100644
--- a/drivers/firmware/arm_ffa/driver.c
+++ b/drivers/firmware/arm_ffa/driver.c
@@ -1092,12 +1092,16 @@ static int ffa_notify_request(struct ffa_device *dev, bool is_per_vcpu,
 	return rc;
 }
 
+#define ffa_partition_supports_notify_recv(dev)	\
+	ffa_partition_check_property(dev, FFA_PARTITION_NOTIFICATION_RECV)
+
 static int ffa_notify_send(struct ffa_device *dev, int notify_id,
 			   bool is_per_vcpu, u16 vcpu)
 {
 	u32 flags = 0;
 
-	if (ffa_notifications_disabled())
+	if (ffa_notifications_disabled() ||
+	    !ffa_partition_supports_notify_recv(dev))
 		return -EOPNOTSUPP;
 
 	if (is_per_vcpu)
diff --git a/include/linux/arm_ffa.h b/include/linux/arm_ffa.h
index c906f666ff5d..13830be5851d 100644
--- a/include/linux/arm_ffa.h
+++ b/include/linux/arm_ffa.h
@@ -221,6 +221,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 can receive notifications */
+#define FFA_PARTITION_NOTIFICATION_RECV	BIT(3)
 /* partition runs in the AArch64 execution state. */
 #define FFA_PARTITION_AARCH64_EXEC	BIT(8)
 	u32 properties;

-- 
2.43.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/4] firmware: arm_ffa: Check if receiving notifications are supported before sending

[Jens Wiklander]

On Mon, Apr 15, 2024 at 6:05 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> Add check to see if the target partition can receive the notifications
> before sending any notifications to the partition.
>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/firmware/arm_ffa/driver.c | 6 +++++-
>  include/linux/arm_ffa.h           | 2 ++
>  2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
> index 52379885a403..d5087e4f6d35 100644
> --- a/drivers/firmware/arm_ffa/driver.c
> +++ b/drivers/firmware/arm_ffa/driver.c
> @@ -1092,12 +1092,16 @@ static int ffa_notify_request(struct ffa_device *dev, bool is_per_vcpu,
>         return rc;
>  }
>
> +#define ffa_partition_supports_notify_recv(dev)        \
> +       ffa_partition_check_property(dev, FFA_PARTITION_NOTIFICATION_RECV)
> +
>  static int ffa_notify_send(struct ffa_device *dev, int notify_id,
>                            bool is_per_vcpu, u16 vcpu)
>  {
>         u32 flags = 0;
>
> -       if (ffa_notifications_disabled())
> +       if (ffa_notifications_disabled() ||
> +           !ffa_partition_supports_notify_recv(dev))
>                 return -EOPNOTSUPP;

A client shouldn't send random notifications. First, it communicates
with the partition to establish which notification ID it should use.
If that fails we should never reach this stage. Unexpected
notifications are caught in other layers.

Cheers,
Jens

>
>         if (is_per_vcpu)
> diff --git a/include/linux/arm_ffa.h b/include/linux/arm_ffa.h
> index c906f666ff5d..13830be5851d 100644
> --- a/include/linux/arm_ffa.h
> +++ b/include/linux/arm_ffa.h
> @@ -221,6 +221,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 can receive notifications */
> +#define FFA_PARTITION_NOTIFICATION_RECV        BIT(3)
>  /* partition runs in the AArch64 execution state. */
>  #define FFA_PARTITION_AARCH64_EXEC     BIT(8)
>         u32 properties;
>
> --
> 2.43.2
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/4] firmware: arm_ffa: Add support for FFA_MSG_SEND2

[Sudeep Holla]

The FFA_MSG_SEND2 can be  used to transmit a partition message from
the Tx buffer of the sender(the driver in this case) endpoint to the Rx
buffer of the receiver endpoint.

An invocation of the FFA_MSG_SEND2 transfers the ownership to the
receiver endpoint(or any intermediate consumer). Completion of an
FFA_MSG_SEND2 invocation transfers the ownership back to the sender
endpoint.

The framework defines the FFA_MSG_SEND2 interface to transmit a partition
message from the Tx buffer of the sender to the Rx buffer of a receiver
and inform the scheduler that the receiver must be run.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_ffa/driver.c | 40 +++++++++++++++++++++++++++++++++++++++
 include/linux/arm_ffa.h           |  9 +++++++++
 2 files changed, 49 insertions(+)

diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
index d5087e4f6d35..6c2602f7e7cc 100644
--- a/drivers/firmware/arm_ffa/driver.c
+++ b/drivers/firmware/arm_ffa/driver.c
@@ -344,6 +344,34 @@ static int ffa_msg_send_direct_req(u16 src_id, u16 dst_id, bool mode_32bit,
 	return -EINVAL;
 }
 
+static int ffa_msg_send2(u16 src_id, u16 dst_id, void *buf, size_t sz)
+{
+	u32 src_dst_ids = PACK_TARGET_INFO(src_id, dst_id);
+	struct ffa_indirect_msg_hdr *msg;
+	ffa_value_t ret;
+
+	mutex_lock(&drv_info->tx_lock);
+
+	msg = drv_info->tx_buffer;
+	msg->flags = 0;
+	msg->res0 = 0;
+	msg->offset = sizeof(*msg);
+	msg->send_recv_id = src_dst_ids;
+	msg->size = sz;
+	memcpy(msg + msg->offset, buf, sz);
+
+	/* flags = 0, sender VMID = 0 works for both physical/virtual NS */
+	invoke_ffa_fn((ffa_value_t){
+		      .a0 = FFA_MSG_SEND2, .a1 = 0, .a2 = 0
+		      }, &ret);
+
+	if (ret.a0 == FFA_ERROR)
+		return ffa_to_linux_errno((int)ret.a2);
+
+	mutex_lock(&drv_info->tx_lock);
+	return 0;
+}
+
 static int ffa_mem_first_frag(u32 func_id, phys_addr_t buf, u32 buf_sz,
 			      u32 frag_len, u32 len, u64 *handle)
 {
@@ -888,6 +916,17 @@ static int ffa_sync_send_receive(struct ffa_device *dev,
 				       dev->mode_32bit, data);
 }
 
+#define ffa_partition_supports_indirect_msg(dev)	\
+	ffa_partition_check_property(dev, FFA_PARTITION_INDIRECT_MSG)
+
+static int ffa_indirect_msg_send(struct ffa_device *dev, void *buf, size_t sz)
+{
+	if (!ffa_partition_supports_direct_recv(dev))
+		return -EOPNOTSUPP;
+
+	return ffa_msg_send2(drv_info->vm_id, dev->vm_id, buf, sz);
+}
+
 static int ffa_memory_share(struct ffa_mem_ops_args *args)
 {
 	if (drv_info->mem_ops_native)
@@ -1167,6 +1206,7 @@ static const struct ffa_info_ops ffa_drv_info_ops = {
 static const struct ffa_msg_ops ffa_drv_msg_ops = {
 	.mode_32bit_set = ffa_mode_32bit_set,
 	.sync_send_receive = ffa_sync_send_receive,
+	.indirect_send = ffa_indirect_msg_send,
 };
 
 static const struct ffa_mem_ops ffa_drv_mem_ops = {
diff --git a/include/linux/arm_ffa.h b/include/linux/arm_ffa.h
index 13830be5851d..d61a6df397f6 100644
--- a/include/linux/arm_ffa.h
+++ b/include/linux/arm_ffa.h
@@ -238,6 +238,14 @@ struct ffa_send_direct_data {
 	unsigned long data4; /* w7/x7 */
 };
 
+struct ffa_indirect_msg_hdr {
+	u32 flags;
+	u32 res0;
+	u32 offset;
+	u32 send_recv_id;
+	u32 size;
+};
+
 struct ffa_mem_region_addr_range {
 	/* The base IPA of the constituent memory region, aligned to 4 kiB */
 	u64 address;
@@ -398,6 +406,7 @@ struct ffa_msg_ops {
 	void (*mode_32bit_set)(struct ffa_device *dev);
 	int (*sync_send_receive)(struct ffa_device *dev,
 				 struct ffa_send_direct_data *data);
+	int (*indirect_send)(struct ffa_device *dev, void *buf, size_t sz);
 };
 
 struct ffa_mem_ops {

-- 
2.43.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/4] firmware: arm_ffa: Add support for FFA_MSG_SEND2

[Bertrand Marquis]

Hi Sudeep,

> On 15 Apr 2024, at 18:05, Sudeep Holla <sudeep.holla@arm.com> wrote:
> 
> The FFA_MSG_SEND2 can be  used to transmit a partition message from
> the Tx buffer of the sender(the driver in this case) endpoint to the Rx
> buffer of the receiver endpoint.
> 
> An invocation of the FFA_MSG_SEND2 transfers the ownership to the
> receiver endpoint(or any intermediate consumer). Completion of an
> FFA_MSG_SEND2 invocation transfers the ownership back to the sender
> endpoint.
> 
> The framework defines the FFA_MSG_SEND2 interface to transmit a partition
> message from the Tx buffer of the sender to the Rx buffer of a receiver
> and inform the scheduler that the receiver must be run.
> 
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
> drivers/firmware/arm_ffa/driver.c | 40 +++++++++++++++++++++++++++++++++++++++
> include/linux/arm_ffa.h           |  9 +++++++++
> 2 files changed, 49 insertions(+)
> 
> diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
> index d5087e4f6d35..6c2602f7e7cc 100644
> --- a/drivers/firmware/arm_ffa/driver.c
> +++ b/drivers/firmware/arm_ffa/driver.c
> @@ -344,6 +344,34 @@ static int ffa_msg_send_direct_req(u16 src_id, u16 dst_id, bool mode_32bit,
> return -EINVAL;
> }
> 
> +static int ffa_msg_send2(u16 src_id, u16 dst_id, void *buf, size_t sz)
> +{
> + u32 src_dst_ids = PACK_TARGET_INFO(src_id, dst_id);
> + struct ffa_indirect_msg_hdr *msg;
> + ffa_value_t ret;
> +
> + mutex_lock(&drv_info->tx_lock);
> +
> + msg = drv_info->tx_buffer;
> + msg->flags = 0;
> + msg->res0 = 0;
> + msg->offset = sizeof(*msg);
> + msg->send_recv_id = src_dst_ids;
> + msg->size = sz;
> + memcpy(msg + msg->offset, buf, sz);

Here there should be a check that the user is not trying to send more data
than what can fit in the TX Buffer.

Other than that LGTM.

Cheers
Bertrand

> +
> + /* flags = 0, sender VMID = 0 works for both physical/virtual NS */
> + invoke_ffa_fn((ffa_value_t){
> +      .a0 = FFA_MSG_SEND2, .a1 = 0, .a2 = 0
> +      }, &ret);
> +
> + if (ret.a0 == FFA_ERROR)
> + return ffa_to_linux_errno((int)ret.a2);
> +
> + mutex_lock(&drv_info->tx_lock);
> + return 0;
> +}
> +
> static int ffa_mem_first_frag(u32 func_id, phys_addr_t buf, u32 buf_sz,
>      u32 frag_len, u32 len, u64 *handle)
> {
> @@ -888,6 +916,17 @@ static int ffa_sync_send_receive(struct ffa_device *dev,
>       dev->mode_32bit, data);
> }
> 
> +#define ffa_partition_supports_indirect_msg(dev) \
> + ffa_partition_check_property(dev, FFA_PARTITION_INDIRECT_MSG)
> +
> +static int ffa_indirect_msg_send(struct ffa_device *dev, void *buf, size_t sz)
> +{
> + if (!ffa_partition_supports_direct_recv(dev))
> + return -EOPNOTSUPP;
> +
> + return ffa_msg_send2(drv_info->vm_id, dev->vm_id, buf, sz);
> +}
> +
> static int ffa_memory_share(struct ffa_mem_ops_args *args)
> {
> if (drv_info->mem_ops_native)
> @@ -1167,6 +1206,7 @@ static const struct ffa_info_ops ffa_drv_info_ops = {
> static const struct ffa_msg_ops ffa_drv_msg_ops = {
> .mode_32bit_set = ffa_mode_32bit_set,
> .sync_send_receive = ffa_sync_send_receive,
> + .indirect_send = ffa_indirect_msg_send,
> };
> 
> static const struct ffa_mem_ops ffa_drv_mem_ops = {
> diff --git a/include/linux/arm_ffa.h b/include/linux/arm_ffa.h
> index 13830be5851d..d61a6df397f6 100644
> --- a/include/linux/arm_ffa.h
> +++ b/include/linux/arm_ffa.h
> @@ -238,6 +238,14 @@ struct ffa_send_direct_data {
> unsigned long data4; /* w7/x7 */
> };
> 
> +struct ffa_indirect_msg_hdr {
> + u32 flags;
> + u32 res0;
> + u32 offset;
> + u32 send_recv_id;
> + u32 size;
> +};
> +
> struct ffa_mem_region_addr_range {
> /* The base IPA of the constituent memory region, aligned to 4 kiB */
> u64 address;
> @@ -398,6 +406,7 @@ struct ffa_msg_ops {
> void (*mode_32bit_set)(struct ffa_device *dev);
> int (*sync_send_receive)(struct ffa_device *dev,
> struct ffa_send_direct_data *data);
> + int (*indirect_send)(struct ffa_device *dev, void *buf, size_t sz);
> };
> 
> struct ffa_mem_ops {
> 
> -- 
> 2.43.2
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/4] firmware: arm_ffa: Add support for FFA_MSG_SEND2

[Sudeep Holla]

On Tue, Apr 16, 2024 at 08:26:55AM +0100, Bertrand Marquis wrote:
> Hi Sudeep,
> 
> > On 15 Apr 2024, at 18:05, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > The FFA_MSG_SEND2 can be  used to transmit a partition message from
> > the Tx buffer of the sender(the driver in this case) endpoint to the Rx
> > buffer of the receiver endpoint.
> >
> > An invocation of the FFA_MSG_SEND2 transfers the ownership to the
> > receiver endpoint(or any intermediate consumer). Completion of an
> > FFA_MSG_SEND2 invocation transfers the ownership back to the sender
> > endpoint.
> >
> > The framework defines the FFA_MSG_SEND2 interface to transmit a partition
> > message from the Tx buffer of the sender to the Rx buffer of a receiver
> > and inform the scheduler that the receiver must be run.
> >
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > ---
> > drivers/firmware/arm_ffa/driver.c | 40 +++++++++++++++++++++++++++++++++++++++
> > include/linux/arm_ffa.h           |  9 +++++++++
> > 2 files changed, 49 insertions(+)
> >
> > diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
> > index d5087e4f6d35..6c2602f7e7cc 100644
> > --- a/drivers/firmware/arm_ffa/driver.c
> > +++ b/drivers/firmware/arm_ffa/driver.c
> > @@ -344,6 +344,34 @@ static int ffa_msg_send_direct_req(u16 src_id, u16 dst_id, bool mode_32bit,
> > return -EINVAL;
> > }
> >
> > +static int ffa_msg_send2(u16 src_id, u16 dst_id, void *buf, size_t sz)
> > +{
> > + u32 src_dst_ids = PACK_TARGET_INFO(src_id, dst_id);
> > + struct ffa_indirect_msg_hdr *msg;
> > + ffa_value_t ret;
> > +
> > + mutex_lock(&drv_info->tx_lock);
> > +
> > + msg = drv_info->tx_buffer;
> > + msg->flags = 0;
> > + msg->res0 = 0;
> > + msg->offset = sizeof(*msg);
> > + msg->send_recv_id = src_dst_ids;
> > + msg->size = sz;
> > + memcpy(msg + msg->offset, buf, sz);
> 
> Here there should be a check that the user is not trying to send more data
> than what can fit in the TX Buffer.
>

Good point.

> Other than that LGTM.
>

Thanks!

--
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/4] firmware: arm_ffa: Add support for FFA_MSG_SEND2

[Jens Wiklander]

On Mon, Apr 15, 2024 at 6:05 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> The FFA_MSG_SEND2 can be  used to transmit a partition message from
> the Tx buffer of the sender(the driver in this case) endpoint to the Rx
> buffer of the receiver endpoint.
>
> An invocation of the FFA_MSG_SEND2 transfers the ownership to the

ownership of the TX buffer to the

> receiver endpoint(or any intermediate consumer). Completion of an
> FFA_MSG_SEND2 invocation transfers the ownership back to the sender

ownership of the buffer back

> endpoint.
>
> The framework defines the FFA_MSG_SEND2 interface to transmit a partition
> message from the Tx buffer of the sender to the Rx buffer of a receiver
> and inform the scheduler that the receiver must be run.
>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/firmware/arm_ffa/driver.c | 40 +++++++++++++++++++++++++++++++++++++++
>  include/linux/arm_ffa.h           |  9 +++++++++
>  2 files changed, 49 insertions(+)
>
> diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
> index d5087e4f6d35..6c2602f7e7cc 100644
> --- a/drivers/firmware/arm_ffa/driver.c
> +++ b/drivers/firmware/arm_ffa/driver.c
> @@ -344,6 +344,34 @@ static int ffa_msg_send_direct_req(u16 src_id, u16 dst_id, bool mode_32bit,
>         return -EINVAL;
>  }
>
> +static int ffa_msg_send2(u16 src_id, u16 dst_id, void *buf, size_t sz)
> +{
> +       u32 src_dst_ids = PACK_TARGET_INFO(src_id, dst_id);
> +       struct ffa_indirect_msg_hdr *msg;
> +       ffa_value_t ret;
> +
> +       mutex_lock(&drv_info->tx_lock);
> +
> +       msg = drv_info->tx_buffer;
> +       msg->flags = 0;
> +       msg->res0 = 0;
> +       msg->offset = sizeof(*msg);
> +       msg->send_recv_id = src_dst_ids;
> +       msg->size = sz;
> +       memcpy(msg + msg->offset, buf, sz);
> +
> +       /* flags = 0, sender VMID = 0 works for both physical/virtual NS */
> +       invoke_ffa_fn((ffa_value_t){
> +                     .a0 = FFA_MSG_SEND2, .a1 = 0, .a2 = 0
> +                     }, &ret);
> +
> +       if (ret.a0 == FFA_ERROR)
> +               return ffa_to_linux_errno((int)ret.a2);
> +
> +       mutex_lock(&drv_info->tx_lock);

mutex_unlock(), before the potential return above?

> +       return 0;
> +}
> +
>  static int ffa_mem_first_frag(u32 func_id, phys_addr_t buf, u32 buf_sz,
>                               u32 frag_len, u32 len, u64 *handle)
>  {
> @@ -888,6 +916,17 @@ static int ffa_sync_send_receive(struct ffa_device *dev,
>                                        dev->mode_32bit, data);
>  }
>
> +#define ffa_partition_supports_indirect_msg(dev)       \
> +       ffa_partition_check_property(dev, FFA_PARTITION_INDIRECT_MSG)
> +
> +static int ffa_indirect_msg_send(struct ffa_device *dev, void *buf, size_t sz)
> +{
> +       if (!ffa_partition_supports_direct_recv(dev))

ffa_partition_supports_indirect_msg(), but I'm not sure we should do
this check at all. The client could do this in advance. Unexpected
FFA_MSG_SEND2 calls are caught in other layers.

Cheers,
Jens

> +               return -EOPNOTSUPP;
> +
> +       return ffa_msg_send2(drv_info->vm_id, dev->vm_id, buf, sz);
> +}
> +
>  static int ffa_memory_share(struct ffa_mem_ops_args *args)
>  {
>         if (drv_info->mem_ops_native)
> @@ -1167,6 +1206,7 @@ static const struct ffa_info_ops ffa_drv_info_ops = {
>  static const struct ffa_msg_ops ffa_drv_msg_ops = {
>         .mode_32bit_set = ffa_mode_32bit_set,
>         .sync_send_receive = ffa_sync_send_receive,
> +       .indirect_send = ffa_indirect_msg_send,
>  };
>
>  static const struct ffa_mem_ops ffa_drv_mem_ops = {
> diff --git a/include/linux/arm_ffa.h b/include/linux/arm_ffa.h
> index 13830be5851d..d61a6df397f6 100644
> --- a/include/linux/arm_ffa.h
> +++ b/include/linux/arm_ffa.h
> @@ -238,6 +238,14 @@ struct ffa_send_direct_data {
>         unsigned long data4; /* w7/x7 */
>  };
>
> +struct ffa_indirect_msg_hdr {
> +       u32 flags;
> +       u32 res0;
> +       u32 offset;
> +       u32 send_recv_id;
> +       u32 size;
> +};
> +
>  struct ffa_mem_region_addr_range {
>         /* The base IPA of the constituent memory region, aligned to 4 kiB */
>         u64 address;
> @@ -398,6 +406,7 @@ struct ffa_msg_ops {
>         void (*mode_32bit_set)(struct ffa_device *dev);
>         int (*sync_send_receive)(struct ffa_device *dev,
>                                  struct ffa_send_direct_data *data);
> +       int (*indirect_send)(struct ffa_device *dev, void *buf, size_t sz);
>  };
>
>  struct ffa_mem_ops {
>
> --
> 2.43.2
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/4] firmware: arm_ffa: Add support for FFA_MSG_SEND2

[Sudeep Holla]

On Tue, Apr 16, 2024 at 09:41:51AM +0200, Jens Wiklander wrote:
> On Mon, Apr 15, 2024 at 6:05 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > The FFA_MSG_SEND2 can be  used to transmit a partition message from
> > the Tx buffer of the sender(the driver in this case) endpoint to the Rx
> > buffer of the receiver endpoint.
> >
> > An invocation of the FFA_MSG_SEND2 transfers the ownership to the
> 
> ownership of the TX buffer to the
> 
> > receiver endpoint(or any intermediate consumer). Completion of an
> > FFA_MSG_SEND2 invocation transfers the ownership back to the sender
> 
> ownership of the buffer back
> 
> > endpoint.
> >
> > The framework defines the FFA_MSG_SEND2 interface to transmit a partition
> > message from the Tx buffer of the sender to the Rx buffer of a receiver
> > and inform the scheduler that the receiver must be run.
> >
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > ---
> >  drivers/firmware/arm_ffa/driver.c | 40 +++++++++++++++++++++++++++++++++++++++
> >  include/linux/arm_ffa.h           |  9 +++++++++
> >  2 files changed, 49 insertions(+)
> >
> > diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
> > index d5087e4f6d35..6c2602f7e7cc 100644
> > --- a/drivers/firmware/arm_ffa/driver.c
> > +++ b/drivers/firmware/arm_ffa/driver.c
> > @@ -344,6 +344,34 @@ static int ffa_msg_send_direct_req(u16 src_id, u16 dst_id, bool mode_32bit,
> >         return -EINVAL;
> >  }
> >
> > +static int ffa_msg_send2(u16 src_id, u16 dst_id, void *buf, size_t sz)
> > +{
> > +       u32 src_dst_ids = PACK_TARGET_INFO(src_id, dst_id);
> > +       struct ffa_indirect_msg_hdr *msg;
> > +       ffa_value_t ret;
> > +
> > +       mutex_lock(&drv_info->tx_lock);
> > +
> > +       msg = drv_info->tx_buffer;
> > +       msg->flags = 0;
> > +       msg->res0 = 0;
> > +       msg->offset = sizeof(*msg);
> > +       msg->send_recv_id = src_dst_ids;
> > +       msg->size = sz;
> > +       memcpy(msg + msg->offset, buf, sz);
> > +
> > +       /* flags = 0, sender VMID = 0 works for both physical/virtual NS */
> > +       invoke_ffa_fn((ffa_value_t){
> > +                     .a0 = FFA_MSG_SEND2, .a1 = 0, .a2 = 0
> > +                     }, &ret);
> > +
> > +       if (ret.a0 == FFA_ERROR)
> > +               return ffa_to_linux_errno((int)ret.a2);
> > +
> > +       mutex_lock(&drv_info->tx_lock);
> 
> mutex_unlock(), before the potential return above?
>

Ah, my bad. Thanks for the catch.

> > +       return 0;
> > +}
> > +
> >  static int ffa_mem_first_frag(u32 func_id, phys_addr_t buf, u32 buf_sz,
> >                               u32 frag_len, u32 len, u64 *handle)
> >  {
> > @@ -888,6 +916,17 @@ static int ffa_sync_send_receive(struct ffa_device *dev,
> >                                        dev->mode_32bit, data);
> >  }
> >
> > +#define ffa_partition_supports_indirect_msg(dev)       \
> > +       ffa_partition_check_property(dev, FFA_PARTITION_INDIRECT_MSG)
> > +
> > +static int ffa_indirect_msg_send(struct ffa_device *dev, void *buf, size_t sz)
> > +{
> > +       if (!ffa_partition_supports_direct_recv(dev))
> 
> ffa_partition_supports_indirect_msg(), but I'm not sure we should do
> this check at all. The client could do this in advance. Unexpected
> FFA_MSG_SEND2 calls are caught in other layers.
>

Good point. I was not sure if it makes sense to add on each message but
I wasn't sure if we can defer this to the client. But based on what you
say, it should be OK do defer it to the client.

So the next question I have is: should we populate properties in the
ffa_device so that client can use the same. I started with that but then
didn't want to expose the info to the client.

I can move the properties to the struct ffa_device and keep these macro
arm_ffa.h for clients to use if they wish. Does that make sense ?

Thanks for taking look at the patches. I will skip responding on other
2 patches as I have asked all my questions as part of this patch and they
apply to those 2 as well.

--
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/4] firmware: arm_ffa: Add support for FFA_MSG_SEND2

[Jens Wiklander]

On Tue, Apr 16, 2024 at 10:48 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Tue, Apr 16, 2024 at 09:41:51AM +0200, Jens Wiklander wrote:
> > On Mon, Apr 15, 2024 at 6:05 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
> > >
> > > The FFA_MSG_SEND2 can be  used to transmit a partition message from
> > > the Tx buffer of the sender(the driver in this case) endpoint to the Rx
> > > buffer of the receiver endpoint.
> > >
> > > An invocation of the FFA_MSG_SEND2 transfers the ownership to the
> >
> > ownership of the TX buffer to the
> >
> > > receiver endpoint(or any intermediate consumer). Completion of an
> > > FFA_MSG_SEND2 invocation transfers the ownership back to the sender
> >
> > ownership of the buffer back
> >
> > > endpoint.
> > >
> > > The framework defines the FFA_MSG_SEND2 interface to transmit a partition
> > > message from the Tx buffer of the sender to the Rx buffer of a receiver
> > > and inform the scheduler that the receiver must be run.
> > >
> > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > > ---
> > >  drivers/firmware/arm_ffa/driver.c | 40 +++++++++++++++++++++++++++++++++++++++
> > >  include/linux/arm_ffa.h           |  9 +++++++++
> > >  2 files changed, 49 insertions(+)
> > >
> > > diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
> > > index d5087e4f6d35..6c2602f7e7cc 100644
> > > --- a/drivers/firmware/arm_ffa/driver.c
> > > +++ b/drivers/firmware/arm_ffa/driver.c
> > > @@ -344,6 +344,34 @@ static int ffa_msg_send_direct_req(u16 src_id, u16 dst_id, bool mode_32bit,
> > >         return -EINVAL;
> > >  }
> > >
> > > +static int ffa_msg_send2(u16 src_id, u16 dst_id, void *buf, size_t sz)
> > > +{
> > > +       u32 src_dst_ids = PACK_TARGET_INFO(src_id, dst_id);
> > > +       struct ffa_indirect_msg_hdr *msg;
> > > +       ffa_value_t ret;
> > > +
> > > +       mutex_lock(&drv_info->tx_lock);
> > > +
> > > +       msg = drv_info->tx_buffer;
> > > +       msg->flags = 0;
> > > +       msg->res0 = 0;
> > > +       msg->offset = sizeof(*msg);
> > > +       msg->send_recv_id = src_dst_ids;
> > > +       msg->size = sz;
> > > +       memcpy(msg + msg->offset, buf, sz);
> > > +
> > > +       /* flags = 0, sender VMID = 0 works for both physical/virtual NS */
> > > +       invoke_ffa_fn((ffa_value_t){
> > > +                     .a0 = FFA_MSG_SEND2, .a1 = 0, .a2 = 0
> > > +                     }, &ret);
> > > +
> > > +       if (ret.a0 == FFA_ERROR)
> > > +               return ffa_to_linux_errno((int)ret.a2);
> > > +
> > > +       mutex_lock(&drv_info->tx_lock);
> >
> > mutex_unlock(), before the potential return above?
> >
>
> Ah, my bad. Thanks for the catch.
>
> > > +       return 0;
> > > +}
> > > +
> > >  static int ffa_mem_first_frag(u32 func_id, phys_addr_t buf, u32 buf_sz,
> > >                               u32 frag_len, u32 len, u64 *handle)
> > >  {
> > > @@ -888,6 +916,17 @@ static int ffa_sync_send_receive(struct ffa_device *dev,
> > >                                        dev->mode_32bit, data);
> > >  }
> > >
> > > +#define ffa_partition_supports_indirect_msg(dev)       \
> > > +       ffa_partition_check_property(dev, FFA_PARTITION_INDIRECT_MSG)
> > > +
> > > +static int ffa_indirect_msg_send(struct ffa_device *dev, void *buf, size_t sz)
> > > +{
> > > +       if (!ffa_partition_supports_direct_recv(dev))
> >
> > ffa_partition_supports_indirect_msg(), but I'm not sure we should do
> > this check at all. The client could do this in advance. Unexpected
> > FFA_MSG_SEND2 calls are caught in other layers.
> >
>
> Good point. I was not sure if it makes sense to add on each message but
> I wasn't sure if we can defer this to the client. But based on what you
> say, it should be OK do defer it to the client.
>
> So the next question I have is: should we populate properties in the
> ffa_device so that client can use the same. I started with that but then
> didn't want to expose the info to the client.
>
> I can move the properties to the struct ffa_device and keep these macro
> arm_ffa.h for clients to use if they wish. Does that make sense ?

Yes, that sounds good.

>
> Thanks for taking look at the patches. I will skip responding on other
> 2 patches as I have asked all my questions as part of this patch and they
> apply to those 2 as well.

OK

Cheers,
Jens

>
> --
> Regards,
> Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel