| Message ID | 20201204121137.2966778-7-sudeep.holla@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | firmware: Add initial support for Arm FF-A | expand |
Hi Sudeep, Some comments below. On Fri, Dec 04, 2020 at 12:11:36PM +0000, Sudeep Holla wrote: > Parse the FFA nodes from the device-tree and register all the partitions > whose services will be used in the kernel. > > In order to also enable in-kernel users of FFA interface, let us add > simple set of operations for such devices. > > The in-kernel users are registered without the character device interface. > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > --- > drivers/firmware/arm_ffa/common.h | 2 + > drivers/firmware/arm_ffa/driver.c | 186 ++++++++++++++++++++++++++++++ > include/linux/arm_ffa.h | 36 +++++- > 3 files changed, 223 insertions(+), 1 deletion(-) > > diff --git a/drivers/firmware/arm_ffa/common.h b/drivers/firmware/arm_ffa/common.h > index d019348bf67d..eb1371c2b2b8 100644 > --- a/drivers/firmware/arm_ffa/common.h > +++ b/drivers/firmware/arm_ffa/common.h > @@ -6,6 +6,7 @@ > #ifndef _FFA_COMMON_H > #define _FFA_COMMON_H > > +#include <linux/arm_ffa.h> > #include <linux/arm-smccc.h> > #include <linux/err.h> > > @@ -17,6 +18,7 @@ typedef ffa_res_t > > int __init arm_ffa_bus_init(void); > void __exit arm_ffa_bus_exit(void); > +bool ffa_device_is_valid(struct ffa_device *ffa_dev); > > #ifdef CONFIG_ARM_FFA_SMCCC > int __init ffa_transport_init(ffa_fn **invoke_ffa_fn); > diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c > index 257b331d781c..3e4ba841dbf8 100644 > --- a/drivers/firmware/arm_ffa/driver.c > +++ b/drivers/firmware/arm_ffa/driver.c > @@ -24,9 +24,13 @@ > > #include <linux/arm_ffa.h> > #include <linux/bitfield.h> > +#include <linux/device.h> > #include <linux/io.h> > +#include <linux/kernel.h> > #include <linux/module.h> > +#include <linux/of.h> > #include <linux/slab.h> > +#include <linux/uuid.h> > > #include "common.h" > > @@ -179,6 +183,20 @@ static int ffa_version_check(u32 *version) > return 0; > } > > +static int ffa_rx_release(void) > +{ > + ffa_res_t ret; > + > + ret = invoke_ffa_fn(FFA_RX_RELEASE, 0, 0, 0, 0, 0, 0, 0); > + > + if (ret.a0 == FFA_ERROR) > + return ffa_to_linux_errno((int)ret.a2); > + > + /* check for ret.a0 == FFA_RX_RELEASE ? */ > + > + return 0; > +} > + > static int ffa_rxtx_map(phys_addr_t tx_buf, phys_addr_t rx_buf, u32 pg_cnt) > { > ffa_res_t ret; > @@ -203,6 +221,50 @@ static int ffa_rxtx_unmap(u16 vm_id) > return 0; > } > > +static int __ffa_partition_info_get(u32 uuid0, u32 uuid1, u32 uuid2, u32 uuid3, > + struct ffa_partition_info **buffer) > +{ > + int count; > + ffa_res_t partition_info; > + > + mutex_lock(&drv_info->rx_lock); > + partition_info = invoke_ffa_fn(FFA_PARTITION_INFO_GET, uuid0, uuid1, > + uuid2, uuid3, 0, 0, 0); > + > + if (partition_info.a0 == FFA_ERROR) > + return ffa_to_linux_errno((int)partition_info.a2); > + > + count = partition_info.a2; > + > + if (buffer) > + memcpy(*buffer, drv_info->rx_buffer, sizeof(*buffer) * count); > + > + ffa_rx_release(); > + > + mutex_unlock(&drv_info->rx_lock); > + > + return count; > +} > + > +static int ffa_partition_probe(const char *uuid_str, > + struct ffa_partition_info *buffer) > +{ > + int count; > + uuid_t uuid; > + u32 uuid0_4[4] = { 0 }; > + > + if (uuid_parse(uuid_str, &uuid)) { > + pr_err("invalid uuid (%s)\n", uuid_str); > + return -ENODEV; > + } > + > + export_uuid((u8 *)uuid0_4, &uuid); > + count = __ffa_partition_info_get(uuid0_4[0], uuid0_4[1], uuid0_4[2], > + uuid0_4[3], &buffer); > + > + return count; > +} > + > #define VM_ID_MASK GENMASK(15, 0) > static int ffa_id_get(u16 *vm_id) > { > @@ -218,9 +280,125 @@ static int ffa_id_get(u16 *vm_id) > return 0; > } > > +static int ffa_msg_send_direct_req(u16 src_id, u16 dst_id, > + struct ffa_send_direct_data *data) > +{ > + u32 src_dst_ids = PACK_TARGET_INFO(src_id, dst_id); > + ffa_res_t ret; > + > + ret = invoke_ffa_fn(FFA_FN_NATIVE(MSG_SEND_DIRECT_REQ), src_dst_ids, 0, > + data->data0, data->data1, data->data2, > + data->data3, data->data4); > + > + while (ret.a0 == FFA_INTERRUPT) > + ret = invoke_ffa_fn(FFA_RUN, ret.a1, 0, 0, 0, 0, 0, 0); > + if (ret.a0 == FFA_ERROR) > + return ffa_to_linux_errno((int)ret.a2); > + > + if (ret.a0 == FFA_FN_NATIVE(MSG_SEND_DIRECT_RESP)) { > + data->data0 = ret.a3; > + data->data1 = ret.a4; > + data->data2 = ret.a5; > + data->data3 = ret.a6; > + data->data4 = ret.a7; > + } > + > + return 0; > +} > + > +static u32 ffa_api_version_get(void) > +{ > + return drv_info->version; > +} > + > +static u16 ffa_partition_id_get(struct ffa_device *dev) > +{ > + return dev->vm_id; > +} > + > +static int ffa_partition_info_get(const char *uuid_str, > + struct ffa_partition_info *buffer) > +{ > + if (ffa_partition_probe(uuid_str, buffer) == 1) > + return 0; > + > + return -ENOENT; > +} > + > +static int ffa_sync_send_receive(struct ffa_device *dev, u16 ep, > + struct ffa_send_direct_data *data) > +{ > + return ffa_msg_send_direct_req(dev->vm_id, ep, data); > +} > + > +static const struct ffa_dev_ops ffa_ops = { > + .api_version_get = ffa_api_version_get, > + .partition_id_get = ffa_partition_id_get, > + .partition_info_get = ffa_partition_info_get, > + .sync_send_receive = ffa_sync_send_receive, > +}; > + > +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); > + > +int ffa_setup_partitions(struct device_node *np) > +{ > + int ret; > + struct device_node *child; > + struct ffa_device *ffa_dev; > + struct ffa_partition_info pbuf; > + const char *p_uuid, *pfx = "Ignoring FFA partition"; > + uuid_t uuid = UUID_INIT(0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0); > + > + for_each_child_of_node(np, child) { The spec says: – If the Nil UUID is specified at the Non-secure virtual FF-A instance, the Hypervisor must provide information for partitions resident in both security states. Doesn't that make this redundant? > + if (!of_device_is_compatible(child, "arm,ffa-1.0-partition")) { > + of_node_put(child); > + continue; > + } > + > + if (of_property_read_string(child, "uuid", &p_uuid)) { > + pr_err("%s: failed to parse \"uuid\" property\n", pfx); > + of_node_put(child); > + continue; > + } > + > + of_node_put(child); > + > + if (uuid_parse(p_uuid, &uuid)) { > + pr_err("%s: invalid \"uuid\" property (%s)\n", > + pfx, p_uuid); > + continue; > + } > + > + ret = ffa_partition_probe(p_uuid, &pbuf); > + if (ret != 1) { If ret is > 1 we're in deep trouble as we have a buffer overrun on the stack. Cheers, Jens > + pr_err("%s: %s partition info probe failed\n", > + pfx, p_uuid); > + return -EINVAL; > + } > + > + ffa_dev = ffa_device_register(p_uuid, pbuf.id); > + if (!ffa_dev) { > + pr_err("%s: failed to register %s\n", pfx, p_uuid); > + continue; > + } > + > + ffa_dev_set_drvdata(ffa_dev, drv_info); > + } > + > + return 0; > +} > + > static int __init ffa_init(void) > { > int ret; > + struct device_node *np; > > ret = arm_ffa_bus_init(); > if (ret) > @@ -267,6 +445,14 @@ static int __init ffa_init(void) > mutex_init(&drv_info->rx_lock); > mutex_init(&drv_info->tx_lock); > > + /* Set up all the partitions */ > + np = of_find_compatible_node(NULL, NULL, "arm,ffa-1.0"); > + if (!np) > + return 0; > + > + ffa_setup_partitions(np); > + of_node_put(np); > + > return 0; > free_pages: > if (drv_info->tx_buffer) > diff --git a/include/linux/arm_ffa.h b/include/linux/arm_ffa.h > index 3defddfb40fc..8604c48289ce 100644 > --- a/include/linux/arm_ffa.h > +++ b/include/linux/arm_ffa.h > @@ -6,7 +6,6 @@ > #ifndef _LINUX_ARM_FFA_H > #define _LINUX_ARM_FFA_H > > -#include <linux/cdev.h> > #include <linux/device.h> > #include <linux/module.h> > #include <linux/types.h> > @@ -47,6 +46,7 @@ void ffa_device_unregister(struct ffa_device *ffa_dev); > int ffa_driver_register(struct ffa_driver *driver, struct module *owner, > const char *mod_name); > void ffa_driver_unregister(struct ffa_driver *driver); > +const struct ffa_dev_ops *ffa_dev_ops_get(struct ffa_device *dev); > > #else > static inline > @@ -66,6 +66,10 @@ ffa_driver_register(struct ffa_driver *driver, struct module *owner, > > static inline void ffa_driver_unregister(struct ffa_driver *driver) {} > > +const struct ffa_dev_ops *ffa_dev_ops_get(struct ffa_device *dev) > +{ > + return NULL; > +} > #endif /* CONFIG_ARM_FFA_TRANSPORT */ > > #define ffa_register(driver) \ > @@ -84,4 +88,34 @@ static inline void ffa_driver_unregister(struct ffa_driver *driver) {} > #define module_ffa_driver(__ffa_driver) \ > module_driver(__ffa_driver, ffa_register, ffa_unregister) > > +/* FFA transport related */ > +struct ffa_partition_info { > + u16 id; > + u16 exec_ctxt; > +/* partition supports receipt of direct requests */ > +#define FFA_PARTITION_DIRECT_RECV BIT(0) > +/* partition can send direct requests. */ > +#define FFA_PARTITION_DIRECT_SEND BIT(1) > +/* partition can send and receive indirect messages. */ > +#define FFA_PARTITION_INDIRECT_MSG BIT(2) > + u32 properties; > +}; > + > +struct ffa_send_direct_data { > + unsigned long data0; > + unsigned long data1; > + unsigned long data2; > + unsigned long data3; > + unsigned long data4; > +}; > + > +struct ffa_dev_ops { > + u32 (*api_version_get)(void); > + u16 (*partition_id_get)(struct ffa_device *dev); > + int (*partition_info_get)(const char *uuid_str, > + struct ffa_partition_info *buffer); > + int (*sync_send_receive)(struct ffa_device *dev, u16 ep, > + struct ffa_send_direct_data *data); > +}; > + > #endif /* _LINUX_ARM_FFA_H */ > -- > 2.25.1 >
Hi Sudeep, Some more comments below. On Fri, Dec 4, 2020 at 1:11 PM Sudeep Holla <sudeep.holla@arm.com> wrote: > > Parse the FFA nodes from the device-tree and register all the partitions > whose services will be used in the kernel. > > In order to also enable in-kernel users of FFA interface, let us add > simple set of operations for such devices. > > The in-kernel users are registered without the character device interface. > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > --- > drivers/firmware/arm_ffa/common.h | 2 + > drivers/firmware/arm_ffa/driver.c | 186 ++++++++++++++++++++++++++++++ > include/linux/arm_ffa.h | 36 +++++- > 3 files changed, 223 insertions(+), 1 deletion(-) > > diff --git a/drivers/firmware/arm_ffa/common.h b/drivers/firmware/arm_ffa/common.h > index d019348bf67d..eb1371c2b2b8 100644 > --- a/drivers/firmware/arm_ffa/common.h > +++ b/drivers/firmware/arm_ffa/common.h > @@ -6,6 +6,7 @@ > #ifndef _FFA_COMMON_H > #define _FFA_COMMON_H > > +#include <linux/arm_ffa.h> > #include <linux/arm-smccc.h> > #include <linux/err.h> > > @@ -17,6 +18,7 @@ typedef ffa_res_t > > int __init arm_ffa_bus_init(void); > void __exit arm_ffa_bus_exit(void); > +bool ffa_device_is_valid(struct ffa_device *ffa_dev); > > #ifdef CONFIG_ARM_FFA_SMCCC > int __init ffa_transport_init(ffa_fn **invoke_ffa_fn); > diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c > index 257b331d781c..3e4ba841dbf8 100644 > --- a/drivers/firmware/arm_ffa/driver.c > +++ b/drivers/firmware/arm_ffa/driver.c > @@ -24,9 +24,13 @@ > > #include <linux/arm_ffa.h> > #include <linux/bitfield.h> > +#include <linux/device.h> > #include <linux/io.h> > +#include <linux/kernel.h> > #include <linux/module.h> > +#include <linux/of.h> > #include <linux/slab.h> > +#include <linux/uuid.h> > > #include "common.h" > > @@ -179,6 +183,20 @@ static int ffa_version_check(u32 *version) > return 0; > } > > +static int ffa_rx_release(void) > +{ > + ffa_res_t ret; > + > + ret = invoke_ffa_fn(FFA_RX_RELEASE, 0, 0, 0, 0, 0, 0, 0); > + > + if (ret.a0 == FFA_ERROR) > + return ffa_to_linux_errno((int)ret.a2); > + > + /* check for ret.a0 == FFA_RX_RELEASE ? */ > + > + return 0; > +} > + > static int ffa_rxtx_map(phys_addr_t tx_buf, phys_addr_t rx_buf, u32 pg_cnt) > { > ffa_res_t ret; > @@ -203,6 +221,50 @@ static int ffa_rxtx_unmap(u16 vm_id) > return 0; > } > > +static int __ffa_partition_info_get(u32 uuid0, u32 uuid1, u32 uuid2, u32 uuid3, > + struct ffa_partition_info **buffer) > +{ > + int count; > + ffa_res_t partition_info; > + > + mutex_lock(&drv_info->rx_lock); > + partition_info = invoke_ffa_fn(FFA_PARTITION_INFO_GET, uuid0, uuid1, > + uuid2, uuid3, 0, 0, 0); > + > + if (partition_info.a0 == FFA_ERROR) > + return ffa_to_linux_errno((int)partition_info.a2); > + > + count = partition_info.a2; > + > + if (buffer) > + memcpy(*buffer, drv_info->rx_buffer, sizeof(*buffer) * count); > + > + ffa_rx_release(); > + > + mutex_unlock(&drv_info->rx_lock); > + > + return count; > +} > + > +static int ffa_partition_probe(const char *uuid_str, > + struct ffa_partition_info *buffer) > +{ > + int count; > + uuid_t uuid; > + u32 uuid0_4[4] = { 0 }; > + > + if (uuid_parse(uuid_str, &uuid)) { > + pr_err("invalid uuid (%s)\n", uuid_str); > + return -ENODEV; > + } > + > + export_uuid((u8 *)uuid0_4, &uuid); > + count = __ffa_partition_info_get(uuid0_4[0], uuid0_4[1], uuid0_4[2], > + uuid0_4[3], &buffer); > + > + return count; > +} > + > #define VM_ID_MASK GENMASK(15, 0) > static int ffa_id_get(u16 *vm_id) > { > @@ -218,9 +280,125 @@ static int ffa_id_get(u16 *vm_id) > return 0; > } > > +static int ffa_msg_send_direct_req(u16 src_id, u16 dst_id, > + struct ffa_send_direct_data *data) > +{ > + u32 src_dst_ids = PACK_TARGET_INFO(src_id, dst_id); > + ffa_res_t ret; > + > + ret = invoke_ffa_fn(FFA_FN_NATIVE(MSG_SEND_DIRECT_REQ), src_dst_ids, 0, > + data->data0, data->data1, data->data2, > + data->data3, data->data4); > + > + while (ret.a0 == FFA_INTERRUPT) > + ret = invoke_ffa_fn(FFA_RUN, ret.a1, 0, 0, 0, 0, 0, 0); > + if (ret.a0 == FFA_ERROR) > + return ffa_to_linux_errno((int)ret.a2); > + > + if (ret.a0 == FFA_FN_NATIVE(MSG_SEND_DIRECT_RESP)) { > + data->data0 = ret.a3; > + data->data1 = ret.a4; > + data->data2 = ret.a5; > + data->data3 = ret.a6; > + data->data4 = ret.a7; > + } > + > + return 0; > +} > + > +static u32 ffa_api_version_get(void) > +{ > + return drv_info->version; > +} > + > +static u16 ffa_partition_id_get(struct ffa_device *dev) > +{ > + return dev->vm_id; > +} > + > +static int ffa_partition_info_get(const char *uuid_str, > + struct ffa_partition_info *buffer) > +{ > + if (ffa_partition_probe(uuid_str, buffer) == 1) > + return 0; > + > + return -ENOENT; > +} > + > +static int ffa_sync_send_receive(struct ffa_device *dev, u16 ep, > + struct ffa_send_direct_data *data) > +{ > + return ffa_msg_send_direct_req(dev->vm_id, ep, data); > +} > + > +static const struct ffa_dev_ops ffa_ops = { > + .api_version_get = ffa_api_version_get, > + .partition_id_get = ffa_partition_id_get, > + .partition_info_get = ffa_partition_info_get, > + .sync_send_receive = ffa_sync_send_receive, > +}; > + > +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); > + > +int ffa_setup_partitions(struct device_node *np) This function is only used internally and the return value is ignored. Maybe make it static void instead? > +{ > + int ret; > + struct device_node *child; > + struct ffa_device *ffa_dev; > + struct ffa_partition_info pbuf; > + const char *p_uuid, *pfx = "Ignoring FFA partition"; > + uuid_t uuid = UUID_INIT(0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0); > + > + for_each_child_of_node(np, child) { of_node_put() should only be called on child in case we're leaving the loop early. for_each_child_of_node() takes care of the other case. > + if (!of_device_is_compatible(child, "arm,ffa-1.0-partition")) { > + of_node_put(child); > + continue; > + } > + > + if (of_property_read_string(child, "uuid", &p_uuid)) { > + pr_err("%s: failed to parse \"uuid\" property\n", pfx); > + of_node_put(child); > + continue; > + } > + > + of_node_put(child); > + > + if (uuid_parse(p_uuid, &uuid)) { > + pr_err("%s: invalid \"uuid\" property (%s)\n", > + pfx, p_uuid); > + continue; > + } > + > + ret = ffa_partition_probe(p_uuid, &pbuf); > + if (ret != 1) { > + pr_err("%s: %s partition info probe failed\n", > + pfx, p_uuid); > + return -EINVAL; > + } > + > + ffa_dev = ffa_device_register(p_uuid, pbuf.id); > + if (!ffa_dev) { > + pr_err("%s: failed to register %s\n", pfx, p_uuid); > + continue; > + } > + > + ffa_dev_set_drvdata(ffa_dev, drv_info); > + } > + > + return 0; > +} > + > static int __init ffa_init(void) > { > int ret; > + struct device_node *np; > > ret = arm_ffa_bus_init(); > if (ret) > @@ -267,6 +445,14 @@ static int __init ffa_init(void) > mutex_init(&drv_info->rx_lock); > mutex_init(&drv_info->tx_lock); > > + /* Set up all the partitions */ > + np = of_find_compatible_node(NULL, NULL, "arm,ffa-1.0"); > + if (!np) > + return 0; > + > + ffa_setup_partitions(np); > + of_node_put(np); > + > return 0; > free_pages: > if (drv_info->tx_buffer) > diff --git a/include/linux/arm_ffa.h b/include/linux/arm_ffa.h > index 3defddfb40fc..8604c48289ce 100644 > --- a/include/linux/arm_ffa.h > +++ b/include/linux/arm_ffa.h > @@ -6,7 +6,6 @@ > #ifndef _LINUX_ARM_FFA_H > #define _LINUX_ARM_FFA_H > > -#include <linux/cdev.h> > #include <linux/device.h> > #include <linux/module.h> > #include <linux/types.h> > @@ -47,6 +46,7 @@ void ffa_device_unregister(struct ffa_device *ffa_dev); > int ffa_driver_register(struct ffa_driver *driver, struct module *owner, > const char *mod_name); > void ffa_driver_unregister(struct ffa_driver *driver); > +const struct ffa_dev_ops *ffa_dev_ops_get(struct ffa_device *dev); > > #else > static inline > @@ -66,6 +66,10 @@ ffa_driver_register(struct ffa_driver *driver, struct module *owner, > > static inline void ffa_driver_unregister(struct ffa_driver *driver) {} > > +const struct ffa_dev_ops *ffa_dev_ops_get(struct ffa_device *dev) > +{ > + return NULL; > +} > #endif /* CONFIG_ARM_FFA_TRANSPORT */ > > #define ffa_register(driver) \ > @@ -84,4 +88,34 @@ static inline void ffa_driver_unregister(struct ffa_driver *driver) {} > #define module_ffa_driver(__ffa_driver) \ > module_driver(__ffa_driver, ffa_register, ffa_unregister) > > +/* FFA transport related */ > +struct ffa_partition_info { > + u16 id; > + u16 exec_ctxt; > +/* partition supports receipt of direct requests */ > +#define FFA_PARTITION_DIRECT_RECV BIT(0) > +/* partition can send direct requests. */ > +#define FFA_PARTITION_DIRECT_SEND BIT(1) > +/* partition can send and receive indirect messages. */ > +#define FFA_PARTITION_INDIRECT_MSG BIT(2) > + u32 properties; > +}; > + > +struct ffa_send_direct_data { > + unsigned long data0; > + unsigned long data1; > + unsigned long data2; > + unsigned long data3; > + unsigned long data4; > +}; A comment describing which register index these maps to would be helpful. > + > +struct ffa_dev_ops { > + u32 (*api_version_get)(void); > + u16 (*partition_id_get)(struct ffa_device *dev); > + int (*partition_info_get)(const char *uuid_str, > + struct ffa_partition_info *buffer); > + int (*sync_send_receive)(struct ffa_device *dev, u16 ep, > + struct ffa_send_direct_data *data); ep can be read directly from dev->vm_id We may need a way to indicate if we're to use the 32bit or 64bit calling convention. OP-TEE depends on being able to use 32bit calls here. Cheers, Jens > +}; > + > #endif /* _LINUX_ARM_FFA_H */ > -- > 2.25.1 >
One more comment below. On Fri, Dec 11, 2020 at 11:45 AM Jens Wiklander <jens.wiklander@linaro.org> wrote: > > Hi Sudeep, > > Some more comments below. > > On Fri, Dec 4, 2020 at 1:11 PM Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > Parse the FFA nodes from the device-tree and register all the partitions > > whose services will be used in the kernel. > > > > In order to also enable in-kernel users of FFA interface, let us add > > simple set of operations for such devices. > > > > The in-kernel users are registered without the character device interface. > > > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > > --- > > drivers/firmware/arm_ffa/common.h | 2 + > > drivers/firmware/arm_ffa/driver.c | 186 ++++++++++++++++++++++++++++++ > > include/linux/arm_ffa.h | 36 +++++- > > 3 files changed, 223 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/firmware/arm_ffa/common.h b/drivers/firmware/arm_ffa/common.h > > index d019348bf67d..eb1371c2b2b8 100644 > > --- a/drivers/firmware/arm_ffa/common.h > > +++ b/drivers/firmware/arm_ffa/common.h > > @@ -6,6 +6,7 @@ > > #ifndef _FFA_COMMON_H > > #define _FFA_COMMON_H > > > > +#include <linux/arm_ffa.h> > > #include <linux/arm-smccc.h> > > #include <linux/err.h> > > > > @@ -17,6 +18,7 @@ typedef ffa_res_t > > > > int __init arm_ffa_bus_init(void); > > void __exit arm_ffa_bus_exit(void); > > +bool ffa_device_is_valid(struct ffa_device *ffa_dev); > > > > #ifdef CONFIG_ARM_FFA_SMCCC > > int __init ffa_transport_init(ffa_fn **invoke_ffa_fn); > > diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c > > index 257b331d781c..3e4ba841dbf8 100644 > > --- a/drivers/firmware/arm_ffa/driver.c > > +++ b/drivers/firmware/arm_ffa/driver.c > > @@ -24,9 +24,13 @@ > > > > #include <linux/arm_ffa.h> > > #include <linux/bitfield.h> > > +#include <linux/device.h> > > #include <linux/io.h> > > +#include <linux/kernel.h> > > #include <linux/module.h> > > +#include <linux/of.h> > > #include <linux/slab.h> > > +#include <linux/uuid.h> > > > > #include "common.h" > > > > @@ -179,6 +183,20 @@ static int ffa_version_check(u32 *version) > > return 0; > > } > > > > +static int ffa_rx_release(void) > > +{ > > + ffa_res_t ret; > > + > > + ret = invoke_ffa_fn(FFA_RX_RELEASE, 0, 0, 0, 0, 0, 0, 0); > > + > > + if (ret.a0 == FFA_ERROR) > > + return ffa_to_linux_errno((int)ret.a2); > > + > > + /* check for ret.a0 == FFA_RX_RELEASE ? */ > > + > > + return 0; > > +} > > + > > static int ffa_rxtx_map(phys_addr_t tx_buf, phys_addr_t rx_buf, u32 pg_cnt) > > { > > ffa_res_t ret; > > @@ -203,6 +221,50 @@ static int ffa_rxtx_unmap(u16 vm_id) > > return 0; > > } > > > > +static int __ffa_partition_info_get(u32 uuid0, u32 uuid1, u32 uuid2, u32 uuid3, > > + struct ffa_partition_info **buffer) > > +{ > > + int count; > > + ffa_res_t partition_info; > > + > > + mutex_lock(&drv_info->rx_lock); > > + partition_info = invoke_ffa_fn(FFA_PARTITION_INFO_GET, uuid0, uuid1, > > + uuid2, uuid3, 0, 0, 0); > > + > > + if (partition_info.a0 == FFA_ERROR) > > + return ffa_to_linux_errno((int)partition_info.a2); > > + > > + count = partition_info.a2; > > + > > + if (buffer) > > + memcpy(*buffer, drv_info->rx_buffer, sizeof(*buffer) * count); > > + > > + ffa_rx_release(); > > + > > + mutex_unlock(&drv_info->rx_lock); > > + > > + return count; > > +} > > + > > +static int ffa_partition_probe(const char *uuid_str, > > + struct ffa_partition_info *buffer) > > +{ > > + int count; > > + uuid_t uuid; > > + u32 uuid0_4[4] = { 0 }; > > + > > + if (uuid_parse(uuid_str, &uuid)) { > > + pr_err("invalid uuid (%s)\n", uuid_str); > > + return -ENODEV; > > + } > > + > > + export_uuid((u8 *)uuid0_4, &uuid); > > + count = __ffa_partition_info_get(uuid0_4[0], uuid0_4[1], uuid0_4[2], > > + uuid0_4[3], &buffer); Wrong byte order? According to section 5.3 of the SMCCC, UUIDs are returned as a single 128-bit value using the SMC32 calling convention. This value is mapped to argument registers x0-x3 on AArch64 (resp. r0-r3 on AArch32). x0 for example shall hold bytes 0 to 3, with byte 0 in the low-order bits. Cheers, Jens > > + > > + return count; > > +} > > + > > #define VM_ID_MASK GENMASK(15, 0) > > static int ffa_id_get(u16 *vm_id) > > { > > @@ -218,9 +280,125 @@ static int ffa_id_get(u16 *vm_id) > > return 0; > > } > > > > +static int ffa_msg_send_direct_req(u16 src_id, u16 dst_id, > > + struct ffa_send_direct_data *data) > > +{ > > + u32 src_dst_ids = PACK_TARGET_INFO(src_id, dst_id); > > + ffa_res_t ret; > > + > > + ret = invoke_ffa_fn(FFA_FN_NATIVE(MSG_SEND_DIRECT_REQ), src_dst_ids, 0, > > + data->data0, data->data1, data->data2, > > + data->data3, data->data4); > > + > > + while (ret.a0 == FFA_INTERRUPT) > > + ret = invoke_ffa_fn(FFA_RUN, ret.a1, 0, 0, 0, 0, 0, 0); > > + if (ret.a0 == FFA_ERROR) > > + return ffa_to_linux_errno((int)ret.a2); > > + > > + if (ret.a0 == FFA_FN_NATIVE(MSG_SEND_DIRECT_RESP)) { > > + data->data0 = ret.a3; > > + data->data1 = ret.a4; > > + data->data2 = ret.a5; > > + data->data3 = ret.a6; > > + data->data4 = ret.a7; > > + } > > + > > + return 0; > > +} > > + > > +static u32 ffa_api_version_get(void) > > +{ > > + return drv_info->version; > > +} > > + > > +static u16 ffa_partition_id_get(struct ffa_device *dev) > > +{ > > + return dev->vm_id; > > +} > > + > > +static int ffa_partition_info_get(const char *uuid_str, > > + struct ffa_partition_info *buffer) > > +{ > > + if (ffa_partition_probe(uuid_str, buffer) == 1) > > + return 0; > > + > > + return -ENOENT; > > +} > > + > > +static int ffa_sync_send_receive(struct ffa_device *dev, u16 ep, > > + struct ffa_send_direct_data *data) > > +{ > > + return ffa_msg_send_direct_req(dev->vm_id, ep, data); > > +} > > + > > +static const struct ffa_dev_ops ffa_ops = { > > + .api_version_get = ffa_api_version_get, > > + .partition_id_get = ffa_partition_id_get, > > + .partition_info_get = ffa_partition_info_get, > > + .sync_send_receive = ffa_sync_send_receive, > > +}; > > + > > +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); > > + > > +int ffa_setup_partitions(struct device_node *np) > This function is only used internally and the return value is ignored. > Maybe make it static void instead? > > > +{ > > + int ret; > > + struct device_node *child; > > + struct ffa_device *ffa_dev; > > + struct ffa_partition_info pbuf; > > + const char *p_uuid, *pfx = "Ignoring FFA partition"; > > + uuid_t uuid = UUID_INIT(0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0); > > + > > + for_each_child_of_node(np, child) { > of_node_put() should only be called on child in case we're leaving the > loop early. for_each_child_of_node() takes care of the other case. > > > + if (!of_device_is_compatible(child, "arm,ffa-1.0-partition")) { > > + of_node_put(child); > > + continue; > > + } > > + > > + if (of_property_read_string(child, "uuid", &p_uuid)) { > > + pr_err("%s: failed to parse \"uuid\" property\n", pfx); > > + of_node_put(child); > > + continue; > > + } > > + > > + of_node_put(child); > > + > > + if (uuid_parse(p_uuid, &uuid)) { > > + pr_err("%s: invalid \"uuid\" property (%s)\n", > > + pfx, p_uuid); > > + continue; > > + } > > + > > + ret = ffa_partition_probe(p_uuid, &pbuf); > > + if (ret != 1) { > > + pr_err("%s: %s partition info probe failed\n", > > + pfx, p_uuid); > > + return -EINVAL; > > + } > > + > > + ffa_dev = ffa_device_register(p_uuid, pbuf.id); > > + if (!ffa_dev) { > > + pr_err("%s: failed to register %s\n", pfx, p_uuid); > > + continue; > > + } > > + > > + ffa_dev_set_drvdata(ffa_dev, drv_info); > > + } > > + > > + return 0; > > +} > > + > > static int __init ffa_init(void) > > { > > int ret; > > + struct device_node *np; > > > > ret = arm_ffa_bus_init(); > > if (ret) > > @@ -267,6 +445,14 @@ static int __init ffa_init(void) > > mutex_init(&drv_info->rx_lock); > > mutex_init(&drv_info->tx_lock); > > > > + /* Set up all the partitions */ > > + np = of_find_compatible_node(NULL, NULL, "arm,ffa-1.0"); > > + if (!np) > > + return 0; > > + > > + ffa_setup_partitions(np); > > + of_node_put(np); > > + > > return 0; > > free_pages: > > if (drv_info->tx_buffer) > > diff --git a/include/linux/arm_ffa.h b/include/linux/arm_ffa.h > > index 3defddfb40fc..8604c48289ce 100644 > > --- a/include/linux/arm_ffa.h > > +++ b/include/linux/arm_ffa.h > > @@ -6,7 +6,6 @@ > > #ifndef _LINUX_ARM_FFA_H > > #define _LINUX_ARM_FFA_H > > > > -#include <linux/cdev.h> > > #include <linux/device.h> > > #include <linux/module.h> > > #include <linux/types.h> > > @@ -47,6 +46,7 @@ void ffa_device_unregister(struct ffa_device *ffa_dev); > > int ffa_driver_register(struct ffa_driver *driver, struct module *owner, > > const char *mod_name); > > void ffa_driver_unregister(struct ffa_driver *driver); > > +const struct ffa_dev_ops *ffa_dev_ops_get(struct ffa_device *dev); > > > > #else > > static inline > > @@ -66,6 +66,10 @@ ffa_driver_register(struct ffa_driver *driver, struct module *owner, > > > > static inline void ffa_driver_unregister(struct ffa_driver *driver) {} > > > > +const struct ffa_dev_ops *ffa_dev_ops_get(struct ffa_device *dev) > > +{ > > + return NULL; > > +} > > #endif /* CONFIG_ARM_FFA_TRANSPORT */ > > > > #define ffa_register(driver) \ > > @@ -84,4 +88,34 @@ static inline void ffa_driver_unregister(struct ffa_driver *driver) {} > > #define module_ffa_driver(__ffa_driver) \ > > module_driver(__ffa_driver, ffa_register, ffa_unregister) > > > > +/* FFA transport related */ > > +struct ffa_partition_info { > > + u16 id; > > + u16 exec_ctxt; > > +/* partition supports receipt of direct requests */ > > +#define FFA_PARTITION_DIRECT_RECV BIT(0) > > +/* partition can send direct requests. */ > > +#define FFA_PARTITION_DIRECT_SEND BIT(1) > > +/* partition can send and receive indirect messages. */ > > +#define FFA_PARTITION_INDIRECT_MSG BIT(2) > > + u32 properties; > > +}; > > + > > +struct ffa_send_direct_data { > > + unsigned long data0; > > + unsigned long data1; > > + unsigned long data2; > > + unsigned long data3; > > + unsigned long data4; > > +}; > A comment describing which register index these maps to would be helpful. > > > + > > +struct ffa_dev_ops { > > + u32 (*api_version_get)(void); > > + u16 (*partition_id_get)(struct ffa_device *dev); > > + int (*partition_info_get)(const char *uuid_str, > > + struct ffa_partition_info *buffer); > > + int (*sync_send_receive)(struct ffa_device *dev, u16 ep, > > + struct ffa_send_direct_data *data); > ep can be read directly from dev->vm_id > We may need a way to indicate if we're to use the 32bit or 64bit > calling convention. OP-TEE depends on being able to use 32bit calls > here. > > Cheers, > Jens > > > +}; > > + > > #endif /* _LINUX_ARM_FFA_H */ > > -- > > 2.25.1 > >
(sorry for late reply) On Fri, Dec 11, 2020 at 11:45:08AM +0100, Jens Wiklander wrote: > On Fri, Dec 4, 2020 at 1:11 PM Sudeep Holla <sudeep.holla@arm.com> wrote: [...] Agreed on all the comments, so have just removed those context. > We may need a way to indicate if we're to use the 32bit or 64bit > calling convention. OP-TEE depends on being able to use 32bit calls > here. I assume it would be OP-TEE indicating it would like to use 32-bit. I am thinking of API from the driver would be like: int (*32bit_mode_only_set)(struct ffa_device *dev); Let me know if that works for you. FF-A driver has no other way to evaluate that and I really don't like that in DT 😉 -- Regards, Sudeep
On Tue, Jan 12, 2021 at 06:04:14PM +0000, Sudeep Holla wrote: > (sorry for late reply) > > On Fri, Dec 11, 2020 at 11:45:08AM +0100, Jens Wiklander wrote: > > On Fri, Dec 4, 2020 at 1:11 PM Sudeep Holla <sudeep.holla@arm.com> wrote: > > [...] > > Agreed on all the comments, so have just removed those context. > > > We may need a way to indicate if we're to use the 32bit or 64bit > > calling convention. OP-TEE depends on being able to use 32bit calls > > here. > > I assume it would be OP-TEE indicating it would like to use 32-bit. > I am thinking of API from the driver would be like: > > int (*32bit_mode_only_set)(struct ffa_device *dev); > > Let me know if that works for you. FF-A driver has no other way to evaluate > that and I really don't like that in DT 😉 That should work for OP-TEE. Thanks, Jens
On Mon, Dec 07, 2020 at 01:30:18PM +0100, Jens Wiklander wrote: > Hi Sudeep, > > Some comments below. > > On Fri, Dec 04, 2020 at 12:11:36PM +0000, Sudeep Holla wrote: [...] > > + > > +int ffa_setup_partitions(struct device_node *np) > > +{ > > + int ret; > > + struct device_node *child; > > + struct ffa_device *ffa_dev; > > + struct ffa_partition_info pbuf; > > + const char *p_uuid, *pfx = "Ignoring FFA partition"; > > + uuid_t uuid = UUID_INIT(0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0); > > + > > + for_each_child_of_node(np, child) { > > The spec says: > – If the Nil UUID is specified at the Non-secure virtual FF-A instance, > the Hypervisor must provide information for partitions resident in both > security states. > That was my initial understanding of the specification. However I was told it is designed mostly for Non-Secure Hypervisor for getting the list of secure partitions and help in allocation of non-secure partition/endpoint IDs. This has been topic of discussion recently. > Doesn't that make this redundant? > Not exactly. One main reason why the discovery API (get partition info) is not much useful here in this context is that it lacks UUID and hence not much of use unless you know UUID. The result we get for Nil UUID needs to be mapped to UUID and I can't think of anyway to do so outside the scope of the spec. I have raised it but not sure if there is a strong requirement to add it. Similarly, I would have like 32 vs 64 bit capable partition info from there ideally, but we are here now. -- Regards, Sudeep
On Fri, Dec 11, 2020 at 11:59:40AM +0100, Jens Wiklander wrote: > One more comment below. > > On Fri, Dec 11, 2020 at 11:45 AM Jens Wiklander > <jens.wiklander@linaro.org> wrote: > > > > Hi Sudeep, > > > > Some more comments below. > > > > On Fri, Dec 4, 2020 at 1:11 PM Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > > > Parse the FFA nodes from the device-tree and register all the partitions > > > whose services will be used in the kernel. > > > > > > In order to also enable in-kernel users of FFA interface, let us add > > > simple set of operations for such devices. > > > > > > The in-kernel users are registered without the character device interface. > > > > > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > > > --- > > > drivers/firmware/arm_ffa/common.h | 2 + > > > drivers/firmware/arm_ffa/driver.c | 186 ++++++++++++++++++++++++++++++ > > > include/linux/arm_ffa.h | 36 +++++- > > > 3 files changed, 223 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/firmware/arm_ffa/common.h b/drivers/firmware/arm_ffa/common.h > > > index d019348bf67d..eb1371c2b2b8 100644 > > > --- a/drivers/firmware/arm_ffa/common.h > > > +++ b/drivers/firmware/arm_ffa/common.h > > > @@ -6,6 +6,7 @@ > > > #ifndef _FFA_COMMON_H > > > #define _FFA_COMMON_H > > > > > > +#include <linux/arm_ffa.h> > > > #include <linux/arm-smccc.h> > > > #include <linux/err.h> > > > > > > @@ -17,6 +18,7 @@ typedef ffa_res_t > > > > > > int __init arm_ffa_bus_init(void); > > > void __exit arm_ffa_bus_exit(void); > > > +bool ffa_device_is_valid(struct ffa_device *ffa_dev); > > > > > > #ifdef CONFIG_ARM_FFA_SMCCC > > > int __init ffa_transport_init(ffa_fn **invoke_ffa_fn); > > > diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c > > > index 257b331d781c..3e4ba841dbf8 100644 > > > --- a/drivers/firmware/arm_ffa/driver.c > > > +++ b/drivers/firmware/arm_ffa/driver.c > > > @@ -24,9 +24,13 @@ > > > > > > #include <linux/arm_ffa.h> > > > #include <linux/bitfield.h> > > > +#include <linux/device.h> > > > #include <linux/io.h> > > > +#include <linux/kernel.h> > > > #include <linux/module.h> > > > +#include <linux/of.h> > > > #include <linux/slab.h> > > > +#include <linux/uuid.h> > > > > > > #include "common.h" > > > > > > @@ -179,6 +183,20 @@ static int ffa_version_check(u32 *version) > > > return 0; > > > } > > > > > > +static int ffa_rx_release(void) > > > +{ > > > + ffa_res_t ret; > > > + > > > + ret = invoke_ffa_fn(FFA_RX_RELEASE, 0, 0, 0, 0, 0, 0, 0); > > > + > > > + if (ret.a0 == FFA_ERROR) > > > + return ffa_to_linux_errno((int)ret.a2); > > > + > > > + /* check for ret.a0 == FFA_RX_RELEASE ? */ > > > + > > > + return 0; > > > +} > > > + > > > static int ffa_rxtx_map(phys_addr_t tx_buf, phys_addr_t rx_buf, u32 pg_cnt) > > > { > > > ffa_res_t ret; > > > @@ -203,6 +221,50 @@ static int ffa_rxtx_unmap(u16 vm_id) > > > return 0; > > > } > > > > > > +static int __ffa_partition_info_get(u32 uuid0, u32 uuid1, u32 uuid2, u32 uuid3, > > > + struct ffa_partition_info **buffer) > > > +{ > > > + int count; > > > + ffa_res_t partition_info; > > > + > > > + mutex_lock(&drv_info->rx_lock); > > > + partition_info = invoke_ffa_fn(FFA_PARTITION_INFO_GET, uuid0, uuid1, > > > + uuid2, uuid3, 0, 0, 0); > > > + > > > + if (partition_info.a0 == FFA_ERROR) > > > + return ffa_to_linux_errno((int)partition_info.a2); > > > + > > > + count = partition_info.a2; > > > + > > > + if (buffer) > > > + memcpy(*buffer, drv_info->rx_buffer, sizeof(*buffer) * count); > > > + > > > + ffa_rx_release(); > > > + > > > + mutex_unlock(&drv_info->rx_lock); > > > + > > > + return count; > > > +} > > > + > > > +static int ffa_partition_probe(const char *uuid_str, > > > + struct ffa_partition_info *buffer) > > > +{ > > > + int count; > > > + uuid_t uuid; > > > + u32 uuid0_4[4] = { 0 }; > > > + > > > + if (uuid_parse(uuid_str, &uuid)) { > > > + pr_err("invalid uuid (%s)\n", uuid_str); > > > + return -ENODEV; > > > + } > > > + > > > + export_uuid((u8 *)uuid0_4, &uuid); > > > + count = __ffa_partition_info_get(uuid0_4[0], uuid0_4[1], uuid0_4[2], > > > + uuid0_4[3], &buffer); > Wrong byte order? > According to section 5.3 of the SMCCC, UUIDs are returned as a single > 128-bit value using the SMC32 calling convention. This value is mapped > to argument registers x0-x3 on AArch64 (resp. r0-r3 on AArch32). x0 > for example shall hold bytes 0 to 3, with byte 0 in the low-order > bits. > I need to spend some time to understand the concern here. Initially I agreed with your analysis and then a quick review make be realise it is all OK. I need to check if my understanding is correct again. I thought I will take example and check here itself. UUID: "fd02c9da-306c-48c7-a49c-bbd827ae86ee" UUID[0] UUID[1] UUID[2] UUID[3] (referring uuid0_4 above) dac902fd c7486c30 d8bb9ca4 ee86ae27 It seems correct as per SMCCC convention to me, or am I missing something obvious ? -- Regards, Sudeep
On Wed, Jan 13, 2021 at 10:44 AM Sudeep Holla <sudeep.holla@arm.com> wrote: [...] > > > > +static int ffa_partition_probe(const char *uuid_str, > > > > + struct ffa_partition_info *buffer) > > > > +{ > > > > + int count; > > > > + uuid_t uuid; > > > > + u32 uuid0_4[4] = { 0 }; > > > > + > > > > + if (uuid_parse(uuid_str, &uuid)) { > > > > + pr_err("invalid uuid (%s)\n", uuid_str); > > > > + return -ENODEV; > > > > + } > > > > + > > > > + export_uuid((u8 *)uuid0_4, &uuid); > > > > + count = __ffa_partition_info_get(uuid0_4[0], uuid0_4[1], uuid0_4[2], > > > > + uuid0_4[3], &buffer); > > Wrong byte order? > > According to section 5.3 of the SMCCC, UUIDs are returned as a single > > 128-bit value using the SMC32 calling convention. This value is mapped > > to argument registers x0-x3 on AArch64 (resp. r0-r3 on AArch32). x0 > > for example shall hold bytes 0 to 3, with byte 0 in the low-order > > bits. > > > > I need to spend some time to understand the concern here. Initially I agreed > with your analysis and then a quick review make be realise it is all OK. > I need to check if my understanding is correct again. I thought I will > take example and check here itself. > > UUID: "fd02c9da-306c-48c7-a49c-bbd827ae86ee" > > UUID[0] UUID[1] UUID[2] UUID[3] (referring uuid0_4 above) > dac902fd c7486c30 d8bb9ca4 ee86ae27 > > It seems correct as per SMCCC convention to me, or am I missing something > obvious ? In this example I'd expect the first register to hold 0xfd02c9da regardless of the byte order of the machine. If there is a different byte order in the receiver it will still be received as 0xfd02c9da. That's how I've understood the specification. Cheers, Jens
On Wed, Jan 13, 2021 at 01:30:56PM +0100, Jens Wiklander wrote: > On Wed, Jan 13, 2021 at 10:44 AM Sudeep Holla <sudeep.holla@arm.com> wrote: > [...] > > > > > +static int ffa_partition_probe(const char *uuid_str, > > > > > + struct ffa_partition_info *buffer) > > > > > +{ > > > > > + int count; > > > > > + uuid_t uuid; > > > > > + u32 uuid0_4[4] = { 0 }; > > > > > + > > > > > + if (uuid_parse(uuid_str, &uuid)) { > > > > > + pr_err("invalid uuid (%s)\n", uuid_str); > > > > > + return -ENODEV; > > > > > + } > > > > > + > > > > > + export_uuid((u8 *)uuid0_4, &uuid); > > > > > + count = __ffa_partition_info_get(uuid0_4[0], uuid0_4[1], uuid0_4[2], > > > > > + uuid0_4[3], &buffer); > > > Wrong byte order? > > > According to section 5.3 of the SMCCC, UUIDs are returned as a single > > > 128-bit value using the SMC32 calling convention. This value is mapped > > > to argument registers x0-x3 on AArch64 (resp. r0-r3 on AArch32). x0 > > > for example shall hold bytes 0 to 3, with byte 0 in the low-order > > > bits. > > > > > > > I need to spend some time to understand the concern here. Initially I agreed > > with your analysis and then a quick review make be realise it is all OK. > > I need to check if my understanding is correct again. I thought I will > > take example and check here itself. > > > > UUID: "fd02c9da-306c-48c7-a49c-bbd827ae86ee" IIUC this maps to (as per RFC4122). fd02c9da = time_low (bytes 0-3) 306c48c7 = time_mid & time_hi_and_version (bytes 4-7) a49cbbd8 = clock_seq_hi_and_reserved, clock_seq_low and bytes/octets 0-1 of node (bytes 8-11) 27ae86ee = bytes 2-5 of node (bytes 12-15) SMCCC says: w0 : bytes 0-3 with byte 0 in the lower order bits. w1 : bytes 4-7 with byte 4 in the lower order bits. w2 : bytes 8-11 with byte 8 in the lower order bits. w3 : bytes 12-15 with byte 12 in the lower order bits. This should amount to: w0 = dac902fd w1 = c7486c30 w2 = d8bb9ca4 w3 = ee86ae27 So, even though RFC4122 uses big-endian i.e network byte order. The UUID is encoded as little-endian as per the SMCCC. What do you reckon? cheers, Achin > > > > UUID[0] UUID[1] UUID[2] UUID[3] (referring uuid0_4 above) > > dac902fd c7486c30 d8bb9ca4 ee86ae27 > > > > It seems correct as per SMCCC convention to me, or am I missing something > > obvious ? > > In this example I'd expect the first register to hold 0xfd02c9da > regardless of the byte order of the machine. If there is a different > byte order in the receiver it will still be received as 0xfd02c9da. > That's how I've understood the specification. > > Cheers, > Jens
On Wed, Jan 13, 2021 at 01:58:56PM +0000, Achin Gupta wrote: > On Wed, Jan 13, 2021 at 01:30:56PM +0100, Jens Wiklander wrote: > > On Wed, Jan 13, 2021 at 10:44 AM Sudeep Holla <sudeep.holla@arm.com> wrote: > > [...] > > > > > > +static int ffa_partition_probe(const char *uuid_str, > > > > > > + struct ffa_partition_info *buffer) > > > > > > +{ > > > > > > + int count; > > > > > > + uuid_t uuid; > > > > > > + u32 uuid0_4[4] = { 0 }; > > > > > > + > > > > > > + if (uuid_parse(uuid_str, &uuid)) { > > > > > > + pr_err("invalid uuid (%s)\n", uuid_str); > > > > > > + return -ENODEV; > > > > > > + } > > > > > > + > > > > > > + export_uuid((u8 *)uuid0_4, &uuid); > > > > > > + count = __ffa_partition_info_get(uuid0_4[0], uuid0_4[1], uuid0_4[2], > > > > > > + uuid0_4[3], &buffer); > > > > Wrong byte order? > > > > According to section 5.3 of the SMCCC, UUIDs are returned as a single > > > > 128-bit value using the SMC32 calling convention. This value is mapped > > > > to argument registers x0-x3 on AArch64 (resp. r0-r3 on AArch32). x0 > > > > for example shall hold bytes 0 to 3, with byte 0 in the low-order > > > > bits. > > > > > > > > > > I need to spend some time to understand the concern here. Initially I agreed > > > with your analysis and then a quick review make be realise it is all OK. > > > I need to check if my understanding is correct again. I thought I will > > > take example and check here itself. > > > > > > UUID: "fd02c9da-306c-48c7-a49c-bbd827ae86ee" > > IIUC this maps to (as per RFC4122). > > fd02c9da = time_low (bytes 0-3) > 306c48c7 = time_mid & time_hi_and_version (bytes 4-7) > a49cbbd8 = clock_seq_hi_and_reserved, clock_seq_low and bytes/octets 0-1 of node (bytes 8-11) > 27ae86ee = bytes 2-5 of node (bytes 12-15) > > SMCCC says: > > w0 : bytes 0-3 with byte 0 in the lower order bits. > w1 : bytes 4-7 with byte 4 in the lower order bits. > w2 : bytes 8-11 with byte 8 in the lower order bits. > w3 : bytes 12-15 with byte 12 in the lower order bits. > > This should amount to: > > w0 = dac902fd > w1 = c7486c30 > w2 = d8bb9ca4 > w3 = ee86ae27 > > So, even though RFC4122 uses big-endian i.e network byte order. The UUID is > encoded as little-endian as per the SMCCC. > > What do you reckon? > Thank Achin, that matches my understanding too. I spent some time looking at RFC4122[1] and concluded what we have is fine. @Jens, one thing to note, I am not claiming to support this driver with big-endian kernel. I plan to take that up once we settle with basic support. > cheers, > Achin > > > > > > > UUID[0] UUID[1] UUID[2] UUID[3] (referring uuid0_4 above) > > > dac902fd c7486c30 d8bb9ca4 ee86ae27 > > > Matches w0-w3 above, thanks for detailed explanation > > > It seems correct as per SMCCC convention to me, or am I missing something > > > obvious ? > > > > In this example I'd expect the first register to hold 0xfd02c9da > > regardless of the byte order of the machine. If there is a different > > byte order in the receiver it will still be received as 0xfd02c9da. > > That's how I've understood the specification. > -- Regards, Sudeep [1] https://tools.ietf.org/html/rfc4122
On Wed, Jan 13, 2021 at 6:20 PM Sudeep Holla <sudeep.holla@arm.com> wrote: > > On Wed, Jan 13, 2021 at 01:58:56PM +0000, Achin Gupta wrote: > > On Wed, Jan 13, 2021 at 01:30:56PM +0100, Jens Wiklander wrote: > > > On Wed, Jan 13, 2021 at 10:44 AM Sudeep Holla <sudeep.holla@arm.com> wrote: > > > [...] > > > > > > > +static int ffa_partition_probe(const char *uuid_str, > > > > > > > + struct ffa_partition_info *buffer) > > > > > > > +{ > > > > > > > + int count; > > > > > > > + uuid_t uuid; > > > > > > > + u32 uuid0_4[4] = { 0 }; > > > > > > > + > > > > > > > + if (uuid_parse(uuid_str, &uuid)) { > > > > > > > + pr_err("invalid uuid (%s)\n", uuid_str); > > > > > > > + return -ENODEV; > > > > > > > + } > > > > > > > + > > > > > > > + export_uuid((u8 *)uuid0_4, &uuid); > > > > > > > + count = __ffa_partition_info_get(uuid0_4[0], uuid0_4[1], uuid0_4[2], > > > > > > > + uuid0_4[3], &buffer); > > > > > Wrong byte order? > > > > > According to section 5.3 of the SMCCC, UUIDs are returned as a single > > > > > 128-bit value using the SMC32 calling convention. This value is mapped > > > > > to argument registers x0-x3 on AArch64 (resp. r0-r3 on AArch32). x0 > > > > > for example shall hold bytes 0 to 3, with byte 0 in the low-order > > > > > bits. > > > > > > > > > > > > > I need to spend some time to understand the concern here. Initially I agreed > > > > with your analysis and then a quick review make be realise it is all OK. > > > > I need to check if my understanding is correct again. I thought I will > > > > take example and check here itself. > > > > > > > > UUID: "fd02c9da-306c-48c7-a49c-bbd827ae86ee" > > > > IIUC this maps to (as per RFC4122). > > > > fd02c9da = time_low (bytes 0-3) > > 306c48c7 = time_mid & time_hi_and_version (bytes 4-7) > > a49cbbd8 = clock_seq_hi_and_reserved, clock_seq_low and bytes/octets 0-1 of node (bytes 8-11) > > 27ae86ee = bytes 2-5 of node (bytes 12-15) > > > > SMCCC says: > > > > w0 : bytes 0-3 with byte 0 in the lower order bits. > > w1 : bytes 4-7 with byte 4 in the lower order bits. > > w2 : bytes 8-11 with byte 8 in the lower order bits. > > w3 : bytes 12-15 with byte 12 in the lower order bits. > > > > This should amount to: > > > > w0 = dac902fd > > w1 = c7486c30 > > w2 = d8bb9ca4 > > w3 = ee86ae27 > > > > So, even though RFC4122 uses big-endian i.e network byte order. The UUID is > > encoded as little-endian as per the SMCCC. > > > > What do you reckon? > > > > Thank Achin, that matches my understanding too. I spent some time looking > at RFC4122[1] and concluded what we have is fine. Thanks for the analysis. > > @Jens, one thing to note, I am not claiming to support this driver with > big-endian kernel. I plan to take that up once we settle with basic support. No worries, the OP-TEE driver doesn't support that either. Cheers, Jens > > > cheers, > > Achin > > > > > > > > > > UUID[0] UUID[1] UUID[2] UUID[3] (referring uuid0_4 above) > > > > dac902fd c7486c30 d8bb9ca4 ee86ae27 > > > > > > Matches w0-w3 above, thanks for detailed explanation > > > > > It seems correct as per SMCCC convention to me, or am I missing something > > > > obvious ? > > > > > > In this example I'd expect the first register to hold 0xfd02c9da > > > regardless of the byte order of the machine. If there is a different > > > byte order in the receiver it will still be received as 0xfd02c9da. > > > That's how I've understood the specification. > > > > -- > Regards, > Sudeep > > [1] https://tools.ietf.org/html/rfc4122
diff --git a/drivers/firmware/arm_ffa/common.h b/drivers/firmware/arm_ffa/common.h index d019348bf67d..eb1371c2b2b8 100644 --- a/drivers/firmware/arm_ffa/common.h +++ b/drivers/firmware/arm_ffa/common.h @@ -6,6 +6,7 @@ #ifndef _FFA_COMMON_H #define _FFA_COMMON_H +#include <linux/arm_ffa.h> #include <linux/arm-smccc.h> #include <linux/err.h> @@ -17,6 +18,7 @@ typedef ffa_res_t int __init arm_ffa_bus_init(void); void __exit arm_ffa_bus_exit(void); +bool ffa_device_is_valid(struct ffa_device *ffa_dev); #ifdef CONFIG_ARM_FFA_SMCCC int __init ffa_transport_init(ffa_fn **invoke_ffa_fn); diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c index 257b331d781c..3e4ba841dbf8 100644 --- a/drivers/firmware/arm_ffa/driver.c +++ b/drivers/firmware/arm_ffa/driver.c @@ -24,9 +24,13 @@ #include <linux/arm_ffa.h> #include <linux/bitfield.h> +#include <linux/device.h> #include <linux/io.h> +#include <linux/kernel.h> #include <linux/module.h> +#include <linux/of.h> #include <linux/slab.h> +#include <linux/uuid.h> #include "common.h" @@ -179,6 +183,20 @@ static int ffa_version_check(u32 *version) return 0; } +static int ffa_rx_release(void) +{ + ffa_res_t ret; + + ret = invoke_ffa_fn(FFA_RX_RELEASE, 0, 0, 0, 0, 0, 0, 0); + + if (ret.a0 == FFA_ERROR) + return ffa_to_linux_errno((int)ret.a2); + + /* check for ret.a0 == FFA_RX_RELEASE ? */ + + return 0; +} + static int ffa_rxtx_map(phys_addr_t tx_buf, phys_addr_t rx_buf, u32 pg_cnt) { ffa_res_t ret; @@ -203,6 +221,50 @@ static int ffa_rxtx_unmap(u16 vm_id) return 0; } +static int __ffa_partition_info_get(u32 uuid0, u32 uuid1, u32 uuid2, u32 uuid3, + struct ffa_partition_info **buffer) +{ + int count; + ffa_res_t partition_info; + + mutex_lock(&drv_info->rx_lock); + partition_info = invoke_ffa_fn(FFA_PARTITION_INFO_GET, uuid0, uuid1, + uuid2, uuid3, 0, 0, 0); + + if (partition_info.a0 == FFA_ERROR) + return ffa_to_linux_errno((int)partition_info.a2); + + count = partition_info.a2; + + if (buffer) + memcpy(*buffer, drv_info->rx_buffer, sizeof(*buffer) * count); + + ffa_rx_release(); + + mutex_unlock(&drv_info->rx_lock); + + return count; +} + +static int ffa_partition_probe(const char *uuid_str, + struct ffa_partition_info *buffer) +{ + int count; + uuid_t uuid; + u32 uuid0_4[4] = { 0 }; + + if (uuid_parse(uuid_str, &uuid)) { + pr_err("invalid uuid (%s)\n", uuid_str); + return -ENODEV; + } + + export_uuid((u8 *)uuid0_4, &uuid); + count = __ffa_partition_info_get(uuid0_4[0], uuid0_4[1], uuid0_4[2], + uuid0_4[3], &buffer); + + return count; +} + #define VM_ID_MASK GENMASK(15, 0) static int ffa_id_get(u16 *vm_id) { @@ -218,9 +280,125 @@ static int ffa_id_get(u16 *vm_id) return 0; } +static int ffa_msg_send_direct_req(u16 src_id, u16 dst_id, + struct ffa_send_direct_data *data) +{ + u32 src_dst_ids = PACK_TARGET_INFO(src_id, dst_id); + ffa_res_t ret; + + ret = invoke_ffa_fn(FFA_FN_NATIVE(MSG_SEND_DIRECT_REQ), src_dst_ids, 0, + data->data0, data->data1, data->data2, + data->data3, data->data4); + + while (ret.a0 == FFA_INTERRUPT) + ret = invoke_ffa_fn(FFA_RUN, ret.a1, 0, 0, 0, 0, 0, 0); + if (ret.a0 == FFA_ERROR) + return ffa_to_linux_errno((int)ret.a2); + + if (ret.a0 == FFA_FN_NATIVE(MSG_SEND_DIRECT_RESP)) { + data->data0 = ret.a3; + data->data1 = ret.a4; + data->data2 = ret.a5; + data->data3 = ret.a6; + data->data4 = ret.a7; + } + + return 0; +} + +static u32 ffa_api_version_get(void) +{ + return drv_info->version; +} + +static u16 ffa_partition_id_get(struct ffa_device *dev) +{ + return dev->vm_id; +} + +static int ffa_partition_info_get(const char *uuid_str, + struct ffa_partition_info *buffer) +{ + if (ffa_partition_probe(uuid_str, buffer) == 1) + return 0; + + return -ENOENT; +} + +static int ffa_sync_send_receive(struct ffa_device *dev, u16 ep, + struct ffa_send_direct_data *data) +{ + return ffa_msg_send_direct_req(dev->vm_id, ep, data); +} + +static const struct ffa_dev_ops ffa_ops = { + .api_version_get = ffa_api_version_get, + .partition_id_get = ffa_partition_id_get, + .partition_info_get = ffa_partition_info_get, + .sync_send_receive = ffa_sync_send_receive, +}; + +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); + +int ffa_setup_partitions(struct device_node *np) +{ + int ret; + struct device_node *child; + struct ffa_device *ffa_dev; + struct ffa_partition_info pbuf; + const char *p_uuid, *pfx = "Ignoring FFA partition"; + uuid_t uuid = UUID_INIT(0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0); + + for_each_child_of_node(np, child) { + if (!of_device_is_compatible(child, "arm,ffa-1.0-partition")) { + of_node_put(child); + continue; + } + + if (of_property_read_string(child, "uuid", &p_uuid)) { + pr_err("%s: failed to parse \"uuid\" property\n", pfx); + of_node_put(child); + continue; + } + + of_node_put(child); + + if (uuid_parse(p_uuid, &uuid)) { + pr_err("%s: invalid \"uuid\" property (%s)\n", + pfx, p_uuid); + continue; + } + + ret = ffa_partition_probe(p_uuid, &pbuf); + if (ret != 1) { + pr_err("%s: %s partition info probe failed\n", + pfx, p_uuid); + return -EINVAL; + } + + ffa_dev = ffa_device_register(p_uuid, pbuf.id); + if (!ffa_dev) { + pr_err("%s: failed to register %s\n", pfx, p_uuid); + continue; + } + + ffa_dev_set_drvdata(ffa_dev, drv_info); + } + + return 0; +} + static int __init ffa_init(void) { int ret; + struct device_node *np; ret = arm_ffa_bus_init(); if (ret) @@ -267,6 +445,14 @@ static int __init ffa_init(void) mutex_init(&drv_info->rx_lock); mutex_init(&drv_info->tx_lock); + /* Set up all the partitions */ + np = of_find_compatible_node(NULL, NULL, "arm,ffa-1.0"); + if (!np) + return 0; + + ffa_setup_partitions(np); + of_node_put(np); + return 0; free_pages: if (drv_info->tx_buffer) diff --git a/include/linux/arm_ffa.h b/include/linux/arm_ffa.h index 3defddfb40fc..8604c48289ce 100644 --- a/include/linux/arm_ffa.h +++ b/include/linux/arm_ffa.h @@ -6,7 +6,6 @@ #ifndef _LINUX_ARM_FFA_H #define _LINUX_ARM_FFA_H -#include <linux/cdev.h> #include <linux/device.h> #include <linux/module.h> #include <linux/types.h> @@ -47,6 +46,7 @@ void ffa_device_unregister(struct ffa_device *ffa_dev); int ffa_driver_register(struct ffa_driver *driver, struct module *owner, const char *mod_name); void ffa_driver_unregister(struct ffa_driver *driver); +const struct ffa_dev_ops *ffa_dev_ops_get(struct ffa_device *dev); #else static inline @@ -66,6 +66,10 @@ ffa_driver_register(struct ffa_driver *driver, struct module *owner, static inline void ffa_driver_unregister(struct ffa_driver *driver) {} +const struct ffa_dev_ops *ffa_dev_ops_get(struct ffa_device *dev) +{ + return NULL; +} #endif /* CONFIG_ARM_FFA_TRANSPORT */ #define ffa_register(driver) \ @@ -84,4 +88,34 @@ static inline void ffa_driver_unregister(struct ffa_driver *driver) {} #define module_ffa_driver(__ffa_driver) \ module_driver(__ffa_driver, ffa_register, ffa_unregister) +/* FFA transport related */ +struct ffa_partition_info { + u16 id; + u16 exec_ctxt; +/* partition supports receipt of direct requests */ +#define FFA_PARTITION_DIRECT_RECV BIT(0) +/* partition can send direct requests. */ +#define FFA_PARTITION_DIRECT_SEND BIT(1) +/* partition can send and receive indirect messages. */ +#define FFA_PARTITION_INDIRECT_MSG BIT(2) + u32 properties; +}; + +struct ffa_send_direct_data { + unsigned long data0; + unsigned long data1; + unsigned long data2; + unsigned long data3; + unsigned long data4; +}; + +struct ffa_dev_ops { + u32 (*api_version_get)(void); + u16 (*partition_id_get)(struct ffa_device *dev); + int (*partition_info_get)(const char *uuid_str, + struct ffa_partition_info *buffer); + int (*sync_send_receive)(struct ffa_device *dev, u16 ep, + struct ffa_send_direct_data *data); +}; + #endif /* _LINUX_ARM_FFA_H */
Parse the FFA nodes from the device-tree and register all the partitions whose services will be used in the kernel. In order to also enable in-kernel users of FFA interface, let us add simple set of operations for such devices. The in-kernel users are registered without the character device interface. Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> --- drivers/firmware/arm_ffa/common.h | 2 + drivers/firmware/arm_ffa/driver.c | 186 ++++++++++++++++++++++++++++++ include/linux/arm_ffa.h | 36 +++++- 3 files changed, 223 insertions(+), 1 deletion(-) -- 2.25.1