* [RFC v2 0/4] kunit: tool: add support for QEMU
@ 2021-04-29 20:51 Brendan Higgins
2021-04-29 20:51 ` [RFC v2 1/4] kunit: Add 'kunit_shutdown' option Brendan Higgins
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Brendan Higgins @ 2021-04-29 20:51 UTC (permalink / raw)
To: shuah, davidgow
Cc: linux-kselftest, kunit-dev, linux-kernel, corbet, linux-doc,
sboyd, keescook, frowand.list, dlatypov, Brendan Higgins
TL;DR: Add support to kunit_tool to dispatch tests via QEMU. Also add
support to immediately shutdown a kernel after running KUnit tests.
Background
----------
KUnit has supported running on all architectures for quite some time;
however, kunit_tool - the script commonly used to invoke KUnit tests -
has only fully supported KUnit run on UML. Its functionality has been
broken up for some time to separate the configure, build, run, and parse
phases making it possible to be used in part on other architectures to a
small extent. Nevertheless, kunit_tool has not supported running tests
on other architectures.
What this patchset does
-----------------------
This patchset introduces first class support to kunit_tool for KUnit to
be run on many popular architectures via QEMU. It does this by adding
two new flags: `--arch` and `--cross_compile`.
`--arch` allows an architecture to be specified by the name the
architecture is given in `arch/`. It uses the specified architecture to
select a minimal amount of Kconfigs and QEMU configs needed for the
architecture to run in QEMU and provide a console from which KTAP
results can be scraped.
`--cross_compile` allows a toolchain prefix to be specified to make
similar to how `CROSS_COMPILE` is used.
Additionally, this patchset revives the previously considered "kunit:
tool: add support for QEMU"[1] patchs. The motivation for this new
kernel command line flags, `kunit_shutdown`, is to better support
running KUnit tests inside of QEMU. For most popular architectures, QEMU
can be made to terminate when the Linux kernel that is being run is
reboted, halted, or powered off. As Kees pointed out in a previous
discussion[2], it is possible to make a kernel initrd that can reboot
the kernel immediately, doing this for every architecture would likely
be infeasible. Instead, just having an option for the kernel to shutdown
when it is done with testing seems a lot simpler, especially since it is
an option which would only available in testing configurations of the
kernel anyway.
What discussion remains for this patchset?
------------------------------------------
The first most obvious thing is settling the debate about
`kunit_shutdown`. If I recall correctly, Kees suggested that it might be
better to just add a new initrd; however, as I mentioned above, now to
support many new architectures, it may be substantially easier to
support this option. So I am hoping with this new usecase, the argument
for `kunit_shutdown` will be more compelling.
The second and likely harder issue is figuring out the best way to
configure and provide configs for running KUnit tests via QEMU. I
provide a pretty primitive way in this patchset which is not super
flexible; for example, for our PPC support we have it set to build big
endian, and POWER8 - we currently don't support a way to change that.
Nevertheless, having sensible defaults is handy too, so we will probably
want to have some support for overriding defaults, while still being
able to have defaults.
[1] http://patches.linaro.org/patch/208336/
[2] https://lkml.org/lkml/2020/6/26/988
Brendan Higgins (3):
Documentation: Add kunit_shutdown to kernel-parameters.txt
kunit: tool: add support for QEMU
Documentation: kunit: document support for QEMU in kunit_tool
David Gow (1):
kunit: Add 'kunit_shutdown' option
.../admin-guide/kernel-parameters.txt | 8 +
Documentation/dev-tools/kunit/usage.rst | 37 +++-
lib/kunit/executor.c | 20 ++
tools/testing/kunit/kunit.py | 33 ++-
tools/testing/kunit/kunit_config.py | 2 +-
tools/testing/kunit/kunit_kernel.py | 209 +++++++++++++++---
tools/testing/kunit/kunit_parser.py | 2 +-
tools/testing/kunit/kunit_tool_test.py | 15 +-
8 files changed, 278 insertions(+), 48 deletions(-)
base-commit: 7af08140979a6e7e12b78c93b8625c8d25b084e2
--
2.31.1.498.g6c1eba8ee3d-goog
^ permalink raw reply [flat|nested] 12+ messages in thread
* [RFC v2 1/4] kunit: Add 'kunit_shutdown' option
2021-04-29 20:51 [RFC v2 0/4] kunit: tool: add support for QEMU Brendan Higgins
@ 2021-04-29 20:51 ` Brendan Higgins
2021-04-29 21:52 ` Daniel Latypov
2021-04-29 20:51 ` [RFC v2 2/4] Documentation: Add kunit_shutdown to kernel-parameters.txt Brendan Higgins
` (2 subsequent siblings)
3 siblings, 1 reply; 12+ messages in thread
From: Brendan Higgins @ 2021-04-29 20:51 UTC (permalink / raw)
To: shuah, davidgow
Cc: linux-kselftest, kunit-dev, linux-kernel, corbet, linux-doc,
sboyd, keescook, frowand.list, dlatypov, Brendan Higgins
From: David Gow <davidgow@google.com>
Add a new kernel command-line option, 'kunit_shutdown', which allows the
user to specify that the kernel poweroff, halt, or reboot after
completing all KUnit tests; this is very handy for running KUnit tests
on UML or a VM so that the UML/VM process exits cleanly immediately
after running all tests without needing a special initramfs.
Signed-off-by: David Gow <davidgow@google.com>
Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
Reviewed-by: Stephen Boyd <sboyd@kernel.org>
---
lib/kunit/executor.c | 20 ++++++++++++++++++++
tools/testing/kunit/kunit_kernel.py | 2 +-
tools/testing/kunit/kunit_parser.py | 2 +-
3 files changed, 22 insertions(+), 2 deletions(-)
diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
index 15832ed446685..7db619624437c 100644
--- a/lib/kunit/executor.c
+++ b/lib/kunit/executor.c
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: GPL-2.0
+#include <linux/reboot.h>
#include <kunit/test.h>
#include <linux/glob.h>
#include <linux/moduleparam.h>
@@ -18,6 +19,9 @@ module_param(filter_glob, charp, 0);
MODULE_PARM_DESC(filter_glob,
"Filter which KUnit test suites run at boot-time, e.g. list*");
+static char *kunit_shutdown;
+core_param(kunit_shutdown, kunit_shutdown, charp, 0644);
+
static struct kunit_suite * const *
kunit_filter_subsuite(struct kunit_suite * const * const subsuite)
{
@@ -82,6 +86,20 @@ static struct suite_set kunit_filter_suites(void)
return filtered;
}
+static void kunit_handle_shutdown(void)
+{
+ if (!kunit_shutdown)
+ return;
+
+ if (!strcmp(kunit_shutdown, "poweroff"))
+ kernel_power_off();
+ else if (!strcmp(kunit_shutdown, "halt"))
+ kernel_halt();
+ else if (!strcmp(kunit_shutdown, "reboot"))
+ kernel_restart(NULL);
+
+}
+
static void kunit_print_tap_header(struct suite_set *suite_set)
{
struct kunit_suite * const * const *suites, * const *subsuite;
@@ -112,6 +130,8 @@ int kunit_run_all_tests(void)
kfree(suite_set.start);
}
+ kunit_handle_shutdown();
+
return 0;
}
diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py
index f309a33256cd3..7d5b77967d48f 100644
--- a/tools/testing/kunit/kunit_kernel.py
+++ b/tools/testing/kunit/kunit_kernel.py
@@ -206,7 +206,7 @@ class LinuxSourceTree(object):
def run_kernel(self, args=None, build_dir='', filter_glob='', timeout=None) -> Iterator[str]:
if not args:
args = []
- args.extend(['mem=1G', 'console=tty'])
+ args.extend(['mem=1G', 'console=tty','kunit_shutdown=halt'])
if filter_glob:
args.append('kunit.filter_glob='+filter_glob)
self._ops.linux_bin(args, timeout, build_dir)
diff --git a/tools/testing/kunit/kunit_parser.py b/tools/testing/kunit/kunit_parser.py
index e8bcc139702e2..8d8d4d70b39dd 100644
--- a/tools/testing/kunit/kunit_parser.py
+++ b/tools/testing/kunit/kunit_parser.py
@@ -49,7 +49,7 @@ class TestStatus(Enum):
kunit_start_re = re.compile(r'TAP version [0-9]+$')
kunit_end_re = re.compile('(List of all partitions:|'
- 'Kernel panic - not syncing: VFS:)')
+ 'Kernel panic - not syncing: VFS:|reboot: System halted)')
def isolate_kunit_output(kernel_output) -> Iterator[str]:
started = False
--
2.31.1.498.g6c1eba8ee3d-goog
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [RFC v2 2/4] Documentation: Add kunit_shutdown to kernel-parameters.txt
2021-04-29 20:51 [RFC v2 0/4] kunit: tool: add support for QEMU Brendan Higgins
2021-04-29 20:51 ` [RFC v2 1/4] kunit: Add 'kunit_shutdown' option Brendan Higgins
@ 2021-04-29 20:51 ` Brendan Higgins
2021-04-29 20:51 ` [RFC v2 3/4] kunit: tool: add support for QEMU Brendan Higgins
2021-04-29 20:51 ` [RFC v2 4/4] Documentation: kunit: document support for QEMU in kunit_tool Brendan Higgins
3 siblings, 0 replies; 12+ messages in thread
From: Brendan Higgins @ 2021-04-29 20:51 UTC (permalink / raw)
To: shuah, davidgow
Cc: linux-kselftest, kunit-dev, linux-kernel, corbet, linux-doc,
sboyd, keescook, frowand.list, dlatypov, Brendan Higgins
Add kunit_shutdown, an option to specify that the kernel shutsdown after
running KUnit tests, to the kernel-parameters.txt documentation.
Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
Reviewed-by: Stephen Boyd <sboyd@kernel.org>
---
Documentation/admin-guide/kernel-parameters.txt | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 04545725f187f..fb2aacb29a4be 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -2233,6 +2233,14 @@
0: force disabled
1: force enabled
+ kunit_shutdown=[KERNEL UNIT TESTING FRAMEWORK] Shutdown kernel after
+ running built-in tests. Tests configured as modules will
+ not be run.
+ Default: (flag not present) don't shutdown
+ poweroff: poweroff the kernel after running tests
+ halt: halt the kernel after running tests
+ reboot: reboot the kernel after running tests
+
kvm.ignore_msrs=[KVM] Ignore guest accesses to unhandled MSRs.
Default is 0 (don't ignore, but inject #GP)
--
2.31.1.498.g6c1eba8ee3d-goog
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [RFC v2 3/4] kunit: tool: add support for QEMU
2021-04-29 20:51 [RFC v2 0/4] kunit: tool: add support for QEMU Brendan Higgins
2021-04-29 20:51 ` [RFC v2 1/4] kunit: Add 'kunit_shutdown' option Brendan Higgins
2021-04-29 20:51 ` [RFC v2 2/4] Documentation: Add kunit_shutdown to kernel-parameters.txt Brendan Higgins
@ 2021-04-29 20:51 ` Brendan Higgins
2021-04-29 23:39 ` Daniel Latypov
2021-05-04 6:08 ` David Gow
2021-04-29 20:51 ` [RFC v2 4/4] Documentation: kunit: document support for QEMU in kunit_tool Brendan Higgins
3 siblings, 2 replies; 12+ messages in thread
From: Brendan Higgins @ 2021-04-29 20:51 UTC (permalink / raw)
To: shuah, davidgow
Cc: linux-kselftest, kunit-dev, linux-kernel, corbet, linux-doc,
sboyd, keescook, frowand.list, dlatypov, Brendan Higgins
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>
---
tools/testing/kunit/kunit.py | 33 +++-
tools/testing/kunit/kunit_config.py | 2 +-
tools/testing/kunit/kunit_kernel.py | 207 +++++++++++++++++++++----
tools/testing/kunit/kunit_tool_test.py | 15 +-
4 files changed, 217 insertions(+), 40 deletions(-)
diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py
index d5144fcb03acd..07ef80062873b 100755
--- a/tools/testing/kunit/kunit.py
+++ b/tools/testing/kunit/kunit.py
@@ -70,10 +70,10 @@ def build_tests(linux: kunit_kernel.LinuxSourceTree,
kunit_parser.print_with_timestamp('Building KUnit Kernel ...')
build_start = time.time()
- success = linux.build_um_kernel(request.alltests,
- request.jobs,
- request.build_dir,
- request.make_options)
+ success = linux.build_kernel(request.alltests,
+ request.jobs,
+ request.build_dir,
+ request.make_options)
build_end = time.time()
if not success:
return KunitResult(KunitStatus.BUILD_FAILURE,
@@ -187,6 +187,14 @@ def add_common_opts(parser) -> None:
help='Path to Kconfig fragment that enables KUnit tests',
metavar='kunitconfig')
+ parser.add_argument('--arch',
+ help='Specifies the architecture to run tests under.',
+ type=str, default='um', metavar='arch')
+
+ parser.add_argument('--cross_compile',
+ help='Sets make\'s CROSS_COMPILE variable.',
+ metavar='cross_compile')
+
def add_build_opts(parser) -> None:
parser.add_argument('--jobs',
help='As in the make command, "Specifies the number of '
@@ -268,7 +276,10 @@ def main(argv, linux=None):
os.mkdir(cli_args.build_dir)
if not linux:
- linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir, kunitconfig_path=cli_args.kunitconfig)
+ linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir,
+ kunitconfig_path=cli_args.kunitconfig,
+ arch=cli_args.arch,
+ cross_compile=cli_args.cross_compile)
request = KunitRequest(cli_args.raw_output,
cli_args.timeout,
@@ -287,7 +298,9 @@ def main(argv, linux=None):
os.mkdir(cli_args.build_dir)
if not linux:
- linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir, kunitconfig_path=cli_args.kunitconfig)
+ linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir,
+ kunitconfig_path=cli_args.kunitconfig,
+ arch=cli_args.arch)
request = KunitConfigRequest(cli_args.build_dir,
cli_args.make_options)
@@ -299,7 +312,9 @@ def main(argv, linux=None):
sys.exit(1)
elif cli_args.subcommand == 'build':
if not linux:
- linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir, kunitconfig_path=cli_args.kunitconfig)
+ linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir,
+ kunitconfig_path=cli_args.kunitconfig,
+ arch=cli_args.arch)
request = KunitBuildRequest(cli_args.jobs,
cli_args.build_dir,
@@ -313,7 +328,9 @@ def main(argv, linux=None):
sys.exit(1)
elif cli_args.subcommand == 'exec':
if not linux:
- linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir)
+ linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir,
+ kunitconfig_path=cli_args.kunitconfig,
+ arch=cli_args.arch)
exec_request = KunitExecRequest(cli_args.timeout,
cli_args.build_dir,
diff --git a/tools/testing/kunit/kunit_config.py b/tools/testing/kunit/kunit_config.py
index 1e2683dcc0e7a..07d76be392544 100644
--- a/tools/testing/kunit/kunit_config.py
+++ b/tools/testing/kunit/kunit_config.py
@@ -53,7 +53,7 @@ class Kconfig(object):
return True
def write_to_file(self, path: str) -> None:
- with open(path, 'w') as f:
+ with open(path, 'a+') as f:
for entry in self.entries():
f.write(str(entry) + '\n')
diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py
index 7d5b77967d48f..b8b3b76aaa17e 100644
--- a/tools/testing/kunit/kunit_kernel.py
+++ b/tools/testing/kunit/kunit_kernel.py
@@ -15,6 +15,8 @@ from typing import Iterator
from contextlib import ExitStack
+from collections import namedtuple
+
import kunit_config
import kunit_parser
@@ -40,6 +42,10 @@ class BuildError(Exception):
class LinuxSourceTreeOperations(object):
"""An abstraction over command line operations performed on a source tree."""
+ def __init__(self, linux_arch, cross_compile):
+ self._linux_arch = linux_arch
+ self._cross_compile = cross_compile
+
def make_mrproper(self) -> None:
try:
subprocess.check_output(['make', 'mrproper'], stderr=subprocess.STDOUT)
@@ -48,12 +54,18 @@ class LinuxSourceTreeOperations(object):
except subprocess.CalledProcessError as e:
raise ConfigError(e.output.decode())
+ def make_arch_qemuconfig(self, build_dir):
+ pass
+
def make_olddefconfig(self, build_dir, make_options) -> None:
- command = ['make', 'ARCH=um', 'olddefconfig']
+ command = ['make', 'ARCH=' + self._linux_arch, 'olddefconfig']
+ if self._cross_compile:
+ command += ['CROSS_COMPILE=' + self._cross_compile]
if make_options:
command.extend(make_options)
if build_dir:
command += ['O=' + build_dir]
+ print(' '.join(command))
try:
subprocess.check_output(command, stderr=subprocess.STDOUT)
except OSError as e:
@@ -61,6 +73,154 @@ class LinuxSourceTreeOperations(object):
except subprocess.CalledProcessError as e:
raise ConfigError(e.output.decode())
+ def make(self, jobs, build_dir, make_options) -> None:
+ command = ['make', 'ARCH=' + self._linux_arch, '--jobs=' + str(jobs)]
+ if make_options:
+ command.extend(make_options)
+ if self._cross_compile:
+ command += ['CROSS_COMPILE=' + self._cross_compile]
+ if build_dir:
+ command += ['O=' + build_dir]
+ print(' '.join(command))
+ try:
+ proc = subprocess.Popen(command,
+ stderr=subprocess.PIPE,
+ stdout=subprocess.DEVNULL)
+ except OSError as e:
+ raise BuildError('Could not call execute make: ' + e)
+ except subprocess.CalledProcessError as e:
+ raise BuildError(e.output)
+ _, stderr = proc.communicate()
+ if proc.returncode != 0:
+ raise BuildError(stderr.decode())
+ if stderr: # likely only due to build warnings
+ print(stderr.decode())
+
+ def run(self, params, timeout, build_dir, outfile) -> None:
+ pass
+
+
+QemuArchParams = namedtuple('QemuArchParams', ['linux_arch',
+ 'qemuconfig',
+ 'qemu_arch',
+ 'kernel_path',
+ 'kernel_command_line',
+ 'extra_qemu_params'])
+
+
+QEMU_ARCHS = {
+ 'i386' : QemuArchParams(linux_arch='i386',
+ qemuconfig='CONFIG_SERIAL_8250=y\nCONFIG_SERIAL_8250_CONSOLE=y',
+ qemu_arch='x86_64',
+ kernel_path='arch/x86/boot/bzImage',
+ kernel_command_line='console=ttyS0',
+ extra_qemu_params=['']),
+ 'x86_64' : QemuArchParams(linux_arch='x86_64',
+ qemuconfig='CONFIG_SERIAL_8250=y\nCONFIG_SERIAL_8250_CONSOLE=y',
+ qemu_arch='x86_64',
+ kernel_path='arch/x86/boot/bzImage',
+ kernel_command_line='console=ttyS0',
+ extra_qemu_params=['']),
+ 'arm' : 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''',
+ qemu_arch='arm',
+ kernel_path='arch/arm/boot/zImage',
+ kernel_command_line='console=ttyAMA0',
+ extra_qemu_params=['-machine virt']),
+ 'arm64' : 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']),
+ 'alpha' : QemuArchParams(linux_arch='alpha',
+ qemuconfig='CONFIG_SERIAL_8250=y\nCONFIG_SERIAL_8250_CONSOLE=y',
+ qemu_arch='alpha',
+ kernel_path='arch/alpha/boot/vmlinux',
+ kernel_command_line='console=ttyS0',
+ extra_qemu_params=['']),
+ 'powerpc' : QemuArchParams(linux_arch='powerpc',
+ qemuconfig='CONFIG_PPC64=y\nCONFIG_SERIAL_8250=y\nCONFIG_SERIAL_8250_CONSOLE=y\nCONFIG_HVC_CONSOLE=y',
+ qemu_arch='ppc64',
+ kernel_path='vmlinux',
+ kernel_command_line='console=ttyS0',
+ extra_qemu_params=['-M pseries', '-cpu power8']),
+ 'riscv' : QemuArchParams(linux_arch='riscv',
+ qemuconfig='CONFIG_SOC_VIRT=y\nCONFIG_SERIAL_8250=y\nCONFIG_SERIAL_8250_CONSOLE=y\nCONFIG_SERIAL_OF_PLATFORM=y\nCONFIG_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']),
+ 's390' : QemuArchParams(linux_arch='s390',
+ qemuconfig='CONFIG_EXPERT=y\nCONFIG_TUNE_ZEC12=y\nCONFIG_NUMA=y\nCONFIG_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',]),
+ 'sparc' : QemuArchParams(linux_arch='sparc',
+ qemuconfig='CONFIG_SERIAL_8250=y\nCONFIG_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']),
+}
+
+class LinuxSourceTreeOperationsQemu(LinuxSourceTreeOperations):
+
+ def __init__(self, qemu_arch_params, cross_compile):
+ super().__init__(linux_arch=qemu_arch_params.linux_arch,
+ cross_compile=cross_compile)
+ self._qemuconfig = qemu_arch_params.qemuconfig
+ self._qemu_arch = qemu_arch_params.qemu_arch
+ self._kernel_path = qemu_arch_params.kernel_path
+ print(self._kernel_path)
+ self._kernel_command_line = qemu_arch_params.kernel_command_line + ' kunit_shutdown=reboot'
+ self._extra_qemu_params = qemu_arch_params.extra_qemu_params
+
+ def make_arch_qemuconfig(self, build_dir):
+ qemuconfig = kunit_config.Kconfig()
+ qemuconfig.parse_from_string(self._qemuconfig)
+ qemuconfig.write_to_file(get_kconfig_path(build_dir))
+
+ def run(self, params, timeout, build_dir, outfile):
+ kernel_path = os.path.join(build_dir, self._kernel_path)
+ qemu_command = ['qemu-system-' + self._qemu_arch,
+ '-nodefaults',
+ '-m', '1024',
+ '-kernel', kernel_path,
+ '-append', '\'' + ' '.join(params + [self._kernel_command_line]) + '\'',
+ '-no-reboot',
+ '-nographic',
+ '-serial stdio'] + self._extra_qemu_params
+ print(' '.join(qemu_command))
+ with open(outfile, 'w') as output:
+ process = subprocess.Popen(' '.join(qemu_command),
+ stdin=subprocess.PIPE,
+ stdout=output,
+ stderr=subprocess.STDOUT,
+ text=True, shell=True)
+ try:
+ process.wait(timeout=timeout)
+ except Exception as e:
+ print(e)
+ process.terminate()
+ return process
+
+class LinuxSourceTreeOperationsUml(LinuxSourceTreeOperations):
+ """An abstraction over command line operations performed on a source tree."""
+
+ def __init__(self):
+ super().__init__(linux_arch='um', cross_compile=None)
+
def make_allyesconfig(self, build_dir, make_options) -> None:
kunit_parser.print_with_timestamp(
'Enabling all CONFIGs for UML...')
@@ -83,32 +243,16 @@ class LinuxSourceTreeOperations(object):
kunit_parser.print_with_timestamp(
'Starting Kernel with all configs takes a few minutes...')
- def make(self, jobs, build_dir, make_options) -> None:
- command = ['make', 'ARCH=um', '--jobs=' + str(jobs)]
- if make_options:
- command.extend(make_options)
- if build_dir:
- command += ['O=' + build_dir]
- try:
- proc = subprocess.Popen(command,
- stderr=subprocess.PIPE,
- stdout=subprocess.DEVNULL)
- except OSError as e:
- raise BuildError('Could not call make command: ' + str(e))
- _, stderr = proc.communicate()
- if proc.returncode != 0:
- raise BuildError(stderr.decode())
- if stderr: # likely only due to build warnings
- print(stderr.decode())
-
- def linux_bin(self, params, timeout, build_dir) -> None:
+ def run(self, params, timeout, build_dir, outfile):
"""Runs the Linux UML binary. Must be named 'linux'."""
linux_bin = get_file_path(build_dir, 'linux')
outfile = get_outfile_path(build_dir)
with open(outfile, 'w') as output:
process = subprocess.Popen([linux_bin] + params,
+ stdin=subprocess.PIPE,
stdout=output,
- stderr=subprocess.STDOUT)
+ stderr=subprocess.STDOUT,
+ text=True)
process.wait(timeout)
def get_kconfig_path(build_dir) -> str:
@@ -123,10 +267,17 @@ def get_outfile_path(build_dir) -> str:
class LinuxSourceTree(object):
"""Represents a Linux kernel source tree with KUnit tests."""
- def __init__(self, build_dir: str, load_config=True, kunitconfig_path='') -> None:
+ def __init__(self, build_dir: str, load_config=True, kunitconfig_path='', arch=None, cross_compile=None) -> None:
signal.signal(signal.SIGINT, self.signal_handler)
-
- self._ops = LinuxSourceTreeOperations()
+ self._ops = None
+ if arch is None or arch == 'um':
+ self._arch = 'um'
+ self._ops = LinuxSourceTreeOperationsUml()
+ elif arch in QEMU_ARCHS:
+ self._arch = arch
+ self._ops = LinuxSourceTreeOperationsQemu(QEMU_ARCHS[arch], cross_compile=cross_compile)
+ else:
+ raise ConfigError(arch + ' is not a valid arch')
if not load_config:
return
@@ -170,6 +321,7 @@ class LinuxSourceTree(object):
os.mkdir(build_dir)
self._kconfig.write_to_file(kconfig_path)
try:
+ self._ops.make_arch_qemuconfig(build_dir)
self._ops.make_olddefconfig(build_dir, make_options)
except ConfigError as e:
logging.error(e)
@@ -192,10 +344,13 @@ class LinuxSourceTree(object):
print('Generating .config ...')
return self.build_config(build_dir, make_options)
- def build_um_kernel(self, alltests, jobs, build_dir, make_options) -> bool:
+ def build_kernel(self, alltests, jobs, build_dir, make_options) -> bool:
try:
if alltests:
+ if self._arch != 'um':
+ raise ConfigError('Only the "um" arch is supported for alltests')
self._ops.make_allyesconfig(build_dir, make_options)
+ self._ops.make_arch_qemuconfig(build_dir)
self._ops.make_olddefconfig(build_dir, make_options)
self._ops.make(jobs, build_dir, make_options)
except (ConfigError, BuildError) as e:
@@ -209,8 +364,8 @@ class LinuxSourceTree(object):
args.extend(['mem=1G', 'console=tty','kunit_shutdown=halt'])
if filter_glob:
args.append('kunit.filter_glob='+filter_glob)
- self._ops.linux_bin(args, timeout, build_dir)
outfile = get_outfile_path(build_dir)
+ self._ops.run(args, timeout, build_dir, outfile)
subprocess.call(['stty', 'sane'])
with open(outfile, 'r') as file:
for line in file:
diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py
index 1ad3049e90698..25e8be95a575d 100755
--- a/tools/testing/kunit/kunit_tool_test.py
+++ b/tools/testing/kunit/kunit_tool_test.py
@@ -297,7 +297,7 @@ class KUnitMainTest(unittest.TestCase):
self.linux_source_mock = mock.Mock()
self.linux_source_mock.build_reconfig = mock.Mock(return_value=True)
- self.linux_source_mock.build_um_kernel = mock.Mock(return_value=True)
+ self.linux_source_mock.build_kernel = mock.Mock(return_value=True)
self.linux_source_mock.run_kernel = mock.Mock(return_value=all_passed_log)
def test_config_passes_args_pass(self):
@@ -308,7 +308,7 @@ class KUnitMainTest(unittest.TestCase):
def test_build_passes_args_pass(self):
kunit.main(['build'], self.linux_source_mock)
self.assertEqual(self.linux_source_mock.build_reconfig.call_count, 0)
- self.linux_source_mock.build_um_kernel.assert_called_once_with(False, 8, '.kunit', None)
+ self.linux_source_mock.build_kernel.assert_called_once_with(False, 8, '.kunit', None)
self.assertEqual(self.linux_source_mock.run_kernel.call_count, 0)
def test_exec_passes_args_pass(self):
@@ -390,7 +390,7 @@ class KUnitMainTest(unittest.TestCase):
def test_build_builddir(self):
build_dir = '.kunit'
kunit.main(['build', '--build_dir', build_dir], self.linux_source_mock)
- self.linux_source_mock.build_um_kernel.assert_called_once_with(False, 8, build_dir, None)
+ self.linux_source_mock.build_kernel.assert_called_once_with(False, 8, build_dir, None)
def test_exec_builddir(self):
build_dir = '.kunit'
@@ -404,14 +404,19 @@ class KUnitMainTest(unittest.TestCase):
mock_linux_init.return_value = self.linux_source_mock
kunit.main(['run', '--kunitconfig=mykunitconfig'])
# Just verify that we parsed and initialized it correctly here.
- mock_linux_init.assert_called_once_with('.kunit', kunitconfig_path='mykunitconfig')
+ mock_linux_init.assert_called_once_with('.kunit',
+ kunitconfig_path='mykunitconfig',
+ arch='um',
+ cross_compile=None)
@mock.patch.object(kunit_kernel, 'LinuxSourceTree')
def test_config_kunitconfig(self, mock_linux_init):
mock_linux_init.return_value = self.linux_source_mock
kunit.main(['config', '--kunitconfig=mykunitconfig'])
# Just verify that we parsed and initialized it correctly here.
- mock_linux_init.assert_called_once_with('.kunit', kunitconfig_path='mykunitconfig')
+ mock_linux_init.assert_called_once_with('.kunit',
+ kunitconfig_path='mykunitconfig',
+ arch='um')
if __name__ == '__main__':
unittest.main()
--
2.31.1.498.g6c1eba8ee3d-goog
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [RFC v2 4/4] Documentation: kunit: document support for QEMU in kunit_tool
2021-04-29 20:51 [RFC v2 0/4] kunit: tool: add support for QEMU Brendan Higgins
` (2 preceding siblings ...)
2021-04-29 20:51 ` [RFC v2 3/4] kunit: tool: add support for QEMU Brendan Higgins
@ 2021-04-29 20:51 ` Brendan Higgins
3 siblings, 0 replies; 12+ messages in thread
From: Brendan Higgins @ 2021-04-29 20:51 UTC (permalink / raw)
To: shuah, davidgow
Cc: linux-kselftest, kunit-dev, linux-kernel, corbet, linux-doc,
sboyd, keescook, frowand.list, dlatypov, Brendan Higgins
Document QEMU support, what it does, and how to use it in kunit_tool.
Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
---
Documentation/dev-tools/kunit/usage.rst | 37 +++++++++++++++++++++----
1 file changed, 31 insertions(+), 6 deletions(-)
diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst
index 650f99590df57..b74bd7c87cc20 100644
--- a/Documentation/dev-tools/kunit/usage.rst
+++ b/Documentation/dev-tools/kunit/usage.rst
@@ -612,14 +612,39 @@ only things to be aware of when doing so.
The biggest impediment will likely be that certain KUnit features and
infrastructure may not support your target environment. For example, at this
time the KUnit Wrapper (``tools/testing/kunit/kunit.py``) does not work outside
-of UML. Unfortunately, there is no way around this. Using UML (or even just a
-particular architecture) allows us to make a lot of assumptions that make it
-possible to do things which might otherwise be impossible.
+of UML and QEMU. Unfortunately, there is no way around this. Using UML and QEMU
+(or even just a particular architecture) allows us to make a lot of assumptions
+that make it possible to do things which might otherwise be impossible.
Nevertheless, all core KUnit framework features are fully supported on all
-architectures, and using them is straightforward: all you need to do is to take
-your kunitconfig, your Kconfig options for the tests you would like to run, and
-merge them into whatever config your are using for your platform. That's it!
+architectures, and using them is straightforward: Most popular architectures
+are supported directly in the KUnit Wrapper via QEMU. Currently, supported
+architectures on QEMU include:
+
+* i386
+* x86_64
+* arm
+* arm64
+* alpha
+* powerpc
+* riscv
+* s390
+* sparc
+
+In order to run KUnit tests on one of these architectures via QEMU with the
+KUnit wrapper, all you need to do is specify the flags ``--arch`` and
+``--cross_compile`` when invoking the KUnit Wrapper. For example, we could run
+the default KUnit tests on ARM in the following manner (assuming we have an ARM
+toolchain installed):
+
+.. code-block:: bash
+
+ tools/testing/kunit/kunit.py run --timeout=60 --jobs=12 --arch=arm --cross_compile=arm-linux-gnueabihf-
+
+Alternatively, if you want to run your tests on real hardware or in some other
+emulation environment, all you need to do is to take your kunitconfig, your
+Kconfig options for the tests you would like to run, and merge them into
+whatever config your are using for your platform. That's it!
For example, let's say you have the following kunitconfig:
--
2.31.1.498.g6c1eba8ee3d-goog
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [RFC v2 1/4] kunit: Add 'kunit_shutdown' option
2021-04-29 20:51 ` [RFC v2 1/4] kunit: Add 'kunit_shutdown' option Brendan Higgins
@ 2021-04-29 21:52 ` Daniel Latypov
0 siblings, 0 replies; 12+ messages in thread
From: Daniel Latypov @ 2021-04-29 21:52 UTC (permalink / raw)
To: Brendan Higgins
Cc: shuah, David Gow, open list:KERNEL SELFTEST FRAMEWORK,
KUnit Development, Linux Kernel Mailing List, Jonathan Corbet,
linux-doc, Stephen Boyd, Kees Cook, frowand.list
On Thu, Apr 29, 2021 at 1:51 PM Brendan Higgins
<brendanhiggins@google.com> wrote:
>
> From: David Gow <davidgow@google.com>
>
> Add a new kernel command-line option, 'kunit_shutdown', which allows the
> user to specify that the kernel poweroff, halt, or reboot after
> completing all KUnit tests; this is very handy for running KUnit tests
> on UML or a VM so that the UML/VM process exits cleanly immediately
> after running all tests without needing a special initramfs.
>
> Signed-off-by: David Gow <davidgow@google.com>
> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> Reviewed-by: Stephen Boyd <sboyd@kernel.org>
Tested-By: Daniel Latypov <dlatypov@google.com>
Testing this out, I think this also helps fix coverage on UML.
I used to have to hack in something like this:
diff --git a/arch/um/os-Linux/util.c b/arch/um/os-Linux/util.c
index 07327425d06e..e13763faedd9 100644
--- a/arch/um/os-Linux/util.c
+++ b/arch/um/os-Linux/util.c
@@ -146,7 +146,7 @@ void os_dump_core(void)
while ((pid = waitpid(-1, NULL, WNOHANG | __WALL)) > 0)
os_kill_ptraced_process(pid, 0);
- uml_abort();
+ exit(127);
}
void um_early_printk(const char *s, unsigned int n)
And afaict, this patch removes the need to do so.
(But I made a few mistakes when initially trying to test that out, so
I'm not 100% certain I didn't make another).
> ---
> lib/kunit/executor.c | 20 ++++++++++++++++++++
> tools/testing/kunit/kunit_kernel.py | 2 +-
> tools/testing/kunit/kunit_parser.py | 2 +-
> 3 files changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
> index 15832ed446685..7db619624437c 100644
> --- a/lib/kunit/executor.c
> +++ b/lib/kunit/executor.c
> @@ -1,5 +1,6 @@
> // SPDX-License-Identifier: GPL-2.0
>
> +#include <linux/reboot.h>
> #include <kunit/test.h>
> #include <linux/glob.h>
> #include <linux/moduleparam.h>
> @@ -18,6 +19,9 @@ module_param(filter_glob, charp, 0);
> MODULE_PARM_DESC(filter_glob,
> "Filter which KUnit test suites run at boot-time, e.g. list*");
>
> +static char *kunit_shutdown;
> +core_param(kunit_shutdown, kunit_shutdown, charp, 0644);
> +
> static struct kunit_suite * const *
> kunit_filter_subsuite(struct kunit_suite * const * const subsuite)
> {
> @@ -82,6 +86,20 @@ static struct suite_set kunit_filter_suites(void)
> return filtered;
> }
>
> +static void kunit_handle_shutdown(void)
> +{
> + if (!kunit_shutdown)
> + return;
> +
> + if (!strcmp(kunit_shutdown, "poweroff"))
> + kernel_power_off();
> + else if (!strcmp(kunit_shutdown, "halt"))
> + kernel_halt();
> + else if (!strcmp(kunit_shutdown, "reboot"))
> + kernel_restart(NULL);
> +
> +}
> +
> static void kunit_print_tap_header(struct suite_set *suite_set)
> {
> struct kunit_suite * const * const *suites, * const *subsuite;
> @@ -112,6 +130,8 @@ int kunit_run_all_tests(void)
> kfree(suite_set.start);
> }
>
> + kunit_handle_shutdown();
> +
> return 0;
> }
>
> diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py
> index f309a33256cd3..7d5b77967d48f 100644
> --- a/tools/testing/kunit/kunit_kernel.py
> +++ b/tools/testing/kunit/kunit_kernel.py
> @@ -206,7 +206,7 @@ class LinuxSourceTree(object):
> def run_kernel(self, args=None, build_dir='', filter_glob='', timeout=None) -> Iterator[str]:
> if not args:
> args = []
> - args.extend(['mem=1G', 'console=tty'])
> + args.extend(['mem=1G', 'console=tty','kunit_shutdown=halt'])
> if filter_glob:
> args.append('kunit.filter_glob='+filter_glob)
> self._ops.linux_bin(args, timeout, build_dir)
> diff --git a/tools/testing/kunit/kunit_parser.py b/tools/testing/kunit/kunit_parser.py
> index e8bcc139702e2..8d8d4d70b39dd 100644
> --- a/tools/testing/kunit/kunit_parser.py
> +++ b/tools/testing/kunit/kunit_parser.py
> @@ -49,7 +49,7 @@ class TestStatus(Enum):
>
> kunit_start_re = re.compile(r'TAP version [0-9]+$')
> kunit_end_re = re.compile('(List of all partitions:|'
> - 'Kernel panic - not syncing: VFS:)')
> + 'Kernel panic - not syncing: VFS:|reboot: System halted)')
>
> def isolate_kunit_output(kernel_output) -> Iterator[str]:
> started = False
> --
> 2.31.1.498.g6c1eba8ee3d-goog
>
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [RFC v2 3/4] kunit: tool: add support for QEMU
2021-04-29 20:51 ` [RFC v2 3/4] kunit: tool: add support for QEMU Brendan Higgins
@ 2021-04-29 23:39 ` Daniel Latypov
2021-04-30 20:01 ` Brendan Higgins
2021-05-04 6:08 ` David Gow
1 sibling, 1 reply; 12+ messages in thread
From: Daniel Latypov @ 2021-04-29 23:39 UTC (permalink / raw)
To: Brendan Higgins
Cc: shuah, David Gow, open list:KERNEL SELFTEST FRAMEWORK,
KUnit Development, Linux Kernel Mailing List, Jonathan Corbet,
linux-doc, Stephen Boyd, Kees Cook, frowand.list
On Thu, Apr 29, 2021 at 1:51 PM 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.
Hmmm, I'm wondering if I'm seeing unrelated breakages.
Applied these patches on top of 55ba0fe059a5 ("Merge tag
'for-5.13-tag' of
git://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux")
$ make mrproper
$ rm -rf .kunit/* # just in case
$ ./tools/testing/kunit/kunit.py run --arch=arm64
...
ERROR:root:arch/arm64/Makefile:25: ld does not support
--fix-cortex-a53-843419; kernel may be susceptible to erratum
arch/arm64/Makefile:44: Detected assembler with broken .inst;
disassembly will be unreliable
gcc: error: unrecognized command-line option ‘-mlittle-endian’
So it seems like my version of ld might be out of date, but my gcc is 10.2.1
Is there a minimum version of the toolchain this would expect that we
can call out in the commit message (and in the Documentation)?
--arch=x86_64 worked just fine for me, however, which is mainly what I
was interested in.
I've mainly just left some nits and comments regarding typechecking.
>
> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> ---
> tools/testing/kunit/kunit.py | 33 +++-
> tools/testing/kunit/kunit_config.py | 2 +-
> tools/testing/kunit/kunit_kernel.py | 207 +++++++++++++++++++++----
> tools/testing/kunit/kunit_tool_test.py | 15 +-
> 4 files changed, 217 insertions(+), 40 deletions(-)
>
> diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py
> index d5144fcb03acd..07ef80062873b 100755
> --- a/tools/testing/kunit/kunit.py
> +++ b/tools/testing/kunit/kunit.py
> @@ -70,10 +70,10 @@ def build_tests(linux: kunit_kernel.LinuxSourceTree,
> kunit_parser.print_with_timestamp('Building KUnit Kernel ...')
>
> build_start = time.time()
> - success = linux.build_um_kernel(request.alltests,
> - request.jobs,
> - request.build_dir,
> - request.make_options)
> + success = linux.build_kernel(request.alltests,
> + request.jobs,
> + request.build_dir,
> + request.make_options)
> build_end = time.time()
> if not success:
> return KunitResult(KunitStatus.BUILD_FAILURE,
> @@ -187,6 +187,14 @@ def add_common_opts(parser) -> None:
> help='Path to Kconfig fragment that enables KUnit tests',
> metavar='kunitconfig')
>
> + parser.add_argument('--arch',
> + help='Specifies the architecture to run tests under.',
optional: perhaps add "(e.g. x86_64, um)"
Most people using this would be able to infer that, but I prefer
strings that are basically enums to document examples of useful
values.
(And x86 is probably the one most people want to use this flag for).
> + type=str, default='um', metavar='arch')
> +
> + parser.add_argument('--cross_compile',
> + help='Sets make\'s CROSS_COMPILE variable.',
> + metavar='cross_compile')
> +
> def add_build_opts(parser) -> None:
> parser.add_argument('--jobs',
> help='As in the make command, "Specifies the number of '
> @@ -268,7 +276,10 @@ def main(argv, linux=None):
> os.mkdir(cli_args.build_dir)
>
> if not linux:
> - linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir, kunitconfig_path=cli_args.kunitconfig)
> + linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir,
> + kunitconfig_path=cli_args.kunitconfig,
> + arch=cli_args.arch,
> + cross_compile=cli_args.cross_compile)
>
> request = KunitRequest(cli_args.raw_output,
> cli_args.timeout,
> @@ -287,7 +298,9 @@ def main(argv, linux=None):
> os.mkdir(cli_args.build_dir)
>
> if not linux:
> - linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir, kunitconfig_path=cli_args.kunitconfig)
> + linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir,
> + kunitconfig_path=cli_args.kunitconfig,
> + arch=cli_args.arch)
>
> request = KunitConfigRequest(cli_args.build_dir,
> cli_args.make_options)
> @@ -299,7 +312,9 @@ def main(argv, linux=None):
> sys.exit(1)
> elif cli_args.subcommand == 'build':
> if not linux:
> - linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir, kunitconfig_path=cli_args.kunitconfig)
> + linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir,
> + kunitconfig_path=cli_args.kunitconfig,
> + arch=cli_args.arch)
>
> request = KunitBuildRequest(cli_args.jobs,
> cli_args.build_dir,
> @@ -313,7 +328,9 @@ def main(argv, linux=None):
> sys.exit(1)
> elif cli_args.subcommand == 'exec':
> if not linux:
> - linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir)
> + linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir,
> + kunitconfig_path=cli_args.kunitconfig,
> + arch=cli_args.arch)
>
> exec_request = KunitExecRequest(cli_args.timeout,
> cli_args.build_dir,
> diff --git a/tools/testing/kunit/kunit_config.py b/tools/testing/kunit/kunit_config.py
> index 1e2683dcc0e7a..07d76be392544 100644
> --- a/tools/testing/kunit/kunit_config.py
> +++ b/tools/testing/kunit/kunit_config.py
> @@ -53,7 +53,7 @@ class Kconfig(object):
> return True
>
> def write_to_file(self, path: str) -> None:
> - with open(path, 'w') as f:
> + with open(path, 'a+') as f:
I might be missing something, but do we need this?
w => a means we wouldn't truncate the file if it exists. I can imagine
I'm missing something that makes it necessary.
+ => opens for read/write: we don't do any reads with f afaict.
> for entry in self.entries():
> f.write(str(entry) + '\n')
>
> diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py
> index 7d5b77967d48f..b8b3b76aaa17e 100644
> --- a/tools/testing/kunit/kunit_kernel.py
> +++ b/tools/testing/kunit/kunit_kernel.py
> @@ -15,6 +15,8 @@ from typing import Iterator
>
> from contextlib import ExitStack
>
> +from collections import namedtuple
> +
> import kunit_config
> import kunit_parser
>
> @@ -40,6 +42,10 @@ class BuildError(Exception):
> class LinuxSourceTreeOperations(object):
> """An abstraction over command line operations performed on a source tree."""
>
> + def __init__(self, linux_arch, cross_compile):
nit: let's annotate these as `linux_arch: str, cross_compile: str` (or
is it Optional[str] here?)
I can see a reader thinking arch might be an enum and that
cross_compile might be a bool.
> + self._linux_arch = linux_arch
> + self._cross_compile = cross_compile
> +
> def make_mrproper(self) -> None:
> try:
> subprocess.check_output(['make', 'mrproper'], stderr=subprocess.STDOUT)
> @@ -48,12 +54,18 @@ class LinuxSourceTreeOperations(object):
> except subprocess.CalledProcessError as e:
> raise ConfigError(e.output.decode())
>
> + def make_arch_qemuconfig(self, build_dir):
> + pass
> +
> def make_olddefconfig(self, build_dir, make_options) -> None:
> - command = ['make', 'ARCH=um', 'olddefconfig']
> + command = ['make', 'ARCH=' + self._linux_arch, 'olddefconfig']
> + if self._cross_compile:
> + command += ['CROSS_COMPILE=' + self._cross_compile]
> if make_options:
> command.extend(make_options)
> if build_dir:
> command += ['O=' + build_dir]
> + print(' '.join(command))
> try:
> subprocess.check_output(command, stderr=subprocess.STDOUT)
> except OSError as e:
> @@ -61,6 +73,154 @@ class LinuxSourceTreeOperations(object):
> except subprocess.CalledProcessError as e:
> raise ConfigError(e.output.decode())
>
> + def make(self, jobs, build_dir, make_options) -> None:
> + command = ['make', 'ARCH=' + self._linux_arch, '--jobs=' + str(jobs)]
> + if make_options:
> + command.extend(make_options)
> + if self._cross_compile:
> + command += ['CROSS_COMPILE=' + self._cross_compile]
> + if build_dir:
> + command += ['O=' + build_dir]
> + print(' '.join(command))
> + try:
> + proc = subprocess.Popen(command,
> + stderr=subprocess.PIPE,
> + stdout=subprocess.DEVNULL)
> + except OSError as e:
> + raise BuildError('Could not call execute make: ' + e)
pytype complains about this.
You'd want `+ str(e)`
> + except subprocess.CalledProcessError as e:
> + raise BuildError(e.output)
> + _, stderr = proc.communicate()
> + if proc.returncode != 0:
> + raise BuildError(stderr.decode())
> + if stderr: # likely only due to build warnings
> + print(stderr.decode())
> +
> + def run(self, params, timeout, build_dir, outfile) -> None:
> + pass
> +
> +
> +QemuArchParams = namedtuple('QemuArchParams', ['linux_arch',
> + 'qemuconfig',
> + 'qemu_arch',
> + 'kernel_path',
> + 'kernel_command_line',
> + 'extra_qemu_params'])
> +
> +
> +QEMU_ARCHS = {
> + 'i386' : QemuArchParams(linux_arch='i386',
> + qemuconfig='CONFIG_SERIAL_8250=y\nCONFIG_SERIAL_8250_CONSOLE=y',
> + qemu_arch='x86_64',
> + kernel_path='arch/x86/boot/bzImage',
> + kernel_command_line='console=ttyS0',
> + extra_qemu_params=['']),
> + 'x86_64' : QemuArchParams(linux_arch='x86_64',
> + qemuconfig='CONFIG_SERIAL_8250=y\nCONFIG_SERIAL_8250_CONSOLE=y',
> + qemu_arch='x86_64',
> + kernel_path='arch/x86/boot/bzImage',
> + kernel_command_line='console=ttyS0',
> + extra_qemu_params=['']),
> + 'arm' : 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''',
> + qemu_arch='arm',
> + kernel_path='arch/arm/boot/zImage',
> + kernel_command_line='console=ttyAMA0',
> + extra_qemu_params=['-machine virt']),
> + 'arm64' : 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']),
> + 'alpha' : QemuArchParams(linux_arch='alpha',
> + qemuconfig='CONFIG_SERIAL_8250=y\nCONFIG_SERIAL_8250_CONSOLE=y',
> + qemu_arch='alpha',
> + kernel_path='arch/alpha/boot/vmlinux',
> + kernel_command_line='console=ttyS0',
> + extra_qemu_params=['']),
> + 'powerpc' : QemuArchParams(linux_arch='powerpc',
> + qemuconfig='CONFIG_PPC64=y\nCONFIG_SERIAL_8250=y\nCONFIG_SERIAL_8250_CONSOLE=y\nCONFIG_HVC_CONSOLE=y',
> + qemu_arch='ppc64',
> + kernel_path='vmlinux',
> + kernel_command_line='console=ttyS0',
> + extra_qemu_params=['-M pseries', '-cpu power8']),
> + 'riscv' : QemuArchParams(linux_arch='riscv',
> + qemuconfig='CONFIG_SOC_VIRT=y\nCONFIG_SERIAL_8250=y\nCONFIG_SERIAL_8250_CONSOLE=y\nCONFIG_SERIAL_OF_PLATFORM=y\nCONFIG_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']),
> + 's390' : QemuArchParams(linux_arch='s390',
> + qemuconfig='CONFIG_EXPERT=y\nCONFIG_TUNE_ZEC12=y\nCONFIG_NUMA=y\nCONFIG_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',]),
> + 'sparc' : QemuArchParams(linux_arch='sparc',
> + qemuconfig='CONFIG_SERIAL_8250=y\nCONFIG_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']),
> +}
Oh my.
I don't know enough to say if there's a better way of doing this.
But I think we should probably split this out into a separate python
file, if this mapping remains necessary.
E.g. in a qemu_configs.py file or the like.
> +
> +class LinuxSourceTreeOperationsQemu(LinuxSourceTreeOperations):
I've called out the two errors pytype gives already, but mypy is even
worse about this new class :(
$ mypy tools/testing/kunit/*.py
tools/testing/kunit/kunit_kernel.py:90: error: Unsupported operand
types for + ("str" and "OSError")
tools/testing/kunit/kunit_kernel.py:278: error: Incompatible types in
assignment (expression has type "LinuxSourceTreeOperationsQemu",
variable has type "Optional[LinuxSourceTreeOperationsUml]")
tools/testing/kunit/kunit_kernel.py:298: error: Item "None" of
"Optional[LinuxSourceTreeOperationsUml]" has no attribute
"make_mrproper"
tools/testing/kunit/kunit_kernel.py:324: error: Item "None" of
"Optional[LinuxSourceTreeOperationsUml]" has no attribute
"make_arch_qemuconfig"
tools/testing/kunit/kunit_kernel.py:325: error: Item "None" of
"Optional[LinuxSourceTreeOperationsUml]" has no attribute
"make_olddefconfig"
tools/testing/kunit/kunit_kernel.py:352: error: Item "None" of
"Optional[LinuxSourceTreeOperationsUml]" has no attribute
"make_allyesconfig"
tools/testing/kunit/kunit_kernel.py:353: error: Item "None" of
"Optional[LinuxSourceTreeOperationsUml]" has no attribute
"make_arch_qemuconfig"
tools/testing/kunit/kunit_kernel.py:354: error: Item "None" of
"Optional[LinuxSourceTreeOperationsUml]" has no attribute
"make_olddefconfig"
tools/testing/kunit/kunit_kernel.py:355: error: Item "None" of
"Optional[LinuxSourceTreeOperationsUml]" has no attribute "make"
tools/testing/kunit/kunit_kernel.py:368: error: Item "None" of
"Optional[LinuxSourceTreeOperationsUml]" has no attribute "run"
So to make up for mypy being less smart, we can do this and get it down 2 errors
diff --git a/tools/testing/kunit/kunit_kernel.py
b/tools/testing/kunit/kunit_kernel.py
index b8b3b76aaa17..a0aaa28db4c1 100644
--- a/tools/testing/kunit/kunit_kernel.py
+++ b/tools/testing/kunit/kunit_kernel.py
@@ -11,7 +11,7 @@ import subprocess
import os
import shutil
import signal
-from typing import Iterator
+from typing import Iterator, Union
from contextlib import ExitStack
@@ -269,15 +269,16 @@ class LinuxSourceTree(object):
def __init__(self, build_dir: str, load_config=True,
kunitconfig_path='', arch=None, cross_compile=None) -> None:
signal.signal(signal.SIGINT, self.signal_handler)
- self._ops = None
+ ops = None # type: Union[None,
LinuxSourceTreeOperationsUml, LinuxSourceTreeOperationsQemu]
if arch is None or arch == 'um':
self._arch = 'um'
- self._ops = LinuxSourceTreeOperationsUml()
+ ops = LinuxSourceTreeOperationsUml()
elif arch in QEMU_ARCHS:
self._arch = arch
- self._ops =
LinuxSourceTreeOperationsQemu(QEMU_ARCHS[arch],
cross_compile=cross_compile)
+ ops =
LinuxSourceTreeOperationsQemu(QEMU_ARCHS[arch],
cross_compile=cross_compile)
else:
raise ConfigError(arch + ' is not a valid arch')
+ self._ops = ops
if not load_config:
return
> +
> + def __init__(self, qemu_arch_params, cross_compile):
> + super().__init__(linux_arch=qemu_arch_params.linux_arch,
> + cross_compile=cross_compile)
> + self._qemuconfig = qemu_arch_params.qemuconfig
> + self._qemu_arch = qemu_arch_params.qemu_arch
> + self._kernel_path = qemu_arch_params.kernel_path
> + print(self._kernel_path)
looks like a leftover debugging print statement?
> + self._kernel_command_line = qemu_arch_params.kernel_command_line + ' kunit_shutdown=reboot'
> + self._extra_qemu_params = qemu_arch_params.extra_qemu_params
> +
> + def make_arch_qemuconfig(self, build_dir):
> + qemuconfig = kunit_config.Kconfig()
> + qemuconfig.parse_from_string(self._qemuconfig)
> + qemuconfig.write_to_file(get_kconfig_path(build_dir))
> +
> + def run(self, params, timeout, build_dir, outfile):
> + kernel_path = os.path.join(build_dir, self._kernel_path)
> + qemu_command = ['qemu-system-' + self._qemu_arch,
> + '-nodefaults',
> + '-m', '1024',
> + '-kernel', kernel_path,
> + '-append', '\'' + ' '.join(params + [self._kernel_command_line]) + '\'',
> + '-no-reboot',
> + '-nographic',
> + '-serial stdio'] + self._extra_qemu_params
> + print(' '.join(qemu_command))
ditto, a debug statement?
Though this one could be useful to actually print out for the user if
we include some more context in the message.
> + with open(outfile, 'w') as output:
> + process = subprocess.Popen(' '.join(qemu_command),
> + stdin=subprocess.PIPE,
> + stdout=output,
> + stderr=subprocess.STDOUT,
> + text=True, shell=True)
> + try:
> + process.wait(timeout=timeout)
> + except Exception as e:
> + print(e)
> + process.terminate()
> + return process
> +
> +class LinuxSourceTreeOperationsUml(LinuxSourceTreeOperations):
> + """An abstraction over command line operations performed on a source tree."""
> +
> + def __init__(self):
> + super().__init__(linux_arch='um', cross_compile=None)
> +
> def make_allyesconfig(self, build_dir, make_options) -> None:
> kunit_parser.print_with_timestamp(
> 'Enabling all CONFIGs for UML...')
> @@ -83,32 +243,16 @@ class LinuxSourceTreeOperations(object):
> kunit_parser.print_with_timestamp(
> 'Starting Kernel with all configs takes a few minutes...')
>
> - def make(self, jobs, build_dir, make_options) -> None:
> - command = ['make', 'ARCH=um', '--jobs=' + str(jobs)]
> - if make_options:
> - command.extend(make_options)
> - if build_dir:
> - command += ['O=' + build_dir]
> - try:
> - proc = subprocess.Popen(command,
> - stderr=subprocess.PIPE,
> - stdout=subprocess.DEVNULL)
> - except OSError as e:
> - raise BuildError('Could not call make command: ' + str(e))
> - _, stderr = proc.communicate()
> - if proc.returncode != 0:
> - raise BuildError(stderr.decode())
> - if stderr: # likely only due to build warnings
> - print(stderr.decode())
> -
> - def linux_bin(self, params, timeout, build_dir) -> None:
> + def run(self, params, timeout, build_dir, outfile):
> """Runs the Linux UML binary. Must be named 'linux'."""
> linux_bin = get_file_path(build_dir, 'linux')
> outfile = get_outfile_path(build_dir)
> with open(outfile, 'w') as output:
> process = subprocess.Popen([linux_bin] + params,
> + stdin=subprocess.PIPE,
> stdout=output,
> - stderr=subprocess.STDOUT)
> + stderr=subprocess.STDOUT,
> + text=True)
> process.wait(timeout)
>
> def get_kconfig_path(build_dir) -> str:
> @@ -123,10 +267,17 @@ def get_outfile_path(build_dir) -> str:
> class LinuxSourceTree(object):
> """Represents a Linux kernel source tree with KUnit tests."""
>
> - def __init__(self, build_dir: str, load_config=True, kunitconfig_path='') -> None:
> + def __init__(self, build_dir: str, load_config=True, kunitconfig_path='', arch=None, cross_compile=None) -> None:
> signal.signal(signal.SIGINT, self.signal_handler)
> -
> - self._ops = LinuxSourceTreeOperations()
> + self._ops = None
> + if arch is None or arch == 'um':
> + self._arch = 'um'
> + self._ops = LinuxSourceTreeOperationsUml()
> + elif arch in QEMU_ARCHS:
> + self._arch = arch
> + self._ops = LinuxSourceTreeOperationsQemu(QEMU_ARCHS[arch], cross_compile=cross_compile)
> + else:
> + raise ConfigError(arch + ' is not a valid arch')
>
> if not load_config:
> return
> @@ -170,6 +321,7 @@ class LinuxSourceTree(object):
> os.mkdir(build_dir)
> self._kconfig.write_to_file(kconfig_path)
> try:
> + self._ops.make_arch_qemuconfig(build_dir)
> self._ops.make_olddefconfig(build_dir, make_options)
> except ConfigError as e:
> logging.error(e)
> @@ -192,10 +344,13 @@ class LinuxSourceTree(object):
> print('Generating .config ...')
> return self.build_config(build_dir, make_options)
>
> - def build_um_kernel(self, alltests, jobs, build_dir, make_options) -> bool:
> + def build_kernel(self, alltests, jobs, build_dir, make_options) -> bool:
> try:
> if alltests:
> + if self._arch != 'um':
> + raise ConfigError('Only the "um" arch is supported for alltests')
> self._ops.make_allyesconfig(build_dir, make_options)
Minor note: pytype doesn't like this.
The code is correct, but pytype can't figure out that ops can't be the
QEMU variant.
Pytype recognizes comments like " # pytype: disable=attribute-error"
but mypy doesn't.
So I don't know that there's a way we can make both of them happy :/
> + self._ops.make_arch_qemuconfig(build_dir)
> self._ops.make_olddefconfig(build_dir, make_options)
> self._ops.make(jobs, build_dir, make_options)
> except (ConfigError, BuildError) as e:
> @@ -209,8 +364,8 @@ class LinuxSourceTree(object):
> args.extend(['mem=1G', 'console=tty','kunit_shutdown=halt'])
> if filter_glob:
> args.append('kunit.filter_glob='+filter_glob)
> - self._ops.linux_bin(args, timeout, build_dir)
> outfile = get_outfile_path(build_dir)
> + self._ops.run(args, timeout, build_dir, outfile)
> subprocess.call(['stty', 'sane'])
> with open(outfile, 'r') as file:
> for line in file:
> diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py
> index 1ad3049e90698..25e8be95a575d 100755
> --- a/tools/testing/kunit/kunit_tool_test.py
> +++ b/tools/testing/kunit/kunit_tool_test.py
> @@ -297,7 +297,7 @@ class KUnitMainTest(unittest.TestCase):
>
> self.linux_source_mock = mock.Mock()
> self.linux_source_mock.build_reconfig = mock.Mock(return_value=True)
> - self.linux_source_mock.build_um_kernel = mock.Mock(return_value=True)
> + self.linux_source_mock.build_kernel = mock.Mock(return_value=True)
> self.linux_source_mock.run_kernel = mock.Mock(return_value=all_passed_log)
>
> def test_config_passes_args_pass(self):
> @@ -308,7 +308,7 @@ class KUnitMainTest(unittest.TestCase):
> def test_build_passes_args_pass(self):
> kunit.main(['build'], self.linux_source_mock)
> self.assertEqual(self.linux_source_mock.build_reconfig.call_count, 0)
> - self.linux_source_mock.build_um_kernel.assert_called_once_with(False, 8, '.kunit', None)
> + self.linux_source_mock.build_kernel.assert_called_once_with(False, 8, '.kunit', None)
> self.assertEqual(self.linux_source_mock.run_kernel.call_count, 0)
>
> def test_exec_passes_args_pass(self):
> @@ -390,7 +390,7 @@ class KUnitMainTest(unittest.TestCase):
> def test_build_builddir(self):
> build_dir = '.kunit'
> kunit.main(['build', '--build_dir', build_dir], self.linux_source_mock)
> - self.linux_source_mock.build_um_kernel.assert_called_once_with(False, 8, build_dir, None)
> + self.linux_source_mock.build_kernel.assert_called_once_with(False, 8, build_dir, None)
>
> def test_exec_builddir(self):
> build_dir = '.kunit'
> @@ -404,14 +404,19 @@ class KUnitMainTest(unittest.TestCase):
> mock_linux_init.return_value = self.linux_source_mock
> kunit.main(['run', '--kunitconfig=mykunitconfig'])
> # Just verify that we parsed and initialized it correctly here.
> - mock_linux_init.assert_called_once_with('.kunit', kunitconfig_path='mykunitconfig')
> + mock_linux_init.assert_called_once_with('.kunit',
> + kunitconfig_path='mykunitconfig',
> + arch='um',
> + cross_compile=None)
>
> @mock.patch.object(kunit_kernel, 'LinuxSourceTree')
> def test_config_kunitconfig(self, mock_linux_init):
> mock_linux_init.return_value = self.linux_source_mock
> kunit.main(['config', '--kunitconfig=mykunitconfig'])
> # Just verify that we parsed and initialized it correctly here.
> - mock_linux_init.assert_called_once_with('.kunit', kunitconfig_path='mykunitconfig')
> + mock_linux_init.assert_called_once_with('.kunit',
> + kunitconfig_path='mykunitconfig',
> + arch='um')
>
> if __name__ == '__main__':
> unittest.main()
> --
> 2.31.1.498.g6c1eba8ee3d-goog
>
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [RFC v2 3/4] kunit: tool: add support for QEMU
2021-04-29 23:39 ` Daniel Latypov
@ 2021-04-30 20:01 ` Brendan Higgins
2021-04-30 20:14 ` Daniel Latypov
0 siblings, 1 reply; 12+ messages in thread
From: Brendan Higgins @ 2021-04-30 20:01 UTC (permalink / raw)
To: Daniel Latypov
Cc: shuah, David Gow, open list:KERNEL SELFTEST FRAMEWORK,
KUnit Development, Linux Kernel Mailing List, Jonathan Corbet,
open list:DOCUMENTATION, Stephen Boyd, Kees Cook, Frank Rowand
On Thu, Apr 29, 2021 at 4:40 PM Daniel Latypov <dlatypov@google.com> wrote:
>
> On Thu, Apr 29, 2021 at 1:51 PM 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.
>
> Hmmm, I'm wondering if I'm seeing unrelated breakages.
> Applied these patches on top of 55ba0fe059a5 ("Merge tag
> 'for-5.13-tag' of
> git://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux")
>
> $ make mrproper
> $ rm -rf .kunit/* # just in case
> $ ./tools/testing/kunit/kunit.py run --arch=arm64
> ...
Huh, did you use a arm64 cross compiler? Maybe that's your issue.
> ERROR:root:arch/arm64/Makefile:25: ld does not support
> --fix-cortex-a53-843419; kernel may be susceptible to erratum
> arch/arm64/Makefile:44: Detected assembler with broken .inst;
> disassembly will be unreliable
> gcc: error: unrecognized command-line option ‘-mlittle-endian’
>
> So it seems like my version of ld might be out of date, but my gcc is 10.2.1
> Is there a minimum version of the toolchain this would expect that we
> can call out in the commit message (and in the Documentation)?
>
> --arch=x86_64 worked just fine for me, however, which is mainly what I
> was interested in.
>
> I've mainly just left some nits and comments regarding typechecking.
>
> >
> > Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> > ---
> > tools/testing/kunit/kunit.py | 33 +++-
> > tools/testing/kunit/kunit_config.py | 2 +-
> > tools/testing/kunit/kunit_kernel.py | 207 +++++++++++++++++++++----
> > tools/testing/kunit/kunit_tool_test.py | 15 +-
> > 4 files changed, 217 insertions(+), 40 deletions(-)
> >
> > diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py
> > index d5144fcb03acd..07ef80062873b 100755
> > --- a/tools/testing/kunit/kunit.py
> > +++ b/tools/testing/kunit/kunit.py
> > @@ -70,10 +70,10 @@ def build_tests(linux: kunit_kernel.LinuxSourceTree,
> > kunit_parser.print_with_timestamp('Building KUnit Kernel ...')
> >
> > build_start = time.time()
> > - success = linux.build_um_kernel(request.alltests,
> > - request.jobs,
> > - request.build_dir,
> > - request.make_options)
> > + success = linux.build_kernel(request.alltests,
> > + request.jobs,
> > + request.build_dir,
> > + request.make_options)
> > build_end = time.time()
> > if not success:
> > return KunitResult(KunitStatus.BUILD_FAILURE,
> > @@ -187,6 +187,14 @@ def add_common_opts(parser) -> None:
> > help='Path to Kconfig fragment that enables KUnit tests',
> > metavar='kunitconfig')
> >
> > + parser.add_argument('--arch',
> > + help='Specifies the architecture to run tests under.',
>
> optional: perhaps add "(e.g. x86_64, um)"
> Most people using this would be able to infer that, but I prefer
> strings that are basically enums to document examples of useful
> values.
Good point, probably want to mention that I take the string name from
the argument passed into the ARCH make variable.
> (And x86 is probably the one most people want to use this flag for).
Yep, but Linux refers to it as i386. Just trying to be consistent.
> > + type=str, default='um', metavar='arch')
> > +
> > + parser.add_argument('--cross_compile',
> > + help='Sets make\'s CROSS_COMPILE variable.',
> > + metavar='cross_compile')
> > +
> > def add_build_opts(parser) -> None:
> > parser.add_argument('--jobs',
> > help='As in the make command, "Specifies the number of '
> > @@ -268,7 +276,10 @@ def main(argv, linux=None):
> > os.mkdir(cli_args.build_dir)
> >
> > if not linux:
> > - linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir, kunitconfig_path=cli_args.kunitconfig)
> > + linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir,
> > + kunitconfig_path=cli_args.kunitconfig,
> > + arch=cli_args.arch,
> > + cross_compile=cli_args.cross_compile)
> >
> > request = KunitRequest(cli_args.raw_output,
> > cli_args.timeout,
> > @@ -287,7 +298,9 @@ def main(argv, linux=None):
> > os.mkdir(cli_args.build_dir)
> >
> > if not linux:
> > - linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir, kunitconfig_path=cli_args.kunitconfig)
> > + linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir,
> > + kunitconfig_path=cli_args.kunitconfig,
> > + arch=cli_args.arch)
> >
> > request = KunitConfigRequest(cli_args.build_dir,
> > cli_args.make_options)
> > @@ -299,7 +312,9 @@ def main(argv, linux=None):
> > sys.exit(1)
> > elif cli_args.subcommand == 'build':
> > if not linux:
> > - linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir, kunitconfig_path=cli_args.kunitconfig)
> > + linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir,
> > + kunitconfig_path=cli_args.kunitconfig,
> > + arch=cli_args.arch)
> >
> > request = KunitBuildRequest(cli_args.jobs,
> > cli_args.build_dir,
> > @@ -313,7 +328,9 @@ def main(argv, linux=None):
> > sys.exit(1)
> > elif cli_args.subcommand == 'exec':
> > if not linux:
> > - linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir)
> > + linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir,
> > + kunitconfig_path=cli_args.kunitconfig,
> > + arch=cli_args.arch)
> >
> > exec_request = KunitExecRequest(cli_args.timeout,
> > cli_args.build_dir,
> > diff --git a/tools/testing/kunit/kunit_config.py b/tools/testing/kunit/kunit_config.py
> > index 1e2683dcc0e7a..07d76be392544 100644
> > --- a/tools/testing/kunit/kunit_config.py
> > +++ b/tools/testing/kunit/kunit_config.py
> > @@ -53,7 +53,7 @@ class Kconfig(object):
> > return True
> >
> > def write_to_file(self, path: str) -> None:
> > - with open(path, 'w') as f:
> > + with open(path, 'a+') as f:
>
> I might be missing something, but do we need this?
>
> w => a means we wouldn't truncate the file if it exists. I can imagine
> I'm missing something that makes it necessary.
> + => opens for read/write: we don't do any reads with f afaict.
Yeah, probably only needs to be a, not a+. But we do want append for
adding the arch configs. Idea is that we want to append those onto
whatever a base kunitconfig.
Nevertheless, your concern is valid, might be a better way to do this. Thoughts?
> > for entry in self.entries():
> > f.write(str(entry) + '\n')
> >
> > diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py
> > index 7d5b77967d48f..b8b3b76aaa17e 100644
> > --- a/tools/testing/kunit/kunit_kernel.py
> > +++ b/tools/testing/kunit/kunit_kernel.py
> > @@ -15,6 +15,8 @@ from typing import Iterator
> >
> > from contextlib import ExitStack
> >
> > +from collections import namedtuple
> > +
> > import kunit_config
> > import kunit_parser
> >
> > @@ -40,6 +42,10 @@ class BuildError(Exception):
> > class LinuxSourceTreeOperations(object):
> > """An abstraction over command line operations performed on a source tree."""
> >
> > + def __init__(self, linux_arch, cross_compile):
>
> nit: let's annotate these as `linux_arch: str, cross_compile: str` (or
> is it Optional[str] here?)
> I can see a reader thinking arch might be an enum and that
> cross_compile might be a bool.
Fair. Will do.
> > + self._linux_arch = linux_arch
> > + self._cross_compile = cross_compile
> > +
> > def make_mrproper(self) -> None:
> > try:
> > subprocess.check_output(['make', 'mrproper'], stderr=subprocess.STDOUT)
> > @@ -48,12 +54,18 @@ class LinuxSourceTreeOperations(object):
> > except subprocess.CalledProcessError as e:
> > raise ConfigError(e.output.decode())
> >
> > + def make_arch_qemuconfig(self, build_dir):
> > + pass
> > +
> > def make_olddefconfig(self, build_dir, make_options) -> None:
> > - command = ['make', 'ARCH=um', 'olddefconfig']
> > + command = ['make', 'ARCH=' + self._linux_arch, 'olddefconfig']
> > + if self._cross_compile:
> > + command += ['CROSS_COMPILE=' + self._cross_compile]
> > if make_options:
> > command.extend(make_options)
> > if build_dir:
> > command += ['O=' + build_dir]
> > + print(' '.join(command))
> > try:
> > subprocess.check_output(command, stderr=subprocess.STDOUT)
> > except OSError as e:
> > @@ -61,6 +73,154 @@ class LinuxSourceTreeOperations(object):
> > except subprocess.CalledProcessError as e:
> > raise ConfigError(e.output.decode())
> >
> > + def make(self, jobs, build_dir, make_options) -> None:
> > + command = ['make', 'ARCH=' + self._linux_arch, '--jobs=' + str(jobs)]
> > + if make_options:
> > + command.extend(make_options)
> > + if self._cross_compile:
> > + command += ['CROSS_COMPILE=' + self._cross_compile]
> > + if build_dir:
> > + command += ['O=' + build_dir]
> > + print(' '.join(command))
> > + try:
> > + proc = subprocess.Popen(command,
> > + stderr=subprocess.PIPE,
> > + stdout=subprocess.DEVNULL)
> > + except OSError as e:
> > + raise BuildError('Could not call execute make: ' + e)
>
> pytype complains about this.
> You'd want `+ str(e)`
Got it. Will do. (I should probably also run pytype.)
> > + except subprocess.CalledProcessError as e:
> > + raise BuildError(e.output)
> > + _, stderr = proc.communicate()
> > + if proc.returncode != 0:
> > + raise BuildError(stderr.decode())
> > + if stderr: # likely only due to build warnings
> > + print(stderr.decode())
> > +
> > + def run(self, params, timeout, build_dir, outfile) -> None:
> > + pass
> > +
> > +
> > +QemuArchParams = namedtuple('QemuArchParams', ['linux_arch',
> > + 'qemuconfig',
> > + 'qemu_arch',
> > + 'kernel_path',
> > + 'kernel_command_line',
> > + 'extra_qemu_params'])
> > +
> > +
> > +QEMU_ARCHS = {
> > + 'i386' : QemuArchParams(linux_arch='i386',
> > + qemuconfig='CONFIG_SERIAL_8250=y\nCONFIG_SERIAL_8250_CONSOLE=y',
> > + qemu_arch='x86_64',
> > + kernel_path='arch/x86/boot/bzImage',
> > + kernel_command_line='console=ttyS0',
> > + extra_qemu_params=['']),
> > + 'x86_64' : QemuArchParams(linux_arch='x86_64',
> > + qemuconfig='CONFIG_SERIAL_8250=y\nCONFIG_SERIAL_8250_CONSOLE=y',
> > + qemu_arch='x86_64',
> > + kernel_path='arch/x86/boot/bzImage',
> > + kernel_command_line='console=ttyS0',
> > + extra_qemu_params=['']),
> > + 'arm' : 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''',
> > + qemu_arch='arm',
> > + kernel_path='arch/arm/boot/zImage',
> > + kernel_command_line='console=ttyAMA0',
> > + extra_qemu_params=['-machine virt']),
> > + 'arm64' : 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']),
> > + 'alpha' : QemuArchParams(linux_arch='alpha',
> > + qemuconfig='CONFIG_SERIAL_8250=y\nCONFIG_SERIAL_8250_CONSOLE=y',
> > + qemu_arch='alpha',
> > + kernel_path='arch/alpha/boot/vmlinux',
> > + kernel_command_line='console=ttyS0',
> > + extra_qemu_params=['']),
> > + 'powerpc' : QemuArchParams(linux_arch='powerpc',
> > + qemuconfig='CONFIG_PPC64=y\nCONFIG_SERIAL_8250=y\nCONFIG_SERIAL_8250_CONSOLE=y\nCONFIG_HVC_CONSOLE=y',
> > + qemu_arch='ppc64',
> > + kernel_path='vmlinux',
> > + kernel_command_line='console=ttyS0',
> > + extra_qemu_params=['-M pseries', '-cpu power8']),
> > + 'riscv' : QemuArchParams(linux_arch='riscv',
> > + qemuconfig='CONFIG_SOC_VIRT=y\nCONFIG_SERIAL_8250=y\nCONFIG_SERIAL_8250_CONSOLE=y\nCONFIG_SERIAL_OF_PLATFORM=y\nCONFIG_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']),
> > + 's390' : QemuArchParams(linux_arch='s390',
> > + qemuconfig='CONFIG_EXPERT=y\nCONFIG_TUNE_ZEC12=y\nCONFIG_NUMA=y\nCONFIG_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',]),
> > + 'sparc' : QemuArchParams(linux_arch='sparc',
> > + qemuconfig='CONFIG_SERIAL_8250=y\nCONFIG_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']),
> > +}
>
> Oh my.
> I don't know enough to say if there's a better way of doing this.
Yeah, I know it's gross, but I did not want to put too much effort
into until I got some feedback on it.
> But I think we should probably split this out into a separate python
> file, if this mapping remains necessary.
> E.g. in a qemu_configs.py file or the like.
Definitely an improvement. Any other thoughts on how to make this look
less gross?
>
> > +
> > +class LinuxSourceTreeOperationsQemu(LinuxSourceTreeOperations):
>
> I've called out the two errors pytype gives already, but mypy is even
> worse about this new class :(
>
> $ mypy tools/testing/kunit/*.py
> tools/testing/kunit/kunit_kernel.py:90: error: Unsupported operand
> types for + ("str" and "OSError")
> tools/testing/kunit/kunit_kernel.py:278: error: Incompatible types in
> assignment (expression has type "LinuxSourceTreeOperationsQemu",
> variable has type "Optional[LinuxSourceTreeOperationsUml]")
> tools/testing/kunit/kunit_kernel.py:298: error: Item "None" of
> "Optional[LinuxSourceTreeOperationsUml]" has no attribute
> "make_mrproper"
> tools/testing/kunit/kunit_kernel.py:324: error: Item "None" of
> "Optional[LinuxSourceTreeOperationsUml]" has no attribute
> "make_arch_qemuconfig"
> tools/testing/kunit/kunit_kernel.py:325: error: Item "None" of
> "Optional[LinuxSourceTreeOperationsUml]" has no attribute
> "make_olddefconfig"
> tools/testing/kunit/kunit_kernel.py:352: error: Item "None" of
> "Optional[LinuxSourceTreeOperationsUml]" has no attribute
> "make_allyesconfig"
> tools/testing/kunit/kunit_kernel.py:353: error: Item "None" of
> "Optional[LinuxSourceTreeOperationsUml]" has no attribute
> "make_arch_qemuconfig"
> tools/testing/kunit/kunit_kernel.py:354: error: Item "None" of
> "Optional[LinuxSourceTreeOperationsUml]" has no attribute
> "make_olddefconfig"
> tools/testing/kunit/kunit_kernel.py:355: error: Item "None" of
> "Optional[LinuxSourceTreeOperationsUml]" has no attribute "make"
> tools/testing/kunit/kunit_kernel.py:368: error: Item "None" of
> "Optional[LinuxSourceTreeOperationsUml]" has no attribute "run"
>
> So to make up for mypy being less smart, we can do this and get it down 2 errors
Sounds good. Will do.
> diff --git a/tools/testing/kunit/kunit_kernel.py
> b/tools/testing/kunit/kunit_kernel.py
> index b8b3b76aaa17..a0aaa28db4c1 100644
> --- a/tools/testing/kunit/kunit_kernel.py
> +++ b/tools/testing/kunit/kunit_kernel.py
> @@ -11,7 +11,7 @@ import subprocess
> import os
> import shutil
> import signal
> -from typing import Iterator
> +from typing import Iterator, Union
>
> from contextlib import ExitStack
>
> @@ -269,15 +269,16 @@ class LinuxSourceTree(object):
>
> def __init__(self, build_dir: str, load_config=True,
> kunitconfig_path='', arch=None, cross_compile=None) -> None:
> signal.signal(signal.SIGINT, self.signal_handler)
> - self._ops = None
> + ops = None # type: Union[None,
> LinuxSourceTreeOperationsUml, LinuxSourceTreeOperationsQemu]
> if arch is None or arch == 'um':
> self._arch = 'um'
> - self._ops = LinuxSourceTreeOperationsUml()
> + ops = LinuxSourceTreeOperationsUml()
> elif arch in QEMU_ARCHS:
> self._arch = arch
> - self._ops =
> LinuxSourceTreeOperationsQemu(QEMU_ARCHS[arch],
> cross_compile=cross_compile)
> + ops =
> LinuxSourceTreeOperationsQemu(QEMU_ARCHS[arch],
> cross_compile=cross_compile)
> else:
> raise ConfigError(arch + ' is not a valid arch')
> + self._ops = ops
>
> if not load_config:
> return
>
> > +
> > + def __init__(self, qemu_arch_params, cross_compile):
> > + super().__init__(linux_arch=qemu_arch_params.linux_arch,
> > + cross_compile=cross_compile)
> > + self._qemuconfig = qemu_arch_params.qemuconfig
> > + self._qemu_arch = qemu_arch_params.qemu_arch
> > + self._kernel_path = qemu_arch_params.kernel_path
> > + print(self._kernel_path)
> looks like a leftover debugging print statement?
Oh yeah. Whoops.
> > + self._kernel_command_line = qemu_arch_params.kernel_command_line + ' kunit_shutdown=reboot'
> > + self._extra_qemu_params = qemu_arch_params.extra_qemu_params
> > +
> > + def make_arch_qemuconfig(self, build_dir):
> > + qemuconfig = kunit_config.Kconfig()
> > + qemuconfig.parse_from_string(self._qemuconfig)
> > + qemuconfig.write_to_file(get_kconfig_path(build_dir))
> > +
> > + def run(self, params, timeout, build_dir, outfile):
> > + kernel_path = os.path.join(build_dir, self._kernel_path)
> > + qemu_command = ['qemu-system-' + self._qemu_arch,
> > + '-nodefaults',
> > + '-m', '1024',
> > + '-kernel', kernel_path,
> > + '-append', '\'' + ' '.join(params + [self._kernel_command_line]) + '\'',
> > + '-no-reboot',
> > + '-nographic',
> > + '-serial stdio'] + self._extra_qemu_params
> > + print(' '.join(qemu_command))
>
> ditto, a debug statement?
> Though this one could be useful to actually print out for the user if
> we include some more context in the message.
Yeah, this was initially a debug, but then I ended up finding it
pretty useful since it spits out a full qemu command that you can just
run outside of the tool, so I left it in.
> > + with open(outfile, 'w') as output:
> > + process = subprocess.Popen(' '.join(qemu_command),
> > + stdin=subprocess.PIPE,
> > + stdout=output,
> > + stderr=subprocess.STDOUT,
> > + text=True, shell=True)
> > + try:
> > + process.wait(timeout=timeout)
> > + except Exception as e:
> > + print(e)
> > + process.terminate()
> > + return process
> > +
> > +class LinuxSourceTreeOperationsUml(LinuxSourceTreeOperations):
> > + """An abstraction over command line operations performed on a source tree."""
> > +
> > + def __init__(self):
> > + super().__init__(linux_arch='um', cross_compile=None)
> > +
> > def make_allyesconfig(self, build_dir, make_options) -> None:
> > kunit_parser.print_with_timestamp(
> > 'Enabling all CONFIGs for UML...')
> > @@ -83,32 +243,16 @@ class LinuxSourceTreeOperations(object):
> > kunit_parser.print_with_timestamp(
> > 'Starting Kernel with all configs takes a few minutes...')
> >
> > - def make(self, jobs, build_dir, make_options) -> None:
> > - command = ['make', 'ARCH=um', '--jobs=' + str(jobs)]
> > - if make_options:
> > - command.extend(make_options)
> > - if build_dir:
> > - command += ['O=' + build_dir]
> > - try:
> > - proc = subprocess.Popen(command,
> > - stderr=subprocess.PIPE,
> > - stdout=subprocess.DEVNULL)
> > - except OSError as e:
> > - raise BuildError('Could not call make command: ' + str(e))
> > - _, stderr = proc.communicate()
> > - if proc.returncode != 0:
> > - raise BuildError(stderr.decode())
> > - if stderr: # likely only due to build warnings
> > - print(stderr.decode())
> > -
> > - def linux_bin(self, params, timeout, build_dir) -> None:
> > + def run(self, params, timeout, build_dir, outfile):
> > """Runs the Linux UML binary. Must be named 'linux'."""
> > linux_bin = get_file_path(build_dir, 'linux')
> > outfile = get_outfile_path(build_dir)
> > with open(outfile, 'w') as output:
> > process = subprocess.Popen([linux_bin] + params,
> > + stdin=subprocess.PIPE,
> > stdout=output,
> > - stderr=subprocess.STDOUT)
> > + stderr=subprocess.STDOUT,
> > + text=True)
> > process.wait(timeout)
> >
> > def get_kconfig_path(build_dir) -> str:
> > @@ -123,10 +267,17 @@ def get_outfile_path(build_dir) -> str:
> > class LinuxSourceTree(object):
> > """Represents a Linux kernel source tree with KUnit tests."""
> >
> > - def __init__(self, build_dir: str, load_config=True, kunitconfig_path='') -> None:
> > + def __init__(self, build_dir: str, load_config=True, kunitconfig_path='', arch=None, cross_compile=None) -> None:
> > signal.signal(signal.SIGINT, self.signal_handler)
> > -
> > - self._ops = LinuxSourceTreeOperations()
> > + self._ops = None
> > + if arch is None or arch == 'um':
> > + self._arch = 'um'
> > + self._ops = LinuxSourceTreeOperationsUml()
> > + elif arch in QEMU_ARCHS:
> > + self._arch = arch
> > + self._ops = LinuxSourceTreeOperationsQemu(QEMU_ARCHS[arch], cross_compile=cross_compile)
> > + else:
> > + raise ConfigError(arch + ' is not a valid arch')
> >
> > if not load_config:
> > return
> > @@ -170,6 +321,7 @@ class LinuxSourceTree(object):
> > os.mkdir(build_dir)
> > self._kconfig.write_to_file(kconfig_path)
> > try:
> > + self._ops.make_arch_qemuconfig(build_dir)
> > self._ops.make_olddefconfig(build_dir, make_options)
> > except ConfigError as e:
> > logging.error(e)
> > @@ -192,10 +344,13 @@ class LinuxSourceTree(object):
> > print('Generating .config ...')
> > return self.build_config(build_dir, make_options)
> >
> > - def build_um_kernel(self, alltests, jobs, build_dir, make_options) -> bool:
> > + def build_kernel(self, alltests, jobs, build_dir, make_options) -> bool:
> > try:
> > if alltests:
> > + if self._arch != 'um':
> > + raise ConfigError('Only the "um" arch is supported for alltests')
> > self._ops.make_allyesconfig(build_dir, make_options)
>
> Minor note: pytype doesn't like this.
> The code is correct, but pytype can't figure out that ops can't be the
> QEMU variant.
>
> Pytype recognizes comments like " # pytype: disable=attribute-error"
> but mypy doesn't.
> So I don't know that there's a way we can make both of them happy :/
Hmmm...I could just add make_allyesconfig() to the base class and make
it raise. That might be cleaner too.
>
> > + self._ops.make_arch_qemuconfig(build_dir)
> > self._ops.make_olddefconfig(build_dir, make_options)
> > self._ops.make(jobs, build_dir, make_options)
> > except (ConfigError, BuildError) as e:
> > @@ -209,8 +364,8 @@ class LinuxSourceTree(object):
> > args.extend(['mem=1G', 'console=tty','kunit_shutdown=halt'])
> > if filter_glob:
> > args.append('kunit.filter_glob='+filter_glob)
> > - self._ops.linux_bin(args, timeout, build_dir)
> > outfile = get_outfile_path(build_dir)
> > + self._ops.run(args, timeout, build_dir, outfile)
> > subprocess.call(['stty', 'sane'])
> > with open(outfile, 'r') as file:
> > for line in file:
> > diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py
> > index 1ad3049e90698..25e8be95a575d 100755
> > --- a/tools/testing/kunit/kunit_tool_test.py
> > +++ b/tools/testing/kunit/kunit_tool_test.py
> > @@ -297,7 +297,7 @@ class KUnitMainTest(unittest.TestCase):
> >
> > self.linux_source_mock = mock.Mock()
> > self.linux_source_mock.build_reconfig = mock.Mock(return_value=True)
> > - self.linux_source_mock.build_um_kernel = mock.Mock(return_value=True)
> > + self.linux_source_mock.build_kernel = mock.Mock(return_value=True)
> > self.linux_source_mock.run_kernel = mock.Mock(return_value=all_passed_log)
> >
> > def test_config_passes_args_pass(self):
> > @@ -308,7 +308,7 @@ class KUnitMainTest(unittest.TestCase):
> > def test_build_passes_args_pass(self):
> > kunit.main(['build'], self.linux_source_mock)
> > self.assertEqual(self.linux_source_mock.build_reconfig.call_count, 0)
> > - self.linux_source_mock.build_um_kernel.assert_called_once_with(False, 8, '.kunit', None)
> > + self.linux_source_mock.build_kernel.assert_called_once_with(False, 8, '.kunit', None)
> > self.assertEqual(self.linux_source_mock.run_kernel.call_count, 0)
> >
> > def test_exec_passes_args_pass(self):
> > @@ -390,7 +390,7 @@ class KUnitMainTest(unittest.TestCase):
> > def test_build_builddir(self):
> > build_dir = '.kunit'
> > kunit.main(['build', '--build_dir', build_dir], self.linux_source_mock)
> > - self.linux_source_mock.build_um_kernel.assert_called_once_with(False, 8, build_dir, None)
> > + self.linux_source_mock.build_kernel.assert_called_once_with(False, 8, build_dir, None)
> >
> > def test_exec_builddir(self):
> > build_dir = '.kunit'
> > @@ -404,14 +404,19 @@ class KUnitMainTest(unittest.TestCase):
> > mock_linux_init.return_value = self.linux_source_mock
> > kunit.main(['run', '--kunitconfig=mykunitconfig'])
> > # Just verify that we parsed and initialized it correctly here.
> > - mock_linux_init.assert_called_once_with('.kunit', kunitconfig_path='mykunitconfig')
> > + mock_linux_init.assert_called_once_with('.kunit',
> > + kunitconfig_path='mykunitconfig',
> > + arch='um',
> > + cross_compile=None)
> >
> > @mock.patch.object(kunit_kernel, 'LinuxSourceTree')
> > def test_config_kunitconfig(self, mock_linux_init):
> > mock_linux_init.return_value = self.linux_source_mock
> > kunit.main(['config', '--kunitconfig=mykunitconfig'])
> > # Just verify that we parsed and initialized it correctly here.
> > - mock_linux_init.assert_called_once_with('.kunit', kunitconfig_path='mykunitconfig')
> > + mock_linux_init.assert_called_once_with('.kunit',
> > + kunitconfig_path='mykunitconfig',
> > + arch='um')
> >
> > if __name__ == '__main__':
> > unittest.main()
> > --
> > 2.31.1.498.g6c1eba8ee3d-goog
> >
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC v2 3/4] kunit: tool: add support for QEMU
2021-04-30 20:01 ` Brendan Higgins
@ 2021-04-30 20:14 ` Daniel Latypov
2021-05-03 21:19 ` Brendan Higgins
0 siblings, 1 reply; 12+ messages in thread
From: Daniel Latypov @ 2021-04-30 20:14 UTC (permalink / raw)
To: Brendan Higgins
Cc: shuah, David Gow, open list:KERNEL SELFTEST FRAMEWORK,
KUnit Development, Linux Kernel Mailing List, Jonathan Corbet,
open list:DOCUMENTATION, Stephen Boyd, Kees Cook, Frank Rowand
On Fri, Apr 30, 2021 at 1:01 PM 'Brendan Higgins' via KUnit
Development <kunit-dev@googlegroups.com> wrote:
>
> On Thu, Apr 29, 2021 at 4:40 PM Daniel Latypov <dlatypov@google.com> wrote:
> >
> > On Thu, Apr 29, 2021 at 1:51 PM 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.
> >
> > Hmmm, I'm wondering if I'm seeing unrelated breakages.
> > Applied these patches on top of 55ba0fe059a5 ("Merge tag
> > 'for-5.13-tag' of
> > git://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux")
> >
> > $ make mrproper
> > $ rm -rf .kunit/* # just in case
> > $ ./tools/testing/kunit/kunit.py run --arch=arm64
> > ...
>
> Huh, did you use a arm64 cross compiler? Maybe that's your issue.
I didn't (and realized that like 2 minutes after I sent the email).
Please disregard.
>
> > ERROR:root:arch/arm64/Makefile:25: ld does not support
> > --fix-cortex-a53-843419; kernel may be susceptible to erratum
> > arch/arm64/Makefile:44: Detected assembler with broken .inst;
> > disassembly will be unreliable
> > gcc: error: unrecognized command-line option ‘-mlittle-endian’
> >
> > So it seems like my version of ld might be out of date, but my gcc is 10.2.1
> > Is there a minimum version of the toolchain this would expect that we
> > can call out in the commit message (and in the Documentation)?
> >
> > --arch=x86_64 worked just fine for me, however, which is mainly what I
> > was interested in.
> >
> > I've mainly just left some nits and comments regarding typechecking.
> >
> > >
> > > Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> > > ---
> > > tools/testing/kunit/kunit.py | 33 +++-
> > > tools/testing/kunit/kunit_config.py | 2 +-
> > > tools/testing/kunit/kunit_kernel.py | 207 +++++++++++++++++++++----
> > > tools/testing/kunit/kunit_tool_test.py | 15 +-
> > > 4 files changed, 217 insertions(+), 40 deletions(-)
> > >
> > > diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py
> > > index d5144fcb03acd..07ef80062873b 100755
> > > --- a/tools/testing/kunit/kunit.py
> > > +++ b/tools/testing/kunit/kunit.py
> > > @@ -70,10 +70,10 @@ def build_tests(linux: kunit_kernel.LinuxSourceTree,
> > > kunit_parser.print_with_timestamp('Building KUnit Kernel ...')
> > >
> > > build_start = time.time()
> > > - success = linux.build_um_kernel(request.alltests,
> > > - request.jobs,
> > > - request.build_dir,
> > > - request.make_options)
> > > + success = linux.build_kernel(request.alltests,
> > > + request.jobs,
> > > + request.build_dir,
> > > + request.make_options)
> > > build_end = time.time()
> > > if not success:
> > > return KunitResult(KunitStatus.BUILD_FAILURE,
> > > @@ -187,6 +187,14 @@ def add_common_opts(parser) -> None:
> > > help='Path to Kconfig fragment that enables KUnit tests',
> > > metavar='kunitconfig')
> > >
> > > + parser.add_argument('--arch',
> > > + help='Specifies the architecture to run tests under.',
> >
> > optional: perhaps add "(e.g. x86_64, um)"
> > Most people using this would be able to infer that, but I prefer
> > strings that are basically enums to document examples of useful
> > values.
>
> Good point, probably want to mention that I take the string name from
> the argument passed into the ARCH make variable.
>
> > (And x86 is probably the one most people want to use this flag for).
>
> Yep, but Linux refers to it as i386. Just trying to be consistent.
Sorry, I meant to say x86_64 there. i386 is what we should call it, yeah.
>
> > > + type=str, default='um', metavar='arch')
> > > +
> > > + parser.add_argument('--cross_compile',
> > > + help='Sets make\'s CROSS_COMPILE variable.',
> > > + metavar='cross_compile')
> > > +
> > > def add_build_opts(parser) -> None:
> > > parser.add_argument('--jobs',
> > > help='As in the make command, "Specifies the number of '
> > > @@ -268,7 +276,10 @@ def main(argv, linux=None):
> > > os.mkdir(cli_args.build_dir)
> > >
> > > if not linux:
> > > - linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir, kunitconfig_path=cli_args.kunitconfig)
> > > + linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir,
> > > + kunitconfig_path=cli_args.kunitconfig,
> > > + arch=cli_args.arch,
> > > + cross_compile=cli_args.cross_compile)
> > >
> > > request = KunitRequest(cli_args.raw_output,
> > > cli_args.timeout,
> > > @@ -287,7 +298,9 @@ def main(argv, linux=None):
> > > os.mkdir(cli_args.build_dir)
> > >
> > > if not linux:
> > > - linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir, kunitconfig_path=cli_args.kunitconfig)
> > > + linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir,
> > > + kunitconfig_path=cli_args.kunitconfig,
> > > + arch=cli_args.arch)
> > >
> > > request = KunitConfigRequest(cli_args.build_dir,
> > > cli_args.make_options)
> > > @@ -299,7 +312,9 @@ def main(argv, linux=None):
> > > sys.exit(1)
> > > elif cli_args.subcommand == 'build':
> > > if not linux:
> > > - linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir, kunitconfig_path=cli_args.kunitconfig)
> > > + linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir,
> > > + kunitconfig_path=cli_args.kunitconfig,
> > > + arch=cli_args.arch)
> > >
> > > request = KunitBuildRequest(cli_args.jobs,
> > > cli_args.build_dir,
> > > @@ -313,7 +328,9 @@ def main(argv, linux=None):
> > > sys.exit(1)
> > > elif cli_args.subcommand == 'exec':
> > > if not linux:
> > > - linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir)
> > > + linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir,
> > > + kunitconfig_path=cli_args.kunitconfig,
> > > + arch=cli_args.arch)
> > >
> > > exec_request = KunitExecRequest(cli_args.timeout,
> > > cli_args.build_dir,
> > > diff --git a/tools/testing/kunit/kunit_config.py b/tools/testing/kunit/kunit_config.py
> > > index 1e2683dcc0e7a..07d76be392544 100644
> > > --- a/tools/testing/kunit/kunit_config.py
> > > +++ b/tools/testing/kunit/kunit_config.py
> > > @@ -53,7 +53,7 @@ class Kconfig(object):
> > > return True
> > >
> > > def write_to_file(self, path: str) -> None:
> > > - with open(path, 'w') as f:
> > > + with open(path, 'a+') as f:
> >
> > I might be missing something, but do we need this?
> >
> > w => a means we wouldn't truncate the file if it exists. I can imagine
> > I'm missing something that makes it necessary.
> > + => opens for read/write: we don't do any reads with f afaict.
>
> Yeah, probably only needs to be a, not a+. But we do want append for
> adding the arch configs. Idea is that we want to append those onto
> whatever a base kunitconfig.
>
> Nevertheless, your concern is valid, might be a better way to do this. Thoughts?
Would it be cleaner to operate on Kconfig objects directly instead of
having the write_to_file() function do appends?
I'm worried this might lead to unintentional statefulness in other cases.
>
> > > for entry in self.entries():
> > > f.write(str(entry) + '\n')
> > >
> > > diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py
> > > index 7d5b77967d48f..b8b3b76aaa17e 100644
> > > --- a/tools/testing/kunit/kunit_kernel.py
> > > +++ b/tools/testing/kunit/kunit_kernel.py
> > > @@ -15,6 +15,8 @@ from typing import Iterator
> > >
> > > from contextlib import ExitStack
> > >
> > > +from collections import namedtuple
> > > +
> > > import kunit_config
> > > import kunit_parser
> > >
> > > @@ -40,6 +42,10 @@ class BuildError(Exception):
> > > class LinuxSourceTreeOperations(object):
> > > """An abstraction over command line operations performed on a source tree."""
> > >
> > > + def __init__(self, linux_arch, cross_compile):
> >
> > nit: let's annotate these as `linux_arch: str, cross_compile: str` (or
> > is it Optional[str] here?)
> > I can see a reader thinking arch might be an enum and that
> > cross_compile might be a bool.
>
> Fair. Will do.
>
> > > + self._linux_arch = linux_arch
> > > + self._cross_compile = cross_compile
> > > +
> > > def make_mrproper(self) -> None:
> > > try:
> > > subprocess.check_output(['make', 'mrproper'], stderr=subprocess.STDOUT)
> > > @@ -48,12 +54,18 @@ class LinuxSourceTreeOperations(object):
> > > except subprocess.CalledProcessError as e:
> > > raise ConfigError(e.output.decode())
> > >
> > > + def make_arch_qemuconfig(self, build_dir):
> > > + pass
> > > +
> > > def make_olddefconfig(self, build_dir, make_options) -> None:
> > > - command = ['make', 'ARCH=um', 'olddefconfig']
> > > + command = ['make', 'ARCH=' + self._linux_arch, 'olddefconfig']
> > > + if self._cross_compile:
> > > + command += ['CROSS_COMPILE=' + self._cross_compile]
> > > if make_options:
> > > command.extend(make_options)
> > > if build_dir:
> > > command += ['O=' + build_dir]
> > > + print(' '.join(command))
> > > try:
> > > subprocess.check_output(command, stderr=subprocess.STDOUT)
> > > except OSError as e:
> > > @@ -61,6 +73,154 @@ class LinuxSourceTreeOperations(object):
> > > except subprocess.CalledProcessError as e:
> > > raise ConfigError(e.output.decode())
> > >
> > > + def make(self, jobs, build_dir, make_options) -> None:
> > > + command = ['make', 'ARCH=' + self._linux_arch, '--jobs=' + str(jobs)]
> > > + if make_options:
> > > + command.extend(make_options)
> > > + if self._cross_compile:
> > > + command += ['CROSS_COMPILE=' + self._cross_compile]
> > > + if build_dir:
> > > + command += ['O=' + build_dir]
> > > + print(' '.join(command))
> > > + try:
> > > + proc = subprocess.Popen(command,
> > > + stderr=subprocess.PIPE,
> > > + stdout=subprocess.DEVNULL)
> > > + except OSError as e:
> > > + raise BuildError('Could not call execute make: ' + e)
> >
> > pytype complains about this.
> > You'd want `+ str(e)`
>
> Got it. Will do. (I should probably also run pytype.)
>
> > > + except subprocess.CalledProcessError as e:
> > > + raise BuildError(e.output)
> > > + _, stderr = proc.communicate()
> > > + if proc.returncode != 0:
> > > + raise BuildError(stderr.decode())
> > > + if stderr: # likely only due to build warnings
> > > + print(stderr.decode())
> > > +
> > > + def run(self, params, timeout, build_dir, outfile) -> None:
> > > + pass
> > > +
> > > +
> > > +QemuArchParams = namedtuple('QemuArchParams', ['linux_arch',
> > > + 'qemuconfig',
> > > + 'qemu_arch',
> > > + 'kernel_path',
> > > + 'kernel_command_line',
> > > + 'extra_qemu_params'])
> > > +
> > > +
> > > +QEMU_ARCHS = {
> > > + 'i386' : QemuArchParams(linux_arch='i386',
> > > + qemuconfig='CONFIG_SERIAL_8250=y\nCONFIG_SERIAL_8250_CONSOLE=y',
> > > + qemu_arch='x86_64',
> > > + kernel_path='arch/x86/boot/bzImage',
> > > + kernel_command_line='console=ttyS0',
> > > + extra_qemu_params=['']),
> > > + 'x86_64' : QemuArchParams(linux_arch='x86_64',
> > > + qemuconfig='CONFIG_SERIAL_8250=y\nCONFIG_SERIAL_8250_CONSOLE=y',
> > > + qemu_arch='x86_64',
> > > + kernel_path='arch/x86/boot/bzImage',
> > > + kernel_command_line='console=ttyS0',
> > > + extra_qemu_params=['']),
> > > + 'arm' : 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''',
> > > + qemu_arch='arm',
> > > + kernel_path='arch/arm/boot/zImage',
> > > + kernel_command_line='console=ttyAMA0',
> > > + extra_qemu_params=['-machine virt']),
> > > + 'arm64' : 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']),
> > > + 'alpha' : QemuArchParams(linux_arch='alpha',
> > > + qemuconfig='CONFIG_SERIAL_8250=y\nCONFIG_SERIAL_8250_CONSOLE=y',
> > > + qemu_arch='alpha',
> > > + kernel_path='arch/alpha/boot/vmlinux',
> > > + kernel_command_line='console=ttyS0',
> > > + extra_qemu_params=['']),
> > > + 'powerpc' : QemuArchParams(linux_arch='powerpc',
> > > + qemuconfig='CONFIG_PPC64=y\nCONFIG_SERIAL_8250=y\nCONFIG_SERIAL_8250_CONSOLE=y\nCONFIG_HVC_CONSOLE=y',
> > > + qemu_arch='ppc64',
> > > + kernel_path='vmlinux',
> > > + kernel_command_line='console=ttyS0',
> > > + extra_qemu_params=['-M pseries', '-cpu power8']),
> > > + 'riscv' : QemuArchParams(linux_arch='riscv',
> > > + qemuconfig='CONFIG_SOC_VIRT=y\nCONFIG_SERIAL_8250=y\nCONFIG_SERIAL_8250_CONSOLE=y\nCONFIG_SERIAL_OF_PLATFORM=y\nCONFIG_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']),
> > > + 's390' : QemuArchParams(linux_arch='s390',
> > > + qemuconfig='CONFIG_EXPERT=y\nCONFIG_TUNE_ZEC12=y\nCONFIG_NUMA=y\nCONFIG_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',]),
> > > + 'sparc' : QemuArchParams(linux_arch='sparc',
> > > + qemuconfig='CONFIG_SERIAL_8250=y\nCONFIG_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']),
> > > +}
> >
> > Oh my.
> > I don't know enough to say if there's a better way of doing this.
>
> Yeah, I know it's gross, but I did not want to put too much effort
> into until I got some feedback on it.
>
> > But I think we should probably split this out into a separate python
> > file, if this mapping remains necessary.
> > E.g. in a qemu_configs.py file or the like.
>
> Definitely an improvement. Any other thoughts on how to make this look
> less gross?
Unfortunately not. Like you, I'm hoping someone else might have some
better ideas how we can maybe push this config out of python.
But if we don't find anything else, I think having the hard-coding
isolated into its own package is fine, tbh.
>
> >
> > > +
> > > +class LinuxSourceTreeOperationsQemu(LinuxSourceTreeOperations):
> >
> > I've called out the two errors pytype gives already, but mypy is even
> > worse about this new class :(
> >
> > $ mypy tools/testing/kunit/*.py
> > tools/testing/kunit/kunit_kernel.py:90: error: Unsupported operand
> > types for + ("str" and "OSError")
> > tools/testing/kunit/kunit_kernel.py:278: error: Incompatible types in
> > assignment (expression has type "LinuxSourceTreeOperationsQemu",
> > variable has type "Optional[LinuxSourceTreeOperationsUml]")
> > tools/testing/kunit/kunit_kernel.py:298: error: Item "None" of
> > "Optional[LinuxSourceTreeOperationsUml]" has no attribute
> > "make_mrproper"
> > tools/testing/kunit/kunit_kernel.py:324: error: Item "None" of
> > "Optional[LinuxSourceTreeOperationsUml]" has no attribute
> > "make_arch_qemuconfig"
> > tools/testing/kunit/kunit_kernel.py:325: error: Item "None" of
> > "Optional[LinuxSourceTreeOperationsUml]" has no attribute
> > "make_olddefconfig"
> > tools/testing/kunit/kunit_kernel.py:352: error: Item "None" of
> > "Optional[LinuxSourceTreeOperationsUml]" has no attribute
> > "make_allyesconfig"
> > tools/testing/kunit/kunit_kernel.py:353: error: Item "None" of
> > "Optional[LinuxSourceTreeOperationsUml]" has no attribute
> > "make_arch_qemuconfig"
> > tools/testing/kunit/kunit_kernel.py:354: error: Item "None" of
> > "Optional[LinuxSourceTreeOperationsUml]" has no attribute
> > "make_olddefconfig"
> > tools/testing/kunit/kunit_kernel.py:355: error: Item "None" of
> > "Optional[LinuxSourceTreeOperationsUml]" has no attribute "make"
> > tools/testing/kunit/kunit_kernel.py:368: error: Item "None" of
> > "Optional[LinuxSourceTreeOperationsUml]" has no attribute "run"
> >
> > So to make up for mypy being less smart, we can do this and get it down 2 errors
>
> Sounds good. Will do.
>
> > diff --git a/tools/testing/kunit/kunit_kernel.py
> > b/tools/testing/kunit/kunit_kernel.py
> > index b8b3b76aaa17..a0aaa28db4c1 100644
> > --- a/tools/testing/kunit/kunit_kernel.py
> > +++ b/tools/testing/kunit/kunit_kernel.py
> > @@ -11,7 +11,7 @@ import subprocess
> > import os
> > import shutil
> > import signal
> > -from typing import Iterator
> > +from typing import Iterator, Union
> >
> > from contextlib import ExitStack
> >
> > @@ -269,15 +269,16 @@ class LinuxSourceTree(object):
> >
> > def __init__(self, build_dir: str, load_config=True,
> > kunitconfig_path='', arch=None, cross_compile=None) -> None:
> > signal.signal(signal.SIGINT, self.signal_handler)
> > - self._ops = None
> > + ops = None # type: Union[None,
> > LinuxSourceTreeOperationsUml, LinuxSourceTreeOperationsQemu]
> > if arch is None or arch == 'um':
> > self._arch = 'um'
> > - self._ops = LinuxSourceTreeOperationsUml()
> > + ops = LinuxSourceTreeOperationsUml()
> > elif arch in QEMU_ARCHS:
> > self._arch = arch
> > - self._ops =
> > LinuxSourceTreeOperationsQemu(QEMU_ARCHS[arch],
> > cross_compile=cross_compile)
> > + ops =
> > LinuxSourceTreeOperationsQemu(QEMU_ARCHS[arch],
> > cross_compile=cross_compile)
> > else:
> > raise ConfigError(arch + ' is not a valid arch')
> > + self._ops = ops
> >
> > if not load_config:
> > return
> >
> > > +
> > > + def __init__(self, qemu_arch_params, cross_compile):
> > > + super().__init__(linux_arch=qemu_arch_params.linux_arch,
> > > + cross_compile=cross_compile)
> > > + self._qemuconfig = qemu_arch_params.qemuconfig
> > > + self._qemu_arch = qemu_arch_params.qemu_arch
> > > + self._kernel_path = qemu_arch_params.kernel_path
> > > + print(self._kernel_path)
> > looks like a leftover debugging print statement?
>
> Oh yeah. Whoops.
>
> > > + self._kernel_command_line = qemu_arch_params.kernel_command_line + ' kunit_shutdown=reboot'
> > > + self._extra_qemu_params = qemu_arch_params.extra_qemu_params
> > > +
> > > + def make_arch_qemuconfig(self, build_dir):
> > > + qemuconfig = kunit_config.Kconfig()
> > > + qemuconfig.parse_from_string(self._qemuconfig)
> > > + qemuconfig.write_to_file(get_kconfig_path(build_dir))
> > > +
> > > + def run(self, params, timeout, build_dir, outfile):
> > > + kernel_path = os.path.join(build_dir, self._kernel_path)
> > > + qemu_command = ['qemu-system-' + self._qemu_arch,
> > > + '-nodefaults',
> > > + '-m', '1024',
> > > + '-kernel', kernel_path,
> > > + '-append', '\'' + ' '.join(params + [self._kernel_command_line]) + '\'',
> > > + '-no-reboot',
> > > + '-nographic',
> > > + '-serial stdio'] + self._extra_qemu_params
> > > + print(' '.join(qemu_command))
> >
> > ditto, a debug statement?
> > Though this one could be useful to actually print out for the user if
> > we include some more context in the message.
>
> Yeah, this was initially a debug, but then I ended up finding it
> pretty useful since it spits out a full qemu command that you can just
> run outside of the tool, so I left it in.
Yeah, I think this one was quite useful after running it a bit.
Maybe something like
print('Running tests with:\n$', ' '.join(qemu_command))
>
> > > + with open(outfile, 'w') as output:
> > > + process = subprocess.Popen(' '.join(qemu_command),
> > > + stdin=subprocess.PIPE,
> > > + stdout=output,
> > > + stderr=subprocess.STDOUT,
> > > + text=True, shell=True)
> > > + try:
> > > + process.wait(timeout=timeout)
> > > + except Exception as e:
> > > + print(e)
> > > + process.terminate()
> > > + return process
> > > +
> > > +class LinuxSourceTreeOperationsUml(LinuxSourceTreeOperations):
> > > + """An abstraction over command line operations performed on a source tree."""
> > > +
> > > + def __init__(self):
> > > + super().__init__(linux_arch='um', cross_compile=None)
> > > +
> > > def make_allyesconfig(self, build_dir, make_options) -> None:
> > > kunit_parser.print_with_timestamp(
> > > 'Enabling all CONFIGs for UML...')
> > > @@ -83,32 +243,16 @@ class LinuxSourceTreeOperations(object):
> > > kunit_parser.print_with_timestamp(
> > > 'Starting Kernel with all configs takes a few minutes...')
> > >
> > > - def make(self, jobs, build_dir, make_options) -> None:
> > > - command = ['make', 'ARCH=um', '--jobs=' + str(jobs)]
> > > - if make_options:
> > > - command.extend(make_options)
> > > - if build_dir:
> > > - command += ['O=' + build_dir]
> > > - try:
> > > - proc = subprocess.Popen(command,
> > > - stderr=subprocess.PIPE,
> > > - stdout=subprocess.DEVNULL)
> > > - except OSError as e:
> > > - raise BuildError('Could not call make command: ' + str(e))
> > > - _, stderr = proc.communicate()
> > > - if proc.returncode != 0:
> > > - raise BuildError(stderr.decode())
> > > - if stderr: # likely only due to build warnings
> > > - print(stderr.decode())
> > > -
> > > - def linux_bin(self, params, timeout, build_dir) -> None:
> > > + def run(self, params, timeout, build_dir, outfile):
> > > """Runs the Linux UML binary. Must be named 'linux'."""
> > > linux_bin = get_file_path(build_dir, 'linux')
> > > outfile = get_outfile_path(build_dir)
> > > with open(outfile, 'w') as output:
> > > process = subprocess.Popen([linux_bin] + params,
> > > + stdin=subprocess.PIPE,
> > > stdout=output,
> > > - stderr=subprocess.STDOUT)
> > > + stderr=subprocess.STDOUT,
> > > + text=True)
> > > process.wait(timeout)
> > >
> > > def get_kconfig_path(build_dir) -> str:
> > > @@ -123,10 +267,17 @@ def get_outfile_path(build_dir) -> str:
> > > class LinuxSourceTree(object):
> > > """Represents a Linux kernel source tree with KUnit tests."""
> > >
> > > - def __init__(self, build_dir: str, load_config=True, kunitconfig_path='') -> None:
> > > + def __init__(self, build_dir: str, load_config=True, kunitconfig_path='', arch=None, cross_compile=None) -> None:
> > > signal.signal(signal.SIGINT, self.signal_handler)
> > > -
> > > - self._ops = LinuxSourceTreeOperations()
> > > + self._ops = None
> > > + if arch is None or arch == 'um':
> > > + self._arch = 'um'
> > > + self._ops = LinuxSourceTreeOperationsUml()
> > > + elif arch in QEMU_ARCHS:
> > > + self._arch = arch
> > > + self._ops = LinuxSourceTreeOperationsQemu(QEMU_ARCHS[arch], cross_compile=cross_compile)
> > > + else:
> > > + raise ConfigError(arch + ' is not a valid arch')
> > >
> > > if not load_config:
> > > return
> > > @@ -170,6 +321,7 @@ class LinuxSourceTree(object):
> > > os.mkdir(build_dir)
> > > self._kconfig.write_to_file(kconfig_path)
> > > try:
> > > + self._ops.make_arch_qemuconfig(build_dir)
> > > self._ops.make_olddefconfig(build_dir, make_options)
> > > except ConfigError as e:
> > > logging.error(e)
> > > @@ -192,10 +344,13 @@ class LinuxSourceTree(object):
> > > print('Generating .config ...')
> > > return self.build_config(build_dir, make_options)
> > >
> > > - def build_um_kernel(self, alltests, jobs, build_dir, make_options) -> bool:
> > > + def build_kernel(self, alltests, jobs, build_dir, make_options) -> bool:
> > > try:
> > > if alltests:
> > > + if self._arch != 'um':
> > > + raise ConfigError('Only the "um" arch is supported for alltests')
> > > self._ops.make_allyesconfig(build_dir, make_options)
> >
> > Minor note: pytype doesn't like this.
> > The code is correct, but pytype can't figure out that ops can't be the
> > QEMU variant.
> >
> > Pytype recognizes comments like " # pytype: disable=attribute-error"
> > but mypy doesn't.
> > So I don't know that there's a way we can make both of them happy :/
>
> Hmmm...I could just add make_allyesconfig() to the base class and make
> it raise. That might be cleaner too.
Hmm, that's probably a good idea.
Side note: I think we might want to keep the existing check as well
just so it's more obvious to a reader that only uml is supported, even
at the cost of some duplication. But that's just personal preference.
>
> >
> > > + self._ops.make_arch_qemuconfig(build_dir)
> > > self._ops.make_olddefconfig(build_dir, make_options)
> > > self._ops.make(jobs, build_dir, make_options)
> > > except (ConfigError, BuildError) as e:
> > > @@ -209,8 +364,8 @@ class LinuxSourceTree(object):
> > > args.extend(['mem=1G', 'console=tty','kunit_shutdown=halt'])
> > > if filter_glob:
> > > args.append('kunit.filter_glob='+filter_glob)
> > > - self._ops.linux_bin(args, timeout, build_dir)
> > > outfile = get_outfile_path(build_dir)
> > > + self._ops.run(args, timeout, build_dir, outfile)
> > > subprocess.call(['stty', 'sane'])
> > > with open(outfile, 'r') as file:
> > > for line in file:
> > > diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py
> > > index 1ad3049e90698..25e8be95a575d 100755
> > > --- a/tools/testing/kunit/kunit_tool_test.py
> > > +++ b/tools/testing/kunit/kunit_tool_test.py
> > > @@ -297,7 +297,7 @@ class KUnitMainTest(unittest.TestCase):
> > >
> > > self.linux_source_mock = mock.Mock()
> > > self.linux_source_mock.build_reconfig = mock.Mock(return_value=True)
> > > - self.linux_source_mock.build_um_kernel = mock.Mock(return_value=True)
> > > + self.linux_source_mock.build_kernel = mock.Mock(return_value=True)
> > > self.linux_source_mock.run_kernel = mock.Mock(return_value=all_passed_log)
> > >
> > > def test_config_passes_args_pass(self):
> > > @@ -308,7 +308,7 @@ class KUnitMainTest(unittest.TestCase):
> > > def test_build_passes_args_pass(self):
> > > kunit.main(['build'], self.linux_source_mock)
> > > self.assertEqual(self.linux_source_mock.build_reconfig.call_count, 0)
> > > - self.linux_source_mock.build_um_kernel.assert_called_once_with(False, 8, '.kunit', None)
> > > + self.linux_source_mock.build_kernel.assert_called_once_with(False, 8, '.kunit', None)
> > > self.assertEqual(self.linux_source_mock.run_kernel.call_count, 0)
> > >
> > > def test_exec_passes_args_pass(self):
> > > @@ -390,7 +390,7 @@ class KUnitMainTest(unittest.TestCase):
> > > def test_build_builddir(self):
> > > build_dir = '.kunit'
> > > kunit.main(['build', '--build_dir', build_dir], self.linux_source_mock)
> > > - self.linux_source_mock.build_um_kernel.assert_called_once_with(False, 8, build_dir, None)
> > > + self.linux_source_mock.build_kernel.assert_called_once_with(False, 8, build_dir, None)
> > >
> > > def test_exec_builddir(self):
> > > build_dir = '.kunit'
> > > @@ -404,14 +404,19 @@ class KUnitMainTest(unittest.TestCase):
> > > mock_linux_init.return_value = self.linux_source_mock
> > > kunit.main(['run', '--kunitconfig=mykunitconfig'])
> > > # Just verify that we parsed and initialized it correctly here.
> > > - mock_linux_init.assert_called_once_with('.kunit', kunitconfig_path='mykunitconfig')
> > > + mock_linux_init.assert_called_once_with('.kunit',
> > > + kunitconfig_path='mykunitconfig',
> > > + arch='um',
> > > + cross_compile=None)
> > >
> > > @mock.patch.object(kunit_kernel, 'LinuxSourceTree')
> > > def test_config_kunitconfig(self, mock_linux_init):
> > > mock_linux_init.return_value = self.linux_source_mock
> > > kunit.main(['config', '--kunitconfig=mykunitconfig'])
> > > # Just verify that we parsed and initialized it correctly here.
> > > - mock_linux_init.assert_called_once_with('.kunit', kunitconfig_path='mykunitconfig')
> > > + mock_linux_init.assert_called_once_with('.kunit',
> > > + kunitconfig_path='mykunitconfig',
> > > + arch='um')
> > >
> > > if __name__ == '__main__':
> > > unittest.main()
> > > --
> > > 2.31.1.498.g6c1eba8ee3d-goog
> > >
>
> --
> 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@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/CAFd5g44OL0BQ74ZGEnYzsLYVXEbOB-hBiNAf1%2BVLO%3DDoZwKOvA%40mail.gmail.com.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC v2 3/4] kunit: tool: add support for QEMU
2021-04-30 20:14 ` Daniel Latypov
@ 2021-05-03 21:19 ` Brendan Higgins
0 siblings, 0 replies; 12+ messages in thread
From: Brendan Higgins @ 2021-05-03 21:19 UTC (permalink / raw)
To: Daniel Latypov
Cc: shuah, David Gow, open list:KERNEL SELFTEST FRAMEWORK,
KUnit Development, Linux Kernel Mailing List, Jonathan Corbet,
open list:DOCUMENTATION, Stephen Boyd, Kees Cook, Frank Rowand
On Fri, Apr 30, 2021 at 01:14:29PM -0700, Daniel Latypov wrote:
> On Fri, Apr 30, 2021 at 1:01 PM 'Brendan Higgins' via KUnit
> Development <kunit-dev@googlegroups.com> wrote:
> >
> > On Thu, Apr 29, 2021 at 4:40 PM Daniel Latypov <dlatypov@google.com> wrote:
> > >
> > > On Thu, Apr 29, 2021 at 1:51 PM 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.
> > >
> > > Hmmm, I'm wondering if I'm seeing unrelated breakages.
> > > Applied these patches on top of 55ba0fe059a5 ("Merge tag
> > > 'for-5.13-tag' of
> > > git://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux")
> > >
> > > $ make mrproper
> > > $ rm -rf .kunit/* # just in case
> > > $ ./tools/testing/kunit/kunit.py run --arch=arm64
> > > ...
> >
> > Huh, did you use a arm64 cross compiler? Maybe that's your issue.
>
> I didn't (and realized that like 2 minutes after I sent the email).
> Please disregard.
As I mention below, I had a conversation offline with David regarding
the problem of figuring out the best way to specify qemu_configs, and I
wanted to post the fruits of that discussion to get some early feedback
on it. I am still working on addressing your other comments, but I think
most of those are pretty straightforward and probably won't require much
discussion.
[...]
> > > > +
> > > > +QemuArchParams = namedtuple('QemuArchParams', ['linux_arch',
> > > > + 'qemuconfig',
> > > > + 'qemu_arch',
> > > > + 'kernel_path',
> > > > + 'kernel_command_line',
> > > > + 'extra_qemu_params'])
> > > > +
> > > > +
> > > > +QEMU_ARCHS = {
> > > > + 'i386' : QemuArchParams(linux_arch='i386',
> > > > + qemuconfig='CONFIG_SERIAL_8250=y\nCONFIG_SERIAL_8250_CONSOLE=y',
> > > > + qemu_arch='x86_64',
> > > > + kernel_path='arch/x86/boot/bzImage',
> > > > + kernel_command_line='console=ttyS0',
> > > > + extra_qemu_params=['']),
> > > > + 'x86_64' : QemuArchParams(linux_arch='x86_64',
> > > > + qemuconfig='CONFIG_SERIAL_8250=y\nCONFIG_SERIAL_8250_CONSOLE=y',
> > > > + qemu_arch='x86_64',
> > > > + kernel_path='arch/x86/boot/bzImage',
> > > > + kernel_command_line='console=ttyS0',
> > > > + extra_qemu_params=['']),
> > > > + 'arm' : 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''',
> > > > + qemu_arch='arm',
> > > > + kernel_path='arch/arm/boot/zImage',
> > > > + kernel_command_line='console=ttyAMA0',
> > > > + extra_qemu_params=['-machine virt']),
> > > > + 'arm64' : 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']),
> > > > + 'alpha' : QemuArchParams(linux_arch='alpha',
> > > > + qemuconfig='CONFIG_SERIAL_8250=y\nCONFIG_SERIAL_8250_CONSOLE=y',
> > > > + qemu_arch='alpha',
> > > > + kernel_path='arch/alpha/boot/vmlinux',
> > > > + kernel_command_line='console=ttyS0',
> > > > + extra_qemu_params=['']),
> > > > + 'powerpc' : QemuArchParams(linux_arch='powerpc',
> > > > + qemuconfig='CONFIG_PPC64=y\nCONFIG_SERIAL_8250=y\nCONFIG_SERIAL_8250_CONSOLE=y\nCONFIG_HVC_CONSOLE=y',
> > > > + qemu_arch='ppc64',
> > > > + kernel_path='vmlinux',
> > > > + kernel_command_line='console=ttyS0',
> > > > + extra_qemu_params=['-M pseries', '-cpu power8']),
> > > > + 'riscv' : QemuArchParams(linux_arch='riscv',
> > > > + qemuconfig='CONFIG_SOC_VIRT=y\nCONFIG_SERIAL_8250=y\nCONFIG_SERIAL_8250_CONSOLE=y\nCONFIG_SERIAL_OF_PLATFORM=y\nCONFIG_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']),
> > > > + 's390' : QemuArchParams(linux_arch='s390',
> > > > + qemuconfig='CONFIG_EXPERT=y\nCONFIG_TUNE_ZEC12=y\nCONFIG_NUMA=y\nCONFIG_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',]),
> > > > + 'sparc' : QemuArchParams(linux_arch='sparc',
> > > > + qemuconfig='CONFIG_SERIAL_8250=y\nCONFIG_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']),
> > > > +}
> > >
> > > Oh my.
> > > I don't know enough to say if there's a better way of doing this.
> >
> > Yeah, I know it's gross, but I did not want to put too much effort
> > into until I got some feedback on it.
> >
> > > But I think we should probably split this out into a separate python
> > > file, if this mapping remains necessary.
> > > E.g. in a qemu_configs.py file or the like.
> >
> > Definitely an improvement. Any other thoughts on how to make this look
> > less gross?
>
> Unfortunately not. Like you, I'm hoping someone else might have some
> better ideas how we can maybe push this config out of python.
>
> But if we don't find anything else, I think having the hard-coding
> isolated into its own package is fine, tbh.
So I chatted offline with David and one idea that came up was to keep
the configs in Python, but make the configs dynamically loaded somehow
with some kind of predefined format so that they are simple to add,
*and* more importantly that adding new ones that don't go upstream won't
be a burden to maintain. Basically a developer could have a QEMU config
that is customized for her usecase, and is no way dependent on any kinds
of changes to any upstream kunit_tool files.
So here is what I came up with: We have these qemu_config files which
can be specified anywhere within (subdirectories included) the
kunit_tool root directory. The qemu_config is a Python file that imports
the `QemuArchParams` object (above) defined by kunit_tool. It specifies
one variable: `QEMU_ARCH` which is of type `QemuArchParams` which is
then used as the arch listed above.
Below I have a proof-of-concept diff, you can see view the proposed
change on Gerrit here:
https://kunit-review.googlesource.com/c/linux/+/4489
diff --git a/tools/testing/kunit/__init__.py b/tools/testing/kunit/__init__.py
new file mode 100644
index 0000000000000..e69de29bb2d1d
diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py
index f89def9e14dcd..eb9daf6896194 100755
--- a/tools/testing/kunit/kunit.py
+++ b/tools/testing/kunit/kunit.py
@@ -198,6 +198,11 @@ def add_common_opts(parser) -> None:
help='Sets make\'s CROSS_COMPILE variable.',
metavar='cross_compile')
+ parser.add_argument('--qemu_config',
+ help=('Takes a path to a path to a file containing '
+ 'a QemuArchParams object.'),
+ type=str, metavar='qemu_config')
+
def add_build_opts(parser) -> None:
parser.add_argument('--jobs',
help='As in the make command, "Specifies the number of '
@@ -282,7 +287,8 @@ def main(argv, linux=None):
linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir,
kunitconfig_path=cli_args.kunitconfig,
arch=cli_args.arch,
- cross_compile=cli_args.cross_compile)
+ cross_compile=cli_args.cross_compile,
+ qemu_config_path=cli_args.qemu_config)
request = KunitRequest(cli_args.raw_output,
cli_args.timeout,
diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py
index 64d0fffc5b86e..bc9b847bda658 100644
--- a/tools/testing/kunit/kunit_kernel.py
+++ b/tools/testing/kunit/kunit_kernel.py
@@ -6,6 +6,7 @@
# Author: Felix Guo <felixguoxiuping@gmail.com>
# Author: Brendan Higgins <brendanhiggins@google.com>
+import importlib.util
import logging
import subprocess
import os
@@ -20,7 +21,7 @@ from collections import namedtuple
import kunit_config
import kunit_parser
-import qemu_configs
+import qemu_config
KCONFIG_PATH = '.config'
KUNITCONFIG_PATH = '.kunitconfig'
@@ -107,7 +108,7 @@ class LinuxSourceTreeOperations(object):
class LinuxSourceTreeOperationsQemu(LinuxSourceTreeOperations):
- def __init__(self, qemu_arch_params: qemu_configs.QemuArchParams, cross_compile: Optional[str]):
+ def __init__(self, qemu_arch_params: qemu_config.QemuArchParams, cross_compile: Optional[str]):
super().__init__(linux_arch=qemu_arch_params.linux_arch,
cross_compile=cross_compile)
self._qemuconfig = qemu_arch_params.qemuconfig
@@ -197,19 +198,34 @@ def get_outfile_path(build_dir) -> str:
def get_source_tree_ops(arch: str, cross_compile: Optional[str]) -> LinuxSourceTreeOperations:
if arch == 'um':
return LinuxSourceTreeOperationsUml()
- elif arch in qemu_configs.QEMU_ARCHS:
- return LinuxSourceTreeOperationsQemu(qemu_configs.QEMU_ARCHS[arch],
+ elif arch in qemu_config.QEMU_ARCHS:
+ return LinuxSourceTreeOperationsQemu(qemu_config.QEMU_ARCHS[arch],
cross_compile=cross_compile)
else:
raise ConfigError(arch + ' is not a valid arch')
+def get_source_tree_ops_from_qemu_config(config_path: str, cross_compile: Optional[str]) -> qemu_config.QemuArchParams:
+ abs_tool_path = os.path.abspath(os.path.dirname(__file__))
+ abs_config_path = os.path.abspath(config_path)
+ if os.path.commonpath([abs_tool_path, abs_config_path]) != abs_tool_path:
+ raise ConfigError('Given QEMU config file is not in a child directory of KUnit tool.')
+ relative_config_path = os.path.relpath(abs_config_path, abs_tool_path)
+ spec = importlib.util.spec_from_file_location('.' + relative_config_path, config_path)
+ config = importlib.util.module_from_spec(spec)
+ spec.loader.exec_module(config)
+ return config.QEMU_ARCH.linux_arch, LinuxSourceTreeOperationsQemu(
+ config.QEMU_ARCH, cross_compile=cross_compile)
+
class LinuxSourceTree(object):
"""Represents a Linux kernel source tree with KUnit tests."""
- def __init__(self, build_dir: str, load_config=True, kunitconfig_path='', arch=None, cross_compile=None) -> None:
+ def __init__(self, build_dir: str, load_config=True, kunitconfig_path='', arch=None, cross_compile=None, qemu_config_path=None) -> None:
signal.signal(signal.SIGINT, self.signal_handler)
- self._arch = 'um' if arch is None else arch
- self._ops = get_source_tree_ops(self._arch, cross_compile)
+ if qemu_config_path:
+ self._arch, self._ops = get_source_tree_ops_from_qemu_config(qemu_config_path, cross_compile)
+ else:
+ self._arch = 'um' if arch is None else arch
+ self._ops = get_source_tree_ops(self._arch, cross_compile)
if not load_config:
return
diff --git a/tools/testing/kunit/qemu_configs.py b/tools/testing/kunit/qemu_config.py
similarity index 100%
rename from tools/testing/kunit/qemu_configs.py
rename to tools/testing/kunit/qemu_config.py
diff --git a/tools/testing/kunit/qemu_configs/__init__.py b/tools/testing/kunit/qemu_configs/__init__.py
new file mode 100644
index 0000000000000..e69de29bb2d1d
diff --git a/tools/testing/kunit/qemu_configs/i386.py b/tools/testing/kunit/qemu_configs/i386.py
new file mode 100644
index 0000000000000..7f079db044cc7
--- /dev/null
+++ b/tools/testing/kunit/qemu_configs/i386.py
@@ -0,0 +1,8 @@
+from ..qemu_config import QemuArchParams
+
+QEMU_ARCH = QemuArchParams(linux_arch='i386',
+ qemuconfig='CONFIG_SERIAL_8250=y\nCONFIG_SERIAL_8250_CONSOLE=y',
+ qemu_arch='x86_64',
+ kernel_path='arch/x86/boot/bzImage',
+ kernel_command_line='console=ttyS0',
+ extra_qemu_params=[''])
An example of how kunit_tool with the above change could be invoked is
as follows:
tools/testing/kunit/kunit.py run --timeout=60 --jobs=12 --qemu_config=./tools/testing/kunit/qemu_configs/i386.py
Let me know wha'cha think!
[...]
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [RFC v2 3/4] kunit: tool: add support for QEMU
2021-04-29 20:51 ` [RFC v2 3/4] kunit: tool: add support for QEMU Brendan Higgins
2021-04-29 23:39 ` Daniel Latypov
@ 2021-05-04 6:08 ` David Gow
2021-05-04 20:07 ` Brendan Higgins
1 sibling, 1 reply; 12+ messages in thread
From: David Gow @ 2021-05-04 6:08 UTC (permalink / raw)
To: Brendan Higgins
Cc: Shuah Khan, open list:KERNEL SELFTEST FRAMEWORK,
KUnit Development, Linux Kernel Mailing List, Jonathan Corbet,
open list:DOCUMENTATION, Stephen Boyd, Kees Cook, Frank Rowand,
Daniel Latypov
On Fri, Apr 30, 2021 at 4:51 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>
I've tested this out on a few architectures (x86-32, ppc64, etc), and
it all seems to work pretty well. I've got quite a few comments below.
The main one -- the location of the qemu configs -- is basically
echoing what Daniel said and what you've done on the Gerrit version,
but I felt was worth having on the record.
Otherwise, a bunch of nitpicks around missing places cross_compile
should be supported.
Still, great to see this happening: it's nice to be able to test on
non-UML architectures more easily.
Tested-by: David Gow <davidgow@google.com>
-- David
> ---
> tools/testing/kunit/kunit.py | 33 +++-
> tools/testing/kunit/kunit_config.py | 2 +-
> tools/testing/kunit/kunit_kernel.py | 207 +++++++++++++++++++++----
> tools/testing/kunit/kunit_tool_test.py | 15 +-
> 4 files changed, 217 insertions(+), 40 deletions(-)
>
> diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py
> index d5144fcb03acd..07ef80062873b 100755
> --- a/tools/testing/kunit/kunit.py
> +++ b/tools/testing/kunit/kunit.py
> @@ -70,10 +70,10 @@ def build_tests(linux: kunit_kernel.LinuxSourceTree,
> kunit_parser.print_with_timestamp('Building KUnit Kernel ...')
>
> build_start = time.time()
> - success = linux.build_um_kernel(request.alltests,
> - request.jobs,
> - request.build_dir,
> - request.make_options)
> + success = linux.build_kernel(request.alltests,
> + request.jobs,
> + request.build_dir,
> + request.make_options)
> build_end = time.time()
> if not success:
> return KunitResult(KunitStatus.BUILD_FAILURE,
> @@ -187,6 +187,14 @@ def add_common_opts(parser) -> None:
> help='Path to Kconfig fragment that enables KUnit tests',
> metavar='kunitconfig')
>
> + parser.add_argument('--arch',
> + help='Specifies the architecture to run tests under.',
> + type=str, default='um', metavar='arch')
It'd be nice to note explicitly that non-'um' values here will run under qemu.
> +
> + parser.add_argument('--cross_compile',
> + help='Sets make\'s CROSS_COMPILE variable.',
> + metavar='cross_compile')
> +
It'd be nice to have a slightly more detailed description here (that
its the compiler name's prefix or something).
> def add_build_opts(parser) -> None:
> parser.add_argument('--jobs',
> help='As in the make command, "Specifies the number of '
> @@ -268,7 +276,10 @@ def main(argv, linux=None):
> os.mkdir(cli_args.build_dir)
>
> if not linux:
> - linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir, kunitconfig_path=cli_args.kunitconfig)
> + linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir,
> + kunitconfig_path=cli_args.kunitconfig,
> + arch=cli_args.arch,
> + cross_compile=cli_args.cross_compile)
>
> request = KunitRequest(cli_args.raw_output,
> cli_args.timeout,
> @@ -287,7 +298,9 @@ def main(argv, linux=None):
> os.mkdir(cli_args.build_dir)
>
> if not linux:
> - linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir, kunitconfig_path=cli_args.kunitconfig)
> + linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir,
> + kunitconfig_path=cli_args.kunitconfig,
> + arch=cli_args.arch)
>
> request = KunitConfigRequest(cli_args.build_dir,
> cli_args.make_options)
> @@ -299,7 +312,9 @@ def main(argv, linux=None):
> sys.exit(1)
> elif cli_args.subcommand == 'build':
> if not linux:
> - linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir, kunitconfig_path=cli_args.kunitconfig)
> + linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir,
> + kunitconfig_path=cli_args.kunitconfig,
> + arch=cli_args.arch)
Why aren't we passing cross_compile through here? It's pretty
important for the 'build' step.
>
> request = KunitBuildRequest(cli_args.jobs,
> cli_args.build_dir,
> @@ -313,7 +328,9 @@ def main(argv, linux=None):
> sys.exit(1)
> elif cli_args.subcommand == 'exec':
> if not linux:
> - linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir)
> + linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir,
> + kunitconfig_path=cli_args.kunitconfig,
> + arch=cli_args.arch)
>
> exec_request = KunitExecRequest(cli_args.timeout,
> cli_args.build_dir,
> diff --git a/tools/testing/kunit/kunit_config.py b/tools/testing/kunit/kunit_config.py
> index 1e2683dcc0e7a..07d76be392544 100644
> --- a/tools/testing/kunit/kunit_config.py
> +++ b/tools/testing/kunit/kunit_config.py
> @@ -53,7 +53,7 @@ class Kconfig(object):
> return True
>
> def write_to_file(self, path: str) -> None:
> - with open(path, 'w') as f:
> + with open(path, 'a+') as f:
> for entry in self.entries():
> f.write(str(entry) + '\n')
>
> diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py
> index 7d5b77967d48f..b8b3b76aaa17e 100644
> --- a/tools/testing/kunit/kunit_kernel.py
> +++ b/tools/testing/kunit/kunit_kernel.py
> @@ -15,6 +15,8 @@ from typing import Iterator
>
> from contextlib import ExitStack
>
> +from collections import namedtuple
> +
> import kunit_config
> import kunit_parser
>
> @@ -40,6 +42,10 @@ class BuildError(Exception):
> class LinuxSourceTreeOperations(object):
> """An abstraction over command line operations performed on a source tree."""
>
> + def __init__(self, linux_arch, cross_compile):
> + self._linux_arch = linux_arch
> + self._cross_compile = cross_compile
> +
> def make_mrproper(self) -> None:
> try:
> subprocess.check_output(['make', 'mrproper'], stderr=subprocess.STDOUT)
> @@ -48,12 +54,18 @@ class LinuxSourceTreeOperations(object):
> except subprocess.CalledProcessError as e:
> raise ConfigError(e.output.decode())
>
> + def make_arch_qemuconfig(self, build_dir):
> + pass
> +
> def make_olddefconfig(self, build_dir, make_options) -> None:
> - command = ['make', 'ARCH=um', 'olddefconfig']
> + command = ['make', 'ARCH=' + self._linux_arch, 'olddefconfig']
> + if self._cross_compile:
> + command += ['CROSS_COMPILE=' + self._cross_compile]
> if make_options:
> command.extend(make_options)
> if build_dir:
> command += ['O=' + build_dir]
> + print(' '.join(command))
> try:
> subprocess.check_output(command, stderr=subprocess.STDOUT)
> except OSError as e:
> @@ -61,6 +73,154 @@ class LinuxSourceTreeOperations(object):
> except subprocess.CalledProcessError as e:
> raise ConfigError(e.output.decode())
>
> + def make(self, jobs, build_dir, make_options) -> None:
> + command = ['make', 'ARCH=' + self._linux_arch, '--jobs=' + str(jobs)]
> + if make_options:
> + command.extend(make_options)
> + if self._cross_compile:
> + command += ['CROSS_COMPILE=' + self._cross_compile]
> + if build_dir:
> + command += ['O=' + build_dir]
> + print(' '.join(command))
> + try:
> + proc = subprocess.Popen(command,
> + stderr=subprocess.PIPE,
> + stdout=subprocess.DEVNULL)
> + except OSError as e:
> + raise BuildError('Could not call execute make: ' + e)
> + except subprocess.CalledProcessError as e:
> + raise BuildError(e.output)
> + _, stderr = proc.communicate()
> + if proc.returncode != 0:
> + raise BuildError(stderr.decode())
> + if stderr: # likely only due to build warnings
> + print(stderr.decode())
> +
> + def run(self, params, timeout, build_dir, outfile) -> None:
> + pass
> +
> +
> +QemuArchParams = namedtuple('QemuArchParams', ['linux_arch',
> + 'qemuconfig',
> + 'qemu_arch',
> + 'kernel_path',
> + 'kernel_command_line',
> + 'extra_qemu_params'])
> +
> +
> +QEMU_ARCHS = {
> + 'i386' : QemuArchParams(linux_arch='i386',
> + qemuconfig='CONFIG_SERIAL_8250=y\nCONFIG_SERIAL_8250_CONSOLE=y',
I'm not keen on these configs being condensed like this in the source.
As mentioned below, moving the QemuArchParams to a separate file
helps, but even then it'd be nice to at least have each entry on a
separate line like with 'arm' below.
The other option would be to have this as a path to a separate
Kconfig/.kunitconfig file with these configs, rather than having them
inline here. I fear that could lead to every architecture needing
several support files, though some (e.g. x86/x86_64/alpha/sparc) could
share a file.
> + qemu_arch='x86_64',
> + kernel_path='arch/x86/boot/bzImage',
> + kernel_command_line='console=ttyS0',
> + extra_qemu_params=['']),
> + 'x86_64' : QemuArchParams(linux_arch='x86_64',
> + qemuconfig='CONFIG_SERIAL_8250=y\nCONFIG_SERIAL_8250_CONSOLE=y',
> + qemu_arch='x86_64',
> + kernel_path='arch/x86/boot/bzImage',
> + kernel_command_line='console=ttyS0',
> + extra_qemu_params=['']),
> + 'arm' : 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''',
> + qemu_arch='arm',
> + kernel_path='arch/arm/boot/zImage',
> + kernel_command_line='console=ttyAMA0',
> + extra_qemu_params=['-machine virt']),
> + 'arm64' : 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']),
> + 'alpha' : QemuArchParams(linux_arch='alpha',
> + qemuconfig='CONFIG_SERIAL_8250=y\nCONFIG_SERIAL_8250_CONSOLE=y',
> + qemu_arch='alpha',
> + kernel_path='arch/alpha/boot/vmlinux',
> + kernel_command_line='console=ttyS0',
> + extra_qemu_params=['']),
> + 'powerpc' : QemuArchParams(linux_arch='powerpc',
> + qemuconfig='CONFIG_PPC64=y\nCONFIG_SERIAL_8250=y\nCONFIG_SERIAL_8250_CONSOLE=y\nCONFIG_HVC_CONSOLE=y',
> + qemu_arch='ppc64',
> + kernel_path='vmlinux',
> + kernel_command_line='console=ttyS0',
> + extra_qemu_params=['-M pseries', '-cpu power8']),
> + 'riscv' : QemuArchParams(linux_arch='riscv',
> + qemuconfig='CONFIG_SOC_VIRT=y\nCONFIG_SERIAL_8250=y\nCONFIG_SERIAL_8250_CONSOLE=y\nCONFIG_SERIAL_OF_PLATFORM=y\nCONFIG_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']),
> + 's390' : QemuArchParams(linux_arch='s390',
> + qemuconfig='CONFIG_EXPERT=y\nCONFIG_TUNE_ZEC12=y\nCONFIG_NUMA=y\nCONFIG_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',]),
> + 'sparc' : QemuArchParams(linux_arch='sparc',
> + qemuconfig='CONFIG_SERIAL_8250=y\nCONFIG_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']),
> +}
So, as mentioned, I agree with Daniel in thinking that this really
doesn't belong here. Ideally, I think these configs should be
specified somewhere else, either as a (alas, very long) series of
command-line arguments, or in a separate file. Moving them to a single
'qemu_configs.py' as Daniel mentioned would be an improvement, but
IMHO still falls well short of ideal. I think each config should be in
its own file, as there are several reasons someone might want to have
multiple different configs for a given architecture and we'll want
people to be able to add/remove these easily in their own
environments.
The proposed solution of having each config in a separate file --
which we discussed offline -- is a big improvement:
https://kunit-review.googlesource.com/c/linux/+/4489
I still think that the python format here is a bit verbose and ugly,
but it's probably worth it compared to adding a parser for a whole new
type of config file. Similarly, I'm not sure about keeping a separate
qemu_configs.py around with the builtin defaults rather than just
having them all as separate files all the time. (Even appreciating
that that requires some filename magic to find the correct file.)
These are all more nitpicky though.
> +
> +class LinuxSourceTreeOperationsQemu(LinuxSourceTreeOperations):
> +
> + def __init__(self, qemu_arch_params, cross_compile):
> + super().__init__(linux_arch=qemu_arch_params.linux_arch,
> + cross_compile=cross_compile)
> + self._qemuconfig = qemu_arch_params.qemuconfig
> + self._qemu_arch = qemu_arch_params.qemu_arch
> + self._kernel_path = qemu_arch_params.kernel_path
> + print(self._kernel_path)
> + self._kernel_command_line = qemu_arch_params.kernel_command_line + ' kunit_shutdown=reboot'
> + self._extra_qemu_params = qemu_arch_params.extra_qemu_params
> +
> + def make_arch_qemuconfig(self, build_dir):
> + qemuconfig = kunit_config.Kconfig()
> + qemuconfig.parse_from_string(self._qemuconfig)
> + qemuconfig.write_to_file(get_kconfig_path(build_dir))
> +
> + def run(self, params, timeout, build_dir, outfile):
> + kernel_path = os.path.join(build_dir, self._kernel_path)
> + qemu_command = ['qemu-system-' + self._qemu_arch,
> + '-nodefaults',
> + '-m', '1024',
> + '-kernel', kernel_path,
> + '-append', '\'' + ' '.join(params + [self._kernel_command_line]) + '\'',
> + '-no-reboot',
> + '-nographic',
> + '-serial stdio'] + self._extra_qemu_params
> + print(' '.join(qemu_command))
> + with open(outfile, 'w') as output:
> + process = subprocess.Popen(' '.join(qemu_command),
> + stdin=subprocess.PIPE,
> + stdout=output,
> + stderr=subprocess.STDOUT,
> + text=True, shell=True)
> + try:
> + process.wait(timeout=timeout)
> + except Exception as e:
> + print(e)
> + process.terminate()
> + return process
> +
> +class LinuxSourceTreeOperationsUml(LinuxSourceTreeOperations):
> + """An abstraction over command line operations performed on a source tree."""
> +
> + def __init__(self):
> + super().__init__(linux_arch='um', cross_compile=None)
> +
Why is cross_compile None here? Shouldn't it be possible to cross
compile UML kernels? (This should, I think, be possible for i386 on an
x86_64 host, for instance.)
> def make_allyesconfig(self, build_dir, make_options) -> None:
> kunit_parser.print_with_timestamp(
> 'Enabling all CONFIGs for UML...')
> @@ -83,32 +243,16 @@ class LinuxSourceTreeOperations(object):
> kunit_parser.print_with_timestamp(
> 'Starting Kernel with all configs takes a few minutes...')
>
> - def make(self, jobs, build_dir, make_options) -> None:
> - command = ['make', 'ARCH=um', '--jobs=' + str(jobs)]
> - if make_options:
> - command.extend(make_options)
> - if build_dir:
> - command += ['O=' + build_dir]
> - try:
> - proc = subprocess.Popen(command,
> - stderr=subprocess.PIPE,
> - stdout=subprocess.DEVNULL)
> - except OSError as e:
> - raise BuildError('Could not call make command: ' + str(e))
> - _, stderr = proc.communicate()
> - if proc.returncode != 0:
> - raise BuildError(stderr.decode())
> - if stderr: # likely only due to build warnings
> - print(stderr.decode())
> -
> - def linux_bin(self, params, timeout, build_dir) -> None:
> + def run(self, params, timeout, build_dir, outfile):
> """Runs the Linux UML binary. Must be named 'linux'."""
> linux_bin = get_file_path(build_dir, 'linux')
> outfile = get_outfile_path(build_dir)
> with open(outfile, 'w') as output:
> process = subprocess.Popen([linux_bin] + params,
> + stdin=subprocess.PIPE,
> stdout=output,
> - stderr=subprocess.STDOUT)
> + stderr=subprocess.STDOUT,
> + text=True)
> process.wait(timeout)
>
> def get_kconfig_path(build_dir) -> str:
> @@ -123,10 +267,17 @@ def get_outfile_path(build_dir) -> str:
> class LinuxSourceTree(object):
> """Represents a Linux kernel source tree with KUnit tests."""
>
> - def __init__(self, build_dir: str, load_config=True, kunitconfig_path='') -> None:
> + def __init__(self, build_dir: str, load_config=True, kunitconfig_path='', arch=None, cross_compile=None) -> None:
> signal.signal(signal.SIGINT, self.signal_handler)
> -
> - self._ops = LinuxSourceTreeOperations()
> + self._ops = None
> + if arch is None or arch == 'um':
> + self._arch = 'um'
> + self._ops = LinuxSourceTreeOperationsUml()
Again, we should probably pass cross_compile through here.
> + elif arch in QEMU_ARCHS:
> + self._arch = arch
> + self._ops = LinuxSourceTreeOperationsQemu(QEMU_ARCHS[arch], cross_compile=cross_compile)
> + else:
> + raise ConfigError(arch + ' is not a valid arch')
>
> if not load_config:
> return
> @@ -170,6 +321,7 @@ class LinuxSourceTree(object):
> os.mkdir(build_dir)
> self._kconfig.write_to_file(kconfig_path)
> try:
> + self._ops.make_arch_qemuconfig(build_dir)
> self._ops.make_olddefconfig(build_dir, make_options)
> except ConfigError as e:
> logging.error(e)
> @@ -192,10 +344,13 @@ class LinuxSourceTree(object):
> print('Generating .config ...')
> return self.build_config(build_dir, make_options)
>
> - def build_um_kernel(self, alltests, jobs, build_dir, make_options) -> bool:
> + def build_kernel(self, alltests, jobs, build_dir, make_options) -> bool:
> try:
> if alltests:
> + if self._arch != 'um':
> + raise ConfigError('Only the "um" arch is supported for alltests')
> self._ops.make_allyesconfig(build_dir, make_options)
> + self._ops.make_arch_qemuconfig(build_dir)
> self._ops.make_olddefconfig(build_dir, make_options)
> self._ops.make(jobs, build_dir, make_options)
> except (ConfigError, BuildError) as e:
> @@ -209,8 +364,8 @@ class LinuxSourceTree(object):
> args.extend(['mem=1G', 'console=tty','kunit_shutdown=halt'])
> if filter_glob:
> args.append('kunit.filter_glob='+filter_glob)
> - self._ops.linux_bin(args, timeout, build_dir)
> outfile = get_outfile_path(build_dir)
> + self._ops.run(args, timeout, build_dir, outfile)
> subprocess.call(['stty', 'sane'])
> with open(outfile, 'r') as file:
> for line in file:
> diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py
> index 1ad3049e90698..25e8be95a575d 100755
> --- a/tools/testing/kunit/kunit_tool_test.py
> +++ b/tools/testing/kunit/kunit_tool_test.py
> @@ -297,7 +297,7 @@ class KUnitMainTest(unittest.TestCase):
>
> self.linux_source_mock = mock.Mock()
> self.linux_source_mock.build_reconfig = mock.Mock(return_value=True)
> - self.linux_source_mock.build_um_kernel = mock.Mock(return_value=True)
> + self.linux_source_mock.build_kernel = mock.Mock(return_value=True)
> self.linux_source_mock.run_kernel = mock.Mock(return_value=all_passed_log)
>
> def test_config_passes_args_pass(self):
> @@ -308,7 +308,7 @@ class KUnitMainTest(unittest.TestCase):
> def test_build_passes_args_pass(self):
> kunit.main(['build'], self.linux_source_mock)
> self.assertEqual(self.linux_source_mock.build_reconfig.call_count, 0)
> - self.linux_source_mock.build_um_kernel.assert_called_once_with(False, 8, '.kunit', None)
> + self.linux_source_mock.build_kernel.assert_called_once_with(False, 8, '.kunit', None)
> self.assertEqual(self.linux_source_mock.run_kernel.call_count, 0)
>
> def test_exec_passes_args_pass(self):
> @@ -390,7 +390,7 @@ class KUnitMainTest(unittest.TestCase):
> def test_build_builddir(self):
> build_dir = '.kunit'
> kunit.main(['build', '--build_dir', build_dir], self.linux_source_mock)
> - self.linux_source_mock.build_um_kernel.assert_called_once_with(False, 8, build_dir, None)
> + self.linux_source_mock.build_kernel.assert_called_once_with(False, 8, build_dir, None)
>
> def test_exec_builddir(self):
> build_dir = '.kunit'
> @@ -404,14 +404,19 @@ class KUnitMainTest(unittest.TestCase):
> mock_linux_init.return_value = self.linux_source_mock
> kunit.main(['run', '--kunitconfig=mykunitconfig'])
> # Just verify that we parsed and initialized it correctly here.
> - mock_linux_init.assert_called_once_with('.kunit', kunitconfig_path='mykunitconfig')
> + mock_linux_init.assert_called_once_with('.kunit',
> + kunitconfig_path='mykunitconfig',
> + arch='um',
> + cross_compile=None)
>
> @mock.patch.object(kunit_kernel, 'LinuxSourceTree')
> def test_config_kunitconfig(self, mock_linux_init):
> mock_linux_init.return_value = self.linux_source_mock
> kunit.main(['config', '--kunitconfig=mykunitconfig'])
> # Just verify that we parsed and initialized it correctly here.
> - mock_linux_init.assert_called_once_with('.kunit', kunitconfig_path='mykunitconfig')
> + mock_linux_init.assert_called_once_with('.kunit',
> + kunitconfig_path='mykunitconfig',
> + arch='um')
>
> if __name__ == '__main__':
> unittest.main()
> --
> 2.31.1.498.g6c1eba8ee3d-goog
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC v2 3/4] kunit: tool: add support for QEMU
2021-05-04 6:08 ` David Gow
@ 2021-05-04 20:07 ` Brendan Higgins
0 siblings, 0 replies; 12+ messages in thread
From: Brendan Higgins @ 2021-05-04 20:07 UTC (permalink / raw)
To: David Gow
Cc: Shuah Khan, open list:KERNEL SELFTEST FRAMEWORK,
KUnit Development, Linux Kernel Mailing List, Jonathan Corbet,
open list:DOCUMENTATION, Stephen Boyd, Kees Cook, Frank Rowand,
Daniel Latypov
On Tue, May 04, 2021 at 02:08:41PM +0800, David Gow wrote:
> On Fri, Apr 30, 2021 at 4:51 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>
>
> I've tested this out on a few architectures (x86-32, ppc64, etc), and
> it all seems to work pretty well. I've got quite a few comments below.
> The main one -- the location of the qemu configs -- is basically
> echoing what Daniel said and what you've done on the Gerrit version,
> but I felt was worth having on the record.
>
> Otherwise, a bunch of nitpicks around missing places cross_compile
> should be supported.
>
> Still, great to see this happening: it's nice to be able to test on
> non-UML architectures more easily.
>
> Tested-by: David Gow <davidgow@google.com>
Sweet, thanks!
> -- David
>
> > ---
> > tools/testing/kunit/kunit.py | 33 +++-
> > tools/testing/kunit/kunit_config.py | 2 +-
> > tools/testing/kunit/kunit_kernel.py | 207 +++++++++++++++++++++----
> > tools/testing/kunit/kunit_tool_test.py | 15 +-
> > 4 files changed, 217 insertions(+), 40 deletions(-)
> >
> > diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py
> > index d5144fcb03acd..07ef80062873b 100755
> > --- a/tools/testing/kunit/kunit.py
> > +++ b/tools/testing/kunit/kunit.py
> > @@ -70,10 +70,10 @@ def build_tests(linux: kunit_kernel.LinuxSourceTree,
> > kunit_parser.print_with_timestamp('Building KUnit Kernel ...')
> >
> > build_start = time.time()
> > - success = linux.build_um_kernel(request.alltests,
> > - request.jobs,
> > - request.build_dir,
> > - request.make_options)
> > + success = linux.build_kernel(request.alltests,
> > + request.jobs,
> > + request.build_dir,
> > + request.make_options)
> > build_end = time.time()
> > if not success:
> > return KunitResult(KunitStatus.BUILD_FAILURE,
> > @@ -187,6 +187,14 @@ def add_common_opts(parser) -> None:
> > help='Path to Kconfig fragment that enables KUnit tests',
> > metavar='kunitconfig')
> >
> > + parser.add_argument('--arch',
> > + help='Specifies the architecture to run tests under.',
> > + type=str, default='um', metavar='arch')
>
> It'd be nice to note explicitly that non-'um' values here will run under qemu.
Good point. Will fix.
> > +
> > + parser.add_argument('--cross_compile',
> > + help='Sets make\'s CROSS_COMPILE variable.',
> > + metavar='cross_compile')
> > +
>
> It'd be nice to have a slightly more detailed description here (that
> its the compiler name's prefix or something).
How about?:
Sets make's CROSS_COMPILE variable; it should be set to a toolchain path
prefix (the prefix of gcc and other tools in your toolchain, for example
`sparc64-linux-gnu-` if you have the sparc toolchain installed on your
system, or `$HOME/toolchains/microblaze/gcc-9.2.0-nolibc/microblaze-linux/bin/microblaze-linux-`
if you have downloaded the microblaze toolchain from the 0-day website
to a directory in your home directory called `toolchains`).
> > def add_build_opts(parser) -> None:
> > parser.add_argument('--jobs',
> > help='As in the make command, "Specifies the number of '
> > @@ -268,7 +276,10 @@ def main(argv, linux=None):
> > os.mkdir(cli_args.build_dir)
> >
> > if not linux:
> > - linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir, kunitconfig_path=cli_args.kunitconfig)
> > + linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir,
> > + kunitconfig_path=cli_args.kunitconfig,
> > + arch=cli_args.arch,
> > + cross_compile=cli_args.cross_compile)
> >
> > request = KunitRequest(cli_args.raw_output,
> > cli_args.timeout,
> > @@ -287,7 +298,9 @@ def main(argv, linux=None):
> > os.mkdir(cli_args.build_dir)
> >
> > if not linux:
> > - linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir, kunitconfig_path=cli_args.kunitconfig)
> > + linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir,
> > + kunitconfig_path=cli_args.kunitconfig,
> > + arch=cli_args.arch)
> >
> > request = KunitConfigRequest(cli_args.build_dir,
> > cli_args.make_options)
> > @@ -299,7 +312,9 @@ def main(argv, linux=None):
> > sys.exit(1)
> > elif cli_args.subcommand == 'build':
> > if not linux:
> > - linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir, kunitconfig_path=cli_args.kunitconfig)
> > + linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir,
> > + kunitconfig_path=cli_args.kunitconfig,
> > + arch=cli_args.arch)
>
> Why aren't we passing cross_compile through here? It's pretty
> important for the 'build' step.
Good catch. Will fix.
> >
> > request = KunitBuildRequest(cli_args.jobs,
> > cli_args.build_dir,
> > @@ -313,7 +328,9 @@ def main(argv, linux=None):
> > sys.exit(1)
> > elif cli_args.subcommand == 'exec':
> > if not linux:
> > - linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir)
> > + linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir,
> > + kunitconfig_path=cli_args.kunitconfig,
> > + arch=cli_args.arch)
> >
> > exec_request = KunitExecRequest(cli_args.timeout,
> > cli_args.build_dir,
> > diff --git a/tools/testing/kunit/kunit_config.py b/tools/testing/kunit/kunit_config.py
> > index 1e2683dcc0e7a..07d76be392544 100644
> > --- a/tools/testing/kunit/kunit_config.py
> > +++ b/tools/testing/kunit/kunit_config.py
> > @@ -53,7 +53,7 @@ class Kconfig(object):
> > return True
> >
> > def write_to_file(self, path: str) -> None:
> > - with open(path, 'w') as f:
> > + with open(path, 'a+') as f:
> > for entry in self.entries():
> > f.write(str(entry) + '\n')
> >
> > diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py
> > index 7d5b77967d48f..b8b3b76aaa17e 100644
> > --- a/tools/testing/kunit/kunit_kernel.py
> > +++ b/tools/testing/kunit/kunit_kernel.py
> > @@ -15,6 +15,8 @@ from typing import Iterator
> >
> > from contextlib import ExitStack
> >
> > +from collections import namedtuple
> > +
> > import kunit_config
> > import kunit_parser
> >
> > @@ -40,6 +42,10 @@ class BuildError(Exception):
> > class LinuxSourceTreeOperations(object):
> > """An abstraction over command line operations performed on a source tree."""
> >
> > + def __init__(self, linux_arch, cross_compile):
> > + self._linux_arch = linux_arch
> > + self._cross_compile = cross_compile
> > +
> > def make_mrproper(self) -> None:
> > try:
> > subprocess.check_output(['make', 'mrproper'], stderr=subprocess.STDOUT)
> > @@ -48,12 +54,18 @@ class LinuxSourceTreeOperations(object):
> > except subprocess.CalledProcessError as e:
> > raise ConfigError(e.output.decode())
> >
> > + def make_arch_qemuconfig(self, build_dir):
> > + pass
> > +
> > def make_olddefconfig(self, build_dir, make_options) -> None:
> > - command = ['make', 'ARCH=um', 'olddefconfig']
> > + command = ['make', 'ARCH=' + self._linux_arch, 'olddefconfig']
> > + if self._cross_compile:
> > + command += ['CROSS_COMPILE=' + self._cross_compile]
> > if make_options:
> > command.extend(make_options)
> > if build_dir:
> > command += ['O=' + build_dir]
> > + print(' '.join(command))
> > try:
> > subprocess.check_output(command, stderr=subprocess.STDOUT)
> > except OSError as e:
> > @@ -61,6 +73,154 @@ class LinuxSourceTreeOperations(object):
> > except subprocess.CalledProcessError as e:
> > raise ConfigError(e.output.decode())
> >
> > + def make(self, jobs, build_dir, make_options) -> None:
> > + command = ['make', 'ARCH=' + self._linux_arch, '--jobs=' + str(jobs)]
> > + if make_options:
> > + command.extend(make_options)
> > + if self._cross_compile:
> > + command += ['CROSS_COMPILE=' + self._cross_compile]
> > + if build_dir:
> > + command += ['O=' + build_dir]
> > + print(' '.join(command))
> > + try:
> > + proc = subprocess.Popen(command,
> > + stderr=subprocess.PIPE,
> > + stdout=subprocess.DEVNULL)
> > + except OSError as e:
> > + raise BuildError('Could not call execute make: ' + e)
> > + except subprocess.CalledProcessError as e:
> > + raise BuildError(e.output)
> > + _, stderr = proc.communicate()
> > + if proc.returncode != 0:
> > + raise BuildError(stderr.decode())
> > + if stderr: # likely only due to build warnings
> > + print(stderr.decode())
> > +
> > + def run(self, params, timeout, build_dir, outfile) -> None:
> > + pass
> > +
> > +
> > +QemuArchParams = namedtuple('QemuArchParams', ['linux_arch',
> > + 'qemuconfig',
> > + 'qemu_arch',
> > + 'kernel_path',
> > + 'kernel_command_line',
> > + 'extra_qemu_params'])
> > +
> > +
> > +QEMU_ARCHS = {
> > + 'i386' : QemuArchParams(linux_arch='i386',
> > + qemuconfig='CONFIG_SERIAL_8250=y\nCONFIG_SERIAL_8250_CONSOLE=y',
>
> I'm not keen on these configs being condensed like this in the source.
> As mentioned below, moving the QemuArchParams to a separate file
> helps, but even then it'd be nice to at least have each entry on a
> separate line like with 'arm' below.
Good to note. I was not sure about the multiline strings, part of me
thought they were an improvement. Part of me thought they were horribly
ugly.
> The other option would be to have this as a path to a separate
> Kconfig/.kunitconfig file with these configs, rather than having them
> inline here. I fear that could lead to every architecture needing
> several support files, though some (e.g. x86/x86_64/alpha/sparc) could
> share a file.
Yeah, I went back and forth on it. Part of me thinks it is nice to have
all your configs in one place. The other part of me agrees with what you
said here. I think I will stick with the way I am doing it now for no
other reason than that's the way I already have it. If someone feels
strongly though, I would be happy to change it.
> > + qemu_arch='x86_64',
> > + kernel_path='arch/x86/boot/bzImage',
> > + kernel_command_line='console=ttyS0',
> > + extra_qemu_params=['']),
> > + 'x86_64' : QemuArchParams(linux_arch='x86_64',
> > + qemuconfig='CONFIG_SERIAL_8250=y\nCONFIG_SERIAL_8250_CONSOLE=y',
> > + qemu_arch='x86_64',
> > + kernel_path='arch/x86/boot/bzImage',
> > + kernel_command_line='console=ttyS0',
> > + extra_qemu_params=['']),
> > + 'arm' : 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''',
> > + qemu_arch='arm',
> > + kernel_path='arch/arm/boot/zImage',
> > + kernel_command_line='console=ttyAMA0',
> > + extra_qemu_params=['-machine virt']),
> > + 'arm64' : 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']),
> > + 'alpha' : QemuArchParams(linux_arch='alpha',
> > + qemuconfig='CONFIG_SERIAL_8250=y\nCONFIG_SERIAL_8250_CONSOLE=y',
> > + qemu_arch='alpha',
> > + kernel_path='arch/alpha/boot/vmlinux',
> > + kernel_command_line='console=ttyS0',
> > + extra_qemu_params=['']),
> > + 'powerpc' : QemuArchParams(linux_arch='powerpc',
> > + qemuconfig='CONFIG_PPC64=y\nCONFIG_SERIAL_8250=y\nCONFIG_SERIAL_8250_CONSOLE=y\nCONFIG_HVC_CONSOLE=y',
> > + qemu_arch='ppc64',
> > + kernel_path='vmlinux',
> > + kernel_command_line='console=ttyS0',
> > + extra_qemu_params=['-M pseries', '-cpu power8']),
> > + 'riscv' : QemuArchParams(linux_arch='riscv',
> > + qemuconfig='CONFIG_SOC_VIRT=y\nCONFIG_SERIAL_8250=y\nCONFIG_SERIAL_8250_CONSOLE=y\nCONFIG_SERIAL_OF_PLATFORM=y\nCONFIG_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']),
> > + 's390' : QemuArchParams(linux_arch='s390',
> > + qemuconfig='CONFIG_EXPERT=y\nCONFIG_TUNE_ZEC12=y\nCONFIG_NUMA=y\nCONFIG_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',]),
> > + 'sparc' : QemuArchParams(linux_arch='sparc',
> > + qemuconfig='CONFIG_SERIAL_8250=y\nCONFIG_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']),
> > +}
>
> So, as mentioned, I agree with Daniel in thinking that this really
> doesn't belong here. Ideally, I think these configs should be
> specified somewhere else, either as a (alas, very long) series of
> command-line arguments, or in a separate file. Moving them to a single
> 'qemu_configs.py' as Daniel mentioned would be an improvement, but
> IMHO still falls well short of ideal. I think each config should be in
> its own file, as there are several reasons someone might want to have
> multiple different configs for a given architecture and we'll want
> people to be able to add/remove these easily in their own
> environments.
>
> The proposed solution of having each config in a separate file --
> which we discussed offline -- is a big improvement:
> https://kunit-review.googlesource.com/c/linux/+/4489
Good to know, I will incorporate it into the next revision.
> I still think that the python format here is a bit verbose and ugly,
> but it's probably worth it compared to adding a parser for a whole new
> type of config file.
That's where I am at with it. I feel as though I have added enough
parsers to the kernel tree. :-)
> Similarly, I'm not sure about keeping a separate
> qemu_configs.py around with the builtin defaults rather than just
> having them all as separate files all the time. (Even appreciating
> that that requires some filename magic to find the correct file.)
> These are all more nitpicky though.
This raises an excellent point, what should we do with the --arch flag
if we add the --qemu_config flag?
The only reason I just added --qemu_config without touching the existing
configs was just because I was being lazy; nevertheless, my original
plan was just to drop --arch entirely and just expect the user to know
that to run KUnit on i386 you need:
tools/testing/kunit/kunit.py run --qemu_config=./tools/testing/kunit/qemu_configs/i386.py
and to run KUnit on arm you need:
tools/testing/kunit/kunit.py run --qemu_config=./tools/testing/kunit/qemu_configs/arm.py --cross_compile=arm-linux-gnueabihf-
(or something like that).
Is that OK, or as you hint, should we do some file system magic so that
you have the option of
tools/testing/kunit/kunit.py run --qemu_config=./tools/testing/kunit/qemu_configs/i386.py
OR
tools/testing/kunit/kunit.py run --arch=i386
?
My preference is for the former since it is more explicit about what it
is doing.
> > +
> > +class LinuxSourceTreeOperationsQemu(LinuxSourceTreeOperations):
> > +
> > + def __init__(self, qemu_arch_params, cross_compile):
> > + super().__init__(linux_arch=qemu_arch_params.linux_arch,
> > + cross_compile=cross_compile)
> > + self._qemuconfig = qemu_arch_params.qemuconfig
> > + self._qemu_arch = qemu_arch_params.qemu_arch
> > + self._kernel_path = qemu_arch_params.kernel_path
> > + print(self._kernel_path)
> > + self._kernel_command_line = qemu_arch_params.kernel_command_line + ' kunit_shutdown=reboot'
> > + self._extra_qemu_params = qemu_arch_params.extra_qemu_params
> > +
> > + def make_arch_qemuconfig(self, build_dir):
> > + qemuconfig = kunit_config.Kconfig()
> > + qemuconfig.parse_from_string(self._qemuconfig)
> > + qemuconfig.write_to_file(get_kconfig_path(build_dir))
> > +
> > + def run(self, params, timeout, build_dir, outfile):
> > + kernel_path = os.path.join(build_dir, self._kernel_path)
> > + qemu_command = ['qemu-system-' + self._qemu_arch,
> > + '-nodefaults',
> > + '-m', '1024',
> > + '-kernel', kernel_path,
> > + '-append', '\'' + ' '.join(params + [self._kernel_command_line]) + '\'',
> > + '-no-reboot',
> > + '-nographic',
> > + '-serial stdio'] + self._extra_qemu_params
> > + print(' '.join(qemu_command))
> > + with open(outfile, 'w') as output:
> > + process = subprocess.Popen(' '.join(qemu_command),
> > + stdin=subprocess.PIPE,
> > + stdout=output,
> > + stderr=subprocess.STDOUT,
> > + text=True, shell=True)
> > + try:
> > + process.wait(timeout=timeout)
> > + except Exception as e:
> > + print(e)
> > + process.terminate()
> > + return process
> > +
> > +class LinuxSourceTreeOperationsUml(LinuxSourceTreeOperations):
> > + """An abstraction over command line operations performed on a source tree."""
> > +
> > + def __init__(self):
> > + super().__init__(linux_arch='um', cross_compile=None)
> > +
>
> Why is cross_compile None here? Shouldn't it be possible to cross
> compile UML kernels? (This should, I think, be possible for i386 on an
> x86_64 host, for instance.)
Fair point. Will fix.
> > def make_allyesconfig(self, build_dir, make_options) -> None:
> > kunit_parser.print_with_timestamp(
> > 'Enabling all CONFIGs for UML...')
> > @@ -83,32 +243,16 @@ class LinuxSourceTreeOperations(object):
> > kunit_parser.print_with_timestamp(
> > 'Starting Kernel with all configs takes a few minutes...')
> >
> > - def make(self, jobs, build_dir, make_options) -> None:
> > - command = ['make', 'ARCH=um', '--jobs=' + str(jobs)]
> > - if make_options:
> > - command.extend(make_options)
> > - if build_dir:
> > - command += ['O=' + build_dir]
> > - try:
> > - proc = subprocess.Popen(command,
> > - stderr=subprocess.PIPE,
> > - stdout=subprocess.DEVNULL)
> > - except OSError as e:
> > - raise BuildError('Could not call make command: ' + str(e))
> > - _, stderr = proc.communicate()
> > - if proc.returncode != 0:
> > - raise BuildError(stderr.decode())
> > - if stderr: # likely only due to build warnings
> > - print(stderr.decode())
> > -
> > - def linux_bin(self, params, timeout, build_dir) -> None:
> > + def run(self, params, timeout, build_dir, outfile):
> > """Runs the Linux UML binary. Must be named 'linux'."""
> > linux_bin = get_file_path(build_dir, 'linux')
> > outfile = get_outfile_path(build_dir)
> > with open(outfile, 'w') as output:
> > process = subprocess.Popen([linux_bin] + params,
> > + stdin=subprocess.PIPE,
> > stdout=output,
> > - stderr=subprocess.STDOUT)
> > + stderr=subprocess.STDOUT,
> > + text=True)
> > process.wait(timeout)
> >
> > def get_kconfig_path(build_dir) -> str:
> > @@ -123,10 +267,17 @@ def get_outfile_path(build_dir) -> str:
> > class LinuxSourceTree(object):
> > """Represents a Linux kernel source tree with KUnit tests."""
> >
> > - def __init__(self, build_dir: str, load_config=True, kunitconfig_path='') -> None:
> > + def __init__(self, build_dir: str, load_config=True, kunitconfig_path='', arch=None, cross_compile=None) -> None:
> > signal.signal(signal.SIGINT, self.signal_handler)
> > -
> > - self._ops = LinuxSourceTreeOperations()
> > + self._ops = None
> > + if arch is None or arch == 'um':
> > + self._arch = 'um'
> > + self._ops = LinuxSourceTreeOperationsUml()
>
> Again, we should probably pass cross_compile through here.
Yep. Will do.
[...]
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2021-05-04 20:08 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-29 20:51 [RFC v2 0/4] kunit: tool: add support for QEMU Brendan Higgins
2021-04-29 20:51 ` [RFC v2 1/4] kunit: Add 'kunit_shutdown' option Brendan Higgins
2021-04-29 21:52 ` Daniel Latypov
2021-04-29 20:51 ` [RFC v2 2/4] Documentation: Add kunit_shutdown to kernel-parameters.txt Brendan Higgins
2021-04-29 20:51 ` [RFC v2 3/4] kunit: tool: add support for QEMU Brendan Higgins
2021-04-29 23:39 ` Daniel Latypov
2021-04-30 20:01 ` Brendan Higgins
2021-04-30 20:14 ` Daniel Latypov
2021-05-03 21:19 ` Brendan Higgins
2021-05-04 6:08 ` David Gow
2021-05-04 20:07 ` Brendan Higgins
2021-04-29 20:51 ` [RFC v2 4/4] Documentation: kunit: document support for QEMU in kunit_tool Brendan Higgins
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).