| Message ID | 20210416162824.25131-53-cfontana@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm cleanup experiment for kvm-only build | expand |
On 16/04/2021 18.27, Claudio Fontana wrote: > Skip the test_device_intro_concrete for now for ARM KVM-only build, > as on ARM we currently build devices for ARM that are not > compatible with a KVM-only build. > > We can remove this workaround when we fix this in KConfig etc, > and we only list and build machines that are compatible with KVM > for KVM-only builds. > > Signed-off-by: Claudio Fontana <cfontana@suse.de> > Cc: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > tests/qtest/device-introspect-test.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/tests/qtest/device-introspect-test.c b/tests/qtest/device-introspect-test.c > index bbec166dbc..1ff15e2247 100644 > --- a/tests/qtest/device-introspect-test.c > +++ b/tests/qtest/device-introspect-test.c > @@ -329,12 +329,30 @@ int main(int argc, char **argv) > qtest_add_func("device/introspect/none", test_device_intro_none); > qtest_add_func("device/introspect/abstract", test_device_intro_abstract); > qtest_add_func("device/introspect/abstract-interfaces", test_abstract_interfaces); > + > + /* > + * XXX currently we build also boards for ARM that are incompatible with KVM. > + * We therefore need to check this explicitly, and only test virt for kvm-only > + * arm builds. > + * After we do the work of Kconfig etc to ensure that only KVM-compatible boards > + * are built for the kvm-only build, we could remove this. > + */ > +#ifndef CONFIG_TCG > + { > + const char *arch = qtest_get_arch(); > + if (strcmp(arch, "arm") == 0 || strcmp(arch, "aarch64") == 0) { > + goto add_machine_test_done; > + } > + } > +#endif /* !CONFIG_TCG */ > if (g_test_quick()) { > qtest_add_data_func("device/introspect/concrete/defaults/none", > g_strdup(common_args), test_device_intro_concrete); > } else { > qtest_cb_for_every_machine(add_machine_test_case, true); > } > + goto add_machine_test_done; That last goto does not make much sense, since the label is right below. Thomas > + add_machine_test_done: > return g_test_run(); > } >
On 4/19/21 12:22 PM, Thomas Huth wrote: > On 16/04/2021 18.27, Claudio Fontana wrote: >> Skip the test_device_intro_concrete for now for ARM KVM-only build, >> as on ARM we currently build devices for ARM that are not >> compatible with a KVM-only build. >> >> We can remove this workaround when we fix this in KConfig etc, >> and we only list and build machines that are compatible with KVM >> for KVM-only builds. >> >> Signed-off-by: Claudio Fontana <cfontana@suse.de> >> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org> >> --- >> tests/qtest/device-introspect-test.c | 18 ++++++++++++++++++ >> 1 file changed, 18 insertions(+) >> >> diff --git a/tests/qtest/device-introspect-test.c b/tests/qtest/device-introspect-test.c >> index bbec166dbc..1ff15e2247 100644 >> --- a/tests/qtest/device-introspect-test.c >> +++ b/tests/qtest/device-introspect-test.c >> @@ -329,12 +329,30 @@ int main(int argc, char **argv) >> qtest_add_func("device/introspect/none", test_device_intro_none); >> qtest_add_func("device/introspect/abstract", test_device_intro_abstract); >> qtest_add_func("device/introspect/abstract-interfaces", test_abstract_interfaces); >> + >> + /* >> + * XXX currently we build also boards for ARM that are incompatible with KVM. >> + * We therefore need to check this explicitly, and only test virt for kvm-only >> + * arm builds. >> + * After we do the work of Kconfig etc to ensure that only KVM-compatible boards >> + * are built for the kvm-only build, we could remove this. >> + */ >> +#ifndef CONFIG_TCG >> + { >> + const char *arch = qtest_get_arch(); >> + if (strcmp(arch, "arm") == 0 || strcmp(arch, "aarch64") == 0) { >> + goto add_machine_test_done; >> + } >> + } >> +#endif /* !CONFIG_TCG */ >> if (g_test_quick()) { >> qtest_add_data_func("device/introspect/concrete/defaults/none", >> g_strdup(common_args), test_device_intro_concrete); >> } else { >> qtest_cb_for_every_machine(add_machine_test_case, true); >> } >> + goto add_machine_test_done; > > That last goto does not make much sense, since the label is right below. It is necessary to remove the warning that is otherwise produced about the unused label IIRC. Ciao, Claudio > > Thomas > > >> + add_machine_test_done: >> return g_test_run(); >> } >> >
On 19/04/2021 12.24, Claudio Fontana wrote: > On 4/19/21 12:22 PM, Thomas Huth wrote: >> On 16/04/2021 18.27, Claudio Fontana wrote: >>> Skip the test_device_intro_concrete for now for ARM KVM-only build, >>> as on ARM we currently build devices for ARM that are not >>> compatible with a KVM-only build. >>> >>> We can remove this workaround when we fix this in KConfig etc, >>> and we only list and build machines that are compatible with KVM >>> for KVM-only builds. >>> >>> Signed-off-by: Claudio Fontana <cfontana@suse.de> >>> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org> >>> --- >>> tests/qtest/device-introspect-test.c | 18 ++++++++++++++++++ >>> 1 file changed, 18 insertions(+) >>> >>> diff --git a/tests/qtest/device-introspect-test.c b/tests/qtest/device-introspect-test.c >>> index bbec166dbc..1ff15e2247 100644 >>> --- a/tests/qtest/device-introspect-test.c >>> +++ b/tests/qtest/device-introspect-test.c >>> @@ -329,12 +329,30 @@ int main(int argc, char **argv) >>> qtest_add_func("device/introspect/none", test_device_intro_none); >>> qtest_add_func("device/introspect/abstract", test_device_intro_abstract); >>> qtest_add_func("device/introspect/abstract-interfaces", test_abstract_interfaces); >>> + >>> + /* >>> + * XXX currently we build also boards for ARM that are incompatible with KVM. >>> + * We therefore need to check this explicitly, and only test virt for kvm-only >>> + * arm builds. >>> + * After we do the work of Kconfig etc to ensure that only KVM-compatible boards >>> + * are built for the kvm-only build, we could remove this. >>> + */ >>> +#ifndef CONFIG_TCG >>> + { >>> + const char *arch = qtest_get_arch(); >>> + if (strcmp(arch, "arm") == 0 || strcmp(arch, "aarch64") == 0) { >>> + goto add_machine_test_done; >>> + } >>> + } >>> +#endif /* !CONFIG_TCG */ >>> if (g_test_quick()) { >>> qtest_add_data_func("device/introspect/concrete/defaults/none", >>> g_strdup(common_args), test_device_intro_concrete); >>> } else { >>> qtest_cb_for_every_machine(add_machine_test_case, true); >>> } >>> + goto add_machine_test_done; >> >> That last goto does not make much sense, since the label is right below. > > It is necessary to remove the warning that is otherwise produced about the unused label IIRC. Ah, ok. Alternatively, you could maybe also drop the label completely and simply write the #ifndef block above like this (note the "else"): #ifndef CONFIG_TCG const char *arch = qtest_get_arch(); if (!strcmp(arch, "arm") || !strcmp(arch, "aarch64")) { /* Do nothing */ } else #endif /* !CONFIG_TCG */ ... not sure what's nicer, though. Thomas
On 4/19/21 12:29 PM, Thomas Huth wrote: > On 19/04/2021 12.24, Claudio Fontana wrote: >> On 4/19/21 12:22 PM, Thomas Huth wrote: >>> On 16/04/2021 18.27, Claudio Fontana wrote: >>>> Skip the test_device_intro_concrete for now for ARM KVM-only build, >>>> as on ARM we currently build devices for ARM that are not >>>> compatible with a KVM-only build. >>>> >>>> We can remove this workaround when we fix this in KConfig etc, >>>> and we only list and build machines that are compatible with KVM >>>> for KVM-only builds. >>>> >>>> Signed-off-by: Claudio Fontana <cfontana@suse.de> >>>> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org> >>>> --- >>>> tests/qtest/device-introspect-test.c | 18 ++++++++++++++++++ >>>> 1 file changed, 18 insertions(+) >>>> >>>> diff --git a/tests/qtest/device-introspect-test.c b/tests/qtest/device-introspect-test.c >>>> index bbec166dbc..1ff15e2247 100644 >>>> --- a/tests/qtest/device-introspect-test.c >>>> +++ b/tests/qtest/device-introspect-test.c >>>> @@ -329,12 +329,30 @@ int main(int argc, char **argv) >>>> qtest_add_func("device/introspect/none", test_device_intro_none); >>>> qtest_add_func("device/introspect/abstract", test_device_intro_abstract); >>>> qtest_add_func("device/introspect/abstract-interfaces", test_abstract_interfaces); >>>> + >>>> + /* >>>> + * XXX currently we build also boards for ARM that are incompatible with KVM. >>>> + * We therefore need to check this explicitly, and only test virt for kvm-only >>>> + * arm builds. >>>> + * After we do the work of Kconfig etc to ensure that only KVM-compatible boards >>>> + * are built for the kvm-only build, we could remove this. >>>> + */ >>>> +#ifndef CONFIG_TCG >>>> + { >>>> + const char *arch = qtest_get_arch(); >>>> + if (strcmp(arch, "arm") == 0 || strcmp(arch, "aarch64") == 0) { >>>> + goto add_machine_test_done; >>>> + } >>>> + } >>>> +#endif /* !CONFIG_TCG */ >>>> if (g_test_quick()) { >>>> qtest_add_data_func("device/introspect/concrete/defaults/none", >>>> g_strdup(common_args), test_device_intro_concrete); >>>> } else { >>>> qtest_cb_for_every_machine(add_machine_test_case, true); >>>> } >>>> + goto add_machine_test_done; >>> >>> That last goto does not make much sense, since the label is right below. >> >> It is necessary to remove the warning that is otherwise produced about the unused label IIRC. > > Ah, ok. > > Alternatively, you could maybe also drop the label completely and simply > write the #ifndef block above like this (note the "else"): > > #ifndef CONFIG_TCG > const char *arch = qtest_get_arch(); > if (!strcmp(arch, "arm") || !strcmp(arch, "aarch64")) { > /* Do nothing */ > } > else > #endif /* !CONFIG_TCG */ > > ... not sure what's nicer, though. > > Thomas > Indeed, I tried a couple of combinations, in the end to me the less confusing was the goto one, the #ifdef containing an open else is in my opinion worse, more error-prone, but I am open to additional comments/ideas. Thanks, Claudio
Claudio Fontana <cfontana@suse.de> writes: > On 4/19/21 12:29 PM, Thomas Huth wrote: >> On 19/04/2021 12.24, Claudio Fontana wrote: >>> On 4/19/21 12:22 PM, Thomas Huth wrote: >>>> On 16/04/2021 18.27, Claudio Fontana wrote: >>>>> Skip the test_device_intro_concrete for now for ARM KVM-only build, >>>>> as on ARM we currently build devices for ARM that are not >>>>> compatible with a KVM-only build. >>>>> >>>>> We can remove this workaround when we fix this in KConfig etc, >>>>> and we only list and build machines that are compatible with KVM >>>>> for KVM-only builds. >>>>> >>>>> Signed-off-by: Claudio Fontana <cfontana@suse.de> >>>>> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org> >>>>> --- >>>>> tests/qtest/device-introspect-test.c | 18 ++++++++++++++++++ >>>>> 1 file changed, 18 insertions(+) >>>>> >>>>> diff --git a/tests/qtest/device-introspect-test.c b/tests/qtest/device-introspect-test.c >>>>> index bbec166dbc..1ff15e2247 100644 >>>>> --- a/tests/qtest/device-introspect-test.c >>>>> +++ b/tests/qtest/device-introspect-test.c >>>>> @@ -329,12 +329,30 @@ int main(int argc, char **argv) >>>>> qtest_add_func("device/introspect/none", test_device_intro_none); >>>>> qtest_add_func("device/introspect/abstract", test_device_intro_abstract); >>>>> qtest_add_func("device/introspect/abstract-interfaces", test_abstract_interfaces); >>>>> + >>>>> + /* >>>>> + * XXX currently we build also boards for ARM that are incompatible with KVM. >>>>> + * We therefore need to check this explicitly, and only test virt for kvm-only >>>>> + * arm builds. >>>>> + * After we do the work of Kconfig etc to ensure that only KVM-compatible boards >>>>> + * are built for the kvm-only build, we could remove this. >>>>> + */ >>>>> +#ifndef CONFIG_TCG >>>>> + { >>>>> + const char *arch = qtest_get_arch(); >>>>> + if (strcmp(arch, "arm") == 0 || strcmp(arch, "aarch64") == 0) { >>>>> + goto add_machine_test_done; >>>>> + } >>>>> + } >>>>> +#endif /* !CONFIG_TCG */ >>>>> if (g_test_quick()) { >>>>> qtest_add_data_func("device/introspect/concrete/defaults/none", >>>>> g_strdup(common_args), test_device_intro_concrete); >>>>> } else { >>>>> qtest_cb_for_every_machine(add_machine_test_case, true); >>>>> } >>>>> + goto add_machine_test_done; >>>> >>>> That last goto does not make much sense, since the label is right below. >>> >>> It is necessary to remove the warning that is otherwise produced about the unused label IIRC. >> >> Ah, ok. >> >> Alternatively, you could maybe also drop the label completely and simply >> write the #ifndef block above like this (note the "else"): >> >> #ifndef CONFIG_TCG >> const char *arch = qtest_get_arch(); >> if (!strcmp(arch, "arm") || !strcmp(arch, "aarch64")) { >> /* Do nothing */ >> } >> else >> #endif /* !CONFIG_TCG */ >> >> ... not sure what's nicer, though. >> >> Thomas >> > > Indeed, I tried a couple of combinations, in the end to me the less confusing was the goto one, > the #ifdef containing an open else is in my opinion worse, more > error-prone, but I am open to additional comments/ideas. Surely a helper makes intent clearer? /* * XXX currently we build also boards for ARM that are incompatible with KVM. * We therefore need to check this explicitly, and only test virt for kvm-only * arm builds. * After we do the work of Kconfig etc to ensure that only KVM-compatible boards * are built for the kvm-only build, we could remove this. */ static bool skip_machine_tests(void) { #ifndef CONFIG_TCG const char *arch = qtest_get_arch(); if (strcmp(arch, "arm") == 0 || strcmp(arch, "aarch64") == 0) { return true; } #endif /* !CONFIG_TCG */ return false; } int main(int argc, char **argv) { g_test_init(&argc, &argv, NULL); qtest_add_func("device/introspect/list", test_device_intro_list); qtest_add_func("device/introspect/list-fields", test_qom_list_fields); qtest_add_func("device/introspect/none", test_device_intro_none); qtest_add_func("device/introspect/abstract", test_device_intro_abstract); qtest_add_func("device/introspect/abstract-interfaces", test_abstract_interfaces); if (!skip_machine_tests()) { if (g_test_quick()) { qtest_add_data_func("device/introspect/concrete/defaults/none", g_strdup(common_args), test_device_intro_concrete); } else { qtest_cb_for_every_machine(add_machine_test_case, true); } } return g_test_run(); } > > Thanks, > > Claudio
On 4/20/21 11:34 AM, Alex Bennée wrote: > > Claudio Fontana <cfontana@suse.de> writes: > >> On 4/19/21 12:29 PM, Thomas Huth wrote: >>> On 19/04/2021 12.24, Claudio Fontana wrote: >>>> On 4/19/21 12:22 PM, Thomas Huth wrote: >>>>> On 16/04/2021 18.27, Claudio Fontana wrote: >>>>>> Skip the test_device_intro_concrete for now for ARM KVM-only build, >>>>>> as on ARM we currently build devices for ARM that are not >>>>>> compatible with a KVM-only build. >>>>>> >>>>>> We can remove this workaround when we fix this in KConfig etc, >>>>>> and we only list and build machines that are compatible with KVM >>>>>> for KVM-only builds. >>>>>> >>>>>> Signed-off-by: Claudio Fontana <cfontana@suse.de> >>>>>> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org> >>>>>> --- >>>>>> tests/qtest/device-introspect-test.c | 18 ++++++++++++++++++ >>>>>> 1 file changed, 18 insertions(+) >>>>>> >>>>>> diff --git a/tests/qtest/device-introspect-test.c b/tests/qtest/device-introspect-test.c >>>>>> index bbec166dbc..1ff15e2247 100644 >>>>>> --- a/tests/qtest/device-introspect-test.c >>>>>> +++ b/tests/qtest/device-introspect-test.c >>>>>> @@ -329,12 +329,30 @@ int main(int argc, char **argv) >>>>>> qtest_add_func("device/introspect/none", test_device_intro_none); >>>>>> qtest_add_func("device/introspect/abstract", test_device_intro_abstract); >>>>>> qtest_add_func("device/introspect/abstract-interfaces", test_abstract_interfaces); >>>>>> + >>>>>> + /* >>>>>> + * XXX currently we build also boards for ARM that are incompatible with KVM. >>>>>> + * We therefore need to check this explicitly, and only test virt for kvm-only >>>>>> + * arm builds. >>>>>> + * After we do the work of Kconfig etc to ensure that only KVM-compatible boards >>>>>> + * are built for the kvm-only build, we could remove this. >>>>>> + */ >>>>>> +#ifndef CONFIG_TCG >>>>>> + { >>>>>> + const char *arch = qtest_get_arch(); >>>>>> + if (strcmp(arch, "arm") == 0 || strcmp(arch, "aarch64") == 0) { >>>>>> + goto add_machine_test_done; >>>>>> + } >>>>>> + } >>>>>> +#endif /* !CONFIG_TCG */ >>>>>> if (g_test_quick()) { >>>>>> qtest_add_data_func("device/introspect/concrete/defaults/none", >>>>>> g_strdup(common_args), test_device_intro_concrete); >>>>>> } else { >>>>>> qtest_cb_for_every_machine(add_machine_test_case, true); >>>>>> } >>>>>> + goto add_machine_test_done; >>>>> >>>>> That last goto does not make much sense, since the label is right below. >>>> >>>> It is necessary to remove the warning that is otherwise produced about the unused label IIRC. >>> >>> Ah, ok. >>> >>> Alternatively, you could maybe also drop the label completely and simply >>> write the #ifndef block above like this (note the "else"): >>> >>> #ifndef CONFIG_TCG >>> const char *arch = qtest_get_arch(); >>> if (!strcmp(arch, "arm") || !strcmp(arch, "aarch64")) { >>> /* Do nothing */ >>> } >>> else >>> #endif /* !CONFIG_TCG */ >>> >>> ... not sure what's nicer, though. >>> >>> Thomas >>> >> >> Indeed, I tried a couple of combinations, in the end to me the less confusing was the goto one, >> the #ifdef containing an open else is in my opinion worse, more >> error-prone, but I am open to additional comments/ideas. > > Surely a helper makes intent clearer? Yes, thank you :-) Ciao, Claudio > > /* > * XXX currently we build also boards for ARM that are incompatible with KVM. > * We therefore need to check this explicitly, and only test virt for kvm-only > * arm builds. > * After we do the work of Kconfig etc to ensure that only KVM-compatible boards > * are built for the kvm-only build, we could remove this. > */ > static bool skip_machine_tests(void) > { > #ifndef CONFIG_TCG > const char *arch = qtest_get_arch(); > if (strcmp(arch, "arm") == 0 || strcmp(arch, "aarch64") == 0) { > return true; > } > #endif /* !CONFIG_TCG */ > return false; > } > > > int main(int argc, char **argv) > { > g_test_init(&argc, &argv, NULL); > > qtest_add_func("device/introspect/list", test_device_intro_list); > qtest_add_func("device/introspect/list-fields", test_qom_list_fields); > qtest_add_func("device/introspect/none", test_device_intro_none); > qtest_add_func("device/introspect/abstract", test_device_intro_abstract); > qtest_add_func("device/introspect/abstract-interfaces", test_abstract_interfaces); > > if (!skip_machine_tests()) { > if (g_test_quick()) { > qtest_add_data_func("device/introspect/concrete/defaults/none", > g_strdup(common_args), test_device_intro_concrete); > } else { > qtest_cb_for_every_machine(add_machine_test_case, true); > } > } > > return g_test_run(); > } > > >> >> Thanks, >> >> Claudio > >
diff --git a/tests/qtest/device-introspect-test.c b/tests/qtest/device-introspect-test.c index bbec166dbc..1ff15e2247 100644 --- a/tests/qtest/device-introspect-test.c +++ b/tests/qtest/device-introspect-test.c @@ -329,12 +329,30 @@ int main(int argc, char **argv) qtest_add_func("device/introspect/none", test_device_intro_none); qtest_add_func("device/introspect/abstract", test_device_intro_abstract); qtest_add_func("device/introspect/abstract-interfaces", test_abstract_interfaces); + + /* + * XXX currently we build also boards for ARM that are incompatible with KVM. + * We therefore need to check this explicitly, and only test virt for kvm-only + * arm builds. + * After we do the work of Kconfig etc to ensure that only KVM-compatible boards + * are built for the kvm-only build, we could remove this. + */ +#ifndef CONFIG_TCG + { + const char *arch = qtest_get_arch(); + if (strcmp(arch, "arm") == 0 || strcmp(arch, "aarch64") == 0) { + goto add_machine_test_done; + } + } +#endif /* !CONFIG_TCG */ if (g_test_quick()) { qtest_add_data_func("device/introspect/concrete/defaults/none", g_strdup(common_args), test_device_intro_concrete); } else { qtest_cb_for_every_machine(add_machine_test_case, true); } + goto add_machine_test_done; + add_machine_test_done: return g_test_run(); }
Skip the test_device_intro_concrete for now for ARM KVM-only build, as on ARM we currently build devices for ARM that are not compatible with a KVM-only build. We can remove this workaround when we fix this in KConfig etc, and we only list and build machines that are compatible with KVM for KVM-only builds. Signed-off-by: Claudio Fontana <cfontana@suse.de> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org> --- tests/qtest/device-introspect-test.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)