| Message ID | 20191007151905.32766-8-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fw_cfg: Run tests on big-endian | expand |
On 10/07/19 17:19, Philippe Mathieu-Daudé wrote: > We have been restricting our fw_cfg tests to the PC machine, > which is a little-endian architecture. > The fw_cfg device is also used on the SPARC and PowerPC > architectures, which can run in big-endian configuration. > > Since we want to be sure our device does not regress > regardless the endianess used, enable this test one > these targets. > > The NUMA selector is X86 specific, restrict it to this arch. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > v2: test ppc32 too (lvivier) > --- > tests/Makefile.include | 2 ++ > tests/fw_cfg-test.c | 33 +++++++++++++++++++++++++++------ > 2 files changed, 29 insertions(+), 6 deletions(-) > > diff --git a/tests/Makefile.include b/tests/Makefile.include > index 3543451ed3..4ae3d5140a 100644 > --- a/tests/Makefile.include > +++ b/tests/Makefile.include > @@ -226,6 +226,7 @@ check-qtest-ppc-y += tests/prom-env-test$(EXESUF) > check-qtest-ppc-y += tests/drive_del-test$(EXESUF) > check-qtest-ppc-y += tests/boot-serial-test$(EXESUF) > check-qtest-ppc-$(CONFIG_M48T59) += tests/m48t59-test$(EXESUF) > +check-qtest-ppc-y += tests/fw_cfg-test$(EXESUF) > > check-qtest-ppc64-y += $(check-qtest-ppc-y) > check-qtest-ppc64-$(CONFIG_PSERIES) += tests/device-plug-test$(EXESUF) > @@ -250,6 +251,7 @@ check-qtest-sh4eb-$(CONFIG_ISA_TESTDEV) = tests/endianness-test$(EXESUF) > check-qtest-sparc-y += tests/prom-env-test$(EXESUF) > check-qtest-sparc-y += tests/m48t59-test$(EXESUF) > check-qtest-sparc-y += tests/boot-serial-test$(EXESUF) > +check-qtest-sparc-y += tests/fw_cfg-test$(EXESUF) > > check-qtest-sparc64-$(CONFIG_ISA_TESTDEV) = tests/endianness-test$(EXESUF) > check-qtest-sparc64-y += tests/prom-env-test$(EXESUF) > diff --git a/tests/fw_cfg-test.c b/tests/fw_cfg-test.c > index 35af0de7e6..1250e87097 100644 > --- a/tests/fw_cfg-test.c > +++ b/tests/fw_cfg-test.c > @@ -210,13 +210,30 @@ static void test_fw_cfg_splash_time(const void *opaque) > > int main(int argc, char **argv) > { > - QTestCtx ctx; > - int ret; > + const char *arch = qtest_get_arch(); > + bool has_numa = false; > + QTestCtx ctx = {}; > + int ret = 0; > > g_test_init(&argc, &argv, NULL); > > - ctx.machine_name = "pc"; > - ctx.fw_cfg = pc_fw_cfg_init(); > + if (g_str_equal(arch, "i386") || g_str_equal(arch, "x86_64")) { > + has_numa = true; > + ctx.machine_name = "pc"; > + ctx.fw_cfg = pc_fw_cfg_init(); > + } else if (g_str_equal(arch, "sparc")) { > + ctx.machine_name = "SS-5"; > + ctx.fw_cfg = mm_fw_cfg_init(0xd00000510ULL); > + } else if (g_str_equal(arch, "ppc") || g_str_equal(arch, "ppc64")) { > + /* > + * The mac99 machine is different for 32/64-bit target: > + * > + * ppc(32): the G4 which can be either little or big endian, > + * ppc64: the G5 (970FX) is only big-endian. > + */ > + ctx.machine_name = "mac99"; > + ctx.fw_cfg = mm_fw_cfg_init(0xf0000510); > + } > > qtest_add_data_func("fw_cfg/signature", &ctx, test_fw_cfg_signature); > qtest_add_data_func("fw_cfg/id", &ctx, test_fw_cfg_id); > @@ -231,14 +248,18 @@ int main(int argc, char **argv) > qtest_add_func("fw_cfg/boot_device", test_fw_cfg_boot_device); > #endif > qtest_add_data_func("fw_cfg/max_cpus", &ctx, test_fw_cfg_max_cpus); > - qtest_add_data_func("fw_cfg/numa", &ctx, test_fw_cfg_numa); > qtest_add_data_func("fw_cfg/boot_menu", &ctx, test_fw_cfg_boot_menu); > qtest_add_data_func("fw_cfg/reboot_timeout", &ctx, > test_fw_cfg_reboot_timeout); > qtest_add_data_func("fw_cfg/splash_time", &ctx, test_fw_cfg_splash_time); > > - ret = g_test_run(); > + if (has_numa) { > + qtest_add_data_func("fw_cfg/numa", &ctx, test_fw_cfg_numa); > + } > > + if (ctx.machine_name) { > + ret = g_test_run(); > + } > g_free(ctx.fw_cfg); > > return ret; > Too many foreign arch bits in this patch for me to give an R-b, but structurally it looks OK to me. Acked-by: Laszlo Ersek <lersek@redhat.com> Thanks Laszlo
Philippe Mathieu-Daudé <philmd@redhat.com> 于2019年10月7日周一 下午11:20写道： > We have been restricting our fw_cfg tests to the PC machine, > which is a little-endian architecture. > The fw_cfg device is also used on the SPARC and PowerPC > architectures, which can run in big-endian configuration. > > Since we want to be sure our device does not regress > regardless the endianess used, enable this test one > these targets. > > The NUMA selector is X86 specific, restrict it to this arch. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > v2: test ppc32 too (lvivier) > --- > tests/Makefile.include | 2 ++ > tests/fw_cfg-test.c | 33 +++++++++++++++++++++++++++------ > 2 files changed, 29 insertions(+), 6 deletions(-) > > diff --git a/tests/Makefile.include b/tests/Makefile.include > index 3543451ed3..4ae3d5140a 100644 > --- a/tests/Makefile.include > +++ b/tests/Makefile.include > @@ -226,6 +226,7 @@ check-qtest-ppc-y += tests/prom-env-test$(EXESUF) > check-qtest-ppc-y += tests/drive_del-test$(EXESUF) > check-qtest-ppc-y += tests/boot-serial-test$(EXESUF) > check-qtest-ppc-$(CONFIG_M48T59) += tests/m48t59-test$(EXESUF) > +check-qtest-ppc-y += tests/fw_cfg-test$(EXESUF) > > check-qtest-ppc64-y += $(check-qtest-ppc-y) > check-qtest-ppc64-$(CONFIG_PSERIES) += tests/device-plug-test$(EXESUF) > @@ -250,6 +251,7 @@ check-qtest-sh4eb-$(CONFIG_ISA_TESTDEV) = > tests/endianness-test$(EXESUF) > check-qtest-sparc-y += tests/prom-env-test$(EXESUF) > check-qtest-sparc-y += tests/m48t59-test$(EXESUF) > check-qtest-sparc-y += tests/boot-serial-test$(EXESUF) > +check-qtest-sparc-y += tests/fw_cfg-test$(EXESUF) > > check-qtest-sparc64-$(CONFIG_ISA_TESTDEV) = tests/endianness-test$(EXESUF) > check-qtest-sparc64-y += tests/prom-env-test$(EXESUF) > diff --git a/tests/fw_cfg-test.c b/tests/fw_cfg-test.c > index 35af0de7e6..1250e87097 100644 > --- a/tests/fw_cfg-test.c > +++ b/tests/fw_cfg-test.c > @@ -210,13 +210,30 @@ static void test_fw_cfg_splash_time(const void > *opaque) > > int main(int argc, char **argv) > { > - QTestCtx ctx; > - int ret; > + const char *arch = qtest_get_arch(); > + bool has_numa = false; > + QTestCtx ctx = {}; > + int ret = 0; > > g_test_init(&argc, &argv, NULL); > > - ctx.machine_name = "pc"; > - ctx.fw_cfg = pc_fw_cfg_init(); > + if (g_str_equal(arch, "i386") || g_str_equal(arch, "x86_64")) { > + has_numa = true; > + ctx.machine_name = "pc"; > + ctx.fw_cfg = pc_fw_cfg_init(); > + } else if (g_str_equal(arch, "sparc")) { > + ctx.machine_name = "SS-5"; > + ctx.fw_cfg = mm_fw_cfg_init(0xd00000510ULL); > + } else if (g_str_equal(arch, "ppc") || g_str_equal(arch, "ppc64")) { > + /* > + * The mac99 machine is different for 32/64-bit target: > + * > + * ppc(32): the G4 which can be either little or big endian, > + * ppc64: the G5 (970FX) is only big-endian. > + */ > + ctx.machine_name = "mac99"; > + ctx.fw_cfg = mm_fw_cfg_init(0xf0000510); > + } > > qtest_add_data_func("fw_cfg/signature", &ctx, test_fw_cfg_signature); > qtest_add_data_func("fw_cfg/id", &ctx, test_fw_cfg_id); > @@ -231,14 +248,18 @@ int main(int argc, char **argv) > qtest_add_func("fw_cfg/boot_device", test_fw_cfg_boot_device); > #endif > qtest_add_data_func("fw_cfg/max_cpus", &ctx, test_fw_cfg_max_cpus); > - qtest_add_data_func("fw_cfg/numa", &ctx, test_fw_cfg_numa); > qtest_add_data_func("fw_cfg/boot_menu", &ctx, test_fw_cfg_boot_menu); > qtest_add_data_func("fw_cfg/reboot_timeout", &ctx, > test_fw_cfg_reboot_timeout); > qtest_add_data_func("fw_cfg/splash_time", &ctx, > test_fw_cfg_splash_time); > > - ret = g_test_run(); > + if (has_numa) { > + qtest_add_data_func("fw_cfg/numa", &ctx, test_fw_cfg_numa); > + } > > + if (ctx.machine_name) { > + ret = g_test_run(); > + } > I think we can omit this if statement. In which case the ctx.machine_name will be NULL? Thanks, Li Qiang > g_free(ctx.fw_cfg); > > return ret; > -- > 2.21.0 > >
Hi Li, On 10/8/19 5:04 PM, Li Qiang wrote: > Philippe Mathieu-Daudé <philmd@redhat.com <mailto:philmd@redhat.com>> 于 > 2019年10月7日周一 下午11:20写道： > > We have been restricting our fw_cfg tests to the PC machine, > which is a little-endian architecture. > The fw_cfg device is also used on the SPARC and PowerPC > architectures, which can run in big-endian configuration. > > Since we want to be sure our device does not regress > regardless the endianess used, enable this test one > these targets. > > The NUMA selector is X86 specific, restrict it to this arch. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com > <mailto:philmd@redhat.com>> > --- > v2: test ppc32 too (lvivier) > --- > tests/Makefile.include | 2 ++ > tests/fw_cfg-test.c | 33 +++++++++++++++++++++++++++------ > 2 files changed, 29 insertions(+), 6 deletions(-) > > diff --git a/tests/Makefile.include b/tests/Makefile.include > index 3543451ed3..4ae3d5140a 100644 > --- a/tests/Makefile.include > +++ b/tests/Makefile.include > @@ -226,6 +226,7 @@ check-qtest-ppc-y += tests/prom-env-test$(EXESUF) > check-qtest-ppc-y += tests/drive_del-test$(EXESUF) > check-qtest-ppc-y += tests/boot-serial-test$(EXESUF) > check-qtest-ppc-$(CONFIG_M48T59) += tests/m48t59-test$(EXESUF) > +check-qtest-ppc-y += tests/fw_cfg-test$(EXESUF) > > check-qtest-ppc64-y += $(check-qtest-ppc-y) > check-qtest-ppc64-$(CONFIG_PSERIES) += tests/device-plug-test$(EXESUF) > @@ -250,6 +251,7 @@ check-qtest-sh4eb-$(CONFIG_ISA_TESTDEV) = > tests/endianness-test$(EXESUF) > check-qtest-sparc-y += tests/prom-env-test$(EXESUF) > check-qtest-sparc-y += tests/m48t59-test$(EXESUF) > check-qtest-sparc-y += tests/boot-serial-test$(EXESUF) > +check-qtest-sparc-y += tests/fw_cfg-test$(EXESUF) > > check-qtest-sparc64-$(CONFIG_ISA_TESTDEV) = > tests/endianness-test$(EXESUF) > check-qtest-sparc64-y += tests/prom-env-test$(EXESUF) > diff --git a/tests/fw_cfg-test.c b/tests/fw_cfg-test.c > index 35af0de7e6..1250e87097 100644 > --- a/tests/fw_cfg-test.c > +++ b/tests/fw_cfg-test.c > @@ -210,13 +210,30 @@ static void test_fw_cfg_splash_time(const void > *opaque) > > int main(int argc, char **argv) > { > - QTestCtx ctx; > - int ret; > + const char *arch = qtest_get_arch(); > + bool has_numa = false; > + QTestCtx ctx = {}; > + int ret = 0; > > g_test_init(&argc, &argv, NULL); > > - ctx.machine_name = "pc"; > - ctx.fw_cfg = pc_fw_cfg_init(); > + if (g_str_equal(arch, "i386") || g_str_equal(arch, "x86_64")) { > + has_numa = true; > + ctx.machine_name = "pc"; > + ctx.fw_cfg = pc_fw_cfg_init(); > + } else if (g_str_equal(arch, "sparc")) { > + ctx.machine_name = "SS-5"; > + ctx.fw_cfg = mm_fw_cfg_init(0xd00000510ULL); > + } else if (g_str_equal(arch, "ppc") || g_str_equal(arch, > "ppc64")) { > + /* > + * The mac99 machine is different for 32/64-bit target: > + * > + * ppc(32): the G4 which can be either little or big endian, > + * ppc64: the G5 (970FX) is only big-endian. > + */ > + ctx.machine_name = "mac99"; > + ctx.fw_cfg = mm_fw_cfg_init(0xf0000510); > + } > > qtest_add_data_func("fw_cfg/signature", &ctx, > test_fw_cfg_signature); > qtest_add_data_func("fw_cfg/id", &ctx, test_fw_cfg_id); > @@ -231,14 +248,18 @@ int main(int argc, char **argv) > qtest_add_func("fw_cfg/boot_device", test_fw_cfg_boot_device); > #endif > qtest_add_data_func("fw_cfg/max_cpus", &ctx, > test_fw_cfg_max_cpus); > - qtest_add_data_func("fw_cfg/numa", &ctx, test_fw_cfg_numa); > qtest_add_data_func("fw_cfg/boot_menu", &ctx, > test_fw_cfg_boot_menu); > qtest_add_data_func("fw_cfg/reboot_timeout", &ctx, > test_fw_cfg_reboot_timeout); > qtest_add_data_func("fw_cfg/splash_time", &ctx, > test_fw_cfg_splash_time); > > - ret = g_test_run(); > + if (has_numa) { > + qtest_add_data_func("fw_cfg/numa", &ctx, test_fw_cfg_numa); > + } > > + if (ctx.machine_name) { > + ret = g_test_run(); > + } > > > I think we can omit this if statement. In which case the > ctx.machine_name will be NULL? Here I thought about the PPC64 tests inheriting the PPC32 ones, and maybe someone update the tests/Makefile.include and this test will run on unexpected architectures. So if the machine is NULL (another arch) we don't crash and return successfully, so the testsuite continue. I might add a comment such: if (ctx.machine_name) { /* Only run whitelisted architecture. */ ret = g_test_run(); } But maybe it is simpler to do at the beginning of main(): if (g_str_equal(arch, "i386") || g_str_equal(arch, "x86_64")) { ... ctx.fw_cfg = mm_fw_cfg_init(0xf0000510); } else { return 0; } What do you think? Thanks for reviewing the whole series :) > Thanks, > Li Qiang > > g_free(ctx.fw_cfg); > > return ret; > -- > 2.21.0 >
Philippe Mathieu-Daudé <philmd@redhat.com> 于2019年10月8日周二 下午11:14写道： > Hi Li, > > On 10/8/19 5:04 PM, Li Qiang wrote: > > Philippe Mathieu-Daudé <philmd@redhat.com <mailto:philmd@redhat.com>> 于 > > 2019年10月7日周一 下午11:20写道： > > > > We have been restricting our fw_cfg tests to the PC machine, > > which is a little-endian architecture. > > The fw_cfg device is also used on the SPARC and PowerPC > > architectures, which can run in big-endian configuration. > > > > Since we want to be sure our device does not regress > > regardless the endianess used, enable this test one > > these targets. > > > > The NUMA selector is X86 specific, restrict it to this arch. > > > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com > > <mailto:philmd@redhat.com>> > > --- > > v2: test ppc32 too (lvivier) > > --- > > tests/Makefile.include | 2 ++ > > tests/fw_cfg-test.c | 33 +++++++++++++++++++++++++++------ > > 2 files changed, 29 insertions(+), 6 deletions(-) > > > > diff --git a/tests/Makefile.include b/tests/Makefile.include > > index 3543451ed3..4ae3d5140a 100644 > > --- a/tests/Makefile.include > > +++ b/tests/Makefile.include > > @@ -226,6 +226,7 @@ check-qtest-ppc-y += tests/prom-env-test$(EXESUF) > > check-qtest-ppc-y += tests/drive_del-test$(EXESUF) > > check-qtest-ppc-y += tests/boot-serial-test$(EXESUF) > > check-qtest-ppc-$(CONFIG_M48T59) += tests/m48t59-test$(EXESUF) > > +check-qtest-ppc-y += tests/fw_cfg-test$(EXESUF) > > > > check-qtest-ppc64-y += $(check-qtest-ppc-y) > > check-qtest-ppc64-$(CONFIG_PSERIES) += > tests/device-plug-test$(EXESUF) > > @@ -250,6 +251,7 @@ check-qtest-sh4eb-$(CONFIG_ISA_TESTDEV) = > > tests/endianness-test$(EXESUF) > > check-qtest-sparc-y += tests/prom-env-test$(EXESUF) > > check-qtest-sparc-y += tests/m48t59-test$(EXESUF) > > check-qtest-sparc-y += tests/boot-serial-test$(EXESUF) > > +check-qtest-sparc-y += tests/fw_cfg-test$(EXESUF) > > > > check-qtest-sparc64-$(CONFIG_ISA_TESTDEV) = > > tests/endianness-test$(EXESUF) > > check-qtest-sparc64-y += tests/prom-env-test$(EXESUF) > > diff --git a/tests/fw_cfg-test.c b/tests/fw_cfg-test.c > > index 35af0de7e6..1250e87097 100644 > > --- a/tests/fw_cfg-test.c > > +++ b/tests/fw_cfg-test.c > > @@ -210,13 +210,30 @@ static void test_fw_cfg_splash_time(const void > > *opaque) > > > > int main(int argc, char **argv) > > { > > - QTestCtx ctx; > > - int ret; > > + const char *arch = qtest_get_arch(); > > + bool has_numa = false; > > + QTestCtx ctx = {}; > > + int ret = 0; > > > > g_test_init(&argc, &argv, NULL); > > > > - ctx.machine_name = "pc"; > > - ctx.fw_cfg = pc_fw_cfg_init(); > > + if (g_str_equal(arch, "i386") || g_str_equal(arch, "x86_64")) { > > + has_numa = true; > > + ctx.machine_name = "pc"; > > + ctx.fw_cfg = pc_fw_cfg_init(); > > + } else if (g_str_equal(arch, "sparc")) { > > + ctx.machine_name = "SS-5"; > > + ctx.fw_cfg = mm_fw_cfg_init(0xd00000510ULL); > > + } else if (g_str_equal(arch, "ppc") || g_str_equal(arch, > > "ppc64")) { > > + /* > > + * The mac99 machine is different for 32/64-bit target: > > + * > > + * ppc(32): the G4 which can be either little or big endian, > > + * ppc64: the G5 (970FX) is only big-endian. > > + */ > > + ctx.machine_name = "mac99"; > > + ctx.fw_cfg = mm_fw_cfg_init(0xf0000510); > > + } > > > > qtest_add_data_func("fw_cfg/signature", &ctx, > > test_fw_cfg_signature); > > qtest_add_data_func("fw_cfg/id", &ctx, test_fw_cfg_id); > > @@ -231,14 +248,18 @@ int main(int argc, char **argv) > > qtest_add_func("fw_cfg/boot_device", test_fw_cfg_boot_device); > > #endif > > qtest_add_data_func("fw_cfg/max_cpus", &ctx, > > test_fw_cfg_max_cpus); > > - qtest_add_data_func("fw_cfg/numa", &ctx, test_fw_cfg_numa); > > qtest_add_data_func("fw_cfg/boot_menu", &ctx, > > test_fw_cfg_boot_menu); > > qtest_add_data_func("fw_cfg/reboot_timeout", &ctx, > > test_fw_cfg_reboot_timeout); > > qtest_add_data_func("fw_cfg/splash_time", &ctx, > > test_fw_cfg_splash_time); > > > > - ret = g_test_run(); > > + if (has_numa) { > > + qtest_add_data_func("fw_cfg/numa", &ctx, test_fw_cfg_numa); > > + } > > > > + if (ctx.machine_name) { > > + ret = g_test_run(); > > + } > > > > > > I think we can omit this if statement. In which case the > > ctx.machine_name will be NULL? > > Here I thought about the PPC64 tests inheriting the PPC32 ones, and > maybe someone update the tests/Makefile.include and this test will run > on unexpected architectures. Sorry, I don't get your point here(PPC64 tests inheriting the PPC32), could you please explain this more? > So if the machine is NULL (another arch) we > IIUC the "-M" option must has an argument, maybe? Thanks, Li Qiang > don't crash and return successfully, so the testsuite continue. > > I might add a comment such: > > if (ctx.machine_name) { > /* Only run whitelisted architecture. */ > ret = g_test_run(); > } > > But maybe it is simpler to do at the beginning of main(): > > if (g_str_equal(arch, "i386") || g_str_equal(arch, "x86_64")) { > ... > ctx.fw_cfg = mm_fw_cfg_init(0xf0000510); > } else { > return 0; > } > > What do you think? > > Thanks for reviewing the whole series :) > > > Thanks, > > Li Qiang > > > > g_free(ctx.fw_cfg); > > > > return ret; > > -- > > 2.21.0 > > >
On 10/08/19 17:04, Li Qiang wrote: > Philippe Mathieu-Daudé <philmd@redhat.com> 于2019年10月7日周一 下午11:20写道： > >> We have been restricting our fw_cfg tests to the PC machine, >> which is a little-endian architecture. >> The fw_cfg device is also used on the SPARC and PowerPC >> architectures, which can run in big-endian configuration. >> >> Since we want to be sure our device does not regress >> regardless the endianess used, enable this test one >> these targets. >> >> The NUMA selector is X86 specific, restrict it to this arch. >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> v2: test ppc32 too (lvivier) >> --- >> tests/Makefile.include | 2 ++ >> tests/fw_cfg-test.c | 33 +++++++++++++++++++++++++++------ >> 2 files changed, 29 insertions(+), 6 deletions(-) >> >> diff --git a/tests/Makefile.include b/tests/Makefile.include >> index 3543451ed3..4ae3d5140a 100644 >> --- a/tests/Makefile.include >> +++ b/tests/Makefile.include >> @@ -226,6 +226,7 @@ check-qtest-ppc-y += tests/prom-env-test$(EXESUF) >> check-qtest-ppc-y += tests/drive_del-test$(EXESUF) >> check-qtest-ppc-y += tests/boot-serial-test$(EXESUF) >> check-qtest-ppc-$(CONFIG_M48T59) += tests/m48t59-test$(EXESUF) >> +check-qtest-ppc-y += tests/fw_cfg-test$(EXESUF) >> >> check-qtest-ppc64-y += $(check-qtest-ppc-y) >> check-qtest-ppc64-$(CONFIG_PSERIES) += tests/device-plug-test$(EXESUF) >> @@ -250,6 +251,7 @@ check-qtest-sh4eb-$(CONFIG_ISA_TESTDEV) = >> tests/endianness-test$(EXESUF) >> check-qtest-sparc-y += tests/prom-env-test$(EXESUF) >> check-qtest-sparc-y += tests/m48t59-test$(EXESUF) >> check-qtest-sparc-y += tests/boot-serial-test$(EXESUF) >> +check-qtest-sparc-y += tests/fw_cfg-test$(EXESUF) >> >> check-qtest-sparc64-$(CONFIG_ISA_TESTDEV) = tests/endianness-test$(EXESUF) >> check-qtest-sparc64-y += tests/prom-env-test$(EXESUF) >> diff --git a/tests/fw_cfg-test.c b/tests/fw_cfg-test.c >> index 35af0de7e6..1250e87097 100644 >> --- a/tests/fw_cfg-test.c >> +++ b/tests/fw_cfg-test.c >> @@ -210,13 +210,30 @@ static void test_fw_cfg_splash_time(const void >> *opaque) >> >> int main(int argc, char **argv) >> { >> - QTestCtx ctx; >> - int ret; >> + const char *arch = qtest_get_arch(); >> + bool has_numa = false; >> + QTestCtx ctx = {}; >> + int ret = 0; >> >> g_test_init(&argc, &argv, NULL); >> >> - ctx.machine_name = "pc"; >> - ctx.fw_cfg = pc_fw_cfg_init(); >> + if (g_str_equal(arch, "i386") || g_str_equal(arch, "x86_64")) { >> + has_numa = true; >> + ctx.machine_name = "pc"; >> + ctx.fw_cfg = pc_fw_cfg_init(); >> + } else if (g_str_equal(arch, "sparc")) { >> + ctx.machine_name = "SS-5"; >> + ctx.fw_cfg = mm_fw_cfg_init(0xd00000510ULL); >> + } else if (g_str_equal(arch, "ppc") || g_str_equal(arch, "ppc64")) { >> + /* >> + * The mac99 machine is different for 32/64-bit target: >> + * >> + * ppc(32): the G4 which can be either little or big endian, >> + * ppc64: the G5 (970FX) is only big-endian. >> + */ >> + ctx.machine_name = "mac99"; >> + ctx.fw_cfg = mm_fw_cfg_init(0xf0000510); >> + } >> >> qtest_add_data_func("fw_cfg/signature", &ctx, test_fw_cfg_signature); >> qtest_add_data_func("fw_cfg/id", &ctx, test_fw_cfg_id); >> @@ -231,14 +248,18 @@ int main(int argc, char **argv) >> qtest_add_func("fw_cfg/boot_device", test_fw_cfg_boot_device); >> #endif >> qtest_add_data_func("fw_cfg/max_cpus", &ctx, test_fw_cfg_max_cpus); >> - qtest_add_data_func("fw_cfg/numa", &ctx, test_fw_cfg_numa); >> qtest_add_data_func("fw_cfg/boot_menu", &ctx, test_fw_cfg_boot_menu); >> qtest_add_data_func("fw_cfg/reboot_timeout", &ctx, >> test_fw_cfg_reboot_timeout); >> qtest_add_data_func("fw_cfg/splash_time", &ctx, >> test_fw_cfg_splash_time); >> >> - ret = g_test_run(); >> + if (has_numa) { >> + qtest_add_data_func("fw_cfg/numa", &ctx, test_fw_cfg_numa); >> + } >> >> + if (ctx.machine_name) { >> + ret = g_test_run(); >> + } >> > > I think we can omit this if statement. In which case the ctx.machine_name > will be NULL? When "arch" differs from all of i386, x86_64, sparc, ppc, ppc64. In that case, the original initializer will remain in effect, from: QTestCtx ctx = {}; (Admittedly, this is an ugly GNU-ism; for standard C, it should be QTestCtx ctx = { 0 }; but the GNU-ism is used quite frequently in QEMU elsewhere, so meh :) ) Thanks Laszlo
On Tue, 8 Oct 2019 at 21:29, Laszlo Ersek <lersek@redhat.com> wrote: > In that case, the original initializer will remain in effect, from: > > QTestCtx ctx = {}; > > (Admittedly, this is an ugly GNU-ism; for standard C, it should be > > QTestCtx ctx = { 0 }; > > but the GNU-ism is used quite frequently in QEMU elsewhere, so meh :) ) This is deliberate -- some compilers grumble about "{ 0 }" in some situations, so we generally recommend using "{}" which is accepted by all compilers we care about. (Cf commit message for commit 039d4e3df0049bdd.) thanks -- PMM
Laszlo Ersek <lersek@redhat.com> 于2019年10月9日周三 上午4:27写道： > On 10/08/19 17:04, Li Qiang wrote: > > Philippe Mathieu-Daudé <philmd@redhat.com> 于2019年10月7日周一 下午11:20写道： > > > >> We have been restricting our fw_cfg tests to the PC machine, > >> which is a little-endian architecture. > >> The fw_cfg device is also used on the SPARC and PowerPC > >> architectures, which can run in big-endian configuration. > >> > >> Since we want to be sure our device does not regress > >> regardless the endianess used, enable this test one > >> these targets. > >> > >> The NUMA selector is X86 specific, restrict it to this arch. > >> > >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > >> --- > >> v2: test ppc32 too (lvivier) > >> --- > >> tests/Makefile.include | 2 ++ > >> tests/fw_cfg-test.c | 33 +++++++++++++++++++++++++++------ > >> 2 files changed, 29 insertions(+), 6 deletions(-) > >> > >> diff --git a/tests/Makefile.include b/tests/Makefile.include > >> index 3543451ed3..4ae3d5140a 100644 > >> --- a/tests/Makefile.include > >> +++ b/tests/Makefile.include > >> @@ -226,6 +226,7 @@ check-qtest-ppc-y += tests/prom-env-test$(EXESUF) > >> check-qtest-ppc-y += tests/drive_del-test$(EXESUF) > >> check-qtest-ppc-y += tests/boot-serial-test$(EXESUF) > >> check-qtest-ppc-$(CONFIG_M48T59) += tests/m48t59-test$(EXESUF) > >> +check-qtest-ppc-y += tests/fw_cfg-test$(EXESUF) > >> > >> check-qtest-ppc64-y += $(check-qtest-ppc-y) > >> check-qtest-ppc64-$(CONFIG_PSERIES) += tests/device-plug-test$(EXESUF) > >> @@ -250,6 +251,7 @@ check-qtest-sh4eb-$(CONFIG_ISA_TESTDEV) = > >> tests/endianness-test$(EXESUF) > >> check-qtest-sparc-y += tests/prom-env-test$(EXESUF) > >> check-qtest-sparc-y += tests/m48t59-test$(EXESUF) > >> check-qtest-sparc-y += tests/boot-serial-test$(EXESUF) > >> +check-qtest-sparc-y += tests/fw_cfg-test$(EXESUF) > >> > >> check-qtest-sparc64-$(CONFIG_ISA_TESTDEV) = > tests/endianness-test$(EXESUF) > >> check-qtest-sparc64-y += tests/prom-env-test$(EXESUF) > >> diff --git a/tests/fw_cfg-test.c b/tests/fw_cfg-test.c > >> index 35af0de7e6..1250e87097 100644 > >> --- a/tests/fw_cfg-test.c > >> +++ b/tests/fw_cfg-test.c > >> @@ -210,13 +210,30 @@ static void test_fw_cfg_splash_time(const void > >> *opaque) > >> > >> int main(int argc, char **argv) > >> { > >> - QTestCtx ctx; > >> - int ret; > >> + const char *arch = qtest_get_arch(); > >> + bool has_numa = false; > >> + QTestCtx ctx = {}; > >> + int ret = 0; > >> > >> g_test_init(&argc, &argv, NULL); > >> > >> - ctx.machine_name = "pc"; > >> - ctx.fw_cfg = pc_fw_cfg_init(); > >> + if (g_str_equal(arch, "i386") || g_str_equal(arch, "x86_64")) { > >> + has_numa = true; > >> + ctx.machine_name = "pc"; > >> + ctx.fw_cfg = pc_fw_cfg_init(); > >> + } else if (g_str_equal(arch, "sparc")) { > >> + ctx.machine_name = "SS-5"; > >> + ctx.fw_cfg = mm_fw_cfg_init(0xd00000510ULL); > >> + } else if (g_str_equal(arch, "ppc") || g_str_equal(arch, "ppc64")) > { > >> + /* > >> + * The mac99 machine is different for 32/64-bit target: > >> + * > >> + * ppc(32): the G4 which can be either little or big endian, > >> + * ppc64: the G5 (970FX) is only big-endian. > >> + */ > >> + ctx.machine_name = "mac99"; > >> + ctx.fw_cfg = mm_fw_cfg_init(0xf0000510); > >> + } > >> > >> qtest_add_data_func("fw_cfg/signature", &ctx, > test_fw_cfg_signature); > >> qtest_add_data_func("fw_cfg/id", &ctx, test_fw_cfg_id); > >> @@ -231,14 +248,18 @@ int main(int argc, char **argv) > >> qtest_add_func("fw_cfg/boot_device", test_fw_cfg_boot_device); > >> #endif > >> qtest_add_data_func("fw_cfg/max_cpus", &ctx, test_fw_cfg_max_cpus); > >> - qtest_add_data_func("fw_cfg/numa", &ctx, test_fw_cfg_numa); > >> qtest_add_data_func("fw_cfg/boot_menu", &ctx, > test_fw_cfg_boot_menu); > >> qtest_add_data_func("fw_cfg/reboot_timeout", &ctx, > >> test_fw_cfg_reboot_timeout); > >> qtest_add_data_func("fw_cfg/splash_time", &ctx, > >> test_fw_cfg_splash_time); > >> > >> - ret = g_test_run(); > >> + if (has_numa) { > >> + qtest_add_data_func("fw_cfg/numa", &ctx, test_fw_cfg_numa); > >> + } > >> > >> + if (ctx.machine_name) { > >> + ret = g_test_run(); > >> + } > >> > > > > I think we can omit this if statement. In which case the ctx.machine_name > > will be NULL? > > When "arch" differs from all of i386, x86_64, sparc, ppc, ppc64. > Yes, I just found the setting 'arch' lost 'else' branch. Then in this case, I prefer 'just return'. There is no need do other things for non-support arch. This is just like some other case, such as usb-hcd-ehci-test.c and ahci-test.c. Also, I don't find any conditional 'g_test_run' calling. Thanks, Li Qiang > > In that case, the original initializer will remain in effect, from: > > QTestCtx ctx = {}; > > (Admittedly, this is an ugly GNU-ism; for standard C, it should be > > QTestCtx ctx = { 0 }; > > but the GNU-ism is used quite frequently in QEMU elsewhere, so meh :) ) > > Thanks > Laszlo >
diff --git a/tests/Makefile.include b/tests/Makefile.include index 3543451ed3..4ae3d5140a 100644 --- a/tests/Makefile.include +++ b/tests/Makefile.include @@ -226,6 +226,7 @@ check-qtest-ppc-y += tests/prom-env-test$(EXESUF) check-qtest-ppc-y += tests/drive_del-test$(EXESUF) check-qtest-ppc-y += tests/boot-serial-test$(EXESUF) check-qtest-ppc-$(CONFIG_M48T59) += tests/m48t59-test$(EXESUF) +check-qtest-ppc-y += tests/fw_cfg-test$(EXESUF) check-qtest-ppc64-y += $(check-qtest-ppc-y) check-qtest-ppc64-$(CONFIG_PSERIES) += tests/device-plug-test$(EXESUF) @@ -250,6 +251,7 @@ check-qtest-sh4eb-$(CONFIG_ISA_TESTDEV) = tests/endianness-test$(EXESUF) check-qtest-sparc-y += tests/prom-env-test$(EXESUF) check-qtest-sparc-y += tests/m48t59-test$(EXESUF) check-qtest-sparc-y += tests/boot-serial-test$(EXESUF) +check-qtest-sparc-y += tests/fw_cfg-test$(EXESUF) check-qtest-sparc64-$(CONFIG_ISA_TESTDEV) = tests/endianness-test$(EXESUF) check-qtest-sparc64-y += tests/prom-env-test$(EXESUF) diff --git a/tests/fw_cfg-test.c b/tests/fw_cfg-test.c index 35af0de7e6..1250e87097 100644 --- a/tests/fw_cfg-test.c +++ b/tests/fw_cfg-test.c @@ -210,13 +210,30 @@ static void test_fw_cfg_splash_time(const void *opaque) int main(int argc, char **argv) { - QTestCtx ctx; - int ret; + const char *arch = qtest_get_arch(); + bool has_numa = false; + QTestCtx ctx = {}; + int ret = 0; g_test_init(&argc, &argv, NULL); - ctx.machine_name = "pc"; - ctx.fw_cfg = pc_fw_cfg_init(); + if (g_str_equal(arch, "i386") || g_str_equal(arch, "x86_64")) { + has_numa = true; + ctx.machine_name = "pc"; + ctx.fw_cfg = pc_fw_cfg_init(); + } else if (g_str_equal(arch, "sparc")) { + ctx.machine_name = "SS-5"; + ctx.fw_cfg = mm_fw_cfg_init(0xd00000510ULL); + } else if (g_str_equal(arch, "ppc") || g_str_equal(arch, "ppc64")) { + /* + * The mac99 machine is different for 32/64-bit target: + * + * ppc(32): the G4 which can be either little or big endian, + * ppc64: the G5 (970FX) is only big-endian. + */ + ctx.machine_name = "mac99"; + ctx.fw_cfg = mm_fw_cfg_init(0xf0000510); + } qtest_add_data_func("fw_cfg/signature", &ctx, test_fw_cfg_signature); qtest_add_data_func("fw_cfg/id", &ctx, test_fw_cfg_id); @@ -231,14 +248,18 @@ int main(int argc, char **argv) qtest_add_func("fw_cfg/boot_device", test_fw_cfg_boot_device); #endif qtest_add_data_func("fw_cfg/max_cpus", &ctx, test_fw_cfg_max_cpus); - qtest_add_data_func("fw_cfg/numa", &ctx, test_fw_cfg_numa); qtest_add_data_func("fw_cfg/boot_menu", &ctx, test_fw_cfg_boot_menu); qtest_add_data_func("fw_cfg/reboot_timeout", &ctx, test_fw_cfg_reboot_timeout); qtest_add_data_func("fw_cfg/splash_time", &ctx, test_fw_cfg_splash_time); - ret = g_test_run(); + if (has_numa) { + qtest_add_data_func("fw_cfg/numa", &ctx, test_fw_cfg_numa); + } + if (ctx.machine_name) { + ret = g_test_run(); + } g_free(ctx.fw_cfg); return ret;
We have been restricting our fw_cfg tests to the PC machine, which is a little-endian architecture. The fw_cfg device is also used on the SPARC and PowerPC architectures, which can run in big-endian configuration. Since we want to be sure our device does not regress regardless the endianess used, enable this test one these targets. The NUMA selector is X86 specific, restrict it to this arch. Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- v2: test ppc32 too (lvivier) --- tests/Makefile.include | 2 ++ tests/fw_cfg-test.c | 33 +++++++++++++++++++++++++++------ 2 files changed, 29 insertions(+), 6 deletions(-)