Patchwork [for,1.6] mips: revert commit b332d24a8e1290954029814d09156b06ede358e2

login
register
mail settings
Submitter Aurelien Jarno
Date Aug. 3, 2013, 10:02 p.m.
Message ID <1375567378-5089-1-git-send-email-aurelien@aurel32.net>
Download mbox | patch
Permalink /patch/264443/
State New
Headers show

Comments

Aurelien Jarno - Aug. 3, 2013, 10:02 p.m.
Now that this code path is not triggered anymore during the tests,
revert commit b332d24a8e1290954029814d09156b06ede358e2. Booting a MIPS
target without kernel nor bios doesn't really make sense.

Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 hw/mips/mips_fulong2e.c |    3 ++-
 hw/mips/mips_jazz.c     |    3 ++-
 hw/mips/mips_malta.c    |    3 ++-
 hw/mips/mips_mipssim.c  |    3 ++-
 4 files changed, 8 insertions(+), 4 deletions(-)
Andreas Färber - Aug. 4, 2013, 12:03 p.m.
Am 04.08.2013 00:02, schrieb Aurelien Jarno:
> Now that this code path is not triggered anymore during the tests,
> revert commit b332d24a8e1290954029814d09156b06ede358e2. Booting a MIPS
> target without kernel nor bios doesn't really make sense.
> 
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>

This is being discussed in http://patchwork.ozlabs.org/patch/262912/ -
so far Anthony has put a hold on further such changes unfortunately.

Andreas
Aurelien Jarno - Aug. 4, 2013, 10:06 p.m.
On Sun, Aug 04, 2013 at 02:03:20PM +0200, Andreas Färber wrote:
> Am 04.08.2013 00:02, schrieb Aurelien Jarno:
> > Now that this code path is not triggered anymore during the tests,
> > revert commit b332d24a8e1290954029814d09156b06ede358e2. Booting a MIPS
> > target without kernel nor bios doesn't really make sense.
> > 
> > Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> 
> This is being discussed in http://patchwork.ozlabs.org/patch/262912/ -
> so far Anthony has put a hold on further such changes unfortunately.
> 

This has been an error for more than 6 years, and nobody complained so
far. I understand that the machines should be testable with qtest, but
such as change has been merged already. Now there is no reason to not
fix this *regression* from version 1.5.

People should understand that QEMU is not only x86, and that not
everything should be done the x86 way.
Andreas Färber - Aug. 5, 2013, 1:43 p.m.
Am 05.08.2013 00:06, schrieb Aurelien Jarno:
> On Sun, Aug 04, 2013 at 02:03:20PM +0200, Andreas Färber wrote:
>> Am 04.08.2013 00:02, schrieb Aurelien Jarno:
>>> Now that this code path is not triggered anymore during the tests,
>>> revert commit b332d24a8e1290954029814d09156b06ede358e2. Booting a MIPS
>>> target without kernel nor bios doesn't really make sense.
>>>
>>> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
>>
>> This is being discussed in http://patchwork.ozlabs.org/patch/262912/ -
>> so far Anthony has put a hold on further such changes unfortunately.
>>
> 
> This has been an error for more than 6 years, and nobody complained so
> far.

Neither QOM nor qtest exist for 6 years, so that is not an argument for
everything. ;)

> I understand that the machines should be testable with qtest, but
> such as change has been merged already. Now there is no reason to not
> fix this *regression* from version 1.5.

Ah, you mean this?
http://git.qemu.org/?p=qemu.git;a=commit;h=b332d24a8e1290954029814d09156b06ede358e2
Wasn't aware. No objection to exit(1) from my side then.

But either way, you shouldn't replace one fprintf() with another
fprintf() but instead use our new error_report() if you touch it
(without trailing \n then). I've updated my qtest enablement series to
use it, v2 handles some more machines.

> People should understand that QEMU is not only x86, and that not
> everything should be done the x86 way.

No need to explain that to me.

I think Anthony's question was rather whether printing random text to
stderr is the best way to address that or whether QEMUMachine could use
some this-machine-needs-a-kernel flag that libvirt or someone can access
and that could be handled in a central place rather than in each machine
as they see fit.

But with the release near and no concrete patches, I don't think that's
1.6 material. Question is, do we want test cases based on cleanups that
work today in 1.6 and work from there, or do we rather wait 'til after
the release and if so, can we get them merged early so that other series
can actually be tested with them.

Regards,
Andreas
Anthony Liguori - Aug. 5, 2013, 4:43 p.m.
Andreas Färber <afaerber@suse.de> writes:

> Am 05.08.2013 00:06, schrieb Aurelien Jarno:
>> On Sun, Aug 04, 2013 at 02:03:20PM +0200, Andreas Färber wrote:
>>> Am 04.08.2013 00:02, schrieb Aurelien Jarno:
>>>> Now that this code path is not triggered anymore during the tests,
>>>> revert commit b332d24a8e1290954029814d09156b06ede358e2. Booting a MIPS
>>>> target without kernel nor bios doesn't really make sense.
>>>>
>>>> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
>>>
>>> This is being discussed in http://patchwork.ozlabs.org/patch/262912/ -
>>> so far Anthony has put a hold on further such changes unfortunately.
>>>
>> 
>> This has been an error for more than 6 years, and nobody complained so
>> far.
>
> Neither QOM nor qtest exist for 6 years, so that is not an argument for
> everything. ;)
>
>> I understand that the machines should be testable with qtest, but
>> such as change has been merged already. Now there is no reason to not
>> fix this *regression* from version 1.5.
>
> Ah, you mean this?
> http://git.qemu.org/?p=qemu.git;a=commit;h=b332d24a8e1290954029814d09156b06ede358e2
> Wasn't aware. No objection to exit(1) from my side then.
>
> But either way, you shouldn't replace one fprintf() with another
> fprintf() but instead use our new error_report() if you touch it
> (without trailing \n then). I've updated my qtest enablement series to
> use it, v2 handles some more machines.
>
>> People should understand that QEMU is not only x86, and that not
>> everything should be done the x86 way.
>
> No need to explain that to me.

I don't object to adding the exit(1) FWIW.

But I also think we should think more about having consistent behavior
across platforms.

It's unexpected that qemu-system-x86_64 does something and
qemu-system-mips does something else.

Maybe -x86_64 should barf is not given anything bootable...

Regards,

Anthony Liguori

>
> I think Anthony's question was rather whether printing random text to
> stderr is the best way to address that or whether QEMUMachine could use
> some this-machine-needs-a-kernel flag that libvirt or someone can access
> and that could be handled in a central place rather than in each machine
> as they see fit.
>
> But with the release near and no concrete patches, I don't think that's
> 1.6 material. Question is, do we want test cases based on cleanups that
> work today in 1.6 and work from there, or do we rather wait 'til after
> the release and if so, can we get them merged early so that other series
> can actually be tested with them.
>
> Regards,
> Andreas
>
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Andreas Färber - Aug. 5, 2013, 5:36 p.m.
Am 05.08.2013 18:43, schrieb Anthony Liguori:
> Andreas Färber <afaerber@suse.de> writes:
> 
>> Am 05.08.2013 00:06, schrieb Aurelien Jarno:
>>> On Sun, Aug 04, 2013 at 02:03:20PM +0200, Andreas Färber wrote:
>>>> Am 04.08.2013 00:02, schrieb Aurelien Jarno:
>>>>> Now that this code path is not triggered anymore during the tests,
>>>>> revert commit b332d24a8e1290954029814d09156b06ede358e2. Booting a MIPS
>>>>> target without kernel nor bios doesn't really make sense.
>>>>>
>>>>> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
>>>>
>>>> This is being discussed in http://patchwork.ozlabs.org/patch/262912/ -
>>>> so far Anthony has put a hold on further such changes unfortunately.
>>>>
>>>
>>> This has been an error for more than 6 years, and nobody complained so
>>> far.
>>
>> Neither QOM nor qtest exist for 6 years, so that is not an argument for
>> everything. ;)
>>
>>> I understand that the machines should be testable with qtest, but
>>> such as change has been merged already. Now there is no reason to not
>>> fix this *regression* from version 1.5.
>>
>> Ah, you mean this?
>> http://git.qemu.org/?p=qemu.git;a=commit;h=b332d24a8e1290954029814d09156b06ede358e2
>> Wasn't aware. No objection to exit(1) from my side then.
>>
>> But either way, you shouldn't replace one fprintf() with another
>> fprintf() but instead use our new error_report() if you touch it
>> (without trailing \n then). I've updated my qtest enablement series to
>> use it, v2 handles some more machines.
>>
>>> People should understand that QEMU is not only x86, and that not
>>> everything should be done the x86 way.
>>
>> No need to explain that to me.
> 
> I don't object to adding the exit(1) FWIW.
> 
> But I also think we should think more about having consistent behavior
> across platforms.
> 
> It's unexpected that qemu-system-x86_64 does something and
> qemu-system-mips does something else.
> 
> Maybe -x86_64 should barf is not given anything bootable...

I would surely hope it does? If SeaBIOS is not found (and optionally
!qtest_enabled()), then it should barf, just like -ppc[64]/-sparc[64]
should barf when they don't find OpenBIOS or OHW respectively. When the
firmware has some limited user interaction such as menus or a command
prompt then there is nothing wrong with exposing that to the user.

The difference for some of these arm/mips/ppc/sh4 targets is that we
don't ship any matching firmware out of the box, thus can't rely on its
presence for qtest.

Personally I have found running -x86_64 with some -device (or
-readconfig) can already be quite a useful test case for QOM devices or
for non-destructive migration testing.

By comparison, having -alpha firmware just print "Hello" does not seem
all that useful to me... I wouldn't mind error'ing out without useful
arguments there.

FWIW the Cocoa UI detects a disk image missing from the command line and
prompts for one - yet another behavior. To boot just into the BIOS I
have to specify /dev/null IIRC.

Regards,
Andreas

>> I think Anthony's question was rather whether printing random text to
>> stderr is the best way to address that or whether QEMUMachine could use
>> some this-machine-needs-a-kernel flag that libvirt or someone can access
>> and that could be handled in a central place rather than in each machine
>> as they see fit.
>>
>> But with the release near and no concrete patches, I don't think that's
>> 1.6 material. Question is, do we want test cases based on cleanups that
>> work today in 1.6 and work from there, or do we rather wait 'til after
>> the release and if so, can we get them merged early so that other series
>> can actually be tested with them.
>>
>> Regards,
>> Andreas
Richard Henderson - Aug. 5, 2013, 7:26 p.m.
On 08/05/2013 07:36 AM, Andreas Färber wrote:
> By comparison, having -alpha firmware just print "Hello" does not seem
> all that useful to me... I wouldn't mind error'ing out without useful
> arguments there.

One of these days I'll get around to adding the bits that let you say
"boot" on the PALcode console prompt.  But in the meantime, I find the
ability to boot the bios up to the "Hello" useful as a smoke-test for
TCG changes.

That said, I can see how it can be confusing for anyone else, it would
be more useful to have something integrated with the testsuite instead,
so I won't object if we want to print an error message for lack of
-kernel or (explicit) -bios command-line arguments.


r~

Patch

diff --git a/hw/mips/mips_fulong2e.c b/hw/mips/mips_fulong2e.c
index b13750d..f0b8d06 100644
--- a/hw/mips/mips_fulong2e.c
+++ b/hw/mips/mips_fulong2e.c
@@ -335,7 +335,8 @@  static void mips_fulong2e_init(QEMUMachineInitArgs *args)
 
         if ((bios_size < 0 || bios_size > BIOS_SIZE) &&
             !kernel_filename && !qtest_enabled()) {
-            fprintf(stderr, "qemu: Warning, could not load MIPS bios '%s'\n", bios_name);
+            fprintf(stderr, "qemu: Could not load MIPS bios '%s'\n", bios_name);
+            exit(1);
         }
     }
 
diff --git a/hw/mips/mips_jazz.c b/hw/mips/mips_jazz.c
index 36677cc..7ecb49a 100644
--- a/hw/mips/mips_jazz.c
+++ b/hw/mips/mips_jazz.c
@@ -178,8 +178,9 @@  static void mips_jazz_init(MemoryRegion *address_space,
         bios_size = -1;
     }
     if ((bios_size < 0 || bios_size > MAGNUM_BIOS_SIZE) && !qtest_enabled()) {
-        fprintf(stderr, "qemu: Warning, could not load MIPS bios '%s'\n",
+        fprintf(stderr, "qemu: Could not load MIPS bios '%s'\n",
                 bios_name);
+        exit(1);
     }
 
     /* Init CPU internal devices */
diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
index f56f34f..5726650 100644
--- a/hw/mips/mips_malta.c
+++ b/hw/mips/mips_malta.c
@@ -1009,8 +1009,9 @@  void mips_malta_init(QEMUMachineInitArgs *args)
             if ((bios_size < 0 || bios_size > BIOS_SIZE) &&
                 !kernel_filename && !qtest_enabled()) {
                 fprintf(stderr,
-                        "qemu: Warning, could not load MIPS bios '%s', and no -kernel argument was specified\n",
+                        "qemu: Could not load MIPS bios '%s', and no -kernel argument was specified\n",
                         bios_name);
+                exit(1);
             }
         }
         /* In little endian mode the 32bit words in the bios are swapped,
diff --git a/hw/mips/mips_mipssim.c b/hw/mips/mips_mipssim.c
index fea1a15..f7ec179 100644
--- a/hw/mips/mips_mipssim.c
+++ b/hw/mips/mips_mipssim.c
@@ -192,8 +192,9 @@  mips_mipssim_init(QEMUMachineInitArgs *args)
     if ((bios_size < 0 || bios_size > BIOS_SIZE) && !kernel_filename) {
         /* Bail out if we have neither a kernel image nor boot vector code. */
         fprintf(stderr,
-                "qemu: Warning, could not load MIPS bios '%s', and no -kernel argument was specified\n",
+                "qemu: Could not load MIPS bios '%s', and no -kernel argument was specified\n",
                 filename);
+        exit(1);
     } else {
         /* We have a boot vector start address. */
         env->active_tc.PC = (target_long)(int32_t)0xbfc00000;