Patchwork [for-1.6?,04/14] gumstix: Don't enforce use of -pflash for qtest

login
register
mail settings
Submitter Andreas Färber
Date July 29, 2013, 5:15 p.m.
Message ID <1375118141-8550-5-git-send-email-afaerber@suse.de>
Download mbox | patch
Permalink /patch/262912/
State New
Headers show

Comments

Andreas Färber - July 29, 2013, 5:15 p.m.
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 hw/arm/gumstix.c | 31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)
Anthony Liguori - July 30, 2013, 1:20 p.m.
Andreas Färber <afaerber@suse.de> writes:

> Signed-off-by: Andreas Färber <afaerber@suse.de>

I'm not sure this is heading in the right direction.

What's the bigger picture here?  Are we saying that all machines must be
launchable without any special parameters?  If so, can we document that
somewhere?

Is that reasonable in practice or are there cases where we must specify
parameters for a machine?

If the above is true, then I don't think there should be if
(qtest_enabled())s guarding this stuff.

Regards,

Anthony Liguori

> ---
>  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"
>  
>  static const int sector_len = 128 * 1024;
>  
> @@ -58,7 +59,7 @@ static void connex_init(QEMUMachineInitArgs *args)
>      cpu = pxa255_init(address_space_mem, connex_ram);
>  
>      dinfo = 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 = 0;
>  #endif
> -    if (!pflash_cfi01_register(0x00000000, NULL, "connext.rom", connex_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", connex_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);
> +        }
>      }
>  
>      /* Interrupt line of NIC is connected to GPIO line 36 */
> @@ -95,7 +99,7 @@ static void verdex_init(QEMUMachineInitArgs *args)
>      cpu = pxa270_init(address_space_mem, verdex_ram, cpu_model ?: "pxa270-c0");
>  
>      dinfo = 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 *args)
>  #else
>      be = 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", 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);
> +        }
>      }
>  
>      /* Interrupt line of NIC is connected to GPIO line 99 */
> -- 
> 1.8.1.4
Peter Maydell - July 30, 2013, 1:35 p.m.
On 30 July 2013 14:20, Anthony Liguori <anthony@codemonkey.ws> wrote:
> What's the bigger picture here?  Are we saying that all machines must be
> launchable without any special parameters?  If so, can we document that
> somewhere?
>
> Is that reasonable in practice or are there cases where we must specify
> parameters for a machine?

Well, it depends how friendly you're feeling. Typically if the
user has specified neither a -kernel nor some kind of flash
image or BIOS, then we could just choose to start executing at
address zero. This will probably either tear through a bunch of
random garbage or end up looping endlessly through an exception
vector. That's exactly what the hardware would do in that case,
but the usual user response is to accuse QEMU of "hanging".

-- PMM
Andreas Färber - July 31, 2013, 10:21 a.m.
Am 30.07.2013 15:20, schrieb Anthony Liguori:
> Andreas Färber <afaerber@suse.de> writes:
> 
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
> 
> I'm not sure this is heading in the right direction.
> 
> What's the bigger picture here?  Are we saying that all machines must be
> 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=qtest, 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?
> 
> If the above is true, then I don't think there should be if
> (qtest_enabled())s guarding this stuff.
> 
> Regards,
> 
> Anthony Liguori
> 
>> ---
>>  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"
>>  
>>  static const int sector_len = 128 * 1024;
>>  
>> @@ -58,7 +59,7 @@ static void connex_init(QEMUMachineInitArgs *args)
>>      cpu = pxa255_init(address_space_mem, connex_ram);
>>  
>>      dinfo = 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 = 0;
>>  #endif
>> -    if (!pflash_cfi01_register(0x00000000, NULL, "connext.rom", connex_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", connex_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);
>> +        }
>>      }
>>  
>>      /* Interrupt line of NIC is connected to GPIO line 36 */
>> @@ -95,7 +99,7 @@ static void verdex_init(QEMUMachineInitArgs *args)
>>      cpu = pxa270_init(address_space_mem, verdex_ram, cpu_model ?: "pxa270-c0");
>>  
>>      dinfo = 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 *args)
>>  #else
>>      be = 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", 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);
>> +        }
>>      }
>>  
>>      /* Interrupt line of NIC is connected to GPIO line 99 */
>> -- 
>> 1.8.1.4

Patch

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"
 
 static const int sector_len = 128 * 1024;
 
@@ -58,7 +59,7 @@  static void connex_init(QEMUMachineInitArgs *args)
     cpu = pxa255_init(address_space_mem, connex_ram);
 
     dinfo = 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 = 0;
 #endif
-    if (!pflash_cfi01_register(0x00000000, NULL, "connext.rom", connex_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", connex_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);
+        }
     }
 
     /* Interrupt line of NIC is connected to GPIO line 36 */
@@ -95,7 +99,7 @@  static void verdex_init(QEMUMachineInitArgs *args)
     cpu = pxa270_init(address_space_mem, verdex_ram, cpu_model ?: "pxa270-c0");
 
     dinfo = 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 *args)
 #else
     be = 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", 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);
+        }
     }
 
     /* Interrupt line of NIC is connected to GPIO line 99 */