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

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 hw/arm/gumstix.c | 31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)

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

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

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