Philippe Mathieu-Daudé <phi...@redhat.com> 于2019年10月8日周二 下午11:14写道：
> Hi Li, > > On 10/8/19 5:04 PM, Li Qiang wrote: > > Philippe Mathieu-Daudé <phi...@redhat.com <mailto:phi...@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é <phi...@redhat.com > > <mailto:phi...@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 > > >
