On Thu, May 20, 2021 at 8:53 PM David Gow davidgow@google.com wrote:
On Wed, May 19, 2021 at 4:43 AM Brendan Higgins brendanhiggins@google.com wrote:
On Mon, May 17, 2021 at 8:01 PM David Gow davidgow@google.com wrote:
On Sat, May 15, 2021 at 3:59 PM David Gow davidgow@google.com wrote:
On Sat, May 8, 2021 at 5:31 AM Brendan Higgins brendanhiggins@google.com wrote:
Add basic support to run QEMU via kunit_tool. Add support for i386, x86_64, arm, arm64, and a bunch more.
Signed-off-by: Brendan Higgins brendanhiggins@google.com Tested-by: David Gow davidgow@google.com
Changes since last revision:
- A number of minor obvious issues pointed out by David and Daniel.
- Added facility for merging Kconfigs at Daniel's suggestion.
- Broke out qemu_configs each into their own config file which is loaded dynamically - mostly at David's suggestion.
This seems pretty good to me. I only have one real complaint -- qemu_configs needing to be in a subdirectory of ./tools/testing/kunit -- but am able to tolerate that (even if I'd prefer not to have it) if it's documented properly.
Otherwise, save for a couple of minor nitpicks, this seems good to go.
Reviewed-by: David Gow davidgow@google.com
One thing I forgot to mention is that I'm not 100% sure about the Kconfig fragments being embedded in the qemu_configs. I still kind-of prefer the idea of them being in separate config files. While I don't
I don't feel strongly either way, but I don't have a good idea on how to implement your idea well. How about we leave it for now, and if you decide you really want to do something about it, you can do it?
think this is necessarily a blocker, I did just realise that, by default, kunit.py run --arch=<non-UM-arch> will pull its default .kunitconfig from arch/um/configs/kunit_defconfig, which definitely feels awkward when UML is not otherwise involved.
Hmmm...this file is identical to tools/testing/kunit/configs/all_tests.config. Maybe we should just use that instead?
That sounds like a better plan. It looks like all_tests.config isn't used anywhere, anyway. I might rename it and replace the arch/um/.../kunit_defconfig version in another patch, then.
Some further thoughts below (which range a bit from "practical suggestion" to "overcomplicated ponderings", so don't feel the pressure to take all of them).
(...snip...)
diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py index e22ade9d91ad5..2bd196fd69e5c 100644 --- a/tools/testing/kunit/kunit_kernel.py +++ b/tools/testing/kunit/kunit_kernel.py @@ -6,23 +6,31 @@ # Author: Felix Guo felixguoxiuping@gmail.com # Author: Brendan Higgins brendanhiggins@google.com
+from __future__ import annotations +import importlib.util import logging import subprocess import os import shutil import signal from typing import Iterator +from typing import Optional
from contextlib import ExitStack
+from collections import namedtuple
import kunit_config import kunit_parser +import qemu_config
KCONFIG_PATH = '.config' KUNITCONFIG_PATH = '.kunitconfig' DEFAULT_KUNITCONFIG_PATH = 'arch/um/configs/kunit_defconfig'
This being in arch/um doesn't seem great if its being used for non-UML builds. Is it worth either: (a) moving this somewhere else (e.g., tools/testing/kunit/configs as with the BROKEN_ALLCONFIG_PATH beflow), or
How about we use: tools/testing/kunit/configs/all_tests.config ? The file is identical.
Yeah: I'm not thrilled with the name all_tests.config, but since it doesn't appear to be being used anywhere, I might just rename it in another patch.
(b) giving each architecture its own kunit_defconfig, possibly in place of the qemuconfig member of QemuArchParams
I'm leaning towards (b), which solves two different sources of ugliness in one go, though it would appear to have the downside that the default .kunitconfig could end up being architecture specific, which isn't great.
Yeah, I am not a fan of trying to solve that problem in this patchset. This is sounding more and more like what should be follow-on work to me.
Yeah, I'm not sure exactly what that should look like yet, anyway.
Let's keep things as they are in this patch. I'll put a follow-up patch to use all_tests.config rather than the arch/um one (possibly as part of my "default to ALL_TESTS" patchset), and if we think of something better that is more architecture specific, we'll do that.
BROKEN_ALLCONFIG_PATH = 'tools/testing/kunit/configs/broken_on_uml.config' OUTFILE_PATH = 'test.log' +ABS_TOOL_PATH = os.path.abspath(os.path.dirname(__file__)) +QEMU_CONFIGS_DIR = os.path.join(ABS_TOOL_PATH, 'qemu_configs')
(...snip...)
diff --git a/tools/testing/kunit/qemu_config.py b/tools/testing/kunit/qemu_config.py new file mode 100644 index 0000000000000..aff1fe0442dbc --- /dev/null +++ b/tools/testing/kunit/qemu_config.py @@ -0,0 +1,17 @@ +# SPDX-License-Identifier: GPL-2.0 +# +# Collection of configs for building non-UML kernels and running them on QEMU. +# +# Copyright (C) 2021, Google LLC. +# Author: Brendan Higgins brendanhiggins@google.com
+from collections import namedtuple
+QemuArchParams = namedtuple('QemuArchParams', ['linux_arch',
'qemuconfig',As mentioned, I'm not thrilled about keeping the Kconfig inline here, and would kind-of prefer it to be in another file. I could live with it if I have to, though. Regardless, 'qemuconfig' is not a
It will be fixed in the next revision.
super-descriptive name, particularly as it's not clear if this is configuring QEMU (no, that's extra_qemu_params'), or configuring the kernel for QEMU compatibility.
Any suggestions on a better name? qemu_build_config_path? These configs contain configs for configuring, building, and running kernels on QEMU.
I don't think we need "qemu" in the name, as this is already part of the QemuArchParams struct, and isn't a qemu config, but a kernel one.
Something along the lines of "kernel_config" (or just "kconfig") maybe?
Doh, I got lost at some point and in my last email I thought you were talking about the name QemuArchParams, not the qemuconfig field within that struct. Looking back it is entirely clear to me that that is what you were talking about, but for some reason I had a brainfart in my last response. So yes, your suggestion sounds very reasonable. Will fix.
'qemu_arch','kernel_path','kernel_command_line','extra_qemu_params'])Nit: newline at end of file.
diff --git a/tools/testing/kunit/qemu_configs/alpha.py b/tools/testing/kunit/qemu_configs/alpha.py new file mode 100644 index 0000000000000..2cc64f848ca2c --- /dev/null +++ b/tools/testing/kunit/qemu_configs/alpha.py @@ -0,0 +1,10 @@ +from ..qemu_config import QemuArchParams
+QEMU_ARCH = QemuArchParams(linux_arch='alpha',
qemuconfig='''+CONFIG_SERIAL_8250=y +CONFIG_SERIAL_8250_CONSOLE=y''',
If these were in a separate file, they could be shared across alpha, i386, x86_64, etc. Of course, that wouldn't gel well with putting them in arch/.../config. If there were some way of listing multiple files, it could form part of the config for several more architectures, though that's probably overcomplicating things.
Yeah, like I said, I have sympathy for what you are saying here, but it really feels like something that can and should be addressed in follow on patches. We could totally address this issue later by expanding this field to take either a string containing a Kconfig, or a path to an external Kconfig; if we do so, it won't cause any migration issues in the future.
Yeah: I think we can solve this if it actually becomes a problem. No need to change anything here.
qemu_arch='alpha',kernel_path='arch/alpha/boot/vmlinux',kernel_command_line='console=ttyS0',extra_qemu_params=[''])diff --git a/tools/testing/kunit/qemu_configs/arm.py b/tools/testing/kunit/qemu_configs/arm.py new file mode 100644 index 0000000000000..29a043b0531a0 --- /dev/null +++ b/tools/testing/kunit/qemu_configs/arm.py @@ -0,0 +1,13 @@ +from ..qemu_config import QemuArchParams
+QEMU_ARCH = QemuArchParams(linux_arch='arm',
qemuconfig='''+CONFIG_ARCH_VIRT=y +CONFIG_SERIAL_AMBA_PL010=y +CONFIG_SERIAL_AMBA_PL010_CONSOLE=y +CONFIG_SERIAL_AMBA_PL011=y +CONFIG_SERIAL_AMBA_PL011_CONSOLE=y''',
Similarly, if in a separate file and there were some multiple-file mechanism, these could mostly be shared between arm & arm64 (ARCH_VIRT being the only problem). Again, probably overcomplicating it at this point though.
Right.
qemu_arch='arm',kernel_path='arch/arm/boot/zImage',kernel_command_line='console=ttyAMA0',extra_qemu_params=['-machine virt'])diff --git a/tools/testing/kunit/qemu_configs/arm64.py b/tools/testing/kunit/qemu_configs/arm64.py new file mode 100644 index 0000000000000..1ba200bc99f0f --- /dev/null +++ b/tools/testing/kunit/qemu_configs/arm64.py @@ -0,0 +1,12 @@ +from ..qemu_config import QemuArchParams
+QEMU_ARCH = QemuArchParams(linux_arch='arm64',
qemuconfig='''+CONFIG_SERIAL_AMBA_PL010=y +CONFIG_SERIAL_AMBA_PL010_CONSOLE=y +CONFIG_SERIAL_AMBA_PL011=y +CONFIG_SERIAL_AMBA_PL011_CONSOLE=y''',
qemu_arch='aarch64',kernel_path='arch/arm64/boot/Image.gz',kernel_command_line='console=ttyAMA0',extra_qemu_params=['-machine virt', '-cpu cortex-a57'])diff --git a/tools/testing/kunit/qemu_configs/i386.py b/tools/testing/kunit/qemu_configs/i386.py new file mode 100644 index 0000000000000..3998af306468e --- /dev/null +++ b/tools/testing/kunit/qemu_configs/i386.py @@ -0,0 +1,10 @@ +from ..qemu_config import QemuArchParams
+QEMU_ARCH = QemuArchParams(linux_arch='i386',
qemuconfig='''+CONFIG_SERIAL_8250=y +CONFIG_SERIAL_8250_CONSOLE=y''',
qemu_arch='x86_64',kernel_path='arch/x86/boot/bzImage',kernel_command_line='console=ttyS0',extra_qemu_params=[''])diff --git a/tools/testing/kunit/qemu_configs/powerpc.py b/tools/testing/kunit/qemu_configs/powerpc.py new file mode 100644 index 0000000000000..46292ce9e368e --- /dev/null +++ b/tools/testing/kunit/qemu_configs/powerpc.py @@ -0,0 +1,12 @@ +from ..qemu_config import QemuArchParams
+QEMU_ARCH = QemuArchParams(linux_arch='powerpc',
qemuconfig='''+CONFIG_PPC64=y +CONFIG_SERIAL_8250=y +CONFIG_SERIAL_8250_CONSOLE=y +CONFIG_HVC_CONSOLE=y''',
qemu_arch='ppc64',kernel_path='vmlinux',kernel_command_line='console=ttyS0',extra_qemu_params=['-M pseries', '-cpu power8'])diff --git a/tools/testing/kunit/qemu_configs/riscv.py b/tools/testing/kunit/qemu_configs/riscv.py new file mode 100644 index 0000000000000..de8c62d465723 --- /dev/null +++ b/tools/testing/kunit/qemu_configs/riscv.py @@ -0,0 +1,31 @@ +from ..qemu_config import QemuArchParams +import os +import os.path +import sys
+GITHUB_OPENSBI_URL = 'https://github.com/qemu/qemu/raw/master/pc-bios/opensbi-riscv64-generic-fw_d...' +OPENSBI_FILE = os.path.basename(GITHUB_OPENSBI_URL)
+if not os.path.isfile(OPENSBI_FILE):
print('\n\nOpenSBI file is not in the current working directory.\n''Would you like me to download it for you from:\n' + GITHUB_OPENSBI_URL + ' ?\n')response = input('yes/[no]: ')if response.strip() == 'yes':os.system('wget ' + GITHUB_OPENSBI_URL)else:sys.exit()+QEMU_ARCH = QemuArchParams(linux_arch='riscv',
qemuconfig='''+CONFIG_SOC_VIRT=y +CONFIG_SERIAL_8250=y +CONFIG_SERIAL_8250_CONSOLE=y +CONFIG_SERIAL_OF_PLATFORM=y +CONFIG_SERIAL_EARLYCON_RISCV_SBI=y''',
qemu_arch='riscv64',kernel_path='arch/riscv/boot/Image',kernel_command_line='console=ttyS0',extra_qemu_params=['-machine virt','-cpu rv64','-bios opensbi-riscv64-generic-fw_dynamic.bin'])diff --git a/tools/testing/kunit/qemu_configs/s390.py b/tools/testing/kunit/qemu_configs/s390.py new file mode 100644 index 0000000000000..04c90332f1098 --- /dev/null +++ b/tools/testing/kunit/qemu_configs/s390.py @@ -0,0 +1,14 @@ +from ..qemu_config import QemuArchParams
+QEMU_ARCH = QemuArchParams(linux_arch='s390',
qemuconfig='''+CONFIG_EXPERT=y +CONFIG_TUNE_ZEC12=y +CONFIG_NUMA=y +CONFIG_MODULES=y''',
qemu_arch='s390x',kernel_path='arch/s390/boot/bzImage',kernel_command_line='console=ttyS0',extra_qemu_params=['-machine s390-ccw-virtio','-cpu qemu',])diff --git a/tools/testing/kunit/qemu_configs/sparc.py b/tools/testing/kunit/qemu_configs/sparc.py new file mode 100644 index 0000000000000..f26b5f27cc5a1 --- /dev/null +++ b/tools/testing/kunit/qemu_configs/sparc.py @@ -0,0 +1,10 @@ +from ..qemu_config import QemuArchParams
+QEMU_ARCH = QemuArchParams(linux_arch='sparc',
qemuconfig='''+CONFIG_SERIAL_8250=y +CONFIG_SERIAL_8250_CONSOLE=y''',
qemu_arch='sparc',kernel_path='arch/sparc/boot/zImage',kernel_command_line='console=ttyS0 mem=256M',extra_qemu_params=['-m 256'])diff --git a/tools/testing/kunit/qemu_configs/x86_64.py b/tools/testing/kunit/qemu_configs/x86_64.py new file mode 100644 index 0000000000000..bd5ab733b92ac --- /dev/null +++ b/tools/testing/kunit/qemu_configs/x86_64.py @@ -0,0 +1,10 @@ +from ..qemu_config import QemuArchParams
+QEMU_ARCH = QemuArchParams(linux_arch='x86_64',
qemuconfig='''+CONFIG_SERIAL_8250=y +CONFIG_SERIAL_8250_CONSOLE=y''',
qemu_arch='x86_64',kernel_path='arch/x86/boot/bzImage',kernel_command_line='console=ttyS0',extra_qemu_params=[''])-- 2.31.1.607.g51e8a6a459-goog