Re: [PATCH 2/8] of: Enable DTB loading on UML for KUnit tests

[David Gow]

On Thu, 2 Mar 2023 at 09:38, Stephen Boyd <sboyd@xxxxxxxxxx> wrote:
>
> To fully exercise common clk framework code in KUnit we need to
> associate 'struct device' pointers with 'struct device_node' pointers so
> that things like clk_get() can parse DT nodes for 'clocks' and so that
> clk providers can use DT to provide clks; the most common mode of
> operation for clk providers.
>
> Adding support to KUnit so that it loads a DTB is fairly simple after
> commit b31297f04e86 ("um: Add devicetree support"). We can simply pass a
> pre-compiled deviectree blob (DTB) on the kunit.py commandline and UML
> will load it. The problem is that tests won't know that the commandline
> has been modified, nor that a DTB has been loaded. Take a different
> approach so that tests can skip if a DTB hasn't been loaded.
>
> Reuse the Makefile logic from the OF unittests to build a DTB into the
> kernel. This DTB will be for the mythical machine "linux,kunit", i.e.
> the devicetree for the KUnit "board". In practice, it is a dtsi file
> that will gather includes for kunit tests that rely in part on a
> devicetree being loaded. The devicetree should only be loaded if
> CONFIG_OF_KUNIT=y. Make that a choice config parallel to the existing
> CONFIG_OF_UNITTEST so that only one devicetree can be loaded in the
> system at a time. Similarly, the kernel commandline option to load a
> DTB is ignored if CONFIG_OF_KUNIT is enabled so that only one DTB is
> loaded at a time.

This feels a little bit like it's just papering over the real problem,
which is that there's no way tests can skip themselves if no DTB is
loaded.

That being said, I do think that there's probably some sense in
supporting the compiled-in DTB as well (it's definitely simpler than
patching kunit.py to always pass the extra command-line option in, for
example).
But maybe it'd be nice to have the command-line option override the
built-in one if present.

>
> Add a simple unit test to confirm that the DTB loading worked. Future
> tests will add to the kunit.dtsi file to include their specific test
> nodes.
>
> Cc: Richard Weinberger <richard@xxxxxx>
> Cc: Anton Ivanov <anton.ivanov@xxxxxxxxxxxxxxxxxx>
> Cc: Johannes Berg <johannes@xxxxxxxxxxxxxxxx>
> Cc: Vincent Whitchurch <vincent.whitchurch@xxxxxxxx>
> Cc: Rob Herring <robh+dt@xxxxxxxxxx>
> Cc: Frank Rowand <frowand.list@xxxxxxxxx>
> Signed-off-by: Stephen Boyd <sboyd@xxxxxxxxxx>
> ---
>  arch/um/kernel/dtb.c            | 29 +++++++++++++++--
>  drivers/of/Kconfig              | 26 ++++++++++++++++
>  drivers/of/Makefile             |  1 +
>  drivers/of/kunit/.kunitconfig   |  4 +++
>  drivers/of/kunit/Makefile       |  4 +++
>  drivers/of/kunit/kunit.dtsi     |  8 +++++
>  drivers/of/kunit/kunit.dtso     |  4 +++
>  drivers/of/kunit/uml_dtb_test.c | 55 +++++++++++++++++++++++++++++++++
>  8 files changed, 128 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/of/kunit/.kunitconfig
>  create mode 100644 drivers/of/kunit/Makefile
>  create mode 100644 drivers/of/kunit/kunit.dtsi
>  create mode 100644 drivers/of/kunit/kunit.dtso
>  create mode 100644 drivers/of/kunit/uml_dtb_test.c
>
> diff --git a/arch/um/kernel/dtb.c b/arch/um/kernel/dtb.c
> index 484141b06938..ee63951b12df 100644
> --- a/arch/um/kernel/dtb.c
> +++ b/arch/um/kernel/dtb.c
> @@ -15,9 +15,32 @@ void uml_dtb_init(void)
>         long long size;
>         void *area;
>
> -       area = uml_load_file(dtb, &size);
> -       if (!area)
> -               return;
> +       if (IS_ENABLED(CONFIG_OF_KUNIT)) {
> +               /*
> +                * __dtbo_kunit_begin[] and __dtbo_kunit_end[] are magically
> +                * created by cmd_dt_S_dtbo in scripts/Makefile.lib from the
> +                * drivers/of/kunit/kunit.dtsi file.
> +                */
> +               extern uint8_t __dtbo_kunit_begin[];
> +               extern uint8_t __dtbo_kunit_end[];
> +
> +               size = __dtbo_kunit_end - __dtbo_kunit_begin;
> +               if (!size) {
> +                       pr_warn("%s: kunit testcases is empty\n", __func__);
> +                       return;
> +               }
> +
> +               /* creating copy */
> +               area = memblock_alloc(size, 8);
> +               if (!area)
> +                       return;
> +
> +               memcpy(area, __dtbo_kunit_begin, size);
> +       } else {

I think this should probably override the KUnit dtb if present (so,
try to load the dtb, and fallback to the builtin one). If not, I think
we should at least print a warning if a DTB is specified on the
command-line, but we're not using it.


> +               area = uml_load_file(dtb, &size);
> +               if (!area)
> +                       return;
> +       }
>
>         if (!early_init_dt_scan(area)) {
>                 pr_err("invalid DTB %s\n", dtb);
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index 80b5fd44ab1c..1f968b6a3dde 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -12,6 +12,20 @@ menuconfig OF
>
>  if OF
>
> +choice
> +       prompt "Devicetree Runtime Tests"
> +       default OF_UNITTEST
> +
> +config OF_KUNIT
> +       bool "Devicetree KUnit support" if KUNIT
> +       depends on UML
> +       select IRQ_DOMAIN
> +       select OF_EARLY_FLATTREE
> +       help
> +         This option builds in KUnit test cases that rely on device tree infrastructure.
> +         A fake Device Tree Blob (DTB) is loaded on the UML kernel running KUnit so that
> +         KUnit tests can test device tree dependent code.
> +
>  config OF_UNITTEST
>         bool "Device Tree runtime unit tests"
>         depends on !SPARC
> @@ -25,6 +39,18 @@ config OF_UNITTEST
>
>           If unsure, say N here, but this option is safe to enable.
>
> +endchoice
> +
> +config OF_DTB_KUNIT_TEST
> +       tristate "Devicetree KUnit DTB Test" if !KUNIT_ALL_TESTS
> +       depends on KUNIT
> +       default KUNIT_ALL_TESTS
> +       help
> +         This option builds unit tests for the "linux,kunit" DTB built into
> +         the UML kernel image.
> +
> +         If unsure, say N here, but this option is safe to enable.
> +
>  config OF_ALL_DTBS
>         bool "Build all Device Tree Blobs"
>         depends on COMPILE_TEST
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index e0360a44306e..16eef3fdf60a 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -19,4 +19,5 @@ obj-y += kexec.o
>  endif
>  endif
>
> +obj-y += kunit/
>  obj-$(CONFIG_OF_UNITTEST) += unittest-data/
> diff --git a/drivers/of/kunit/.kunitconfig b/drivers/of/kunit/.kunitconfig
> new file mode 100644
> index 000000000000..1def0ad30d29
> --- /dev/null
> +++ b/drivers/of/kunit/.kunitconfig
> @@ -0,0 +1,4 @@
> +CONFIG_KUNIT=y
> +CONFIG_OF=y
> +CONFIG_OF_KUNIT=y
> +CONFIG_OF_DTB_KUNIT_TEST=y
> diff --git a/drivers/of/kunit/Makefile b/drivers/of/kunit/Makefile
> new file mode 100644
> index 000000000000..ffe0447e1ac7
> --- /dev/null
> +++ b/drivers/of/kunit/Makefile
> @@ -0,0 +1,4 @@
> +# SPDX-License-Identifier: GPL-2.0
> +obj-$(CONFIG_OF_KUNIT) += kunit.dtbo.o
> +
> +obj-$(CONFIG_OF_DTB_KUNIT_TEST) += uml_dtb_test.o
> diff --git a/drivers/of/kunit/kunit.dtsi b/drivers/of/kunit/kunit.dtsi
> new file mode 100644
> index 000000000000..82f6c3e2b8d5
> --- /dev/null
> +++ b/drivers/of/kunit/kunit.dtsi
> @@ -0,0 +1,8 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/ {
> +       model = "KUnit UML";
> +       compatible = "linux,kunit";
> +};
> +
> +/* Include testcase dtsi files below */
> diff --git a/drivers/of/kunit/kunit.dtso b/drivers/of/kunit/kunit.dtso
> new file mode 100644
> index 000000000000..50187e8d1422
> --- /dev/null
> +++ b/drivers/of/kunit/kunit.dtso
> @@ -0,0 +1,4 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/dts-v1/;
> +
> +#include "kunit.dtsi"
> diff --git a/drivers/of/kunit/uml_dtb_test.c b/drivers/of/kunit/uml_dtb_test.c
> new file mode 100644
> index 000000000000..8966c9ebf51f
> --- /dev/null
> +++ b/drivers/of/kunit/uml_dtb_test.c
> @@ -0,0 +1,55 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * KUnit tests for DTB loading on UML
> + */
> +#include <linux/kconfig.h>
> +#include <linux/of.h>
> +#include <linux/of_fdt.h>
> +
> +#include <kunit/test.h>
> +
> +/*
> + * Test that of_machine_is_compatible() returns positive int when loaded DTB
> + * matches.
> + */
> +static void uml_dtb_of_machine_compatible_test(struct kunit *test)
> +{
> +       KUNIT_EXPECT_GT(test, of_machine_is_compatible("linux,kunit"), 0);
> +}
> +
> +/*
> + * Test that of_flat_dt_get_machine_name() returns the expected 'model' from the
> + * loaded DTB.
> + */
> +static void uml_dtb_of_flat_dt_get_machine_name_test(struct kunit *test)
> +{
> +       KUNIT_EXPECT_STREQ(test, of_flat_dt_get_machine_name(), "KUnit UML");
> +}
> +
> +static struct kunit_case uml_dtb_test_cases[] = {
> +       KUNIT_CASE(uml_dtb_of_machine_compatible_test),
> +       KUNIT_CASE(uml_dtb_of_flat_dt_get_machine_name_test),
> +       {}
> +};
> +
> +static int uml_dtb_test_init(struct kunit *test)
> +{
> +       if (!IS_ENABLED(CONFIG_OF_KUNIT))
> +               kunit_skip(test, "requires CONFIG_OF_KUNIT");
> +
> +       return 0;
> +}
> +
> +/*
> + * Test suite to confirm DTB is loaded on UML.
> + */
> +static struct kunit_suite uml_dtb_suite = {
> +       .name = "uml_dtb",
> +       .init = uml_dtb_test_init,
> +       .test_cases = uml_dtb_test_cases,
> +};
> +
> +kunit_test_suites(
> +       &uml_dtb_suite,
> +);
> +MODULE_LICENSE("GPL");
> --
> https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/
> https://git.kernel.org/pub/scm/linux/kernel/git/sboyd/spmi.git
>
> --
> You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@xxxxxxxxxxxxxxxx.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20230302013822.1808711-3-sboyd%40kernel.org.

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature