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!
[...]