[PATCH v9 07/10] arm_ffa: introduce Sandbox test cases for UCLASS_FFA

Simon Glass sjg at chromium.org
Fri Mar 10 21:49:59 CET 2023


Hi Abdellatif,

On Fri, 10 Mar 2023 at 06:10, Abdellatif El Khlifi
<abdellatif.elkhlifi at 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 at arm.com>
> Cc: Tom Rini <trini at konsulko.com>
> Cc: Simon Glass <sjg at chromium.org>
> Cc: Ilias Apalodimas <ilias.apalodimas at linaro.org>
> Cc: Jens Wiklander <jens.wiklander at 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 at 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 at 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 at arm.com>
> + *
> + * Authors:
> + *   Abdellatif El Khlifi <abdellatif.elkhlifi at 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


More information about the U-Boot mailing list