| Message ID | 20230120184825.31626-8-farosas@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/arm: Allow CONFIG_TCG=n builds | expand |
On 20/1/23 19:48, Fabiano Rosas wrote: > The migration tests are currently broken for an aarch64 host because > the tests pass no 'machine' and 'cpu' options on the QEMU command > line. Most other architectures define a default value in QEMU for > these options, but arm does not. There was some discussions around that in the past: https://lore.kernel.org/qemu-devel/20190621153806.13489-1-wainersm@redhat.com/ https://lore.kernel.org/qemu-devel/CAFEAcA9NBu+L4wHfkLTv93wy90wjnV05EZ12PT6PmLjdZ5h_YA@mail.gmail.com/ > Add these options to the test class in case the test is being executed > in an aarch64 host. I'm not sure what we are aiming to test here. Migration in general? If so, any random machine should work. By hardcoding the 'virt' machine, at least this test is reproducible. I'd rather fix that generically as "if a test requires a default machine and the target doesn't provide any default, then SKIP the test". Then adding machine-specific tests. Can be done on top, so Acked-by: Philippe Mathieu-Daudé <philmd@linaro.org> > Signed-off-by: Fabiano Rosas <farosas@suse.de> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > --- > tests/avocado/migration.py | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/tests/avocado/migration.py b/tests/avocado/migration.py > index 4b25680c50..ffd3db0f35 100644 > --- a/tests/avocado/migration.py > +++ b/tests/avocado/migration.py > @@ -11,6 +11,8 @@ > > > import tempfile > +import os > + > from avocado_qemu import QemuSystemTest > from avocado import skipUnless > > @@ -26,6 +28,14 @@ class Migration(QemuSystemTest): > > timeout = 10 > > + def setUp(self): > + super().setUp() > + > + arch = os.uname()[4] > + if arch == 'aarch64': > + self.machine = 'virt' > + self.cpu = 'max' > + > @staticmethod > def migration_finished(vm): > return vm.command('query-migrate')['status'] in ('completed', 'failed')
Philippe Mathieu-Daudé <philmd@linaro.org> writes: > On 20/1/23 19:48, Fabiano Rosas wrote: >> The migration tests are currently broken for an aarch64 host because >> the tests pass no 'machine' and 'cpu' options on the QEMU command >> line. Most other architectures define a default value in QEMU for >> these options, but arm does not. > > There was some discussions around that in the past: > https://lore.kernel.org/qemu-devel/20190621153806.13489-1-wainersm@redhat.com/ > https://lore.kernel.org/qemu-devel/CAFEAcA9NBu+L4wHfkLTv93wy90wjnV05EZ12PT6PmLjdZ5h_YA@mail.gmail.com/ There's more than one topic being discussed, specially in this last thread, but here's my two cents. About defaults: It's probably best to be explicit in tests. And if we wanted, have a separate test to make sure the lack of an option still does what it's expected, either outputting a message or behaving the same as the explicit version. About host architecture-specific tests: Unless we're talking about KVM, I see no point. Having to change hosts to test agnostic features makes no sense (the migration test is one example). About generic tests: If a feature is required to behave the same for all architectures/machines/cpus then sure. But most low level stuff would be quite dependent on specifics. >> Add these options to the test class in case the test is being executed >> in an aarch64 host. > > I'm not sure what we are aiming to test here. > > Migration in general? If so, any random machine should work. > By hardcoding the 'virt' machine, at least this test is reproducible. Yeah, I cannot say for sure there isn't some machine property that gets transferred during migration. It seemed more conservative to define a specific one. > I'd rather fix that generically as "if a test requires a default > machine and the target doesn't provide any default, then SKIP the > test". Then adding machine-specific tests. Can be done on top, so I agree, but the only tests that should *require* a default are the ones that test the command line parsing or adjacent features. We could always test "-machine foo" and then separately test that the lack of a machine option still gives the Foo machine. The fact that we sometimes use defaults to be able to have the same-ish command line for every case is more of a limitation of our testing infrastructure in my opinion.
On 23/1/23 15:37, Fabiano Rosas wrote: > Philippe Mathieu-Daudé <philmd@linaro.org> writes: > >> On 20/1/23 19:48, Fabiano Rosas wrote: >>> The migration tests are currently broken for an aarch64 host because >>> the tests pass no 'machine' and 'cpu' options on the QEMU command >>> line. Most other architectures define a default value in QEMU for >>> these options, but arm does not. >> >> There was some discussions around that in the past: >> https://lore.kernel.org/qemu-devel/20190621153806.13489-1-wainersm@redhat.com/ >> https://lore.kernel.org/qemu-devel/CAFEAcA9NBu+L4wHfkLTv93wy90wjnV05EZ12PT6PmLjdZ5h_YA@mail.gmail.com/ > > There's more than one topic being discussed, specially in this last > thread, but here's my two cents. > > About defaults: It's probably best to be explicit in tests. And if we > wanted, have a separate test to make sure the lack of an option still > does what it's expected, either outputting a message or behaving the > same as the explicit version. > > About host architecture-specific tests: Unless we're talking about KVM, > I see no point. Having to change hosts to test agnostic features makes > no sense (the migration test is one example). > > About generic tests: If a feature is required to behave the same for all > architectures/machines/cpus then sure. But most low level stuff would be > quite dependent on specifics. > >>> Add these options to the test class in case the test is being executed >>> in an aarch64 host. >> >> I'm not sure what we are aiming to test here. >> >> Migration in general? If so, any random machine should work. >> By hardcoding the 'virt' machine, at least this test is reproducible. > > Yeah, I cannot say for sure there isn't some machine property that gets > transferred during migration. It seemed more conservative to define a > specific one. Why did you choose 'virt' and not 'xlnx-versal-virt' or 'sbsa-ref'? What does this test require? Any machine running KVM? Adding Juan and David for migration since I'm still confused trying to understand what we are trying to test here... >> I'd rather fix that generically as "if a test requires a default >> machine and the target doesn't provide any default, then SKIP the >> test". Then adding machine-specific tests. Can be done on top, so > > I agree, but the only tests that should *require* a default are the ones > that test the command line parsing or adjacent features. We could always > test "-machine foo" and then separately test that the lack of a machine > option still gives the Foo machine. > > The fact that we sometimes use defaults to be able to have the same-ish > command line for every case is more of a limitation of our testing > infrastructure in my opinion.
Philippe Mathieu-Daudé <philmd@linaro.org> writes: > On 23/1/23 15:37, Fabiano Rosas wrote: >> Philippe Mathieu-Daudé <philmd@linaro.org> writes: >> >>> On 20/1/23 19:48, Fabiano Rosas wrote: >>>> The migration tests are currently broken for an aarch64 host because >>>> the tests pass no 'machine' and 'cpu' options on the QEMU command >>>> line. Most other architectures define a default value in QEMU for >>>> these options, but arm does not. >>> >>> There was some discussions around that in the past: >>> https://lore.kernel.org/qemu-devel/20190621153806.13489-1-wainersm@redhat.com/ >>> https://lore.kernel.org/qemu-devel/CAFEAcA9NBu+L4wHfkLTv93wy90wjnV05EZ12PT6PmLjdZ5h_YA@mail.gmail.com/ >> >> There's more than one topic being discussed, specially in this last >> thread, but here's my two cents. >> >> About defaults: It's probably best to be explicit in tests. And if we >> wanted, have a separate test to make sure the lack of an option still >> does what it's expected, either outputting a message or behaving the >> same as the explicit version. >> >> About host architecture-specific tests: Unless we're talking about KVM, >> I see no point. Having to change hosts to test agnostic features makes >> no sense (the migration test is one example). >> >> About generic tests: If a feature is required to behave the same for all >> architectures/machines/cpus then sure. But most low level stuff would be >> quite dependent on specifics. >> >>>> Add these options to the test class in case the test is being executed >>>> in an aarch64 host. >>> >>> I'm not sure what we are aiming to test here. >>> >>> Migration in general? If so, any random machine should work. >>> By hardcoding the 'virt' machine, at least this test is reproducible. >> >> Yeah, I cannot say for sure there isn't some machine property that gets >> transferred during migration. It seemed more conservative to define a >> specific one. > > Why did you choose 'virt' and not 'xlnx-versal-virt' or 'sbsa-ref'? It is the only one guaranteed to be present with both TCG and KVM.
Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > On 23/1/23 15:37, Fabiano Rosas wrote: >> Philippe Mathieu-Daudé <philmd@linaro.org> writes: >> >>> On 20/1/23 19:48, Fabiano Rosas wrote: >>>> The migration tests are currently broken for an aarch64 host because >>>> the tests pass no 'machine' and 'cpu' options on the QEMU command >>>> line. Most other architectures define a default value in QEMU for >>>> these options, but arm does not. >>> >>> There was some discussions around that in the past: >>> https://lore.kernel.org/qemu-devel/20190621153806.13489-1-wainersm@redhat.com/ >>> https://lore.kernel.org/qemu-devel/CAFEAcA9NBu+L4wHfkLTv93wy90wjnV05EZ12PT6PmLjdZ5h_YA@mail.gmail.com/ >> There's more than one topic being discussed, specially in this last >> thread, but here's my two cents. >> About defaults: It's probably best to be explicit in tests. And if >> we >> wanted, have a separate test to make sure the lack of an option still >> does what it's expected, either outputting a message or behaving the >> same as the explicit version. >> About host architecture-specific tests: Unless we're talking about >> KVM, >> I see no point. Having to change hosts to test agnostic features makes >> no sense (the migration test is one example). >> About generic tests: If a feature is required to behave the same for >> all >> architectures/machines/cpus then sure. But most low level stuff would be >> quite dependent on specifics. >> >>>> Add these options to the test class in case the test is being executed >>>> in an aarch64 host. >>> >>> I'm not sure what we are aiming to test here. >>> >>> Migration in general? If so, any random machine should work. >>> By hardcoding the 'virt' machine, at least this test is reproducible. >> Yeah, I cannot say for sure there isn't some machine property that >> gets >> transferred during migration. It seemed more conservative to define a >> specific one. > > Why did you choose 'virt' and not 'xlnx-versal-virt' or 'sbsa-ref'? > > What does this test require? Any machine running KVM? > > Adding Juan and David for migration since I'm still confused trying > to understand what we are trying to test here... No clue really from my side either. ARM machine types are a mystery. But on one hand: - arm is a sane architecture when there is no default machine learn x86, learn. - I don't know either which one to use. Later, Juan. >>> I'd rather fix that generically as "if a test requires a default >>> machine and the target doesn't provide any default, then SKIP the >>> test". Then adding machine-specific tests. Can be done on top, so >> I agree, but the only tests that should *require* a default are the >> ones >> that test the command line parsing or adjacent features. We could always >> test "-machine foo" and then separately test that the lack of a machine >> option still gives the Foo machine. >> The fact that we sometimes use defaults to be able to have the >> same-ish >> command line for every case is more of a limitation of our testing >> infrastructure in my opinion.
diff --git a/tests/avocado/migration.py b/tests/avocado/migration.py index 4b25680c50..ffd3db0f35 100644 --- a/tests/avocado/migration.py +++ b/tests/avocado/migration.py @@ -11,6 +11,8 @@ import tempfile +import os + from avocado_qemu import QemuSystemTest from avocado import skipUnless @@ -26,6 +28,14 @@ class Migration(QemuSystemTest): timeout = 10 + def setUp(self): + super().setUp() + + arch = os.uname()[4] + if arch == 'aarch64': + self.machine = 'virt' + self.cpu = 'max' + @staticmethod def migration_finished(vm): return vm.command('query-migrate')['status'] in ('completed', 'failed')