Hi Abdellatif,
On Fri, 10 Mar 2023 at 06:10, Abdellatif El Khlifi abdellatif.elkhlifi@arm.com wrote:
Add functional test cases for the FF-A core driver
These tests rely on the FF-A Sandbox driver which helps in inspecting the FF-A core driver.
Signed-off-by: Abdellatif El Khlifi abdellatif.elkhlifi@arm.com Cc: Tom Rini trini@konsulko.com Cc: Simon Glass sjg@chromium.org Cc: Ilias Apalodimas ilias.apalodimas@linaro.org Cc: Jens Wiklander jens.wiklander@linaro.org
Changelog:
v9: align FF-A sandbox tests with FF-A discovery through DM
v8:
- update partition_info_get() second argument to be an SP count
- pass NULL device pointer to the FF-A bus discovery and operations
v7: set the tests to use 64-bit direct messaging
v4: align sandbox tests with the new FF-A driver interfaces and new way of error handling
v1: introduce sandbox tests
MAINTAINERS | 1 + test/dm/Makefile | 2 + test/dm/ffa.c | 380 +++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 383 insertions(+) create mode 100644 test/dm/ffa.c
diff --git a/MAINTAINERS b/MAINTAINERS index 2b9d33e964..6939b832e4 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -275,6 +275,7 @@ F: doc/usage/cmd/armffa.rst F: drivers/firmware/arm-ffa/ F: include/arm_ffa.h F: include/sandbox_arm_ffa.h +F: test/dm/ffa.c
ARM FREESCALE IMX M: Stefano Babic sbabic@denx.de diff --git a/test/dm/Makefile b/test/dm/Makefile index 7a79b6e1a2..b554b170db 100644 --- a/test/dm/Makefile +++ b/test/dm/Makefile @@ -1,6 +1,7 @@ # SPDX-License-Identifier: GPL-2.0+ # # Copyright (c) 2013 Google, Inc +# Copyright 2022-2023 Arm Limited and/or its affiliates open-source-office@arm.com
obj-$(CONFIG_UT_DM) += test-dm.o
@@ -85,6 +86,7 @@ obj-$(CONFIG_POWER_DOMAIN) += power-domain.o obj-$(CONFIG_ACPI_PMC) += pmc.o obj-$(CONFIG_DM_PMIC) += pmic.o obj-$(CONFIG_DM_PWM) += pwm.o +obj-$(CONFIG_SANDBOX_FFA) += ffa.o obj-$(CONFIG_QFW) += qfw.o obj-$(CONFIG_RAM) += ram.o obj-y += regmap.o diff --git a/test/dm/ffa.c b/test/dm/ffa.c new file mode 100644 index 0000000000..d978feda72 --- /dev/null +++ b/test/dm/ffa.c @@ -0,0 +1,380 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*
- Functional tests for UCLASS_FFA class
- Copyright 2022-2023 Arm Limited and/or its affiliates open-source-office@arm.com
- Authors:
- Abdellatif El Khlifi abdellatif.elkhlifi@arm.com
- */
+#include <common.h> +#include <console.h> +#include <dm.h> +#include <sandbox_arm_ffa.h> +#include "../../drivers/firmware/arm-ffa/sandbox_arm_ffa_priv.h"
I suppose this is OK, but you could put is in arch/sandbox/include/asm/ perhaps?
+#include <dm/test.h> +#include <test/test.h> +#include <test/ut.h>
+/* Macros */
+#define LOG_MSG_SZ (100) +#define LOG_CMD_SZ (LOG_MSG_SZ * 2)
+/* Functional tests for the UCLASS_FFA */
+static int dm_test_ffa_log(struct unit_test_state *uts, char *msg) +{
char cmd[LOG_CMD_SZ] = {0};console_record_reset();snprintf(cmd, LOG_CMD_SZ, "echo \"%s\"", msg);run_command(cmd, 0);ut_assert_console_end();return 0;+}
+static int check_fwk_version(struct ffa_priv *priv, struct sandbox_ffa_priv *sdx_priv,
struct unit_test_state *uts)+{
if (priv->dscvry_info.fwk_version != sdx_priv->fwk_version) {char msg[LOG_MSG_SZ] = {0};snprintf(msg, LOG_MSG_SZ,"[%s]: Error: framework version: core = 0x%x , sandbox = 0x%x", __func__,priv->dscvry_info.fwk_version,sdx_priv->fwk_version);dm_test_ffa_log(uts, msg);return CMD_RET_FAILURE;}return 0;+}
+static int check_endpoint_id(struct ffa_priv *priv, struct unit_test_state *uts) +{
if (priv->id) {char msg[LOG_MSG_SZ] = {0};snprintf(msg, LOG_MSG_SZ,"[%s]: Error: endpoint id: core = 0x%x", __func__, priv->id);dm_test_ffa_log(uts, msg);return CMD_RET_FAILURE;}return 0;+}
+static int check_rxtxbuf(struct ffa_priv *priv, struct unit_test_state *uts) +{
if (!priv->pair.rxbuf && priv->pair.txbuf) {char msg[LOG_MSG_SZ] = {0};snprintf(msg, LOG_MSG_SZ, "[%s]: Error: rxbuf = %p txbuf = %p", __func__,priv->pair.rxbuf,priv->pair.txbuf);dm_test_ffa_log(uts, msg);return CMD_RET_FAILURE;}return 0;+}
+static int check_features(struct ffa_priv *priv, struct unit_test_state *uts) +{
char msg[LOG_MSG_SZ] = {0};if (priv->pair.rxtx_min_pages != RXTX_4K &&priv->pair.rxtx_min_pages != RXTX_16K &&priv->pair.rxtx_min_pages != RXTX_64K) {snprintf(msg,LOG_MSG_SZ,"[%s]: Error: FFA_RXTX_MAP features = 0x%lx",__func__,priv->pair.rxtx_min_pages);dm_test_ffa_log(uts, msg);return CMD_RET_FAILURE;}return 0;+}
+static int check_rxbuf_mapped_flag(u32 queried_func_id,
u8 rxbuf_mapped,struct unit_test_state *uts)+{
char msg[LOG_MSG_SZ] = {0};switch (queried_func_id) {case FFA_RXTX_MAP:{
Code style is sometimes a bit off. There should not be a {} around this block, for example.
if (rxbuf_mapped)return 0;break;}case FFA_RXTX_UNMAP:{if (!rxbuf_mapped)return 0;break;}default:return CMD_RET_FAILURE;}snprintf(msg, LOG_MSG_SZ, "[%s]: Error: %s mapping issue", __func__,(queried_func_id == FFA_RXTX_MAP ? "FFA_RXTX_MAP" : "FFA_RXTX_UNMAP"));dm_test_ffa_log(uts, msg);return CMD_RET_FAILURE;+}
+static int check_rxbuf_release_flag(u8 rxbuf_owned, struct unit_test_state *uts) +{
if (rxbuf_owned) {char msg[LOG_MSG_SZ] = {0};
Can you drop the assignment?
snprintf(msg, LOG_MSG_SZ, "[%s]: Error: RX buffer not released", __func__);dm_test_ffa_log(uts, msg);return CMD_RET_FAILURE;}return 0;+}
+static int test_ffa_msg_send_direct_req(u16 part_id, struct unit_test_state *uts) +{
struct ffa_send_direct_data msg = {0};u8 cnt;struct udevice *ffa_dev = NULL;struct ffa_bus_ops *ffa_ops = NULL;
Unnecessary assignments aagin. Please look through and clean these up.
uclass_get_device_by_name(UCLASS_FFA, "arm_ffa", &ffa_dev);ut_assertok(!ffa_dev);ffa_ops = (struct ffa_bus_ops *)ffa_bus_get_ops(ffa_dev);ut_assertok(!ffa_ops);
As mentioned earlier the operations should be in the uclass, rather than having one uclass operation that returns the operations. I do wonder which uclass you are copying, to end up with this?
ut_assertok(ffa_ops->sync_send_receive(ffa_dev, part_id, &msg, 1));for (cnt = 0; cnt < sizeof(struct ffa_send_direct_data) / sizeof(u64); cnt++)ut_assertok(((u64 *)&msg)[cnt] != 0xffffffffffffffff);return 0;+}
+static int test_partitions_and_comms(const char *service_uuid,
struct sandbox_ffa_priv *sdx_priv,struct unit_test_state *uts)+{
u32 count = 0;struct ffa_partition_info *parts_info;u32 info_idx, exp_info_idx;int ret;struct udevice *ffa_dev = NULL;struct ffa_bus_ops *ffa_ops = NULL;uclass_get_device_by_name(UCLASS_FFA, "arm_ffa", &ffa_dev);ut_assertok(!ffa_dev);
ut_assertnonnull(ffa_dev)
Also, please just use 'dev' as there is only one in this file
[..]
/* FFA_PARTITION_INFO_GET / FFA_MSG_SEND_DIRECT_REQ */ret = test_partitions_and_comms(svc2_uuid, sdx_priv, uts);ut_assertok(ret != 0);
ut_assertok() is intended to check that something is 0, meaning no error. It is confusing to use it for other things.
Here I think you want to say:
ut_assertok(ret)
?
If you want to assert something special, use ut_assert()
/* Test FFA_RX_RELEASE */rxbuf_flag = 1;ut_assertok(sandbox_ffa_query_core_state(sdx_dev, FFA_RX_RELEASE, &func_data));ut_assertok(check_rxbuf_release_flag(rxbuf_flag, uts));/* Test FFA_RXTX_UNMAP */ut_assertok(ffa_ops->rxtx_unmap(ffa_dev));rxbuf_flag = 1;ut_assertok(sandbox_ffa_query_core_state(sdx_dev, FFA_RXTX_UNMAP, &func_data));ut_assertok(check_rxbuf_mapped_flag(FFA_RXTX_UNMAP, rxbuf_flag, uts));return 0;+}
+DM_TEST(dm_test_ffa_ack, UT_TESTF_SCAN_FDT | UT_TESTF_CONSOLE_REC);
+static int dm_test_ffa_nack(struct unit_test_state *uts) +{
struct ffa_priv *priv = NULL;struct sandbox_ffa_priv *sdx_priv = NULL;const char *valid_svc_uuid = SANDBOX_SERVICE1_UUID;const char *unvalid_svc_uuid = SANDBOX_SERVICE3_UUID;const char *unvalid_svc_uuid_str = SANDBOX_SERVICE4_UUID;struct ffa_send_direct_data msg = {0};int ret;u32 count = 0;u16 part_id = 0;struct udevice *ffa_dev = NULL, *sdx_dev = NULL;struct ffa_bus_ops *ffa_ops = NULL;/* Test probing the FF-A sandbox driver, then binding the FF-A bus driver */uclass_get_device_by_name(UCLASS_FFA, "sandbox_arm_ffa", &sdx_dev);ut_assertok(!sdx_dev);/* Test probing the FF-A bus driver */uclass_get_device_by_name(UCLASS_FFA, "arm_ffa", &ffa_dev);ut_assertok(!ffa_dev);ffa_ops = (struct ffa_bus_ops *)ffa_bus_get_ops(ffa_dev);ut_assertok(!ffa_ops);/* Get a pointer to the FF-A core and sandbox drivers private data */priv = dev_get_priv(ffa_dev);sdx_priv = dev_get_priv(sdx_dev);/* Make sure private data pointers are retrieved */ut_assertok(priv == 0);ut_assertok(sdx_priv == 0);
Those two cannot fail :-)
/* Query partitions count using invalid arguments */ret = ffa_ops->partition_info_get(ffa_dev, unvalid_svc_uuid, NULL, NULL);ut_assertok(ret != -EINVAL);
I think you are using ut_assertok() instead of ut_assert(). Please switch.
This one should be
ut_asserteq(-EINVAL, ret)
/* Query partitions count using an invalid UUID string */ret = ffa_ops->partition_info_get(ffa_dev, unvalid_svc_uuid_str, &count, NULL);ut_assertok(ret != -EINVAL);/* Query partitions count using an invalid UUID (no matching SP) */count = 0;ret = ffa_ops->partition_info_get(ffa_dev, unvalid_svc_uuid, &count, NULL);ut_assertok(count != 0);/* Query partitions count using a valid UUID */count = 0;ret = ffa_ops->partition_info_get(ffa_dev, valid_svc_uuid, &count, NULL);/* Make sure partitions are detected */ut_assertok(ret != 0);ut_assertok(count != SANDBOX_SP_COUNT_PER_VALID_SERVICE);/* Send data to an invalid partition */ret = ffa_ops->sync_send_receive(ffa_dev, part_id, &msg, 1);ut_assertok(ret != -EINVAL);/* Send data to a valid partition */part_id = priv->partitions.descs[0].info.id;ret = ffa_ops->sync_send_receive(ffa_dev, part_id, &msg, 1);ut_assertok(ret != 0);return 0;+}
+DM_TEST(dm_test_ffa_nack, UT_TESTF_SCAN_FDT | UT_TESTF_CONSOLE_REC);
2.25.1
This all looks find once you tidy up the nits.
Regards, Simon