From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47583) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UnUQn-0002Jt-V7 for qemu-devel@nongnu.org; Fri, 14 Jun 2013 09:52:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UnUQk-00028C-Te for qemu-devel@nongnu.org; Fri, 14 Jun 2013 09:52:45 -0400 Received: from e34.co.us.ibm.com ([32.97.110.152]:46497) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UnUQk-00027n-Ik for qemu-devel@nongnu.org; Fri, 14 Jun 2013 09:52:42 -0400 Received: from /spool/local by e34.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 14 Jun 2013 07:52:42 -0600 From: Anthony Liguori In-Reply-To: <1371208516-7857-12-git-send-email-armbru@redhat.com> References: <1371208516-7857-1-git-send-email-armbru@redhat.com> <1371208516-7857-12-git-send-email-armbru@redhat.com> Date: Fri, 14 Jun 2013 08:52:09 -0500 Message-ID: <878v2cg5py.fsf@codemonkey.ws> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v3 11/16] boot-order-test: Better separate target-specific and generic parts List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster , qemu-devel@nongnu.org Cc: jan.kiszka@siemens.com, Alexander Graf , alex.williamson@redhat.com, qemu-ppc@nongnu.org, aviksil@linux.vnet.ibm.com, afaerber@suse.de Markus Armbruster writes: > The initial version did just PC. I didn't bother to separate out > generic parts, because I don't like to abstract from a single case. > > Now we have two cases, PC and PowerMac, and I'm about to add two more. > Time to do it right. > > To ease review, this commit changes the code without in-place, and > only the next commit reorders it for better readability. > > Cc: Andreas F=C3=A4rber > Cc: Alexander Graf > Cc: qemu-ppc@nongnu.org > Signed-off-by: Markus Armbruster > --- > tests/boot-order-test.c | 160 ++++++++++++++++++++++++++++++------------= ------ > 1 file changed, 99 insertions(+), 61 deletions(-) > > diff --git a/tests/boot-order-test.c b/tests/boot-order-test.c > index f6dece6..23edacf 100644 > --- a/tests/boot-order-test.c > +++ b/tests/boot-order-test.c > @@ -15,106 +15,141 @@ > #include "libqtest.h" > #include "qemu/bswap.h" >=20=20 > -static void test_pc_cmos_byte(int reg, int expected) > +static uint8_t read_mc146818(uint16_t port, uint8_t reg) > { > - int actual; > - > - outb(0x70, reg); > - actual =3D inb(0x71); > - g_assert_cmphex(actual, =3D=3D, expected); > + outb(port, reg); > + return inb(port + 1); > } It's worth looking at the rtc-test case and pulling out cmos_read/write() into libqos I suspect. Regards, Anthony Liguori >=20=20 > -static void test_pc_cmos(uint8_t boot1, uint8_t boot2) > +static uint64_t read_boot_order_pc(void) > { > - test_pc_cmos_byte(0x38, boot1); > - test_pc_cmos_byte(0x3d, boot2); > + uint8_t b1 =3D read_mc146818(0x70, 0x38); > + uint8_t b2 =3D read_mc146818(0x70, 0x3d); > + > + return b1 | (b2 << 8); > } >=20=20 > -static void test_pc_with_args(const char *test_args, > - uint8_t boot1, uint8_t boot2, > - uint8_t reboot1, uint8_t reboot2) > +static void test_a_boot_order(const char *machine, > + const char *test_args, > + uint64_t (*read_boot_order)(void), > + uint64_t expected_boot, > + uint64_t expected_reboot) > { > - char *args =3D g_strdup_printf("-nodefaults -display none %s", test_= args); > + char *args; > + uint64_t actual; >=20=20 > + args =3D g_strdup_printf("-nodefaults -display none%s%s %s", > + machine ? " -M " : "", > + machine ?: "", > + test_args); > qtest_start(args); > - test_pc_cmos(boot1, boot2); > + actual =3D read_boot_order(); > + g_assert_cmphex(actual, =3D=3D, expected_boot); > qmp("{ 'execute': 'system_reset' }"); > - test_pc_cmos(reboot1, reboot2); > + actual =3D read_boot_order(); > + g_assert_cmphex(actual, =3D=3D, expected_reboot); > qtest_quit(global_qtest); > g_free(args); > } >=20=20 > +typedef struct { > + const char *args; > + uint64_t expected_boot; > + uint64_t expected_reboot; > +} boot_order_test; > + > +static void test_boot_orders(const char *machine, > + uint64_t (*read_boot_order)(void), > + const boot_order_test *tests) > +{ > + int i; > + > + for (i =3D 0; tests[i].args; i++) { > + test_a_boot_order(machine, tests[i].args, > + read_boot_order, > + tests[i].expected_boot, > + tests[i].expected_reboot); > + } > +} > + > +static const boot_order_test test_cases_pc[] =3D { > + { "", > + 0x1230, 0x1230 }, > + { "-no-fd-bootchk", > + 0x1231, 0x1231 }, > + { "-boot c", > + 0x0200, 0x0200 }, > + { "-boot nda", > + 0x3410, 0x3410 }, > + { "-boot order=3D", > + 0, 0 }, > + { "-boot order=3D -boot order=3Dc", > + 0x0200, 0x0200 }, > + { "-boot once=3Da", > + 0x0100, 0x1230 }, > + { "-boot once=3Da -no-fd-bootchk", > + 0x0101, 0x1231 }, > + { "-boot once=3Da,order=3Dc", > + 0x0100, 0x0200 }, > + { "-boot once=3Dd -boot order=3Dnda", > + 0x0300, 0x3410 }, > + { "-boot once=3Da -boot once=3Db -boot once=3Dc", > + 0x0200, 0x1230 }, > + {} > +}; > + > static void test_pc_boot_order(void) > { > - test_pc_with_args("", 0x30, 0x12, 0x30, 0x12); > - test_pc_with_args("-no-fd-bootchk", 0x31, 0x12, 0x31, 0x12); > - test_pc_with_args("-boot c", 0, 0x02, 0, 0x02); > - test_pc_with_args("-boot nda", 0x10, 0x34, 0x10, 0x34); > - test_pc_with_args("-boot order=3D", 0, 0, 0, 0); > - test_pc_with_args("-boot order=3D -boot order=3Dc", 0, 0x02, 0, 0x02= ); > - test_pc_with_args("-boot once=3Da", 0, 0x01, 0x30, 0x12); > - test_pc_with_args("-boot once=3Da -no-fd-bootchk", 0x01, 0x01, 0x31,= 0x12); > - test_pc_with_args("-boot once=3Da,order=3Dc", 0, 0x01, 0, 0x02); > - test_pc_with_args("-boot once=3Dd -boot order=3Dnda", 0, 0x03, 0x10,= 0x34); > - test_pc_with_args("-boot once=3Da -boot once=3Db -boot once=3Dc", > - 0, 0x02, 0x30, 0x12); > + test_boot_orders(NULL, read_boot_order_pc, test_cases_pc); > } >=20=20 > -#define G3BEIGE_CFG_ADDR 0xf0000510 > -#define MAC99_CFG_ADDR 0xf0000510 > +#define PMAC_CFG_ADDR 0xf0000510 >=20=20 > #define NO_QEMU_PROTOS > #include "hw/nvram/fw_cfg.h" > #undef NO_QEMU_PROTOS >=20=20 > -static void powermac_fw_cfg_read(bool newworld, uint16_t cmd, > - uint8_t *buf, unsigned int len) > +static void read_fw_cfg(uint64_t cfg_addr, uint16_t cmd, > + void *buf, size_t len) > { > - unsigned int i; > + uint8_t *p =3D buf; > + size_t i; >=20=20 > - writew(newworld ? MAC99_CFG_ADDR : G3BEIGE_CFG_ADDR, cmd); > + writew(cfg_addr, cmd); > for (i =3D 0; i < len; i++) { > - buf[i] =3D readb((newworld ? MAC99_CFG_ADDR : G3BEIGE_CFG_ADDR) = + 2); > + p[i] =3D readb(cfg_addr + 2); > } > } >=20=20 > -static uint16_t powermac_fw_cfg_read16(bool newworld, uint16_t cmd) > +static uint16_t read_fw_cfg_i16(uint64_t cfg_addr, uint16_t cmd) > { > uint16_t value; >=20=20 > - powermac_fw_cfg_read(newworld, cmd, (uint8_t *)&value, sizeof(value)= ); > + read_fw_cfg(cfg_addr, cmd, &value, sizeof(value)); > return le16_to_cpu(value); > } >=20=20 > -static void test_powermac_with_args(bool newworld, const char *extra_arg= s, > - uint16_t expected_boot, > - uint16_t expected_reboot) > +static uint64_t read_boot_order_pmac(void) > { > - char *args =3D g_strdup_printf("-nodefaults -display none -machine %= s %s", > - newworld ? "mac99" : "g3beige", extra_a= rgs); > - uint16_t actual; > - qtest_start(args); > - actual =3D powermac_fw_cfg_read16(newworld, FW_CFG_BOOT_DEVICE); > - g_assert_cmphex(actual, =3D=3D, expected_boot); > - qmp("{ 'execute': 'system_reset' }"); > - actual =3D powermac_fw_cfg_read16(newworld, FW_CFG_BOOT_DEVICE); > - g_assert_cmphex(actual, =3D=3D, expected_reboot); > - qtest_quit(global_qtest); > - g_free(args); > + return read_fw_cfg_i16(PMAC_CFG_ADDR, FW_CFG_BOOT_DEVICE); > } >=20=20 > -static void test_powermac_boot_order(void) > -{ > - int i; > +static const boot_order_test test_cases_fw_cfg[] =3D { > + { "", 'c', 'c' }, > + { "-boot c", 'c', 'c' }, > + { "-boot d", 'd', 'd' }, > + { "-boot once=3Dd,order=3Dc", 'd', 'c' }, > + {} > +}; >=20=20 > - for (i =3D 0; i < 2; i++) { > - bool newworld =3D (i =3D=3D 1); > +static void test_pmac_oldworld_boot_order(void) > +{ > + test_boot_orders("g3beige", read_boot_order_pmac, test_cases_fw_cfg); > +} >=20=20 > - test_powermac_with_args(newworld, "", 'c', 'c'); > - test_powermac_with_args(newworld, "-boot c", 'c', 'c'); > - test_powermac_with_args(newworld, "-boot d", 'd', 'd'); > - test_powermac_with_args(newworld, "-boot once=3Dd,order=3Dc", 'd= ', 'c'); > - } > +static void test_pmac_newworld_boot_order(void) > +{ > + test_boot_orders("mac99", read_boot_order_pmac, test_cases_fw_cfg); > } >=20=20 > int main(int argc, char *argv[]) > @@ -126,7 +161,10 @@ int main(int argc, char *argv[]) > if (strcmp(arch, "i386") =3D=3D 0 || strcmp(arch, "x86_64") =3D=3D 0= ) { > qtest_add_func("boot-order/pc", test_pc_boot_order); > } else if (strcmp(arch, "ppc") =3D=3D 0 || strcmp(arch, "ppc64") =3D= =3D 0) { > - qtest_add_func("boot-order/powermac", test_powermac_boot_order); > + qtest_add_func("boot-order/pmac_oldworld", > + test_pmac_oldworld_boot_order); > + qtest_add_func("boot-order/pmac_newworld", > + test_pmac_newworld_boot_order); > } >=20=20 > return g_test_run(); > --=20 > 1.7.11.7