From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59133) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V4WOX-0007rM-6h for qemu-devel@nongnu.org; Wed, 31 Jul 2013 09:24:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V4WOR-0002wR-Hc for qemu-devel@nongnu.org; Wed, 31 Jul 2013 09:24:49 -0400 Received: from cantor2.suse.de ([195.135.220.15]:54637 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V4TXW-0005gb-Sz for qemu-devel@nongnu.org; Wed, 31 Jul 2013 06:21:55 -0400 Message-ID: <51F8E53D.8060105@suse.de> Date: Wed, 31 Jul 2013 12:21:49 +0200 From: =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= MIME-Version: 1.0 References: <1375118141-8550-1-git-send-email-afaerber@suse.de> <1375118141-8550-5-git-send-email-afaerber@suse.de> <87bo5k5gln.fsf@codemonkey.ws> In-Reply-To: <87bo5k5gln.fsf@codemonkey.ws> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH for-1.6? 04/14] gumstix: Don't enforce use of -pflash for qtest List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: Peter Maydell , qemu-devel@nongnu.org, Paul Brook Am 30.07.2013 15:20, schrieb Anthony Liguori: > Andreas F=C3=A4rber writes: >=20 >> Signed-off-by: Andreas F=C3=A4rber >=20 > I'm not sure this is heading in the right direction. >=20 > What's the bigger picture here? Are we saying that all machines must b= e > launchable without any special parameters? If so, can we document that > somewhere? Well, the picture I see is that many of these machines were contributed ages ago and the contributors are no longer actively maintaining them. But the machines don't seem fundamentally broken, at least not as far as QEMU modeling/API is concerned, so I see no need to rip them out. These patches were tailored for 1.6, so tried to avoid invasive changes (originally I had even avoided the reindent but did it since checkpatch.pl complained about suspect indent). The picture for qtest is that a machine doing random exit()s is untestable. But there is no need to actually specify -kernel for any machine with accel=3Dqtest, since the loaded code will not get executed. For -pflash it's slightly different (an additional device that is not tested if omitted) but it did not work with -pflash /dev/null or -pflash /dev/zero (I originally used -kernel /dev/null and cleaned this up with these patches). When executing a machine without loaded image, the memory is hopefully zero-initialized, which may or may not correspond to a nop instruction; in that case TCG would sequentially execute, well, nothing until it hits memory beyond RAM MemoryRegions, resulting in execution outside of RAM or ROM error. Another aspect is that all fprintf()s are being replaced with error_report() for consistency. Also I noticed that if one uses qtest with a non-existing -machine, it will display the list of available machines and hang (Ctrl-C helps). Replacing errors with warnings might work, but then we'd still want to suppress them from the qtest output. Regards, Andreas > Is that reasonable in practice or are there cases where we must specify > parameters for a machine? >=20 > If the above is true, then I don't think there should be if > (qtest_enabled())s guarding this stuff. >=20 > Regards, >=20 > Anthony Liguori >=20 >> --- >> hw/arm/gumstix.c | 31 +++++++++++++++++++------------ >> 1 file changed, 19 insertions(+), 12 deletions(-) >> >> diff --git a/hw/arm/gumstix.c b/hw/arm/gumstix.c >> index b8cab10..27117fa 100644 >> --- a/hw/arm/gumstix.c >> +++ b/hw/arm/gumstix.c >> @@ -42,6 +42,7 @@ >> #include "hw/boards.h" >> #include "sysemu/blockdev.h" >> #include "exec/address-spaces.h" >> +#include "sysemu/qtest.h" >> =20 >> static const int sector_len =3D 128 * 1024; >> =20 >> @@ -58,7 +59,7 @@ static void connex_init(QEMUMachineInitArgs *args) >> cpu =3D pxa255_init(address_space_mem, connex_ram); >> =20 >> dinfo =3D drive_get(IF_PFLASH, 0, 0); >> - if (!dinfo) { >> + if (!dinfo && !qtest_enabled()) { >> fprintf(stderr, "A flash image must be given with the " >> "'pflash' parameter\n"); >> exit(1); >> @@ -69,11 +70,14 @@ static void connex_init(QEMUMachineInitArgs *args) >> #else >> be =3D 0; >> #endif >> - if (!pflash_cfi01_register(0x00000000, NULL, "connext.rom", conne= x_rom, >> - dinfo->bdrv, sector_len, connex_rom / = sector_len, >> - 2, 0, 0, 0, 0, be)) { >> - fprintf(stderr, "qemu: Error registering flash memory.\n"); >> - exit(1); >> + if (dinfo) { >> + if (!pflash_cfi01_register(0x00000000, NULL, "connext.rom", c= onnex_rom, >> + dinfo->bdrv, sector_len, >> + connex_rom / sector_len, >> + 2, 0, 0, 0, 0, be)) { >> + fprintf(stderr, "qemu: Error registering flash memory.\n"= ); >> + exit(1); >> + } >> } >> =20 >> /* Interrupt line of NIC is connected to GPIO line 36 */ >> @@ -95,7 +99,7 @@ static void verdex_init(QEMUMachineInitArgs *args) >> cpu =3D pxa270_init(address_space_mem, verdex_ram, cpu_model ?: "= pxa270-c0"); >> =20 >> dinfo =3D drive_get(IF_PFLASH, 0, 0); >> - if (!dinfo) { >> + if (!dinfo && !qtest_enabled()) { >> fprintf(stderr, "A flash image must be given with the " >> "'pflash' parameter\n"); >> exit(1); >> @@ -106,11 +110,14 @@ static void verdex_init(QEMUMachineInitArgs *arg= s) >> #else >> be =3D 0; >> #endif >> - if (!pflash_cfi01_register(0x00000000, NULL, "verdex.rom", verdex= _rom, >> - dinfo->bdrv, sector_len, verdex_rom / = sector_len, >> - 2, 0, 0, 0, 0, be)) { >> - fprintf(stderr, "qemu: Error registering flash memory.\n"); >> - exit(1); >> + if (dinfo) { >> + if (!pflash_cfi01_register(0x00000000, NULL, "verdex.rom", ve= rdex_rom, >> + dinfo->bdrv, sector_len, >> + verdex_rom / sector_len, >> + 2, 0, 0, 0, 0, be)) { >> + fprintf(stderr, "qemu: Error registering flash memory.\n"= ); >> + exit(1); >> + } >> } >> =20 >> /* Interrupt line of NIC is connected to GPIO line 99 */ >> --=20 >> 1.8.1.4 --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=C3=BCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=C3=B6rffer; HRB 16746 AG N=C3=BC= rnberg