| Message ID | 20190322095818.19914-3-etienne.carriere@linaro.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v3,1/4] boot/arm-trusted-firmware: support alternate image files | expand |
Hello, + Arnout On Fri, Mar 22, 2019 at 06:58 AM, Etienne Carriere wrote: > This change adds argument --local-emulator to run-tests to use a > locally built emulator for the runtime test. Target test shall provide > a config_emulator attribute unless what test is inconsistent. > > Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org> > --- > Changes v2 -> v3: > - Add an argument to run-tests to use locally built emulator. > Test can still enforce use of locally built emulator from > boot method argument local=True|False (still defaults to False). I am not strongly against it. But let me ask: Are you or someone else having trouble with any other test case when using "old" qemu versions from host? If the answer is no, maybe we should keep things simple and use a patch very similar to v2 of this one, just adding the host-qemu to the test case config fragment and the 'local' argument to Emulator.run. I say similar to v2 because I think your implementation in v3 with the 'qemu_bin' variable is better than v2. If the answer is yes, be aware that one could download the docker image that we use in GitLab CI and run the tests inside it. This image has qemu 2.8.1 that currently run all tests we have without any problem AFAIK. IMO, we remove --local-emulator and add local=True and host-qemu to the config fragment of any tests (currently only the one in the next patch) that needs a qemu newer than 2.8.1. Another approach "to have an generic option to script run-tests to build the emulator within the test config" (quoting your reply to v2) would be: Instead of adding a command line argument to run-tests, create a new flag in the test case class, perhaps named "build_emulator". If that flag is True, the host-qemu is automatically added to the config fragment by the test infra. The same flag could be propagated to the Emulator.init so we don't need to pass 'local' to Emulator.run. If for some reason a flag (boolean) is not enough, for example if the host-qemu would not always be set with same options (I don't know if that is the case), the test case that needs the local build of emulator could pass config_emulator and that would make the test infra to know that it needs to build and use the locally built emulator. Again, no need to pass 'local' to Emulator.run or the user to pass a command line argument. This way the test case tells the test infra that it needs an emulator built locally instead of the user that starts the test. Thoughts? [snip] > @@ -30,13 +35,21 @@ class Emulator(object): > # > # options: array of command line options to pass to Qemu > # > - def boot(self, arch, kernel=None, kernel_cmdline=None, options=None): > + # local: if True, the locally built qemu host tool is used instead of a > + # qemu host tool found from the PATH. > + # > + def boot(self, arch, kernel=None, kernel_cmdline=None, options=None, > + local=None): A better default value is local=False > if arch in ["armv7", "armv5"]: > qemu_arch = "arm" > else: > qemu_arch = arch > > - qemu_cmd = ["qemu-system-{}".format(qemu_arch), > + qemu_bin = "qemu-system-{}".format(qemu_arch) > + if self.local_emulator or local: > + qemu_bin = os.path.join(self.builddir, 'host', 'bin', qemu_bin) > + > + qemu_cmd = [qemu_bin, > "-serial", "stdio", > "-display", "none"] Regards, Ricardo
Hello Ricardo and Etienne, On Sat, 30 Mar 2019 00:30:01 -0300 Ricardo Martincoski <ricardo.martincoski@gmail.com> wrote: > I am not strongly against it. > But let me ask: > Are you or someone else having trouble with any other test case when using > "old" qemu versions from host? > > If the answer is no, maybe we should keep things simple and use a patch very > similar to v2 of this one, just adding the host-qemu to the test case config > fragment and the 'local' argument to Emulator.run. > I say similar to v2 because I think your implementation in v3 with the > 'qemu_bin' variable is better than v2. I totally agree here. Having this option --local-emulator doesn't make much sense. We would have to use it for some tests, but not for some other tests. Instead, we want each test to know whether it needs a recent enough version of Qemu, or whether the system-provided Qemu version is good enough. > Another approach "to have an generic option to script run-tests to build the > emulator within the test config" (quoting your reply to v2) would be: > Instead of adding a command line argument to run-tests, create a new flag in > the test case class, perhaps named "build_emulator". If that flag is True, the > host-qemu is automatically added to the config fragment by the test infra. The > same flag could be propagated to the Emulator.init so we don't need to pass > 'local' to Emulator.run. Yes, something like that sounds good. Best regards, Thomas
Hello Ricardo, Hello Thomas, Thanks for your feedback and sorry for this late answers. On Tue, 2 Apr 2019 at 09:06, Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote: > > Hello Ricardo and Etienne, > > On Sat, 30 Mar 2019 00:30:01 -0300 > Ricardo Martincoski <ricardo.martincoski@gmail.com> wrote: > > > I am not strongly against it. > > But let me ask: > > Are you or someone else having trouble with any other test case when using > > "old" qemu versions from host? > > > > If the answer is no, maybe we should keep things simple and use a patch very > > similar to v2 of this one, just adding the host-qemu to the test case config > > fragment and the 'local' argument to Emulator.run. > > I say similar to v2 because I think your implementation in v3 with the > > 'qemu_bin' variable is better than v2. Up to my knowledge, when running qemu-system-arm without secure world emulation, old Qemu versions are fine (I.e. v2.x). But when emulating arm w/ trustzone (qemu-system-arm -machine secure=on ...), one must use at least Qemu v2.10, and recommended version is v3.1. - v2.10 is necessary, because of commit b29fd33db578 ("target/arm: use DISAS_EXIT for eret handling") that is mandated. - v3.1 is recommended since commit fb23d693a3e0 ("hw/arm/virt: add DT property /secure-chosen/stdout-path indicating secure UART"). Without this fix, secure/non-secure world consoles are mixed resulting in weird traces (unless one fully disables secure world runtime traces). > > I totally agree here. Having this option --local-emulator doesn't make > much sense. We would have to use it for some tests, but not for some > other tests. > > Instead, we want each test to know whether it needs a recent enough > version of Qemu, or whether the system-provided Qemu version is good > enough. Fair enough. > > > Another approach "to have an generic option to script run-tests to build the > > emulator within the test config" (quoting your reply to v2) would be: > > Instead of adding a command line argument to run-tests, create a new flag in > > the test case class, perhaps named "build_emulator". If that flag is True, the > > host-qemu is automatically added to the config fragment by the test infra. The > > same flag could be propagated to the Emulator.init so we don't need to pass > > 'local' to Emulator.run. Ok, I think I see your idea. I'll come up with a v4 for this whole patch series. Thanks again etienne > > Yes, something like that sounds good. > > Best regards, > > Thomas > -- > Thomas Petazzoni, CTO, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
diff --git a/support/testing/infra/basetest.py b/support/testing/infra/basetest.py index a176bc328a..c59e0a3443 100644 --- a/support/testing/infra/basetest.py +++ b/support/testing/infra/basetest.py @@ -51,6 +51,8 @@ class BRConfigTest(unittest.TestCase): def setUp(self): self.show_msg("Starting") + if self.local_emulator: + self.config = self.config + self.config_emulator self.b = Builder(self.config, self.builddir, self.logtofile) if not self.keepbuilds: @@ -78,7 +80,8 @@ class BRTest(BRConfigTest): self.show_msg("Building done") self.emulator = Emulator(self.builddir, self.downloaddir, - self.logtofile, self.timeout_multiplier) + self.logtofile, self.timeout_multiplier, + self.local_emulator) def tearDown(self): if self.emulator: diff --git a/support/testing/infra/emulator.py b/support/testing/infra/emulator.py index 802e89d4b4..13e19df497 100644 --- a/support/testing/infra/emulator.py +++ b/support/testing/infra/emulator.py @@ -1,14 +1,19 @@ import pexpect +import os + import infra class Emulator(object): - def __init__(self, builddir, downloaddir, logtofile, timeout_multiplier): + def __init__(self, builddir, downloaddir, logtofile, timeout_multiplier, + local_emulator): self.qemu = None self.downloaddir = downloaddir self.logfile = infra.open_log_file(builddir, "run", logtofile) + self.builddir = builddir + self.local_emulator = local_emulator # We use elastic runners on the cloud to runs our tests. Those runners # can take a long time to run the emulator. Use a timeout multiplier # when running the tests to avoid sporadic failures. @@ -30,13 +35,21 @@ class Emulator(object): # # options: array of command line options to pass to Qemu # - def boot(self, arch, kernel=None, kernel_cmdline=None, options=None): + # local: if True, the locally built qemu host tool is used instead of a + # qemu host tool found from the PATH. + # + def boot(self, arch, kernel=None, kernel_cmdline=None, options=None, + local=None): if arch in ["armv7", "armv5"]: qemu_arch = "arm" else: qemu_arch = arch - qemu_cmd = ["qemu-system-{}".format(qemu_arch), + qemu_bin = "qemu-system-{}".format(qemu_arch) + if self.local_emulator or local: + qemu_bin = os.path.join(self.builddir, 'host', 'bin', qemu_bin) + + qemu_cmd = [qemu_bin, "-serial", "stdio", "-display", "none"] diff --git a/support/testing/run-tests b/support/testing/run-tests index 813b927045..c667fc2e4f 100755 --- a/support/testing/run-tests +++ b/support/testing/run-tests @@ -31,6 +31,8 @@ def main(): help='BR2_JLEVEL to use for each testcase') parser.add_argument('--timeout-multiplier', type=int, default=1, help='increase timeouts (useful for slow machines)') + parser.add_argument('--local-emulator', action='store_true', + help='use a locally built emulator') args = parser.parse_args() @@ -107,6 +109,8 @@ def main(): return 1 BRConfigTest.timeout_multiplier = args.timeout_multiplier + BRConfigTest.local_emulator = args.local_emulator + nose2_args = ["-v", "-N", str(args.testcases), "-s", test_dir,
This change adds argument --local-emulator to run-tests to use a locally built emulator for the runtime test. Target test shall provide a config_emulator attribute unless what test is inconsistent. Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org> --- Changes v2 -> v3: - Add an argument to run-tests to use locally built emulator. Test can still enforce use of locally built emulator from boot method argument local=True|False (still defaults to False). Changes v1 -> v2: - No commit not in v1 series. --- support/testing/infra/basetest.py | 5 ++++- support/testing/infra/emulator.py | 19 ++++++++++++++++--- support/testing/run-tests | 4 ++++ 3 files changed, 24 insertions(+), 4 deletions(-)