From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 14B14C6FD19 for ; Fri, 10 Mar 2023 20:55:44 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id B1EB18600E; Fri, 10 Mar 2023 21:51:15 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=chromium.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (1024-bit key; unprotected) header.d=chromium.org header.i=@chromium.org header.b="UHDM9aMS"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 5F35385FF6; Fri, 10 Mar 2023 21:50:42 +0100 (CET) Received: from mail-yw1-x112e.google.com (mail-yw1-x112e.google.com [IPv6:2607:f8b0:4864:20::112e]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id D77F985FDD for ; Fri, 10 Mar 2023 21:50:34 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=chromium.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=sjg@google.com Received: by mail-yw1-x112e.google.com with SMTP id 00721157ae682-536bbef1c5eso120706977b3.9 for ; Fri, 10 Mar 2023 12:50:34 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; t=1678481434; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=XXqZ8bF3nm6lOlJRIMCQ3dcujqDwac09jYVd7fxvyCg=; b=UHDM9aMSCRUkj7GaO7Pu+Fg15NdRjVCUJ9xoKlYoR1BsIQyJ6Bzv9dXO1C5mkTk5pS Dl/2OvAfDqQz/1TMgKeUfcj2Ws0Wy8VAx5GfeW4QQN2EPZaIhg/CtAhlk8NO7NEc/R6b RhhTO3HqbMAiCFIvQGkjeJHDns23RnVsiAm+0= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1678481434; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=XXqZ8bF3nm6lOlJRIMCQ3dcujqDwac09jYVd7fxvyCg=; b=j9w2JZTNQzTwK1od5vGJqxUjBHA+vV+ucyeUozJIq+gc6E9aIpsiQ+Edc6VTMGh8P8 3htA+aALJvSXUwrLHGiqygUPa/sRcoxHA+/+OY1/HsLYNi+2Ah3yrdkvOJHCaD9wAUhU exiwUuU4t6outG9BjWsIJJZcKIdrODwPTB4wXV0PjW3QpBvMcjocpjh0+LUJEmF+Ijmn LeGAF21asl0MhXMjU4SMUtVUN3npRBhU8Be/QZTIeVxS68EZEh/dDVh1vIxMuwi86T4U WoAWKJmPUGB5iUbeDWyXDazZA+D88+bkZZMnfdVOXIaZdympRnF8CexOAwMb/JP4MFft 196A== X-Gm-Message-State: AO0yUKXE0eQpCs+7BD5nooZh5DE7ODDOJACTXYoADb2SG4UYxtlntTME chKUrGcRRTym8LMqtzTixnSVuq9QlHwWxNvQE6xnAg== X-Google-Smtp-Source: AK7set8Dg6l8VLUNdGVhIt9G0pAIeftb1AFyuUJclLe2kxnRAy49MjcxRuVSo/zh1XuUFNlWBHxWc06MhIiU4z1rnRE= X-Received: by 2002:a81:4422:0:b0:534:eef8:caa9 with SMTP id r34-20020a814422000000b00534eef8caa9mr16645452ywa.8.1678481434191; Fri, 10 Mar 2023 12:50:34 -0800 (PST) MIME-Version: 1.0 References: <20230310141016.137986-1-abdellatif.elkhlifi@arm.com> <20230310141016.137986-8-abdellatif.elkhlifi@arm.com> In-Reply-To: <20230310141016.137986-8-abdellatif.elkhlifi@arm.com> From: Simon Glass Date: Fri, 10 Mar 2023 12:49:59 -0800 Message-ID: Subject: Re: [PATCH v9 07/10] arm_ffa: introduce Sandbox test cases for UCLASS_FFA To: Abdellatif El Khlifi Cc: Drew.Reed@arm.com, achin.gupta@arm.com, ilias.apalodimas@linaro.org, jens.wiklander@linaro.org, nd@arm.com, robh@kernel.org, sudeep.holla@arm.com, trini@konsulko.com, u-boot@lists.denx.de, xueliang.zhong@arm.com Content-Type: text/plain; charset="UTF-8" X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.8 at phobos.denx.de X-Virus-Status: Clean Hi Abdellatif, On Fri, 10 Mar 2023 at 06:10, Abdellatif El Khlifi 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 > Cc: Tom Rini > Cc: Simon Glass > Cc: Ilias Apalodimas > Cc: Jens Wiklander > > --- > 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 > 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 > > 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 > + * > + * Authors: > + * Abdellatif El Khlifi > + */ > + > +#include > +#include > +#include > +#include > +#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 > +#include > +#include > + > +/* 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