| Message ID | 20220622035326.759935-1-davidgow@google.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Brendan Higgins |
| Headers | show |
| Series | [RFC] kunit: tool: Enable virtio/PCI by default on UML | expand |
On Tue, Jun 21, 2022 at 8:53 PM David Gow <davidgow@google.com> wrote: > > There are several tests which depend on PCI, and hence need a bunch of > extra options to run under UML. This makes it awkward to give > configuration instructions (whether in documentation, or as part of a > .kunitconfig file), as two separate, incompatible sets of config options > are required for UML and "most other architectures". > > For non-UML architectures, it's possible to add default kconfig options > via the qemu_config python files, but there's no equivalent for UML. Add > a new tools/testing/kunit/configs/arch_uml.config file containing extra > kconfig options to use on UML. > > Signed-off-by: David Gow <davidgow@google.com> > --- > > It's really ugly to have to type: > --kconfig_add CONFIG_VIRTIO_UML=y > --kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y > when running many tests under UML, particularly since it isn't required > on other architectures. > > This came up in discussion with Daniel this morning, and while the > ability to repeat the --kunitconfig flag would go some way to alleviate > this, having to add: > --kunitconfig ./tools/testing/kunit/config/uml_pci.kunitconfig > isn't all that much better. > > So it seems like adding something by default would be nice. > > This implementation is not perfect (in particular, there's no easy way > of _disabling_ these options now, though [1] probably will help). The I assume the missing link for [1] is https://lore.kernel.org/linux-kselftest/20220520224200.3764027-1-dlatypov@google.com/ Note that we'll have to update one of these patches depending on the order they go in. [1] had to change make_arch_qemuconfig() to ensure that arch/qemu configs have *lower* priority than user-specified overrides. So we'll either need to do the same here or update [1] for this to work out. > 'arch_uml.config' filename can be bikeshedded, too. > > Thoughts? I was initially against effectively hard-coding these in, but it feels like making CONFIG_PCI=y work by default on UML is worth it. We've talked about architecture-specific options in kunitconfig files and it's only ever been about PCI. Nothing else has come up yet, so this patch would eliminate that concern for now.
On Tue, Jun 21, 2022 at 8:53 PM David Gow <davidgow@google.com> wrote: > > There are several tests which depend on PCI, and hence need a bunch of > extra options to run under UML. This makes it awkward to give > configuration instructions (whether in documentation, or as part of a > .kunitconfig file), as two separate, incompatible sets of config options > are required for UML and "most other architectures". > > For non-UML architectures, it's possible to add default kconfig options > via the qemu_config python files, but there's no equivalent for UML. Add > a new tools/testing/kunit/configs/arch_uml.config file containing extra > kconfig options to use on UML. > > Signed-off-by: David Gow <davidgow@google.com> > --- > > It's really ugly to have to type: > --kconfig_add CONFIG_VIRTIO_UML=y > --kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y > when running many tests under UML, particularly since it isn't required > on other architectures. > > This came up in discussion with Daniel this morning, and while the > ability to repeat the --kunitconfig flag would go some way to alleviate > this, having to add: > --kunitconfig ./tools/testing/kunit/config/uml_pci.kunitconfig > isn't all that much better. > > So it seems like adding something by default would be nice. > > This implementation is not perfect (in particular, there's no easy way > of _disabling_ these options now, though [1] probably will help). The > 'arch_uml.config' filename can be bikeshedded, too. > > Thoughts? I am supportive of adding the PCI dependency as a default for UML; I agree that there is a common desire to use PCI for tests and enabling it on UML is wonky. One additional note: It looks like this patch breaks kunit_tool: https://linux-review.googlesource.com/c/linux/kernel/git/torvalds/linux/+/21871 The python error in the log seems legitimate: https://storage.googleapis.com/oss-prow/pr-logs/pull/linux-review.googlesource.com_linux_kernel_git_torvalds_linux/21871/linux-kernel-mailing-list-presubmit/1539456886976286720/build-log.txt This line in particular: AttributeError: 'LinuxSourceTreeOperationsUml' object has no attribute 'make_arch_qemuconfig' > --- > tools/testing/kunit/configs/arch_uml.config | 5 +++++ > tools/testing/kunit/kunit_kernel.py | 11 ++++++++--- > 2 files changed, 13 insertions(+), 3 deletions(-) > create mode 100644 tools/testing/kunit/configs/arch_uml.config > > diff --git a/tools/testing/kunit/configs/arch_uml.config b/tools/testing/kunit/configs/arch_uml.config > new file mode 100644 > index 000000000000..e824ce43b05a > --- /dev/null > +++ b/tools/testing/kunit/configs/arch_uml.config > @@ -0,0 +1,5 @@ > +# Config options which are added to UML builds by default > + > +# Enable virtio/pci, as a lot of tests require it. > +CONFIG_VIRTIO_UML=y > +CONFIG_UML_PCI_OVER_VIRTIO=y > diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py > index 3539efaf99ba..05e7b1e188d7 100644 > --- a/tools/testing/kunit/kunit_kernel.py > +++ b/tools/testing/kunit/kunit_kernel.py > @@ -26,6 +26,7 @@ KUNITCONFIG_PATH = '.kunitconfig' > OLD_KUNITCONFIG_PATH = 'last_used_kunitconfig' > DEFAULT_KUNITCONFIG_PATH = 'tools/testing/kunit/configs/default.config' > BROKEN_ALLCONFIG_PATH = 'tools/testing/kunit/configs/broken_on_uml.config' > +UML_KCONFIG_PATH = 'tools/testing/kunit/configs/arch_uml.config' > OUTFILE_PATH = 'test.log' > ABS_TOOL_PATH = os.path.abspath(os.path.dirname(__file__)) > QEMU_CONFIGS_DIR = os.path.join(ABS_TOOL_PATH, 'qemu_configs') > @@ -53,7 +54,7 @@ class LinuxSourceTreeOperations: > except subprocess.CalledProcessError as e: > raise ConfigError(e.output.decode()) > > - def make_arch_qemuconfig(self, base_kunitconfig: kunit_config.Kconfig) -> None: > + def make_arch_config(self, base_kunitconfig: kunit_config.Kconfig) -> None: > pass > > def make_allyesconfig(self, build_dir: str, make_options) -> None: > @@ -109,7 +110,7 @@ class LinuxSourceTreeOperationsQemu(LinuxSourceTreeOperations): > 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, base_kunitconfig: kunit_config.Kconfig) -> None: > + def make_arch_config(self, base_kunitconfig: kunit_config.Kconfig) -> None: > kconfig = kunit_config.parse_from_string(self._kconfig) > base_kunitconfig.merge_in_entries(kconfig) > > @@ -137,6 +138,10 @@ class LinuxSourceTreeOperationsUml(LinuxSourceTreeOperations): > def __init__(self, cross_compile=None): > super().__init__(linux_arch='um', cross_compile=cross_compile) > > + def make_arch_config(self, base_kunitconfig: kunit_config.Kconfig) -> None: > + kconfig = kunit_config.parse_file(UML_KCONFIG_PATH) > + base_kunitconfig.merge_in_entries(kconfig) > + > def make_allyesconfig(self, build_dir: str, make_options) -> None: > kunit_parser.print_with_timestamp( > 'Enabling all CONFIGs for UML...') > @@ -313,7 +318,7 @@ class LinuxSourceTree: > return self.build_config(build_dir, make_options) > > existing_kconfig = kunit_config.parse_file(kconfig_path) > - self._ops.make_arch_qemuconfig(self._kconfig) > + self._ops.make_arch_config(self._kconfig) > if self._kconfig.is_subset_of(existing_kconfig) and not self._kunitconfig_changed(build_dir): > return True > print('Regenerating .config ...') > -- > 2.37.0.rc0.104.g0611611a94-goog >
diff --git a/tools/testing/kunit/configs/arch_uml.config b/tools/testing/kunit/configs/arch_uml.config new file mode 100644 index 000000000000..e824ce43b05a --- /dev/null +++ b/tools/testing/kunit/configs/arch_uml.config @@ -0,0 +1,5 @@ +# Config options which are added to UML builds by default + +# Enable virtio/pci, as a lot of tests require it. +CONFIG_VIRTIO_UML=y +CONFIG_UML_PCI_OVER_VIRTIO=y diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py index 3539efaf99ba..05e7b1e188d7 100644 --- a/tools/testing/kunit/kunit_kernel.py +++ b/tools/testing/kunit/kunit_kernel.py @@ -26,6 +26,7 @@ KUNITCONFIG_PATH = '.kunitconfig' OLD_KUNITCONFIG_PATH = 'last_used_kunitconfig' DEFAULT_KUNITCONFIG_PATH = 'tools/testing/kunit/configs/default.config' BROKEN_ALLCONFIG_PATH = 'tools/testing/kunit/configs/broken_on_uml.config' +UML_KCONFIG_PATH = 'tools/testing/kunit/configs/arch_uml.config' OUTFILE_PATH = 'test.log' ABS_TOOL_PATH = os.path.abspath(os.path.dirname(__file__)) QEMU_CONFIGS_DIR = os.path.join(ABS_TOOL_PATH, 'qemu_configs') @@ -53,7 +54,7 @@ class LinuxSourceTreeOperations: except subprocess.CalledProcessError as e: raise ConfigError(e.output.decode()) - def make_arch_qemuconfig(self, base_kunitconfig: kunit_config.Kconfig) -> None: + def make_arch_config(self, base_kunitconfig: kunit_config.Kconfig) -> None: pass def make_allyesconfig(self, build_dir: str, make_options) -> None: @@ -109,7 +110,7 @@ class LinuxSourceTreeOperationsQemu(LinuxSourceTreeOperations): 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, base_kunitconfig: kunit_config.Kconfig) -> None: + def make_arch_config(self, base_kunitconfig: kunit_config.Kconfig) -> None: kconfig = kunit_config.parse_from_string(self._kconfig) base_kunitconfig.merge_in_entries(kconfig) @@ -137,6 +138,10 @@ class LinuxSourceTreeOperationsUml(LinuxSourceTreeOperations): def __init__(self, cross_compile=None): super().__init__(linux_arch='um', cross_compile=cross_compile) + def make_arch_config(self, base_kunitconfig: kunit_config.Kconfig) -> None: + kconfig = kunit_config.parse_file(UML_KCONFIG_PATH) + base_kunitconfig.merge_in_entries(kconfig) + def make_allyesconfig(self, build_dir: str, make_options) -> None: kunit_parser.print_with_timestamp( 'Enabling all CONFIGs for UML...') @@ -313,7 +318,7 @@ class LinuxSourceTree: return self.build_config(build_dir, make_options) existing_kconfig = kunit_config.parse_file(kconfig_path) - self._ops.make_arch_qemuconfig(self._kconfig) + self._ops.make_arch_config(self._kconfig) if self._kconfig.is_subset_of(existing_kconfig) and not self._kunitconfig_changed(build_dir): return True print('Regenerating .config ...')
There are several tests which depend on PCI, and hence need a bunch of extra options to run under UML. This makes it awkward to give configuration instructions (whether in documentation, or as part of a .kunitconfig file), as two separate, incompatible sets of config options are required for UML and "most other architectures". For non-UML architectures, it's possible to add default kconfig options via the qemu_config python files, but there's no equivalent for UML. Add a new tools/testing/kunit/configs/arch_uml.config file containing extra kconfig options to use on UML. Signed-off-by: David Gow <davidgow@google.com> --- It's really ugly to have to type: --kconfig_add CONFIG_VIRTIO_UML=y --kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y when running many tests under UML, particularly since it isn't required on other architectures. This came up in discussion with Daniel this morning, and while the ability to repeat the --kunitconfig flag would go some way to alleviate this, having to add: --kunitconfig ./tools/testing/kunit/config/uml_pci.kunitconfig isn't all that much better. So it seems like adding something by default would be nice. This implementation is not perfect (in particular, there's no easy way of _disabling_ these options now, though [1] probably will help). The 'arch_uml.config' filename can be bikeshedded, too. Thoughts? --- tools/testing/kunit/configs/arch_uml.config | 5 +++++ tools/testing/kunit/kunit_kernel.py | 11 ++++++++--- 2 files changed, 13 insertions(+), 3 deletions(-) create mode 100644 tools/testing/kunit/configs/arch_uml.config